Skip to content

[cmis] Fix enter_password to include standard password entry method - #724

Open
Junchao-Mellanox wants to merge 1 commit into
sonic-net:masterfrom
Junchao-Mellanox:public/pr-19
Open

[cmis] Fix enter_password to include standard password entry method#724
Junchao-Mellanox wants to merge 1 commit into
sonic-net:masterfrom
Junchao-Mellanox:public/pr-19

Conversation

@Junchao-Mellanox

@Junchao-Mellanox Junchao-Mellanox commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Why I did it

CMIS defines two ways to deliver the host password that unlocks protected CDB/EEPROM access:

  1. CDB command 0001h (CdbCmdHandler.enter_password()), and
  2. writing the 4-byte password MSB-first to the Password Entry Area at page 00h bytes 122-125.

CDB command 0001h is not honored by all CMIS modules. On modules that unlock only via the Password Entry Area, password entry silently fails, which blocks every password-protected CDB/EEPROM operation — firmware download/run/commit and SED-protected access among them. This change makes password entry work across both module families without changing behavior for modules that already unlock via CDB.

How I did it

  • cdb.py — Reworked enter_password() to first write the password to the Password Entry Area (page 00h bytes - api/public/cmis.py — New CmisApi.enter_password_via_memory(password), which writes the password to the Password Entry Area and then interprets the result:
    • _supports_password_cmd_result() gates on CMIS revision — PasswordCmdResult (00h:42.3-0) was introduced in CMIS 5.3, so on earlier modules (or when the revision can't be parsed) those bits are reserved and are never interpreted.
    • _read_password_cmd_result() polls PasswordCmdResult past the transient "validation in progress" / unreadable states, bounded by a 1000 ms timeout at 20 ms intervals (per CMIS 8.2.14 the module updates the result within tWRITE).
    • Verdict handling: HOST_ACCEPTED/MODULE_ACCEPTED → True; NOT_ACCEPTED → False; pre-5.3, NOT_SUPPORTED, or undetermined → best-effort True on a successful write.
  • api/public/cdb_fw.py — New CmisCdbFw._enter_password() becomes the single entry point for all password entry. It validates the password (int in 0..0xFFFFFFFF), tries CDB 0001h first because that path is synchronous (send_cmd waits for and checks CDB status), and falls back to the Password Entry Area write on failure — or goes straight to it when no CDB handler is available. All existing call sites (get_module_fw_info, FW run/commit/download-start password retries, and cdb_enter_host_password) now go through it.
  • cdb/cdb.py — CdbCmdHandler.enter_password() keeps its original CDB-only behavior; only the docstring is updated to state that the fallback is the caller's responsibility.
  • fields/consts.py — Added PASSWORD_ENTRY (offset 122, size 4), PASSWORD_CMD_RESULT, the PASSWORD_RESULT_* code constants, PASSWORD_RESULT_MIN_CMIS_REV = (5, 3), and the poll interval/timeout constants, with comments citing CMIS 8.2.14.
  • mem_maps/.../page00_lower.py — Registered the PasswordEntry field (NumberRegField, format=">I", writable — the format packs MSB-first) and the 4-bit PasswordCmdResult field at 00h:42.

How to verify it

Unit tests in tests/sonic_xcvr/test_cmis.py and tests/sonic_xcvr/test_cdb.py:

  • _supports_password_cmd_result gating across CMIS 4.0 / 5.2 / 5.3 / 5.4 / 6.0 and unparseable revisions.
  • _read_password_cmd_result polling past IN_PROGRESS and None reads, plus the timeout case.
  • enter_password_via_memory: write failure, pre-5.3 best-effort (no register read), all four PasswordCmdResult codes, and the undetermined-result case.
  • _enter_password: CDB success short-circuits the fallback; fallback is invoked and its result propagated on CDB failure; no-CDB-handler path; invalid passwords ("not_an_int", -1, 0x100000000, 12.5, None) rejected without touching either path.
  • CdbCmdHandler.enter_password retains coverage for CDB failure propagation and invalid-input rejection without sending a command.

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@Junchao-Mellanox

Copy link
Copy Markdown
Contributor Author

Hi @prgeor , @mihirpat1 , could you please help review this? Thanks!

stephenxs
stephenxs previously approved these changes Jul 21, 2026
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb.py Outdated
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb.py Outdated
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@bgallagher-nexthop bgallagher-nexthop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

Comment thread sonic_platform_base/sonic_xcvr/fields/cdb_consts.py Outdated
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

keboliu
keboliu previously approved these changes Jul 28, 2026
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb.py Outdated
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb.py Outdated
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb.py Outdated
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb.py Outdated
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb.py Outdated
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@Junchao-Mellanox
Junchao-Mellanox requested a review from prgeor July 30, 2026 03:13
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb_fw.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 extends the SONiC CMIS transceiver support to enter the host password using the CMIS-standard Password Entry Area (page 00h bytes 122–125, MSB-first) and to optionally confirm acceptance via the PasswordCmdResult register (CMIS 5.3+). It also wires this capability into the CDB handler path and updates unit tests accordingly.

Changes:

  • Added CMIS-side password entry via Password Entry Area with optional polling of PasswordCmdResult (CMIS 5.3+).
  • Extended the CMIS page00 lower memory map and field constants to expose Password Entry Area and PasswordCmdResult.
  • Updated CDB/CDB-FW handler construction and tests to support delegating password entry to CMIS as a fallback path.

Reviewed changes

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

Show a summary per file
File Description
tests/sonic_xcvr/test_cmis.py Adds unit tests for CMIS password-entry support checks and polling behavior.
tests/sonic_xcvr/test_cdb.py Updates CDB handler tests to include CMIS fallback behavior and weakref semantics.
tests/sonic_xcvr/test_cdb_fw.py Updates firmware handler tests for the new handler constructor signature.
sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page00_lower.py Adds CMIS page00 mappings for Password Entry Area and PasswordCmdResult.
sonic_platform_base/sonic_xcvr/fields/consts.py Introduces constants for password entry/result registers and polling parameters.
sonic_platform_base/sonic_xcvr/cdb/cdb.py Adds CMIS API weakref and password-entry fallback wiring in CdbCmdHandler.
sonic_platform_base/sonic_xcvr/cdb/cdb_fw.py Updates firmware handler to pass through CMIS API reference.
sonic_platform_base/sonic_xcvr/api/public/cmis.py Implements Password Entry Area write + PasswordCmdResult polling and CMIS revision gating.
sonic_platform_base/sonic_xcvr/api/public/cdb_fw.py Passes cmis_api=self into the CDB firmware handler factory.

Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb.py Outdated
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb_fw.py Outdated
Comment thread sonic_platform_base/sonic_xcvr/api/public/cmis.py
Comment thread sonic_platform_base/sonic_xcvr/cdb/cdb.py Outdated
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

`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.

- `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.

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>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants