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

[Dell] S6000 I2C not responding to certain optics #8736

Merged
merged 3 commits into from
Oct 21, 2021

Conversation

santhosh-kt
Copy link
Contributor

@santhosh-kt santhosh-kt commented Sep 13, 2021

Why I did it

  • "sfputil show eeprom" and "sfpshow eeprom" commands displays inconsistency results for certain optics.
  • For every execution of the commands, various results are displayed.
  • Only certain optics EEPROM contents were not retrieved.

How I did it

On branch Master
Your branch is up to date with 'origin/Master'.

Changes to be committed:
(use "git restore --staged ..." to unstage)
modified: device/dell/x86_64-dell_s6000_s1220-r0/plugins/sfputil.py
modified: platform/broadcom/sonic-platform-modules-dell/s6000/modules/dell_s6000_platform.c
modified: platform/broadcom/sonic-platform-modules-dell/s6000/scripts/s6000_platform.sh
modified: platform/broadcom/sonic-platform-modules-dell/s6000/sonic_platform/sfp.py

How to verify it

  • Just execute "sfputil show eeprom" and "sfpshow eeprom" commands.

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

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

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

@santhosh-kt santhosh-kt marked this pull request as ready for review September 15, 2021 04:37
@prgeor prgeor self-assigned this Oct 18, 2021
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.

@santhosh-kt please address comment

@@ -72,10 +74,19 @@ def get_presence(self, port_num):
if port_num < self.port_start or port_num > self.port_end:
return False

try:
Copy link
Contributor

Choose a reason for hiding this comment

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

why is qsfp_modprs pin access needs a lock? Reading of sysfs entry shouldn't need lock. I see this https://github.com/Azure/sonic-buildimage/blob/master/platform/broadcom/sonic-platform-modules-dell/s6000/modules/dell_s6000_platform.c#L286 already thread safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lock is provided for writing qsfp_modsel register.
When multiple ports are enabled via qsfp_modsel register, we observed that qsfp_modprs values are being wrong. At a time only one port should be enabled in qsfp_modsel.


return modsel_state

def set_modsel(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

If you bring the lock inside set_modsel(), then you don't have to lock/unlock every place where set_modsel() is called, keeps code changes to minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lock is based on set_modsel() but we cant introduce it here.
A scenario:

  1. When sfputil tries to read a port, it checks for modprs register and to read that we are setting modsel register.
  2. Before we actually reading modprs register, if pmon docker or any other process trying to set modsel register then it will have the race condition here.
  3. To avoid this we are locking before modsel register is being set and releasing the lock only after the requirements are over(In our case after retriving modprs register).

@santhosh-kt santhosh-kt requested a review from prgeor October 21, 2021 09:17
@prgeor prgeor merged commit 992dc47 into sonic-net:master Oct 21, 2021
qiluo-msft pushed a commit that referenced this pull request Oct 27, 2021
* [Dell] S6000 I2C not responding to certain optics

* Revising return states

* Moved lock file from /var/run/platform_cache to /etc/sonic
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.

3 participants