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] Use sdk sysfs instead of ethtool #12480

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

Junchao-Mellanox
Copy link
Collaborator

Why I did it

Currently, implementation:

  • Reading EEPROM: via ethtool
  • Writing EEPROM: via mlxreg tool.

The problem is that spawning new process of ethtool/mlxreg is very slow. To optimize the performance, this PR will use SDK provided sys fs for EEPROM reading/writing.

How I did it

Replace the logic of sfp.read_eeprom and sfp.write_eeprom by SDK sys fs.

How to verify it

Manual test
Unit test cases (new test case added)
sonic-mgmt regression

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

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

Description for the changelog

Ensure to add label/tag for the feature raised. example - PR#2174 where, Generic Config and Update feature has been labelled as GCU.

Link to config_db schema for YANG module changes

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

@liat-grozovik
Copy link
Collaborator

@Junchao-Mellanox now that PR #12542 is merged, should this PR be ready for review?

@Junchao-Mellanox Junchao-Mellanox marked this pull request as ready for review October 31, 2022 01:50
@prgeor
Copy link
Contributor

prgeor commented Nov 2, 2022

@Junchao-Mellanox do you mind testing your changes with sonic-net/sonic-utilities#2379 ?

@Junchao-Mellanox
Copy link
Collaborator Author

@Junchao-Mellanox do you mind testing your changes with sonic-net/sonic-utilities#2379 ?

Sure

@Junchao-Mellanox
Copy link
Collaborator Author

Hi @prgeor , I ran some tests based on sonic-net/sonic-utilities#2379. I did not find issue on the current bug, but I found sonic-net/sonic-utilities#2379 itself contains some issues:

  1. This line is a confusing, https://github.com/sonic-net/sonic-utilities/blob/569edf3b75840b1db844c5a4797679babc412c4e/sfputil/main.py#L738. Better change name eeprom_hexdump_sff8636 to something else because it is not only for sff8636.
        if id[0] == 0x3:
            output = eeprom_hexdump_sff8472(port, physical_port, page)
        else:
            output = eeprom_hexdump_sff8636(port, physical_port, page)
  1. A2H is not a mandatory for sff8472, but the command line output nothing if A2H is not available:
        page_dump = platform_chassis.get_sfp(physical_port).read_eeprom(SFF8472_A0_SIZE, PAGE_SIZE)
        if page_dump is None:
            click.echo("Error: Failed to read EEPROM for A2h!")
            sys.exit(ERROR_NOT_IMPLEMENTED)

@prgeor prgeor merged commit 830b7d8 into sonic-net:master Nov 3, 2022
Junchao-Mellanox added a commit to Junchao-Mellanox/sonic-buildimage that referenced this pull request Nov 4, 2022
Conflicts:
	platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py
@Junchao-Mellanox
Copy link
Collaborator Author

No clean cherry-pick, will create PR to 202205

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.

5 participants