-
Notifications
You must be signed in to change notification settings - Fork 204
[cmis] Only create CdbFwHandler if module support CDB #611
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.
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
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
36ff28d to
9fb086f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
9fb086f to
a996e1c
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
a996e1c to
add299f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
0069d49 to
5184f95
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
5184f95 to
edb3a9b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
edb3a9b to
6f66355
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where CdbFwHandler was being instantiated for CMIS modules that don't support CDB (Command DataBlock), causing initialization failures due to an assertion error. The fix introduces lazy initialization of the CDB firmware handler, only creating it when explicitly requested and when CDB is actually supported by the module.
Key Changes:
- Converted
cdb_fw_hdlrfrom a constructor parameter to a lazily-initialized property - Added
init_cdb_fw_handlerflag to control whether the handler should be created - Removed premature CDB handler instantiation from the factory and test code
Reviewed changes
Copilot reviewed 7 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 | Implements lazy initialization pattern for CDB firmware handler with property-based access |
| sonic_platform_base/sonic_xcvr/api/public/c_cmis.py | Updates CCmisApi constructor to pass through the init_cdb_fw_handler flag |
| sonic_platform_base/sonic_xcvr/xcvr_api_factory.py | Removes CDB imports and switches to lazy initialization via init_cdb_fw_handler=True |
| tests/sonic_xcvr/test_cmis.py | Adds tests for the new lazy initialization behavior and CDB handler creation logic |
| tests/sonic_xcvr/test_ccmis.py | Updates test setup to use init_cdb_fw_handler flag instead of passing CDB handler directly |
| tests/sonic_xcvr/test_sfp_optoe_base.py | Updates test setup to disable CDB handler initialization for test cases |
| tests/sonic_xcvr/test_cdb_fw.py | Fixes test assertions to expect False instead of True when initialization fails |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Junchao-Mellanox <[email protected]>
Signed-off-by: Junchao-Mellanox <[email protected]>
a05fe32 to
5d0f046
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Junchao-Mellanox <[email protected]>
5d0f046 to
c98e56f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Junchao-Mellanox <[email protected]>
ed60263 to
07cfcda
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi @judyjoseph , @prgeor , could you please review and sign-off? |
|
Cherry-pick PR to 202511: #619 |
Description
CdbFwHandler is created to handle CDB related command for cmis module. It should be created only CDB is supported by this module. However, the current implementation add an assertion in init function which will fail any cmis module which does not support CDB:
The error logs:
Motivation and Context
Only create CdbFwHandler if module support CDB
How Has This Been Tested?
Manual test with modules that support and not support CDB
Unit test
Additional Information (Optional)