Skip to content

[layer1] CDB Commands with Advertised MaxDuration#2343

Open
pnakka28 wants to merge 2 commits into
sonic-net:masterfrom
pnakka28:max_duration_timeout
Open

[layer1] CDB Commands with Advertised MaxDuration#2343
pnakka28 wants to merge 2 commits into
sonic-net:masterfrom
pnakka28:max_duration_timeout

Conversation

@pnakka28
Copy link
Copy Markdown

Overview

This PR document proposes to use module advertised MaxDuration timeouts for CDB firmware commands according to the CMIS spec. The current CDB firmware upgrade path applies a fixed timeout to all commands. The CMIS spec defines MaxDuration fields for supported CDB commands that advertise how much time a module actually needs to execute each command.

Motivation

  • Documents timeout behavior in one place so it is easy to find during debugging.
  • Ensures CMIS compliance by respecting module advertised timing.

What this PR covers

  • Which CDB commands should use the module advertised MaxDuration as their timeout.
  • Which CDB commands retain fixed default timeouts (commands without a CMIS-defined MaxDuration field).
  • How advertised values are queried, parsed, and applied.
  • What changes are needed in sonic-platform-common.

MSFT ADO - 38035182

Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>
@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

No pipelines are associated with this pull request.

Comment thread doc/layer1/cmis-firmware-upgrade/cmis_max_duration.md Outdated

The following table lists all CDB commands involved in the firmware upgrade process along with proposed timeout behavior:

#### Table 3: Upgrade Workflow CDB Commands
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.

@pnakka28 please highlight what is the default timeout value used in sonic

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@prgeor addressed this to highlight default timeout


### Functional Requirements

1. For CDB commands where the CMIS defines a MaxDuration field, use the module advertised value as the command timeout.
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.

Should we introduce some tolerance other than the "MaxDuration" value?

1. **Parse MaxDuration in `initFwHandler()`** — Extend the existing CDB 0041h response parsing to extract MaxDuration fields (bytes 144–153), read the M multiplier from bit 137.3, multiply the U16 values by M, and store the resulting ms values as instance attributes.

2. **Pass timeouts to `send_cmd()`** — Update the following methods to pass their respective MaxDuration value:
- `start_fw_download()` -> `timeout_start`
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.

Again, I assume we need to allow some tolerance other than the MaxDuration. start_fw_download does not mean that module has received the CDB command. There are latency from sonic to module, and there are latency if I2C is busy.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@Junchao-Mellanox Yes, we need some tolerance. I updated the doc to reflect that.

Signed-off-by: Pavan Kalyan Nakka <pnakka@microsoft.com>
@mssonicbld
Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines
Copy link
Copy Markdown

No pipelines are associated with this pull request.

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.

4 participants