Skip to content
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

[Mellanox] Revert LPM implementation to the old way #17096

Merged
merged 10 commits into from
Nov 27, 2023

Conversation

keboliu
Copy link
Collaborator

@keboliu keboliu commented Nov 4, 2023

Why I did it

The current low power mode setting implementation requests the user to set the port to admin down first before toggling LP mode, this is not backward compatible, now revert it to the old way so that the user can toggle the LP mode regardless of the port admin status.

Work item tracking
  • Microsoft ADO (number only):

How I did it

Revert the recent changes related to LPM in PR #14130 and #16545

How to verify it

run all sfputil and SFP platform API related tests on all the Mellanox platforms.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211
  • 202305

Tested branch (Please provide the tested image version)

  • Verified on top of 202305 branch commit 733a902

Description for the changelog

Link to config_db schema for YANG module changes

A picture of a cute animal (not mandatory but encouraged)

@keboliu
Copy link
Collaborator Author

keboliu commented Nov 4, 2023

/azpw run ms_conflict

@mssonicbld
Copy link
Collaborator

/AzurePipelines run ms_conflict

Copy link

No pipelines are associated with this pull request.

@keboliu
Copy link
Collaborator Author

keboliu commented Nov 4, 2023

/azpw run Azure.sonic-buildimage

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-buildimage

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Kebo Liu <kebol@nvidia.com>
@keboliu keboliu marked this pull request as ready for review November 16, 2023 03:11
@keboliu keboliu changed the title [Mellanox] Revert SFP platform API set_lpmode and get_lpmode implementation [Mellanox] Revert LPM implementation to the old way Nov 16, 2023
@keboliu keboliu requested a review from prgeor November 16, 2023 03:14
@prgeor
Copy link
Contributor

prgeor commented Nov 17, 2023

@keboliu can you please elaborate more on the backward compatibility?

Copy link
Collaborator

@dgsudharsan dgsudharsan left a comment

Choose a reason for hiding this comment

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

Hi @keboliu . This reverts the fixes introduced in #16545. Can you please ensure that fix is not reverted?

…he dependency on sx_api importing

Signed-off-by: Kebo Liu <kebol@nvidia.com>
@keboliu
Copy link
Collaborator Author

keboliu commented Nov 17, 2023

@keboliu can you please elaborate more on the backward compatibility?

Hi @prgeor if you remember our current LPM implementation will require the user to explicitly shut down (admin down) the port first, this is not consistence with our original implementation - the user can toggle the LPM regardless of the port admin status.

now in this PR we change it back, allowing the user to toggle the LPM w/o shutting down the ports beforehand.

@keboliu
Copy link
Collaborator Author

keboliu commented Nov 17, 2023

Hi @keboliu . This reverts the fixes introduced in #16545. Can you please ensure that fix is not reverted?

issue fixed.

@liat-grozovik
Copy link
Collaborator

@prgeor please help to review this PR.
@keboliu please have the PR of 202205 related to this one as we need the fix there. but the reason for dedicated PR is because of no clean cherry pick

Signed-off-by: Kebo Liu <kebol@nvidia.com>
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.

@keboliu will be easy for future reference if you could include the Reverted PR# in "How I did it
"

Signed-off-by: Kebo Liu <kebol@nvidia.com>
@keboliu
Copy link
Collaborator Author

keboliu commented Nov 22, 2023

@keboliu will be easy for future reference if you could include the Reverted PR# in "How I did it "

updated.

@liat-grozovik liat-grozovik merged commit bf4a2e3 into sonic-net:master Nov 27, 2023
12 checks passed
@keboliu
Copy link
Collaborator Author

keboliu commented Nov 28, 2023

verified on top of 202305 commit 733a902
@StormLiangMS would you please help to cherry-pick?

@keboliu keboliu deleted the master_revert_lpm branch November 28, 2023 03:09
mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Nov 30, 2023
- Why I did it
The current low power mode setting implementation requests the user to set the port to admin down first before toggling LP mode, this is not backward compatible, now revert it to the old way so that the user can toggle the LP mode regardless of the port admin status.

- How I did it
Revert the recent changes related to LPM in PR sonic-net#14130 and sonic-net#16545

- How to verify it
Run all sfputil and SFP platform API related tests on all the Mellanox platforms.

Signed-off-by: Kebo Liu <kebol@nvidia.com>
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202305: #17366

yxieca pushed a commit that referenced this pull request Dec 4, 2023
- Why I did it
The current low power mode setting implementation requests the user to set the port to admin down first before toggling LP mode, this is not backward compatible, now revert it to the old way so that the user can toggle the LP mode regardless of the port admin status.

- How I did it
Revert the recent changes related to LPM in PR #14130 and #16545

- How to verify it
Run all sfputil and SFP platform API related tests on all the Mellanox platforms.

Signed-off-by: Kebo Liu <kebol@nvidia.com>
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