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

[BFN] Updated platform plugins #9540

Merged
merged 35 commits into from
Jan 17, 2022
Merged

Conversation

akokhan
Copy link
Contributor

@akokhan akokhan commented Dec 15, 2021

Signed-off-by: Andriy Kokhan andriyx.kokhan@intel.com

Co-authored-by: KostiantynYarovyiBf kostiantynx.yarovyi@intel.com
Co-authored-by: Vadym Yashchenko vadymx.yashchenko@intel.com
Co-authored-by: Dmytro Lytvynenko dmytrox.lytvynenko@intel.com
Co-authored-by: Volodymyr Boiko volodymyrx.boiko@intel.com
Co-authored-by: Petro Bratash petrox.bratash@intel.com

Why I did it

Added missing implementation for BFN platform APIs

How I did it

How to verify it

Run sonic-mgmt platform TCs

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

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

Description for the changelog

Depends on #9564

@akokhan akokhan mentioned this pull request Dec 15, 2021
5 tasks
@lgtm-com
Copy link

lgtm-com bot commented Dec 15, 2021

This pull request introduces 6 alerts when merging f79c084 into d05afb5 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 2 for Variable defined multiple times
  • 1 for Unused local variable
  • 1 for Wrong number of arguments in a class instantiation

@lgtm-com
Copy link

lgtm-com bot commented Dec 15, 2021

This pull request introduces 6 alerts when merging 13714d2 into d05afb5 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 2 for Variable defined multiple times
  • 1 for Unused local variable
  • 1 for Wrong number of arguments in a class instantiation

@lgtm-com
Copy link

lgtm-com bot commented Dec 15, 2021

This pull request introduces 6 alerts when merging 8e1c743 into d05afb5 - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 2 for Variable defined multiple times
  • 1 for Unused local variable
  • 1 for Wrong number of arguments in a class instantiation

@akokhan akokhan force-pushed the newport_platform_api branch 2 times, most recently from 35cc487 to 6a9c4b8 Compare December 16, 2021 14:33
@lgtm-com
Copy link

lgtm-com bot commented Dec 16, 2021

This pull request introduces 3 alerts when merging 6a9c4b8c8509b6539d280a4d79d57f6cb1988e9c into f3df6e2 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Variable defined multiple times
  • 1 for Wrong number of arguments in a class instantiation

@lgtm-com
Copy link

lgtm-com bot commented Dec 17, 2021

This pull request introduces 1 alert when merging 07cf78f33b76551b17a69c7e284ddb230ea06b1b into 36673c1 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

@lgtm-com
Copy link

lgtm-com bot commented Dec 17, 2021

This pull request introduces 1 alert when merging 373e7d282e7bfa3c266a450f1e5be48f9626fcf2 into 36673c1 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

@akokhan
Copy link
Contributor Author

akokhan commented Dec 17, 2021

This pull request introduces 1 alert when merging 373e7d2 into 36673c1 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

False negative, same as reported here: github/codeql#3617

@akokhan akokhan closed this Dec 17, 2021
@akokhan akokhan reopened this Dec 17, 2021
@akokhan akokhan force-pushed the newport_platform_api branch 2 times, most recently from 57dd4ac to 61dfb02 Compare December 17, 2021 16:57
@lgtm-com
Copy link

lgtm-com bot commented Dec 17, 2021

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

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

@akokhan
Copy link
Contributor Author

akokhan commented Dec 17, 2021

@lguohan , please approve and merge

@akokhan
Copy link
Contributor Author

akokhan commented Dec 18, 2021

@sujinmkang , please approve and merge. Thank you

@lgtm-com
Copy link

lgtm-com bot commented Dec 21, 2021

This pull request introduces 1 alert when merging 1d8832d into 67e40b5 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

@akokhan akokhan marked this pull request as draft December 22, 2021 13:45
@akokhan akokhan marked this pull request as ready for review December 22, 2021 17:06
@lgtm-com
Copy link

lgtm-com bot commented Dec 22, 2021

This pull request introduces 1 alert when merging 8dfbbff11e535c25f49da61bb087a6184cf49fad into 3aec728 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

Andriy Kokhan and others added 5 commits December 22, 2021 11:45
vboykox and others added 3 commits December 22, 2021 11:45
Signed-off-by: Volodymyr Boyko <volodymyrx.boiko@intel.com>
…_eval() issue

Signed-off-by: Andriy Kokhan <andriyx.kokhan@intel.com>
* Signed-off-by: Vadym Yashchenko <vadymx.yashchenko@intel.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 22, 2021

This pull request introduces 2 alerts when merging b1aa104 into 3aec728 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation
  • 1 for Unnecessary delete statement in function

@akokhan akokhan marked this pull request as draft December 22, 2021 20:50
@akokhan akokhan marked this pull request as ready for review December 23, 2021 08:13
…ult config generation

Signed-off-by: Andriy Kokhan <andriyx.kokhan@intel.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 23, 2021

This pull request introduces 2 alerts when merging 83751fe into 48a648c - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation
  • 1 for Unnecessary delete statement in function

