Skip to content

minerva-ag: Block vr access during update to prevent update fail #2639

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
Victor-Jhong wants to merge 2 commits into
base: main
Choose a base branch
from
Open

minerva-ag: Block vr access during update to prevent update fail #2639

Victor-Jhong wants to merge 2 commits into from

Conversation

@Victor-Jhong
Copy link
Contributor

@Victor-Jhong Victor-Jhong commented Dec 18, 2025

Summary:

  • Block vr access during update to prevent update fail

Test Plan:

  • Build code: Pass

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 18, 2025
@meta-codesync
Copy link

meta-codesync bot commented Dec 18, 2025

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D89447570. (Because this pull request was imported automatically, there will not be any future comments.)

@Victor-Jhong Victor-Jhong force-pushed the minerva-ag/block_vr_access_during_update_to_prevent_update_fail branch from 63a42dc to 36b1737 Compare December 19, 2025 07:34
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Dec 19, 2025

@Victor-Jhong has updated the pull request. You must reimport the pull request before landing.

jamesatha
jamesatha requested changes Jan 14, 2026
View reviewed changes
Copy link
Contributor

@jamesatha jamesatha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the relevant cppcheck error

common/service/pldm/pldm_firmware_update.c
const uint8_t *comp_image_version_str,
uint8_t comp_image_version_str_len)
{
return PLDM_SUCCESS;
Copy link
Contributor

@jamesatha jamesatha Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we put an real implementation in here? If not, can you address the cppcheck error on line 1082?

Copy link
Contributor Author

@Victor-Jhong Victor-Jhong Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning can be safely ignored; it's a limitation of static analysis, not an understanding of the weak function override mechanism.

@Victor-Jhong Victor-Jhong force-pushed the minerva-ag/block_vr_access_during_update_to_prevent_update_fail branch from 36b1737 to 2b992d6 Compare January 15, 2026 02:21
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 15, 2026

@Victor-Jhong has updated the pull request. You must reimport the pull request before landing.

jamesatha
jamesatha requested changes Jan 15, 2026
View reviewed changes
meta-facebook/minerva-ag/src/platform/plat_pldm_fw_update.c Outdated
resp_p->get_value = plat_force_update_flag;

exit:
*resp_len = sizeof(struct _sensor_polling_cmd_resp);
Copy link
Contributor

@jamesatha jamesatha Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be struct _force_update_flag_get_cmd_resp?

Copy link
Contributor Author

@Victor-Jhong Victor-Jhong Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right. Although both structs happen to have the same size (5 bytes), using the wrong struct name is semantically incorrect and could cause confusion.
Thanks for the review!

@Victor-Jhong Victor-Jhong force-pushed the minerva-ag/block_vr_access_during_update_to_prevent_update_fail branch from 2b992d6 to d63c513 Compare January 16, 2026 02:26
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 16, 2026

@Victor-Jhong has updated the pull request. You must reimport the pull request before landing.

@Victor-Jhong Victor-Jhong force-pushed the minerva-ag/block_vr_access_during_update_to_prevent_update_fail branch from d63c513 to 952bb05 Compare January 16, 2026 02:32
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 16, 2026

@Victor-Jhong has updated the pull request. You must reimport the pull request before landing.

@Victor-Jhong Victor-Jhong force-pushed the minerva-ag/block_vr_access_during_update_to_prevent_update_fail branch from 952bb05 to 38fd1ff Compare January 16, 2026 02:41
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jan 16, 2026

@Victor-Jhong has updated the pull request. You must reimport the pull request before landing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jamesatha jamesatha jamesatha requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Victor-Jhong @facebook-github-bot @jamesatha