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

Allow null on SAI_PORT_ATTR_PORT_SERDES_ID #1914

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Oct 17, 2023

This attribute is readonly, and if port serdes was removed then this attribute can return null object id

This attribute is readonly, and if port serdes was removed
then this attribute can return null object id
@Junchao-Mellanox
Copy link

This PR is to fix a crash issue while removing port from orchagent portsorch.cpp. The crash is at line https://github.com/sonic-net/sonic-sairedis/blob/65323ca51db8947e8a39db5c9c0ef1fe4b27a59f/syncd/SaiSwitch.cpp#L1105 . Error flow like this:

orchagent calls portsorch.cpp::removePortBulk:

a. it removes port serdes first by calling removePortSerdesAttribute, this will remove serdes from VIDTORID and RIDTOVID

b. it continues and calls remove port which will call Syncd.cpp::processOidRemove

c. In Syncd.cpp::processOidRemove, it calls SaiSwitch.cpp::collectPortRelatedObjects

d. In SaiSwitch.cpp::collectPortRelatedObjects, it gets serdes object as a related object to the port. As serdes object does not allow null, it gets a real object ID.

e. It calls SaiSwitch.cpp::postPortRemove, and it calls getVidForRid for each related object

f. As serdes VID RID mapping has been removed by step#a, it crashes at  https://github.com/sonic-net/sonic-sairedis/blob/65323ca51db8947e8a39db5c9c0ef1fe4b27a59f/syncd/SaiSwitch.cpp#L1109

@kcudnik kcudnik requested a review from lguohan October 17, 2023 10:15
Copy link
Contributor

@srikrishnagopu srikrishnagopu left a comment

Choose a reason for hiding this comment

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

Couple of questions for my understanding

  • In step d, what OID is being returned here ? The serdes OID that has been deleted ?
  • With this fix, portRelatedObjects will not contain the serdes object anymore ?

@Junchao-Mellanox
Copy link

Couple of questions for my understanding

  • In step d, what OID is being returned here ? The serdes OID that has been deleted ?
  • With this fix, portRelatedObjects will not contain the serdes object anymore ?
  1. serdes OID. The RIDTOVID mapping has been deleted
  2. yes. With the fix, Vendor SAI is allowed to return NULL object ID after removing serdes, and NULL object ID would not be added to portRelatedObjects

@kcudnik kcudnik merged commit f981a1f into opencomputeproject:master Oct 20, 2023
3 checks passed
@kcudnik kcudnik deleted the nullserdes branch October 20, 2023 16:51
@Junchao-Mellanox
Copy link

Hi @kcudnik , many thanks to have this PR merged. Are we going to take this new commit to sonic?

@kcudnik
Copy link
Collaborator Author

kcudnik commented Oct 22, 2023

Yes, we can update sai pointer

@kcudnik
Copy link
Collaborator Author

kcudnik commented Oct 22, 2023

here is PR sonic-net/sonic-sairedis#1311 including that change

rlhui pushed a commit that referenced this pull request Nov 1, 2023
This attribute is readonly, and if port serdes was removed
then this attribute can return null object id
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