Separate FW version read API for Hisense 2x100G AOC cable + enable image validity check for dual bank modules by pnakka28 · Pull Request #631 · sonic-net/sonic-platform-common

pnakka28 · 2026-03-10T21:42:52Z

Description

Revert single-bank firmware handling from the default CMIS get_module_fw_info() and add a vendor-specific override for single-bank modules. This change moves single-bank handling out of the default CMIS API into a vendor-specific override and fixes incorrect inactive firmware version reporting for dual-bank modules during firmware upgrades.

Changes:

Add CmisAocSingleBankApi class that overrides get_module_fw_info() to handle single-bank modules
Add tests covering all firmware info scenarios and factory routing

Motivation and Context

While the previous implementation worked correctly for single-bank modules, it caused incorrect inactive firmware reporting for dual-bank modules during firmware upgrades. When a firmware download was interrupted or an image on the inactive bank was invalid, the inactive firmware version would still be reported from EEPROM instead of N/A.

To fix this, the single-bank handling code in the default CMIS API has been reverted so that dual-bank modules correctly report N/A for invalid images. A separate vendor-specific override has been added for single-bank modules, where the single-bank firmware handling is actually needed. This approach ensures:

Dual-bank modules report firmware info correctly during upgrades
Single-bank modules continue to work with inactive firmware read from EEPROM

How Has This Been Tested?

Verified sfputil show fwversion on single-bank modules shows inactive firmware read from EEPROM
Verified sfputil show fwversion on dual-bank CMIS modules still shows both Image A and Image B correctly
Verified dual-bank module with invalid image after interrupted firmware download reports inactive firmware as N/A
Verified xcvrd logs show no tracebacks or exceptions
Verified the DB is getting populated with the correct firmware versions

Additional Information (Optional)

MSFT ADO - 29333553

…age validity check for dual bank modules - Add CmisAocSingleBankApi class for single-bank modules - Override get_module_fw_info() to handle single-bank - Add vendor routing for Hisense modules - Add tests for single-bank firmware info and factory routing Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

mssonicbld · 2026-03-10T21:43:03Z

/azp run

azure-pipelines · 2026-03-10T21:43:12Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

This PR adjusts CMIS firmware version reporting so dual-bank modules no longer report an “inactive” version when the inactive image is invalid, while preserving the needed single-bank behavior via a Hisense-specific CMIS API override routed through XcvrApiFactory.

Changes:

Revert single-bank inactive FW fallback in the default CMIS get_module_fw_info() so invalid images correctly yield N/A on dual-bank modules.
Add CmisAocSingleBankApi (Hisense 2x100G AOC) to read inactive FW from EEPROM for single-bank modules.
Add factory routing + unit tests intended to cover the new vendor override path.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`sonic_platform_base/sonic_xcvr/api/public/cmis.py`	Restores dual-bank semantics by always using Image A/B version (which becomes `N/A` when invalid) for inactive FW.
`sonic_platform_base/sonic_xcvr/api/hisense/aoc_2x100g.py`	Introduces Hisense single-bank override that reads inactive FW from EEPROM.
`sonic_platform_base/sonic_xcvr/xcvr_api_factory.py`	Routes Hisense DEF8504-2x100G part numbers to the single-bank API implementation.
`tests/sonic_xcvr/test_xcvr_api_factory.py`	Adds a Hisense routing test (currently not asserting routing correctness).
`tests/sonic_xcvr/test_aoc_2x100g.py`	Adds unit tests for the new Hisense single-bank `get_module_fw_info()` override (assertions are currently too weak).
`setup.py`	Ensures the new `sonic_platform_base.sonic_xcvr.api.hisense` package is included in the distribution.

tests/sonic_xcvr/test_xcvr_api_factory.py

tests/sonic_xcvr/test_aoc_2x100g.py

sonic_platform_base/sonic_xcvr/api/hisense/aoc_2x100g.py

sonic_platform_base/sonic_xcvr/xcvr_api_factory.py

Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

mssonicbld · 2026-03-10T23:32:15Z

/azp run

azure-pipelines · 2026-03-10T23:32:25Z

Azure Pipelines successfully started running 1 pipeline(s).

