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

[sfpshow/sfputil] Enhance sfpshow and sfputil to behavior correctly on RJ45 ports #2111

Merged
merged 18 commits into from
Jun 24, 2022

Conversation

keboliu
Copy link
Collaborator

@keboliu keboliu commented Mar 22, 2022

What I did

For RJ45 port, sfpshow and sfputil need to be enhanced to behavior correctly:

  1. "show interface transceiver eeprom" and "sfputil show eeprom" will return "SFP EEPROM is not applicable for RJ45 port"
admin@sonic:~$ show interface transceiver eeprom Ethernet0
Ethernet0: SFP EEPROM is not applicable for RJ45 port

admin@sonic:~$ sudo sfputil show eeprom -p Ethernet0
Ethernet0: SFP EEPROM is not applicable for RJ45 port
  1. "show interface transceiver error-status" will return with 'N/A'
admin@sonic:~$ show interface transceiver error-status Ethernet0
Port       Error Status
---------  --------------
Ethernet0  N/A
  1. "show interface transceiver lpmode" and "sfputil show lpmode" will return with 'N/A'
admin@sonic:~$ show interface transceiver lpmode Ethernet0
Port       Low-power Mode
---------  ----------------
Ethernet0  N/A

admin@sonic:~$ sudo sfputil show lpmode -p Ethernet0
Port       Low-power Mode
---------  ----------------
Ethernet0  N/A
  1. "sfputil lpmode set" will be rejected with error msg "{Disabling|Enabling} low-power mode is not applicable for RJ45 port Ethernet{x}"
admin@sonic:~$ sudo sfputil lpmode on Ethernet0
Enabling low-power mode is not applicable for RJ45 port Ethernet0.
admin@sonic:~$ sudo sfputil lpmode off Ethernet0
Disabling low-power mode is not applicable for RJ45 port Ethernet0.
  1. "sfputil reset" will be rejected with error msg: "Reset is not applicable for RJ45 port Ethernet{x}"
admin@sonic:~$ sudo sfputil reset Ethernet0
Reset is not applicable for RJ45 port Ethernet0.

How I did it

check the port type, if it's RJ45 ports then handle it seperately.

How to verify it

run above command on platform with RJ45 ports

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)

keboliu and others added 11 commits March 22, 2022 19:47
Signed-off-by: Kebo Liu <kebol@nvidia.com>
Signed-off-by: Kebo Liu <kebol@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Stephen Sun <stephens@nvidia.com>
Signed-off-by: Kebo Liu <kebol@nvidia.com>
@keboliu keboliu requested review from prgeor and stephenxs and removed request for stephenxs March 22, 2022 12:39
@lgtm-com
Copy link

lgtm-com bot commented Mar 22, 2022

This pull request introduces 1 alert when merging cc540ed into 61b1396 - view on LGTM.com

new alerts:

  • 1 for Unused named argument in formatting call

@stephenxs
Copy link
Collaborator

The building errors are caused by dependent mock dbs in #2110 has not yet been merged.

Signed-off-by: Kebo Liu <kebol@nvidia.com>
scripts/sfpshow Show resolved Hide resolved
@keboliu
Copy link
Collaborator Author

keboliu commented Apr 19, 2022

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scripts/sfpshow Outdated Show resolved Hide resolved
sfputil/main.py Outdated Show resolved Hide resolved
@stephenxs
Copy link
Collaborator

stephenxs commented Apr 19, 2022 via email

@stephenxs
Copy link
Collaborator

stephenxs commented Apr 19, 2022 via email

@stephenxs
Copy link
Collaborator

stephenxs commented Apr 19, 2022

@prgeor
The solution is like this:

  1. when the system starts, platform API reports present for all RJ45 ports
  2. Xcvrd works as usual, fetch all the info for all the RJ45 ports and push them into TRANSCEIVER_INFO table
  3. In case RJ45 ports enter not present state, an error status will be reported by platform API. This is not an error that blocks reading eeprom. So xcvrd will not remove port from TRANSCEIVER_INFO table
  4. Platform API will never report remove to xcvrd, meaning RJ45 ports will never be removed from transceiver info
  5. Cli needs to be updated, for RJ45 ports, if there is an error in TRANSCEIVER_STATUS table, it should display not present or unknown. Same as other apps that will represent the status

By doing so,

  • Only CLI needs to be updated. xcvrd will not be touched.
  • No new table is required
  • It's possible to support unknown status for RJ45 ports

@prgeor prgeor self-assigned this Apr 19, 2022
@liat-grozovik
Copy link
Collaborator

@prgeor any additional points to discuss in order to signoff this PR?

…port (#17)

* Revert the logic to fetch presence status from error status

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Unit test

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Fix error

Signed-off-by: Stephen Sun <stephens@nvidia.com>
@stephenxs
Copy link
Collaborator

@prgeor For now, we will follow the current solution to fetch the port type of RJ45 - reading it from TRANSCEIVER_INFO table.
Can you check and approve it?

Signed-off-by: Stephen Sun <stephens@nvidia.com>
@stephenxs
Copy link
Collaborator

@prgeor The solution is like this:

  1. when the system starts, platform API reports present for all RJ45 ports
  2. Xcvrd works as usual, fetch all the info for all the RJ45 ports and push them into TRANSCEIVER_INFO table
  3. In case RJ45 ports enter not present state, an error status will be reported by platform API. This is not an error that blocks reading eeprom. So xcvrd will not remove port from TRANSCEIVER_INFO table
  4. Platform API will never report remove to xcvrd, meaning RJ45 ports will never be removed from transceiver info
  5. Cli needs to be updated, for RJ45 ports, if there is an error in TRANSCEIVER_STATUS table, it should display not present or unknown. Same as other apps that will represent the status

By doing so,

  • Only CLI needs to be updated. xcvrd will not be touched.
  • No new table is required
  • It's possible to support unknown status for RJ45 ports

This is not available anymore. it's the previous impelmetnation.

Comment on lines +295 to +300
def is_rj45_port_from_db(port_name, db):
intf_type = db.get(db.STATE_DB, 'TRANSCEIVER_INFO|{}'.format(port_name), 'type')
return intf_type == RJ45_PORT_TYPE


def is_rj45_port_from_api(port_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

@keboliu can put comments in the code why we need two different functions for getting the port type? Also when should one use each of them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@prgeor please check the latest commits for the comments on the usage of the functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

@keboliu as we discussed, lets have sfptuil always fetch the values by reading the eeprom and sfpshow utility use the DB to read the values. Hope this will be scoped and fixed in future PR.

@keboliu keboliu force-pushed the sfpshow-sfputil-support-rj45 branch from e8eacb3 to 09a891a Compare June 22, 2022 05:39
Signed-off-by: Kebo Liu <kebol@nvidia.com>
Signed-off-by: Kebo Liu <kebol@nvidia.com>
@prgeor prgeor merged commit f64d280 into sonic-net:master Jun 24, 2022
yxieca pushed a commit that referenced this pull request Jul 28, 2022
…n RJ45 ports (#2111)

* enhance show interface transceiver eeprom logic with RJ45 port support

Signed-off-by: Kebo Liu <kebol@nvidia.com>

* enhance sfputil to support RJ45 port, exclude error status

* fix sfputil issue on RJ45 port

Signed-off-by: Kebo Liu <kebol@nvidia.com>

* [RJ45] change the way to judge port type and add more UT test case

Signed-off-by: Kebo Liu <kebol@nvidia.com>

* [sfputil] simplity the logic for RJ45 support

Signed-off-by: Kebo Liu <kebol@nvidia.com>

* Support sfputil show present

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Support rj45 in sfpshow

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Add test case for sfputil with RJ45 supported

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Add mock data for RJ45 ports into STATE_DB

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Add test for sfputil show for RJ45 ports

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* remove debug code in sfputil test case

Signed-off-by: Kebo Liu <kebol@nvidia.com>

* remove unnecessary argument for format()

Signed-off-by: Kebo Liu <kebol@nvidia.com>

* Revert the logic to fetch presence status from error status for RJ45 port (#17)

* Revert the logic to fetch presence status from error status

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Unit test

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Fix error

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* Add test cases to cover lpmode and error status

Signed-off-by: Stephen Sun <stephens@nvidia.com>

* add comments to describe the usage of functions to judge the port type

Signed-off-by: Kebo Liu <kebol@nvidia.com>

* add more testcase for sfputil

Signed-off-by: Kebo Liu <kebol@nvidia.com>

* fix typo in testcase name

Signed-off-by: Kebo Liu <kebol@nvidia.com>

Co-authored-by: Stephen Sun <stephens@nvidia.com>
Co-authored-by: Stephen Sun <5379172+stephenxs@users.noreply.github.com>
@keboliu keboliu deleted the sfpshow-sfputil-support-rj45 branch October 28, 2023 03:26
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.

6 participants