[BFN] Updated platform plugins #9540

akokhan · 2021-12-15T14:31:36Z

Signed-off-by: Andriy Kokhan [email protected]

Co-authored-by: KostiantynYarovyiBf [email protected]
Co-authored-by: Vadym Yashchenko [email protected]
Co-authored-by: Dmytro Lytvynenko [email protected]
Co-authored-by: Volodymyr Boiko [email protected]
Co-authored-by: Petro Bratash [email protected]

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)

Description for the changelog

Depends on #9564

lgtm-com · 2021-12-15T14:55:09Z

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 · 2021-12-15T15:53:21Z

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 · 2021-12-15T17:50:48Z

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

lgtm-com · 2021-12-16T14:59:27Z

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 · 2021-12-17T08:31:32Z

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 · 2021-12-17T13:39:43Z

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 · 2021-12-17T16:49:57Z

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

lgtm-com · 2021-12-17T17:21:31Z

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 · 2021-12-17T19:07:50Z

@lguohan , please approve and merge

akokhan · 2021-12-18T07:19:34Z

@sujinmkang , please approve and merge. Thank you

lgtm-com · 2021-12-21T07:58:28Z

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

lgtm-com · 2021-12-22T17:30:29Z

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

Signed-off-by: Andriy Kokhan <[email protected]>

Signed-off-by: Volodymyr Boyko <[email protected]>

…_eval() issue Signed-off-by: Andriy Kokhan <[email protected]>

* Signed-off-by: Vadym Yashchenko <[email protected]>

lgtm-com · 2021-12-22T20:08:45Z

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

….literal_eval() issue" This reverts commit 1e73127.

…ult config generation Signed-off-by: Andriy Kokhan <[email protected]>

lgtm-com · 2021-12-23T08:53:16Z

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 · 2021-12-26T08:14:45Z

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 · 2021-12-30T15:42:06Z

@lguohan , please approve and merge.

akokhan · 2022-01-05T18:18:09Z

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

prgeor · 2022-01-11T05:28:00Z

platform/barefoot/sonic-platform-modules-bfn-montara/sonic_platform/sfp.py

+QSFP_TYPE = "QSFP"
+QSFP_DD_TYPE = "QSFP_DD"
+
+
 class SfpUtil(SfpUtilBase):


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

@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?

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 <[email protected]>

lgtm-com · 2022-01-13T16:27:36Z

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

prgeor · 2022-01-14T05:23:20Z

platform/barefoot/sonic-platform-modules-bfn-montara/sonic_platform/sfp.py

+            return self.SFP_STATUS_UNPLUGGED
+        return self.SFP_STATUS_OK
+
+    def tx_disable(self, tx_disable):


tx_disable is not platform specific. Why override? its already available here https://github.com/Azure/sonic-platform-common/blob/master/sonic_platform_base/sonic_xcvr/api/public/sff8436.py#L227

prgeor · 2022-01-14T05:23:53Z

platform/barefoot/sonic-platform-modules-bfn-montara/sonic_platform/sfp.py

+            return self.tx_disable_channel(0xF, tx_disable)
+        return False
+
+    def tx_disable_channel(self, channel, disable):


this is not platform specific. why override this?

prgeor · 2022-01-14T05:24:44Z

platform/barefoot/sonic-platform-modules-bfn-montara/sonic_platform/sfp.py

+        _, pwr_override = pltfm_mgr_try(get_qsfp_power_override)
+        return pwr_override
+
+    def set_power_override(self, power_override, power_set):


already implemented here https://github.com/Azure/sonic-platform-common/blob/master/sonic_platform_base/sonic_xcvr/api/public/sff8436.py#L253 why override here?

prgeor · 2022-01-14T05:39:47Z

platform/barefoot/sonic-platform-modules-bfn-montara/sonic_platform/sfp.py

-    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):


if not overriding, can this be removed?

akokhan · 2022-01-14T08:50:58Z

@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 · 2022-01-17T05:44:42Z

@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.

akokhan · 2022-01-17T07:11:01Z

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

akokhan requested a review from lguohan as a code owner December 15, 2021 14:31

akokhan mentioned this pull request Dec 15, 2021

[BFN] Updated platform APIs impl #9309

Closed

5 tasks

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

akokhan force-pushed the newport_platform_api branch from 6a9c4b8 to 07cf78f Compare December 17, 2021 08:04

akokhan force-pushed the newport_platform_api branch from 8062eec to 373e7d2 Compare December 17, 2021 13:17

akokhan closed this Dec 17, 2021

akokhan reopened this Dec 17, 2021

akokhan force-pushed the newport_platform_api branch 2 times, most recently from 57dd4ac to 61dfb02 Compare December 17, 2021 16:57

akokhan mentioned this pull request Dec 20, 2021

[202012BR][Barefoot] Wedge100bf Add HW-defined thermal thresholds #9584

Closed

5 tasks

akokhan marked this pull request as draft December 22, 2021 13:45

akokhan marked this pull request as ready for review December 22, 2021 17:06

Andriy Kokhan and others added 5 commits December 22, 2021 11:45

[BFN] Updated platform APIs impl

32e6ecb

Signed-off-by: Andriy Kokhan <[email protected]>

Extended BFN platform SFP APIs implementation

