Skip to content

[device/celestica]: Add support for xcvrd daemon in silverstone#91

Open
mudsut4ke wants to merge 13 commits intozhenggen-xu:201811-TH3from
mudsut4ke:201811-silverstone-sfputil-xcvrd
Open

[device/celestica]: Add support for xcvrd daemon in silverstone#91
mudsut4ke wants to merge 13 commits intozhenggen-xu:201811-TH3from
mudsut4ke:201811-silverstone-sfputil-xcvrd

Conversation

@mudsut4ke
Copy link
Copy Markdown

@mudsut4ke mudsut4ke commented Oct 28, 2020

- What I did

  • Add function to sfputil support xcvrd daemon

- How I did it

  • Add get_transceiver_change_event function to support xcvrd daemon
  • Added a workaround command to make the link go up.

- How to verify it

@stevenlu99
Copy link
Copy Markdown

stevenlu99 commented Oct 28, 2020

at function _bring_up_port_link
It seems does not initialize CMIS-4.0 init sequence, instead it only put port in loopback. this will link up but would not able to pass traffic.

Also, it does not change back to page-0 at end of function

@mudsut4ke
Copy link
Copy Markdown
Author

at function _bring_up_port_link
It seems does not initialize CMIS-4.0 init sequence, instead it only put port in loopback. this will link up but would not able to pass traffic.

Also, it does not change back to page-0 at end of function

@stevenlu99 , Please review a new init script in 9f33f91

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

why we enabled all ports by default? This should be controlled by configDB data.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I will remove it (this one should added on our debug image only)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This comment should be removed.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We need make this script works for multiple optical vendors mentioned in email.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

cmis4_init.sh

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This HWSKU should use 1x400G?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

change the name of port_config_file_path to cmis_init_file_path?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is this seconds or miliseconds, please make a comment.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

updated

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

timeout is milliseconds, what is POLL_INTERVAL unit? If it is seconds, this min() parameters do not seem to be correct.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This should be renamed to something related to initializing the optical modules.

@mudsut4ke mudsut4ke force-pushed the 201811-silverstone-sfputil-xcvrd branch from 37b59e1 to 8f53880 Compare October 30, 2020 07:02
@mudsut4ke mudsut4ke force-pushed the 201811-silverstone-sfputil-xcvrd branch from 8f53880 to 8400496 Compare October 30, 2020 07:39
@mudsut4ke mudsut4ke force-pushed the 201811-silverstone-sfputil-xcvrd branch from 8626456 to 1c4e42f Compare October 30, 2020 08:01
@stevenlu99
Copy link
Copy Markdown

@mudsut4ke if you use cmis_init.sh to initialize cmis-4.0 transceivers to 4x100G mode, please keep in mind that this script was originally programmed to demonstrate cmis-4.0 init sequence for Innolight, for different vendors, the sequences might have differences.
Another 2 cents:

  1. please be careful with sleep(s) in the script. it need fine tuned
  2. How do you plan to check cmis-4.0 init was successes? In case of init failed, do you plan add retry?

@mudsut4ke
Copy link
Copy Markdown
Author

@stevenlu99 Thanks for the feedback.
I will try to add another command to check port status and add retry for failed case.

@mudsut4ke mudsut4ke marked this pull request as ready for review November 5, 2020 03:36
@zhenggen-xu
Copy link
Copy Markdown
Owner

Please include the test results for at least Innolight and Eoptolink optics, and the ports should come up with 4x100G mode.

print "Error: unable to read file: %s" % str(e)
return None

def _is_port_device_present(self, port_idx):
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

make this function return true or false, not a number that could be anything.

@mudsut4ke
Copy link
Copy Markdown
Author

@stevenlu99 , @zhenggen-xu

I have updated code follow your comment and I also attached log to PR description

@zhenggen-xu
Copy link
Copy Markdown
Owner

We mentioned many times in email, we are looking for 4x100G mode of the optics (Both Innolight and Eoptolink), I don't see that in the log, it seems to be a wrong HWSKU and wrong optical modes.

@shivanangi
Copy link
Copy Markdown

Hello Zhenggen,

I see a parallel PR #92 running. Do you still need the qualification on Innodisk and Eptolink?

Thanks.

@mudsut4ke
Copy link
Copy Markdown
Author

mudsut4ke commented Nov 25, 2020

We mentioned many times in email, we are looking for 4x100G mode of the optics (Both Innolight and Eoptolink), I don't see that in the log, it seems to be a wrong HWSKU and wrong optical modes.

@zhenggen-xu , We encountered an error (port link down) in 4x100G mode test with 1x400G port connected to 4x100G port and our team tried to figure out the root cause of this link down issue.

However, does the test result you want use the same port link topology as above?

And I also have one more question about #92,
Do I need to rebase sfputil.py code to use that modified version from Broadcom ?

@zhenggen-xu
Copy link
Copy Markdown
Owner

We mentioned many times in email, we are looking for 4x100G mode of the optics (Both Innolight and Eoptolink), I don't see that in the log, it seems to be a wrong HWSKU and wrong optical modes.

@zhenggen-xu , We encountered an error (port link down) in 4x100G mode test with 1x400G port connected to 4x100G port and our team tried to figure out the root cause of this link down issue.

However, does the test result you want use the same port link topology as above?

And I also have one more question about #92,
Do I need to rebase sfputil.py code to use that modified version from Broadcom ?

I suggest you work with BRCM to figure out the solution going forward. As for the use case, we wanted 4x100G mode, and at least Innolight and Eoptolink, both with CMIS 4.0.

zhenggen-xu pushed a commit that referenced this pull request Dec 15, 2020
…te submodules (sonic-net#4852)

* src/sonic-platform-common 75698a8...82bbeab (9):
  > [sfputil] Make SfpUtilHelper.get_physical_to_logical noexcept as in SfpUtilBase (sonic-net#96)
  > [sfp_base] Update return value documentation of channel-specific methods (sonic-net#98)
  > [sfp] Tweak key names of some transceiver info fields (sonic-net#97)
  > fix typo:  portconfig.ini to port_config.ini (#94)
  > [chassis_base] Add platform API support for system LED (#91)
  > Add PCIe check commad  (#64)
  > [sfputilbase.py] Don't try to print EEPROM sysfs file name if we failed to read from it (#81)                                                                                    
  > [sfputilbase | sfputilhelper] Add support of platform.json (#72)
  > [eeprom] Add try-except to catch the IOError (#85)

* src/sonic-platform-daemons 0f4fd83...abe115e (2):
  > [xcvrd] Tweak some transceiver info key names (#62)
  > [psud][thermalctld] Always get fan/PSU LED status from platform API to avoid status inconsistencies (#59)                                                                        

* src/sonic-utilities fd7781b...16a33f2 (9):
  > [config] Fix syntax error (sonic-net#966)
  > [config] Fix indentation level in _get_disabled_services_list() (sonic-net#965)
  > a4e64d1 [sonic_installer] Refactor sonic_installer code (sonic-net#953)
  > 90efd62 [Show | Command Reference] Add Port breakout Show Command (sonic-net#859)
  > [sfpshow][mock_state_db] Tweak key names of some transceiver info fields (sonic-net#958)
  > [show] Add missing verbose option to "show line" (sonic-net#961)
  > [filter-fdb] Check VLAN Presence When Filter FDB (sonic-net#957)
  > [master]fix sonic-net#4716 show ipv6 interfaces neighbor_ip is N/A issue (sonic-net#948)
  > Fix for command. show interface transceiver eeprom -d Ethernet (sonic-net#955)

Note: sonic-utilities update fixes sonic-net#4716
zhenggen-xu pushed a commit that referenced this pull request Dec 15, 2020
- sonic-net/sonic-platform-daemons@1893c40
  - Fix the xcvrd theowing error on sfprecover function on getKeys() not valid.
- sonic-net/sonic-platform-daemons@65fa443
  - Merge pull request #90 from abdosi/multiasic-fix
- sonic-net/sonic-platform-daemons@7f812c9
  - [xcvrd] Don't log unnecessary messages upon empty transceiver change event (#53)
- sonic-net/sonic-platform-daemons@0969202
  - Fix pcied daemon failure (#91)

Signed-off-by: Petro Bratash <petrox.bratash@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants