Skip to content

[orchagent]: Enhance initSaiPhyApi#2367

Merged
Blueve merged 4 commits intosonic-net:masterfrom
andywongarista:enhance-saihelper-pikez
Jul 18, 2022
Merged

[orchagent]: Enhance initSaiPhyApi#2367
Blueve merged 4 commits intosonic-net:masterfrom
andywongarista:enhance-saihelper-pikez

Conversation

@andywongarista
Copy link
Contributor

@andywongarista andywongarista commented Jul 4, 2022

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

@aaronpayment @andywongarista
Add support for generic hwinfo string in gearbox_config.json
e7b6f07 
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
@aaronpayment @andywongarista
Add support to allow Firmware Major Version to return unsupported"
51e9e9e 
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>
@andywongarista andywongarista requested a review from prsunny as a code owner July 4, 2022 22:43
prsunny
prsunny reviewed Jul 6, 2022
View reviewed changes
Junchao-Mellanox
Junchao-Mellanox reviewed Jul 6, 2022
View reviewed changes
orchagent/saihelper.cpp Outdated
if (status == SAI_STATUS_SUCCESS) {
phy->firmware_major_version = string(attr.value.chardata);
} else if( status == SAI_STATUS_NOT_SUPPORTED ) {
phy->firmware_major_version = "";
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox Jul 6, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@andywongarista andywongarista Jul 6, 2022

Choose a reason for hiding this comment

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

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

orchagent/saihelper.cpp Outdated
{
if (status == SAI_STATUS_SUCCESS) {
phy->firmware_major_version = string(attr.value.chardata);
} else if( status == SAI_STATUS_NOT_SUPPORTED ) {
Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox Jul 6, 2022

Choose a reason for hiding this comment

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

Should this be handled in vendor SAI instead of swss?

Copy link
Contributor Author

@andywongarista andywongarista Jul 6, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@Junchao-Mellanox Junchao-Mellanox Jul 7, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@andywongarista andywongarista Jul 7, 2022

Choose a reason for hiding this comment

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

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 andywongarista mentioned this pull request Jul 6, 2022
Merged
7 tasks
@lguohan lguohan requested a review from jimmyzhai July 6, 2022 18:07
@andywongarista
Copy link
Contributor Author

andywongarista commented Jul 7, 2022

/azp run Azure.sonic-swss

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 7, 2022

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

@andywongarista
Copy link
Contributor Author

andywongarista commented Jul 7, 2022

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

@prsunny
Copy link
Collaborator

prsunny commented Jul 9, 2022

Try /azpw run

@aaronpayment
Copy link

aaronpayment commented Jul 9, 2022

/azpw run Azure.sonic-swss

prgeor
prgeor reviewed Jul 11, 2022
View reviewed changes
jimmyzhai
jimmyzhai reviewed Jul 12, 2022
View reviewed changes
@Blueve Blueve merged commit 9045995 into sonic-net:master Jul 18, 2022
@andywongarista andywongarista mentioned this pull request Jul 20, 2022
Merged
@andywongarista
Copy link
Contributor Author

andywongarista commented Jul 20, 2022

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

jimmyzhai pushed a commit that referenced this pull request Jul 26, 2022
@andywongarista
Revert hwinfo count change (#2383)
dc88d55 
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.
yxieca pushed a commit that referenced this pull request Jul 28, 2022
@andywongarista @aaronpayment @yxieca
[orchagent]: Enhance initSaiPhyApi (#2367)
c03996f 
* 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>
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
@andywongarista @aaronpayment @preetham-singh
[orchagent]: Enhance initSaiPhyApi (sonic-net#2367)
563c849 
* 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>
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
@andywongarista @preetham-singh
Revert hwinfo count change (sonic-net#2383)
a6c0d18 
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.
yxieca pushed a commit that referenced this pull request Aug 19, 2022
@andywongarista @yxieca
Revert hwinfo count change (#2383)
4085d3f 
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.
Janetxxx pushed a commit to Janetxxx/sonic-swss that referenced this pull request Nov 10, 2025
@andywongarista @aaronpayment
[orchagent]: Enhance initSaiPhyApi (sonic-net#2367)
a537e38 
* 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>
Janetxxx pushed a commit to Janetxxx/sonic-swss that referenced this pull request Nov 10, 2025
@andywongarista
Revert hwinfo count change (sonic-net#2383)
a6a12b7 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@prgeor prgeor prgeor left review comments

@jimmyzhai jimmyzhai jimmyzhai approved these changes

@Junchao-Mellanox Junchao-Mellanox Junchao-Mellanox approved these changes

@prsunny prsunny Awaiting requested review from prsunny prsunny is a code owner

Assignees

No one assigned

Labels

Arista-720DT-48S Included in 202205 Branch Request for 202205 Branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@andywongarista @prsunny @aaronpayment @jimmyzhai @prgeor @Junchao-Mellanox @lguohan @Blueve @yxieca