24bbb90

Update sfp.py

cb13424

[BFN] Extended SFP platform plugin implementation

384b36a

Signed-off-by: Andriy Kokhan <[email protected]>

[BFN] Extended Fans platform plugin implementation

c6693ad

vboykox and others added 3 commits December 22, 2021 11:45

[BFN] Fix thrift for SFPs API

ec5afc4

Signed-off-by: Volodymyr Boyko <[email protected]>

In platform.json, replaced 'false' with '0' to workaround ast.literal…

1e73127

…_eval() issue Signed-off-by: Andriy Kokhan <[email protected]>

[Newport] Thermal manager (#23)

b1aa104

* Signed-off-by: Vadym Yashchenko <[email protected]>

akokhan force-pushed the newport_platform_api branch from 81e125a to b1aa104 Compare December 22, 2021 19:45

akokhan marked this pull request as draft December 22, 2021 20:50

akokhan force-pushed the newport_platform_api branch from bd55b30 to 83751fe Compare December 23, 2021 08:11

Revert "In platform.json, replaced 'false' with '0' to workaround ast…

2a4643e

….literal_eval() issue" This reverts commit 1e73127.

akokhan marked this pull request as ready for review December 23, 2021 08:13

Removed 'controllable' options from platform.json to fix factory defa…

83751fe

…ult config generation Signed-off-by: Andriy Kokhan <[email protected]>

Update thermal_manager.py

8b734dc

sujinmkang requested review from prgeor and vdahiya12 January 10, 2022 18:50

sujinmkang previously approved these changes Jan 10, 2022

View reviewed changes

prgeor requested changes Jan 11, 2022

View reviewed changes

Migrated SFP plugin to sonic_xcvr API (#30)

a06129c

Signed-off-by: Andriy Kokhan <[email protected]>

akokhan dismissed sujinmkang’s stale review via a06129c January 13, 2022 16:03

akokhan requested a review from prgeor January 13, 2022 17:07

prgeor reviewed Jan 14, 2022

View reviewed changes

prgeor approved these changes Jan 17, 2022

View reviewed changes

prgeor merged commit 4037867 into sonic-net:master Jan 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BFN] Updated platform plugins #9540

[BFN] Updated platform plugins #9540

akokhan commented Dec 15, 2021 •

edited

Loading

lgtm-com bot commented Dec 15, 2021

lgtm-com bot commented Dec 15, 2021

lgtm-com bot commented Dec 15, 2021

lgtm-com bot commented Dec 16, 2021

lgtm-com bot commented Dec 17, 2021

lgtm-com bot commented Dec 17, 2021

akokhan commented Dec 17, 2021

lgtm-com bot commented Dec 17, 2021

akokhan commented Dec 17, 2021

akokhan commented Dec 18, 2021

lgtm-com bot commented Dec 21, 2021

lgtm-com bot commented Dec 22, 2021

lgtm-com bot commented Dec 22, 2021

lgtm-com bot commented Dec 23, 2021

lgtm-com bot commented Dec 26, 2021

akokhan commented Dec 30, 2021

akokhan commented Jan 5, 2022

prgeor Jan 11, 2022

akokhan Jan 11, 2022

prgeor Jan 11, 2022

lgtm-com bot commented Jan 13, 2022

prgeor Jan 14, 2022

prgeor Jan 14, 2022

prgeor Jan 14, 2022

prgeor Jan 14, 2022

akokhan commented Jan 14, 2022

prgeor commented Jan 17, 2022

akokhan commented Jan 17, 2022

[BFN] Updated platform plugins #9540

[BFN] Updated platform plugins #9540

Conversation

akokhan commented Dec 15, 2021 • edited Loading

Why I did it

How I did it

How to verify it

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

Description for the changelog

lgtm-com bot commented Dec 15, 2021

lgtm-com bot commented Dec 15, 2021

lgtm-com bot commented Dec 15, 2021

lgtm-com bot commented Dec 16, 2021

lgtm-com bot commented Dec 17, 2021

lgtm-com bot commented Dec 17, 2021

akokhan commented Dec 17, 2021

lgtm-com bot commented Dec 17, 2021

akokhan commented Dec 17, 2021

akokhan commented Dec 18, 2021

lgtm-com bot commented Dec 21, 2021

lgtm-com bot commented Dec 22, 2021

lgtm-com bot commented Dec 22, 2021

lgtm-com bot commented Dec 23, 2021

lgtm-com bot commented Dec 26, 2021

akokhan commented Dec 30, 2021

akokhan commented Jan 5, 2022

prgeor Jan 11, 2022

Choose a reason for hiding this comment

akokhan Jan 11, 2022

Choose a reason for hiding this comment

prgeor Jan 11, 2022

Choose a reason for hiding this comment

lgtm-com bot commented Jan 13, 2022

prgeor Jan 14, 2022

Choose a reason for hiding this comment

prgeor Jan 14, 2022

Choose a reason for hiding this comment

prgeor Jan 14, 2022

Choose a reason for hiding this comment

prgeor Jan 14, 2022

Choose a reason for hiding this comment

akokhan commented Jan 14, 2022

prgeor commented Jan 17, 2022

akokhan commented Jan 17, 2022

akokhan commented Dec 15, 2021 •

edited

Loading