[cmis] Fix enter_password to include standard password entry method - #724
Open
Junchao-Mellanox wants to merge 4 commits into
Open
[cmis] Fix enter_password to include standard password entry method#724Junchao-Mellanox wants to merge 4 commits into
Junchao-Mellanox wants to merge 4 commits into
Conversation
#### Why I did it `CdbCmdHandler.enter_password()` previously delivered the CMIS host password only via CDB command `0001h`. This is not the standard mechanism defined by the CMIS spec and is not honored by all modules — the standard, universally-supported way to unlock password-protected CDB/EEPROM access is to write the 4-byte host password (MSB first) to the Password Entry Area at page 00h bytes 122-125. As a result, password entry would fail on modules that only unlock via the Password Entry Area, blocking protected CDB/EEPROM operations (e.g. firmware/SED-protected access) on those modules. #### How I did it - `cdb.py` — Reworked `enter_password()` to first write the password to the Password Entry Area (page 00h bytes 122-125, MSB first) using `write_raw`, which is the standard method honored by all CMIS modules. If that write fails, it falls back to the original CDB command `0001h` path for modules that rely on it. Input validation (integer in range `0..0xFFFFFFFF`) is preserved, and `struct` is used to pack the 32-bit password big-endian. - `cdb_consts.py` — Added `CDB_HOST_PASSWORD_ENTRY_OFFSET = 122` and `CDB_HOST_PASSWORD_ENTRY_SIZE = 4` with documentation describing the Password Entry Area register layout. - `test_cdb.py` — Updated existing tests to assert the password is now written MSB-first to the Password Entry Area (and that no CDB command is sent when the register write succeeds), and added `test_enter_password_fallback_to_cdb_command` to cover the fallback path when the register write fails. #### How to verify it Ran the updated unit tests in `tests/sonic_xcvr/test_cdb.py`, covering: - `test_enter_password_valid` — password written MSB-first to the Password Entry Area, no CDB command issued. - `test_enter_password_default` — default password (`0x00001011`) written to the Password Entry Area. - `test_enter_password_fallback_to_cdb_command` — CDB command `0001h` used as fallback when the register write returns `False`. Signed-off-by: Junchao Chen <junchao@mellanox.com>
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
Author
|
Hi @prgeor , @mihirpat1 , could you please help review this? Thanks! |
stephenxs
previously approved these changes
Jul 21, 2026
Signed-off-by: Junchao Chen <junchao@mellanox.com>
Junchao-Mellanox
force-pushed
the
public/pr-19
branch
from
July 22, 2026 09:23
600270d to
b6282d6
Compare
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
bgallagher-nexthop
approved these changes
Jul 22, 2026
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Reduced the polling interval for password result checks. Signed-off-by: Junchao Chen <junchao@mellanox.com>
Junchao-Mellanox
force-pushed
the
public/pr-19
branch
from
July 23, 2026 02:53
8676fb0 to
2abdcf0
Compare
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
keboliu
previously approved these changes
Jul 28, 2026
prgeor
reviewed
Jul 28, 2026
prgeor
reviewed
Jul 28, 2026
prgeor
reviewed
Jul 28, 2026
prgeor
reviewed
Jul 28, 2026
prgeor
reviewed
Jul 28, 2026
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Junchao Chen <junchao@mellanox.com>
Junchao-Mellanox
force-pushed
the
public/pr-19
branch
from
July 29, 2026 05:54
59eeec7 to
2d17bd7
Compare
Collaborator
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
prgeor
reviewed
Jul 31, 2026
| class CdbFwHandler(CdbCmdHandler): | ||
| def __init__(self, reader, writer, mem_map): | ||
| super(CdbFwHandler, self).__init__(reader, writer, mem_map) | ||
| def __init__(self, reader, writer, mem_map, cmis_api): |
Collaborator
There was a problem hiding this comment.
@Junchao-Mellanox please avoid polluting the namespace by introducing cmis_api into the CdbFwHandler. The handler is a service provider....the API is the user lets not inter-twine both of them.
Contributor
Author
There was a problem hiding this comment.
Hi @prgeor , we cannot achieve following 2 items together:
- Not putting cmis_api to CdbFwHandler (Your current review comment)
- Put all memory password related logic to CmisAPI, cmis memory map, cmis consts. (Your previous review comment)
If we choose 1, we will need to put all memory password related logic to Cdb scope, including some duplicate logic such as implementing cdb.get_cmis_rev ; otherwise, we will need to put cmis_api to CdbFwHandler. I don't see other option. Please decide what to do.
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
6 participants
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.
Why I did it
CdbCmdHandler.enter_password()previously delivered the CMIS host password only via CDB command0001h. This is not the standard mechanism defined by the CMIS spec and is not honored by all modules — the standard, universally-supported way to unlock password-protected CDB/EEPROM access is to write the 4-byte host password (MSB first) to the Password Entry Area at page 00h bytes 122-125. As a result, password entry would fail on modules that only unlock via the Password Entry Area, blocking protected CDB/EEPROM operations (e.g. firmware/SED-protected access) on those modules.How I did it
cdb.py— Reworkedenter_password()to first write the password to the Password Entry Area (page 00h bytes 122-125, MSB first) usingwrite_raw, which is the standard method honored by all CMIS modules. If that write fails, it falls back to the original CDB command0001hpath for modules that rely on it. Input validation (integer in range0..0xFFFFFFFF) is preserved, andstructis used to pack the 32-bit password big-endian.cdb_consts.py— AddedCDB_HOST_PASSWORD_ENTRY_OFFSET = 122andCDB_HOST_PASSWORD_ENTRY_SIZE = 4with documentation describing the Password Entry Area register layout.test_cdb.py— Updated existing tests to assert the password is now written MSB-first to the Password Entry Area (and that no CDB command is sent when the register write succeeds), and addedtest_enter_password_fallback_to_cdb_commandto cover the fallback path when the register write fails.How to verify it
Ran the updated unit tests in
tests/sonic_xcvr/test_cdb.py, covering:test_enter_password_valid— password written MSB-first to the Password Entry Area, no CDB command issued.test_enter_password_default— default password (0x00001011) written to the Password Entry Area.test_enter_password_fallback_to_cdb_command— CDB command0001hused as fallback when the register write returnsFalse.