[CMIS] Fix VDM freeze/unfreeze as per CMIS 5.4 - #757
Merged
Merged
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
prgeor
marked this pull request as ready for review
September 16, 2026 01:07
Collaborator
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings affect map loading, backward compatibility, and test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request updates CMIS VDM freeze/unfreeze handling for CMIS 5.4 by modeling the Page 2F control register fields.
Changes:
- Adds VDM control bit-field definitions and constants.
- Updates freeze/unfreeze operations to use the freeze-request field.
- Adds reserved, power-saving, and duty-cycle mappings.
File summaries
| File | Reviewed changes and findings |
|---|---|
sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page2f.py |
Defines Page 2F VDM fields. Critical (2 votes): the duty-cycle field mapping uses an invalid bit position and prevents map loading. Moderate (2 votes): making VDM_CONTROL read-only breaks existing callers. Moderate (1 vote): tests do not verify the field name or bit layout. |
sonic_platform_base/sonic_xcvr/fields/consts.py |
Adds constants for the VDM control fields. |
sonic_platform_base/sonic_xcvr/api/public/cmis.py |
Updates freeze/unfreeze to target VDM_FREEZE_REQUEST. Moderate (2 votes): existing tests do not exercise nested-field lookup or read-modify-write behavior. |
Review details
Suppressed comments (1)
sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page2f.py:30
- The existing VDM tests only mock
xcvr_eeprom.writeand check its return value, so they do not verify the new field name/value or the Page 2F bit layout. Add a focused memory-map/API test that checks freeze writes bit 7 throughVDM_FREEZE_REQUESTand preserves the other control bits; otherwise a wrong mapping can pass the suite.
RegBitsField(consts.VDM_FREEZE_REQUEST, bitpos=7, size=1, ro=False),
RegBitsField(consts.VDM_POWER_SAVING_MODE, bitpos=6, size=1, ro=False),
RegBitsField(consts.VDM_MON_DUTY_CYCLE, bitpos=5, size=4, ro=False),
RegBitsField(consts.VDM_RESERVED, bitpos=0, size=2, ro=True)
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| NumberRegField(consts.VDM_CONTROL, self.getaddr(144), | ||
| RegBitsField(consts.VDM_FREEZE_REQUEST, bitpos=7, size=1, ro=False), | ||
| RegBitsField(consts.VDM_POWER_SAVING_MODE, bitpos=6, size=1, ro=False), | ||
| RegBitsField(consts.VDM_MON_DUTY_CYCLE, bitpos=5, size=4, ro=False), |
Comment on lines
+30
to
+31
| RegBitsField(consts.VDM_RESERVED, bitpos=0, size=2, ro=True) | ||
| ), |
Signed-off-by: Prince George <prgeor@microsoft.com>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Prince George <prgeor@microsoft.com>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Collaborator
|
This PR has backport request label(s) for branch(es): 202605, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202605: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
mihirpat1
reviewed
Sep 16, 2026
Contributor
|
@prgeor Can you please help to fix the test failure? |
Signed-off-by: Prince George <prgeor@microsoft.com>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Prince George <prgeor@microsoft.com>
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
mihirpat1
approved these changes
Sep 17, 2026
Contributor
|
Hi @vaibhavhd can you help take this 202605 please? Thanks! |
Contributor
|
@mihirpat1 @prgeor can you please help share the testing done on the 202605 branch as well? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix VDM Freeze/Unfreeze as per CMIS 5.4
Motivation and Context
VDM Freeze/Unfreeze is NOT working in modules following 5.4 spec
How Has This Been Tested?
Tested this on module following CMIS 5.3 and 5.4
Additional Information (Optional)