[orchagent]: Enhance initSaiPhyApi #2367

andywongarista · 2022-07-04T22:43:42Z

What I did

Added support for PHYs that do not have firmware support and would return SAI_STATUS_NOT SUPPORTED when attempting to retrieve SAI_SWITCH_ATTR_FIRMWARE_MAJOR_VERSION
Removed format check on gearbox_config.json hwinfo string

Why I did it

Some PHYs (e.g. BCM54182) do not have firmware support and will return SAI_STATUS_NOT SUPPORTED when attempting to retrieve SAI_SWITCH_ATTR_FIRMWARE_MAJOR_VERSION. This should not fail initSaiPhyApi.
SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO is defined to be a string but the exact format is vendor specific. Thus, the current format check in initSaiPhyApi should not be present.

How I verified it
Tested on Arista platform with BCM54182 and appropriate PAI driver

Details if related

The SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO formatting is vendor specific. * Remove the formating check that assumes its of the mdio sysfs format * Note the the count remains without including the NULL termintor, which is not compliant with the SAI header definintion that indicates a NULL terminated string. Signed-off-by: aaronp@arista.com

Some external phys do not support Firmware upgrades and therefore do not have a firmware version. The SAI_SWITCH_ATTR_FIRMWARE_MAJOR_VERSION may return SAI_STATUS_ATTR_NOT_SUPPORTED which needs to be gracefully supported and allow the phy to be created. * Allow SAI_STATUS_NOT_SUPPORTED return value and set version to empty string. Signed-off-by: Aaron Payment <aaronp@arista.com>

orchagent/saihelper.cpp

Junchao-Mellanox · 2022-07-06T06:12:57Z

orchagent/saihelper.cpp