@lgtm-com
Copy link

lgtm-com bot commented Dec 26, 2021

This pull request introduces 1 alert when merging 8b734dc into 36d8660 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

@akokhan
Copy link
Contributor Author

akokhan commented Dec 30, 2021

@lguohan , please approve and merge.

@akokhan
Copy link
Contributor Author

akokhan commented Jan 5, 2022

@lguohan , please approve and merge this one and #9564 which is related.

sujinmkang
sujinmkang previously approved these changes Jan 10, 2022
QSFP_TYPE = "QSFP"
QSFP_DD_TYPE = "QSFP_DD"


class SfpUtil(SfpUtilBase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use Sfp refactoring because SFP handling in SONiC has moved to mostly platform independent code. Ref: SFP refactoring HLD.

Use SfpOptoeBase class instead of SfpUtilBase (which is going to be phased out)

Example Dell platform moved to using SFP refactored code : - #9016

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prgeor , thank you for reviewing. Totally make sense. Looks like redesigning into sonic_xcvr approach and testing will take some time. Can we have these changes merged as-is (mainly because of other components) and refactor SFP part in a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

sonic_xcvr is already tested so your platform is good to use sonic_xcvr approach. There is very little code that is platform specific when it comes to supporting the transceivers, so very less platform specific code means less testing and bugs for your platform. please let me know if you face any issue while moving to sonic_xcvr.

Signed-off-by: Andriy Kokhan <andriyx.kokhan@intel.com>
@lgtm-com
Copy link

lgtm-com bot commented Jan 13, 2022

This pull request introduces 1 alert when merging a06129c into 8bb9ed6 - view on LGTM.com

new alerts:

  • 1 for Wrong number of arguments in a class instantiation

@akokhan akokhan requested a review from prgeor January 13, 2022 17:07
return self.SFP_STATUS_UNPLUGGED
return self.SFP_STATUS_OK

def tx_disable(self, tx_disable):
Copy link
Contributor

Choose a reason for hiding this comment

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

return self.tx_disable_channel(0xF, tx_disable)
return False

def tx_disable_channel(self, channel, disable):
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not platform specific. why override this?

_, pwr_override = pltfm_mgr_try(get_qsfp_power_override)
return pwr_override

def set_power_override(self, power_override, power_set):
Copy link
Contributor

Choose a reason for hiding this comment

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

def set_lpmode(self, lpmode):
with Sfp.sfputil.eeprom_action() as u:
return u.set_low_power_mode(self.port_num, lpmode)
def write_eeprom(self, offset, num_bytes, write_buffer):
Copy link
Contributor

Choose a reason for hiding this comment

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

if not overriding, can this be removed?

@akokhan
Copy link
Contributor Author

akokhan commented Jan 14, 2022

@prgeor , thank you for reviewing the changes.

As you can see, in this SFP plugin we expose SFP eeprom content through the file cache which is updated each time get_eeprom_path() is executed. Some APIs we still implement explicitly through SDK platform calls. Mostly to improve performance. Also, since eeprom is emulated through the file cache, we override write_eeprom() to disable write access which does not make sense in this case. Instead, whenever is needed, we implement write APIs explicitly. E.g., tx_disable(), tx_disable_channel(). The implementation can be extended and/or improved in the future.

@prgeor
Copy link
Contributor

prgeor commented Jan 17, 2022

@prgeor , thank you for reviewing the changes.

As you can see, in this SFP plugin we expose SFP eeprom content through the file cache which is updated each time get_eeprom_path() is executed. Some APIs we still implement explicitly through SDK platform calls. Mostly to improve performance. Also, since eeprom is emulated through the file cache, we override write_eeprom() to disable write access which does not make sense in this case. Instead, whenever is needed, we implement write APIs explicitly. E.g., tx_disable(), tx_disable_channel(). The implementation can be extended and/or improved in the future.

@prgeor , thank you for reviewing the changes.

As you can see, in this SFP plugin we expose SFP eeprom content through the file cache which is updated each time get_eeprom_path() is executed. Some APIs we still implement explicitly through SDK platform calls. Mostly to improve performance. Also, since eeprom is emulated through the file cache, we override write_eeprom() to disable write access which does not make sense in this case. Instead, whenever is needed, we implement write APIs explicitly. E.g., tx_disable(), tx_disable_channel(). The implementation can be extended and/or improved in the future.

In future when your platform needs to support QSFP-DD then your platform will have to duplicate the code in sonic_xcvr. Currently Xcvrd only collects DOM related info from transceiver, but in future QSFP-DD module will need initialization which has data path link synchronization requirement see ref. sonic-net/SONiC#916. Xcvrd is platform independent and won't support any platform specific requirement and hence its important for platform to re-use sonic_xcvr which is compliant with various MSA specifications.

@prgeor prgeor merged commit 4037867 into sonic-net:master Jan 17, 2022
@akokhan
Copy link
Contributor Author

akokhan commented Jan 17, 2022

@prgeor , thank you for the detailed explanation. I really appreciate this. We will plan and adjust SFP plugin implementation accordingly.

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.

9 participants