prgeor · 2026-03-11T04:51:43Z

sonic_platform_base/sonic_xcvr/api/hisense/aoc_2x100g.py

+logger = logging.getLogger(__name__)
+
+class CmisAocSingleBankApi(CmisApi):
+    def get_module_fw_info(self):


@pnakka28 can we use CdbFwHandler ?

sonic-platform-common/sonic_platform_base/sonic_xcvr/cdb/cdb_fw.py

Line 44 in 19451b5

def get_firmware_info(self):

@prgeor get_module_fw_info() is the method called by sfputil show fwversion and is also called internally by get_transceiver_info_firmware_versions() in cmis.py. If we override get_firmware_info(), all these calls would still use the base get_module_fw_info() (current) which reads Image B from invalid CDB registers on single-bank modules. Overriding get_module_fw_info() ensures correct single-bank behavior.

@pnakka28 please fix as discussed offline.

@prgeor fixed to use CdbFwHandler

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.

You can also share your feedback on Copilot code review. Take the survey.

sonic_platform_base/sonic_xcvr/xcvr_api_factory.py

tests/sonic_xcvr/test_aoc_2x100g.py

tests/sonic_xcvr/test_xcvr_api_factory.py

prgeor · 2026-03-16T14:43:33Z

@pnakka28 please address copilot review

- Replaced CDB byte parsing with CdbFwHandler.get_firmware_info() - Updated tests to mock CdbFwHandler instead of raw CDB responses Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

mssonicbld · 2026-03-16T19:54:53Z

/azp run

azure-pipelines · 2026-03-16T19:55:04Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

You can also share your feedback on Copilot code review. Take the survey.

pnakka28 · 2026-03-17T17:19:08Z

sonic_platform_base/sonic_xcvr/api/hisense/aoc_2x100g.py

+            RunningImage = 'A'
+            ActiveFirmware = ImageA
+            # In case of single bank module, inactive firmware version can be read from EEPROM
+            InactiveFirmware = self.get_module_inactive_firmware() + ".0"


get_module_inactive_firmware() reads from EEPROM registers. This code path only executes when ImageA is running, so the module is running with CDB support not a flat memory. EEPROM registers will always return valid values here.

@pnakka28 "EEPROM registers will always return valid values here." -- is this correct even if there are i2c read errors?

@prgeor i2c read errors could cause get_module_inactive_firmware() to return N/A. will add guard to append for only valid string.

@prgeor added guard for read errors

sonic_platform_base/sonic_xcvr/api/hisense/aoc_2x100g.py

mssonicbld · 2026-03-17T18:39:05Z

/azp run

azure-pipelines · 2026-03-17T18:39:15Z

Azure Pipelines successfully started running 1 pipeline(s).

sonic_platform_base/sonic_xcvr/api/hisense/aoc_2x100g.py

mssonicbld · 2026-03-18T17:43:07Z

/azp run

azure-pipelines · 2026-03-18T17:43:18Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

You can also share your feedback on Copilot code review. Take the survey.

sonic_platform_base/sonic_xcvr/api/hisense/aoc_2x100g.py

tests/sonic_xcvr/test_aoc_2x100g.py

sonic_platform_base/sonic_xcvr/api/public/cmis.py

Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

mssonicbld · 2026-03-19T17:21:52Z

/azp run

azure-pipelines · 2026-03-19T17:22:01Z

Azure Pipelines successfully started running 1 pipeline(s).

prgeor · 2026-03-19T20:20:39Z

sonic_platform_base/sonic_xcvr/api/hisense/aoc_2x100g.py

+        if fw_info is False or fw_info is None:
+            return {'status': False, 'info': "Failed to get firmware info", 'result': None}
+
+        fw_status = fw_info.get(cdb_consts.CDB1_FIRMWARE_STATUS, {})


@pnakka28 this decoding logic is common even for non-Hisense module. Can you fix the common

sonic-platform-common/sonic_platform_base/sonic_xcvr/api/public/cmis.py

Line 1929 in 972ff46

if status == 1 and self.cdb.cdb_chkcode(rpl) == rpl_chkcode:

and override what is needed for single bank?

@prgeor fixed the decoding logic for common

- Refactor to use CdbFwHandler for parsing firmware info instead of decoding raw CDB reply bytes - Added subclass single-bank firmware modules that read inactive firmware from EEPROM Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

mssonicbld · 2026-03-23T21:55:57Z

/azp run

azure-pipelines · 2026-03-23T21:56:06Z

Azure Pipelines successfully started running 1 pipeline(s).

prgeor · 2026-03-24T15:20:43Z

@copilot review the code

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

Copilot · 2026-03-24T16:15:00Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+            self._cdb_fw_hdlr = self._create_cdb_fw_handler()
+            if self._cdb_fw_hdlr is None:
+                return {'status': False, 'info': "CDB FW handler init failed", 'result': None}
+
+        fw_info = self._cdb_fw_hdlr.get_firmware_info()


get_module_fw_info() now instantiates CdbFwHandler and calls get_firmware_info(), but _create_cdb_fw_handler() constructs CdbFwHandler which asserts inside __init__ when initFwHandler() fails (e.g., module busy / CDB failure). Since this method doesn’t catch exceptions, callers can get an unhandled AssertionError instead of a clean {'status': False, ...} response. Wrap handler creation / firmware-info reads in exception handling (and avoid relying on assert for runtime control flow) so get_module_fw_info() fails gracefully.

Suggested change

self._cdb_fw_hdlr = self._create_cdb_fw_handler()

if self._cdb_fw_hdlr is None:

return {'status': False, 'info': "CDB FW handler init failed", 'result': None}

fw_info = self._cdb_fw_hdlr.get_firmware_info()

try:

self._cdb_fw_hdlr = self._create_cdb_fw_handler()

except AssertionError as e:

logger.error("CDB FW handler init assertion failed: %s", e)

return {'status': False, 'info': "CDB FW handler init failed", 'result': None}

except Exception as e:

logger.exception("Unexpected error during CDB FW handler init: %s", e)

return {'status': False, 'info': "CDB FW handler init failed", 'result': None}

if self._cdb_fw_hdlr is None:

return {'status': False, 'info': "CDB FW handler init failed", 'result': None}

try:

fw_info = self._cdb_fw_hdlr.get_firmware_info()

except Exception as e:

logger.exception("Exception while getting firmware info: %s", e)

return {'status': False, 'info': "Failed to get firmware info", 'result': None}

@pnakka28 Can you please look into this?

@mihirpat1 addressed this comment

Copilot · 2026-03-24T16:15:00Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+                return {'status': False, 'info': "CDB FW handler init failed", 'result': None}
+
+        fw_info = self._cdb_fw_hdlr.get_firmware_info()
+        if fw_info is False or fw_info is None:


The previous get_module_fw_info() logic retried on the CDB “need to enter password” status (0x46) by calling module_enter_password() and re-issuing the command. With the new _cdb_fw_hdlr path, password-required cases will just return “Failed to get firmware info” (and may trip the handler init assert). Consider preserving the password-handling behavior either in CdbFwHandler or in get_module_fw_info() so modules requiring password don’t regress.

Suggested change

if fw_info is False or fw_info is None:

if fw_info is False or fw_info is None:

# Attempt to handle password-required cases by entering the module password

enter_password = getattr(self, 'module_enter_password', None)

if callable(enter_password):

# If password entry fails explicitly, propagate failure

if enter_password() is False:

return {'status': False, 'info': "Failed to enter module password", 'result': None}

# Retry fetching firmware info after entering password

fw_info = self._cdb_fw_hdlr.get_firmware_info()

if fw_info is False or fw_info is None:

@prgeor Do we still need to preserve the password entering logic or okay to remove it?

sonic_platform_base/sonic_xcvr/api/public/cmis.py

Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

mssonicbld · 2026-03-24T18:47:54Z

/azp run

azure-pipelines · 2026-03-24T18:48:03Z

Azure Pipelines successfully started running 1 pipeline(s).

prgeor requested review from Copilot, mihirpat1 and prgeor March 10, 2026 22:00

Copilot started reviewing on behalf of prgeor March 10, 2026 22:01 View session

Copilot AI reviewed Mar 10, 2026

View reviewed changes

Addressed PR comments

1b06611

Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

prgeor reviewed Mar 11, 2026

View reviewed changes

mihirpat1 requested a review from Copilot March 13, 2026 23:33

Copilot started reviewing on behalf of mihirpat1 March 13, 2026 23:34 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

sonic_platform_base/sonic_xcvr/xcvr_api_factory.py Show resolved Hide resolved

tests/sonic_xcvr/test_aoc_2x100g.py Outdated Show resolved Hide resolved

tests/sonic_xcvr/test_xcvr_api_factory.py Show resolved Hide resolved

tests/sonic_xcvr/test_xcvr_api_factory.py Show resolved Hide resolved

Switched to CdbFwHandler for firmware info

eeec1ec

- Replaced CDB byte parsing with CdbFwHandler.get_firmware_info() - Updated tests to mock CdbFwHandler instead of raw CDB responses Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

mihirpat1 requested a review from Copilot March 17, 2026 17:02

Copilot started reviewing on behalf of mihirpat1 March 17, 2026 17:03 View session

Copilot AI reviewed Mar 17, 2026

View reviewed changes

mihirpat1 reviewed Mar 18, 2026

View reviewed changes

pnakka28 force-pushed the singlebank_fw branch from 90c350d to ab07aa8 Compare March 18, 2026 17:43

mihirpat1 requested a review from Copilot March 19, 2026 16:43

Copilot started reviewing on behalf of mihirpat1 March 19, 2026 16:44 View session

Copilot AI reviewed Mar 19, 2026

View reviewed changes

sonic_platform_base/sonic_xcvr/api/hisense/aoc_2x100g.py Outdated Show resolved Hide resolved

tests/sonic_xcvr/test_aoc_2x100g.py Outdated Show resolved Hide resolved

sonic_platform_base/sonic_xcvr/api/public/cmis.py Outdated Show resolved Hide resolved

Addressed PR comments

a9d8714

Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

pnakka28 force-pushed the singlebank_fw branch from ab07aa8 to a9d8714 Compare March 19, 2026 17:21

mihirpat1 previously approved these changes Mar 19, 2026

View reviewed changes

prgeor reviewed Mar 19, 2026

View reviewed changes

Fixed decoding logic for firmware info

c474f46

- Refactor to use CdbFwHandler for parsing firmware info instead of decoding raw CDB reply bytes - Added subclass single-bank firmware modules that read inactive firmware from EEPROM Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

pnakka28 dismissed mihirpat1’s stale review via c474f46 March 23, 2026 21:55

mihirpat1 requested a review from Copilot March 24, 2026 16:10

Copilot started reviewing on behalf of mihirpat1 March 24, 2026 16:10 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

Addressed PR comments

10bd489

Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>

mihirpat1 approved these changes Mar 24, 2026

View reviewed changes

-        if fw_info is False or fw_info is None:
+        if fw_info is False or fw_info is None:
+            # Attempt to handle password-required cases by entering the module password
+            enter_password = getattr(self, 'module_enter_password', None)
+            if callable(enter_password):
+                # If password entry fails explicitly, propagate failure
+                if enter_password() is False:
+                    return {'status': False, 'info': "Failed to enter module password", 'result': None}
+                # Retry fetching firmware info after entering password
+                fw_info = self._cdb_fw_hdlr.get_firmware_info()
+        if fw_info is False or fw_info is None:

Conversation

pnakka28 commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

mssonicbld commented Mar 10, 2026

Uh oh!

azure-pipelines bot commented Mar 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Mar 10, 2026

Uh oh!

azure-pipelines bot commented Mar 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

prgeor commented Mar 16, 2026

Uh oh!

mssonicbld commented Mar 16, 2026

Uh oh!

azure-pipelines bot commented Mar 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mssonicbld commented Mar 17, 2026

Uh oh!

azure-pipelines bot commented Mar 17, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Mar 18, 2026

Uh oh!

azure-pipelines bot commented Mar 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mssonicbld commented Mar 19, 2026

pnakka28 commented Mar 10, 2026 •

edited

Loading