Skip to content

[sfputil debug] Fix issue: do not check output status when CMIS version is lower than 5.0#3938

Merged
prgeor merged 1 commit intosonic-net:masterfrom
Junchao-Mellanox:master-tx-rx-output
Aug 1, 2025
Merged

[sfputil debug] Fix issue: do not check output status when CMIS version is lower than 5.0#3938
prgeor merged 1 commit intosonic-net:masterfrom
Junchao-Mellanox:master-tx-rx-output

Conversation

@Junchao-Mellanox
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox commented Jun 27, 2025

What I did

OutputStatusTx and OutputStatusRx are only supported since CMIS 5.0. sfputil debug tx-output and sfputil debug rx-output should check them only if CMIS rev is higher or equal to 5.0.

How I did it

  1. Use get_tx_disable and get_rx_disable to check if the corresponding bits have been set by sfputil debug command
  2. Check the CMIS rev, ignore checking of OutputStatusTx and OutputStatusRx if CMIS rev is below 5.0.

How to verify it

  1. Manual test
  2. unit test

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox Junchao-Mellanox marked this pull request as draft June 27, 2025 02:16
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Collaborator Author

/azpw run sonic-utilities

@mssonicbld
Copy link
Collaborator

/AzurePipelines run sonic-utilities

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@qiluo-msft
Copy link
Contributor

/azp run Azure.sonic-utilities

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prgeor prgeor requested a review from mihirpat1 July 2, 2025 17:10
@prgeor
Copy link
Contributor

prgeor commented Jul 2, 2025

@Junchao-Mellanox draft?

@Junchao-Mellanox Junchao-Mellanox marked this pull request as ready for review July 10, 2025 01:53
@Junchao-Mellanox
Copy link
Collaborator Author

Hi @prgeor , @mihirpat1 , could you please help review it? thanks!

Copy link
Contributor

@prgeor prgeor left a comment

Choose a reason for hiding this comment

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

@Junchao-Mellanox am I reading the wrong spec?
txdisable

Copy link
Contributor

@prgeor prgeor left a comment

Choose a reason for hiding this comment

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

seems we don't need this change.

@Junchao-Mellanox
Copy link
Collaborator Author

Junchao-Mellanox commented Jul 13, 2025

Hi @prgeor , the PR is needed, we cannot check the tx/rx output status according to CMIS 4.2 spec:
image
image

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prgeor prgeor merged commit d86b2b6 into sonic-net:master Aug 1, 2025
7 checks passed
@Junchao-Mellanox Junchao-Mellanox deleted the master-tx-rx-output branch August 1, 2025 12:29
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to msft-202412: Azure/sonic-utilities.msft#212

@Junchao-Mellanox
Copy link
Collaborator Author

For the 202505 cherry-pick conflict, need take #3911 first, then there will be no conflict. I have added label 202505 to PR #3911. @prgeor @mihirpat1 for awareness.

@yejianquan
Copy link
Contributor

@yejianquan
Copy link
Contributor

Hi @Junchao-Mellanox , I've synced up with @prgeor , this PR is not suggested to be included 202505 branch,
hence remove the request label for now, please sync up with Prince, describe business requirement if this PR is still needed of 202505 branch.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants