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

Collect module EEPROM data in dump #3009

Merged
merged 16 commits into from
Dec 20, 2023

Conversation

Junchao-Mellanox
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox commented Oct 10, 2023

What I did

Add a new CLI sfputil eeprom-hexdump-all to dump all pages for module
Support collecting module EEPROM in show techsupport command

How I did it

HLD: sonic-net/SONiC#1476

How to verify it

Manual test
Unit test
Regression 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)

EEPROM hexdump for module 1
        Lower page 0h
        00000000 11 08 06 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000010 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000020 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000030 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000040 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000050 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000060 00 00 00 00 00 00 00 00  00 00 00 00 00 01 08 00 |................|
        00000070 00 10 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        Upper page 0h
        00000000 11 08 06 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000010 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000020 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000030 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000040 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000050 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000060 00 00 00 00 00 00 00 00  00 00 00 00 00 01 08 00 |................|
        00000070 00 10 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        Page 1h
        00000000 11 08 06 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000010 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000020 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000030 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000040 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000050 00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
        00000060 00 00 00 00 00 00 00 00  00 00 00 00 00 01 08 00 |................|
        00000070 00 10 00 00 00 00 00 00  00 00 00 00 00 00 00 00 |................|
...
EEPROM hexdump for module 2
...

@prgeor
Copy link
Contributor

prgeor commented Oct 10, 2023

@Junchao-Mellanox
Copy link
Collaborator Author

Hi @prgeor , I checked command reference doc, it mentioned:

  3. sonic-clear, sfputil, etc., This document does not explain these scripts also.

So, it seems that we don't add doc for sfputil.

@prgeor
Copy link
Contributor

prgeor commented Oct 13, 2023

Hi @prgeor , I checked command reference doc, it mentioned:

  3. sonic-clear, sfputil, etc., This document does not explain these scripts also.

So, it seems that we don't add doc for sfputil.

@Junchao-Mellanox we can remove that statement. Recently we added sfputil firmware download command to this doc. It will be good to capture all CLIs in this Repo in one doc

sfputil/main.py Outdated Show resolved Hide resolved
sfputil/main.py Outdated Show resolved Hide resolved
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 I would suggest to enhance existing APIs: eeprom_hexdump_sff8472() to eeprom_hexdump_sff8472(port, physical_port, pages). Let the caller take care of what pages or list of pages to pass based upon if the cable support only flat memory, if cable is CMIS, SFF etc.. This way we can reuse existing API. You can take approach of is_xcvr_optical() (see sonic-mgmt repo) to know if SFP+ is copper or not.

sfputil/main.py Outdated Show resolved Hide resolved
sfputil/main.py Outdated Show resolved Hide resolved
sfputil/main.py Outdated Show resolved Hide resolved
sfputil/main.py Outdated Show resolved Hide resolved
@Junchao-Mellanox
Copy link
Collaborator Author

Junchao-Mellanox commented Nov 9, 2023

@Junchao-Mellanox I would suggest to enhance existing APIs: eeprom_hexdump_sff8472() to eeprom_hexdump_sff8472(port, physical_port, pages). Let the caller take care of what pages or list of pages to pass based upon if the cable support only flat memory, if cable is CMIS, SFF etc.. This way we can reuse existing API. You can take approach of is_xcvr_optical() (see sonic-mgmt repo) to know if SFP+ is copper or not.

Hi @prgeor ,
I was thinking of reusing eeprom_hexdump_sff8472. But I found it is very difficult. eeprom_hexdump_sff8472 is designed for single port:

  1. There are many code like sys.exit(ERROR_NOT_IMPLEMENTED). It would exit the CLI if any port got unexpected result. This is not expected for multi ports dump.
  2. It dumps EEPROM for logical port like Ethernet0, Ethernet4. This is OK for single port, but not so good for multi port dump. For multi port dump, I choose to dump eeprom for physical port so that split port won't be dump many times.

I understand your point is to reuse existing code and keep the design clean. I totally agree with that. So, how about this:
I will totally replace eeprom_hexdump_sff8472 with dump_eeprom_pages_sff8472 in a future PR. I don't want to do it in current PR because:

  1. I don't want to take the risk to break existing function (to change the logic for single port).
  2. It would be a major change to replace it
  3. single port dump is not scope of my feature

@liat-grozovik
Copy link
Collaborator

@prgeor could you please help to review feedback changes?
this is needed for 202311

sfputil/main.py Outdated Show resolved Hide resolved
sfputil/main.py Outdated Show resolved Hide resolved
sfputil/main.py Outdated Show resolved Hide resolved
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.

@mihirpat1 could you also please review?

prgeor
prgeor previously approved these changes Dec 8, 2023
@prgeor prgeor requested a review from mihirpat1 December 8, 2023 05:58
@keboliu
Copy link
Collaborator

keboliu commented Dec 19, 2023

@prgeor @mihirpat1 could you please sign-off?

mihirpat1
mihirpat1 previously approved these changes Dec 19, 2023
@prgeor prgeor merged commit a68d3d3 into sonic-net:master Dec 20, 2023
5 checks passed
@prgeor
Copy link
Contributor

prgeor commented Dec 20, 2023

@Junchao-Mellanox please check cherry-pick conflict

@Junchao-Mellanox Junchao-Mellanox deleted the master_debug_tool branch December 20, 2023 04:53
Junchao-Mellanox added a commit to Junchao-Mellanox/sonic-utilities that referenced this pull request Jan 16, 2024
Junchao-Mellanox added a commit to Junchao-Mellanox/sonic-utilities that referenced this pull request Jan 18, 2024
Junchao-Mellanox added a commit to Junchao-Mellanox/sonic-utilities that referenced this pull request Jan 18, 2024
yxieca pushed a commit that referenced this pull request Jan 18, 2024
yuazhe added a commit to yuazhe/sonic-utilities that referenced this pull request Feb 1, 2024
sonic-net#3009 breaks the number
base rule, treats the page number CLI input as decimal rather than hex,
this is used to fix it.

Signed-off-by: Yuanzhe, Liu <[email protected]>
yuazhe added a commit to yuazhe/sonic-utilities that referenced this pull request Feb 1, 2024
sonic-net#3009 breaks the number
base rule, treats the page number CLI input as decimal rather than hex,
this is used to fix it.

Signed-off-by: Yuanzhe, Liu <[email protected]>
yuazhe added a commit to yuazhe/sonic-utilities that referenced this pull request Feb 1, 2024
sonic-net#3009 breaks the number
base rule, treats the page number CLI input as decimal rather than hex,
this is used to fix it.

Signed-off-by: Yuanzhe, Liu <[email protected]>
yuazhe added a commit to yuazhe/sonic-utilities that referenced this pull request Feb 1, 2024
sonic-net#3009 breaks the number
base rule, treats the page number CLI input as decimal rather than hex,
this is used to fix it.

it also containes a redundancy function definition, so remove it.

Signed-off-by: Yuanzhe, Liu <[email protected]>
yuazhe added a commit to yuazhe/sonic-utilities that referenced this pull request Feb 1, 2024
sonic-net#3009 breaks the number
base rule, treats the page number CLI input as decimal rather than hex,
this is used to fix it.

it also containes a redundancy function definition, so remove it.

Signed-off-by: Yuanzhe, Liu <[email protected]>
yuazhe added a commit to yuazhe/sonic-utilities that referenced this pull request Feb 2, 2024
sonic-net#3009 breaks the number
base rule, treats the page number CLI input as decimal rather than hex,
this is used to fix it.

it also containes a redundancy function definition, so remove it.

Signed-off-by: Yuanzhe, Liu <[email protected]>
yuazhe added a commit to yuazhe/sonic-utilities that referenced this pull request Feb 2, 2024
sonic-net#3009 breaks the number
base rule, treats the page number CLI input as decimal rather than hex,
this is used to fix it.

it also containes a redundancy function definition, so remove it.

Signed-off-by: Yuanzhe, Liu <[email protected]>
yuazhe added a commit to yuazhe/sonic-utilities that referenced this pull request Feb 2, 2024
sonic-net#3009 breaks the number
base rule, treats the page number CLI input as decimal rather than hex,
this is used to fix it.

it also containes a redundancy function definition, so remove it.

Signed-off-by: Yuanzhe, Liu <[email protected]>
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