+    if (status == SAI_STATUS_SUCCESS) {
+       phy->firmware_major_version = string(attr.value.chardata);
+    } else if( status == SAI_STATUS_NOT_SUPPORTED ) {
+       phy->firmware_major_version = "";


I am not sure the empty string is good enough here. Should it be "N/A" or "Not Supported"?

I'm fine with changing it to N/A. I don't think there are any strict requirements here.

Junchao-Mellanox · 2022-07-06T06:14:08Z

orchagent/saihelper.cpp

-    {
+    if (status == SAI_STATUS_SUCCESS) {
+       phy->firmware_major_version = string(attr.value.chardata);
+    } else if( status == SAI_STATUS_NOT_SUPPORTED ) {


Should this be handled in vendor SAI instead of swss?

Vendor SAI will handle returning the appropriate status and setting the attr variable appropriately, and swss will update the local phy object appropriately. I'm not sure I understand why the responsibilities would need to change.

If we do this change in swss, it would change the behavior for all platforms. However, BCM54182(or a few limited platforms) is the only platform that requires this change. That is why I thought it should be vendor SAI's responsibility to do so.
Anyway, if this change has no harm to other platforms, I can live with it. I don't have strong opinion here.

Yes, this should not affect other platforms if they are still returning SAI_STATUS_FAILURE. But I think the change here is appropriate to allow initSaiPhyApi to return with success on platforms that don't support firmware.

andywongarista · 2022-07-07T18:41:53Z

/azp run Azure.sonic-swss

azure-pipelines · 2022-07-07T18:41:58Z

Commenter does not have sufficient privileges for PR 2367 in repo Azure/sonic-swss

andywongarista · 2022-07-07T18:49:17Z

Can someone with sufficient privileges rerun the tests? The errors do not look related.

prsunny · 2022-07-09T00:14:17Z

Try /azpw run

aaronpayment · 2022-07-09T15:41:20Z

/azpw run Azure.sonic-swss

orchagent/saihelper.cpp

andywongarista · 2022-07-20T17:44:20Z

I opened #2383 to revert the change here that increases the hwinfo count

What I did Revert change from #2367 which increases count associated with SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO by 1, as well as the memset. Why I did it Original intention of this change was to accommodate sairedis behaviour when copying null-terminated string; original behaviour is that the null-terminator would not be copied and so receiver of the hwinfo (PAI) would see non-null terminated string. Reverting this change so that old behaviour is maintained and PAI driver is responsible for not relying on string to be null terminated.

* Add support for generic hwinfo string in gearbox_config.json The SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO formatting is vendor specific. * Remove the formating check that assumes its of the mdio sysfs format * Note the the count remains without including the NULL termintor, which is not compliant with the SAI header definintion that indicates a NULL terminated string. Signed-off-by: aaronp@arista.com * Add support to allow Firmware Major Version to return unsupported" Some external phys do not support Firmware upgrades and therefore do not have a firmware version. The SAI_SWITCH_ATTR_FIRMWARE_MAJOR_VERSION may return SAI_STATUS_ATTR_NOT_SUPPORTED which needs to be gracefully supported and allow the phy to be created. * Allow SAI_STATUS_NOT_SUPPORTED return value and set version to empty string. Signed-off-by: Aaron Payment <aaronp@arista.com> * Address review comments * Address review comments, fix hwinfo Co-authored-by: Aaron Payment <aaronp@arista.com>

What I did Revert change from sonic-net#2367 which increases count associated with SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO by 1, as well as the memset. Why I did it Original intention of this change was to accommodate sairedis behaviour when copying null-terminated string; original behaviour is that the null-terminator would not be copied and so receiver of the hwinfo (PAI) would see non-null terminated string. Reverting this change so that old behaviour is maintained and PAI driver is responsible for not relying on string to be null terminated.

What I did Revert change from #2367 which increases count associated with SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO by 1, as well as the memset. Why I did it Original intention of this change was to accommodate sairedis behaviour when copying null-terminated string; original behaviour is that the null-terminator would not be copied and so receiver of the hwinfo (PAI) would see non-null terminated string. Reverting this change so that old behaviour is maintained and PAI driver is responsible for not relying on string to be null terminated.

aaronpayment added 2 commits July 4, 2022 15:26

andywongarista requested a review from prsunny as a code owner July 4, 2022 22:43

prsunny reviewed Jul 6, 2022

View reviewed changes

orchagent/saihelper.cpp Outdated Show resolved Hide resolved

prsunny requested review from Junchao-Mellanox, prsunny and prgeor July 6, 2022 00:33

Junchao-Mellanox reviewed Jul 6, 2022

View reviewed changes

andywongarista mentioned this pull request Jul 6, 2022

[Arista] Add missing configs for 720DT-48S sonic-net/sonic-buildimage#11362

Merged

7 tasks

lguohan requested a review from jimmyzhai July 6, 2022 18:07

lguohan added the Arista-720DT-48S label Jul 6, 2022

Address review comments

9883f23

Junchao-Mellanox previously approved these changes Jul 7, 2022

View reviewed changes

prgeor reviewed Jul 11, 2022

View reviewed changes

orchagent/saihelper.cpp Outdated Show resolved Hide resolved

jimmyzhai reviewed Jul 12, 2022

View reviewed changes

orchagent/saihelper.cpp Outdated Show resolved Hide resolved

Blueve added the Request for 202205 Branch label Jul 13, 2022

Address review comments, fix hwinfo

b949238

andywongarista dismissed Junchao-Mellanox’s stale review via b949238 July 15, 2022 22:30

Junchao-Mellanox approved these changes Jul 18, 2022

View reviewed changes

jimmyzhai approved these changes Jul 18, 2022

View reviewed changes

Blueve merged commit 9045995 into sonic-net:master Jul 18, 2022

andywongarista mentioned this pull request Jul 20, 2022

Revert hwinfo count change #2383

Merged

yxieca added the Included in 202205 Branch label Jul 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[orchagent]: Enhance initSaiPhyApi #2367

[orchagent]: Enhance initSaiPhyApi #2367

andywongarista commented Jul 4, 2022

Junchao-Mellanox Jul 6, 2022

andywongarista Jul 6, 2022

Junchao-Mellanox Jul 6, 2022

andywongarista Jul 6, 2022

Junchao-Mellanox Jul 7, 2022

andywongarista Jul 7, 2022

andywongarista commented Jul 7, 2022

azure-pipelines bot commented Jul 7, 2022

andywongarista commented Jul 7, 2022

prsunny commented Jul 9, 2022

aaronpayment commented Jul 9, 2022

andywongarista commented Jul 20, 2022

[orchagent]: Enhance initSaiPhyApi #2367

[orchagent]: Enhance initSaiPhyApi #2367

Conversation

andywongarista commented Jul 4, 2022

Junchao-Mellanox Jul 6, 2022

Choose a reason for hiding this comment

andywongarista Jul 6, 2022

Choose a reason for hiding this comment

Junchao-Mellanox Jul 6, 2022

Choose a reason for hiding this comment

andywongarista Jul 6, 2022

Choose a reason for hiding this comment

Junchao-Mellanox Jul 7, 2022

Choose a reason for hiding this comment

andywongarista Jul 7, 2022

Choose a reason for hiding this comment

andywongarista commented Jul 7, 2022

azure-pipelines bot commented Jul 7, 2022

andywongarista commented Jul 7, 2022

prsunny commented Jul 9, 2022

aaronpayment commented Jul 9, 2022

andywongarista commented Jul 20, 2022