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

[Mellanox] Update the sfp platform API to get the ext_specification_compliance with new way #5123

Merged
merged 2 commits into from
Aug 14, 2020

Conversation

keboliu
Copy link
Collaborator

@keboliu keboliu commented Aug 7, 2020

- Why I did it
In PR sonic-net/sonic-platform-common#112 updated the way to parse ext_specification_compliance, the platform API need to be updated accordingly.

- How I did it
Update the platform API implementation with calling dedicated parse function which defined in the platform-common.

- How to verify it
check whether xcvrd work normally
check CLI show transceiver output

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

  • 201811
  • 201911
  • 202006

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@keboliu
Copy link
Collaborator Author

keboliu commented Aug 7, 2020

this PR has a dependency on sonic-net/sonic-platform-common#112

XCVR_INTFACE_BULK_WIDTH_QSFP = 65
XCVR_INTFACE_BULK_WIDTH_QSFP = 20
Copy link
Contributor

@jleveque jleveque Aug 7, 2020

Choose a reason for hiding this comment

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

Is it possible to move this constant to the sonic_sfp package in sonic-platform-common, and have it tied to a specific parser, rather than having each vendor specify it? This would allow us to keep all of this data in the "bulk info" and would prevent future issues if we decide to parse more fields as part of the bulk info. We could also specify the offsets as constants as well, so that the vendor can reference them by name rather than using unnamed integers. What do you think of this proposal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it doable, but not sure whether it's applicable to other vendors? I didn't deep dive into it but it seems to me that some vendors implemented it in a quite different way, like DELL.
And for the OFFSET, in Mellanox implementation, is not the absolute OFFSET starting from the beginning of the eeprom, it's a relative one for the implementation convenience.

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems to me that some vendors implemented it in a quite different way, like DELL.

Yes, this is something I would like to avoid, and I feel that maybe defining these constants in the sonic_sfp package might prevent this, as all vendors would use the same constants.

And for the OFFSET, in Mellanox implementation, is not the absolute OFFSET starting from the beginning of the eeprom, it's a relative one for the implementation convenience.

Understood. If we define all of this in the sonic_sfp package, we would just have to make it clear. But the offsets should be fixed, not vendor specific, so I feel like these definitions belong as part of the sonic_sfp library itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let me clarify, the current implementation is like this: in the vendor's implementation of the platform APIs, it read out the raw data from the EEPROM(with the given offset and width which defined by the constants we are talking about), and then feed the raw data to the parser functions defined in the common functions. So the idea is to move the definition of the offset and width to the platform common and when vendors read the EEPROM they should use the offset and width
constants defined in the platform common.

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 along the lines of what I was thinking. As long as the offsets are only dependent on the SFP type, and are not vendor/platform-specific, we should abstract away the implementation as much as possible. We can take these changes for now to fix other vendors' implementations, and we can discuss this further offline.

@jleveque
Copy link
Contributor

sonic-net/sonic-platform-common#112 has been merged. This PR now depends on the submodule update @ #5173

@jleveque jleveque merged commit bf3c901 into sonic-net:master Aug 14, 2020
abdosi pushed a commit that referenced this pull request Aug 14, 2020
…ompliance with new way (#5123)

Update the platform API implementation with calling dedicated parse function which defined in the platform-common as defined by sonic-net/sonic-platform-common#112
@keboliu keboliu deleted the update_platform_api branch January 16, 2021 01:15
santhosh-kt pushed a commit to santhosh-kt/sonic-buildimage that referenced this pull request Feb 25, 2021
…ompliance with new way (sonic-net#5123)

Update the platform API implementation with calling dedicated parse function which defined in the platform-common as defined by sonic-net/sonic-platform-common#112
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.

None yet

3 participants