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.

Sign up for GitHub

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

Jump to bottom

[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
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90c0f0c
[Mellanox] Update the sfp platform API to get the ext_specification_c…
keboliu Aug 7, 2020
0220591
restore XCVR_INTFACE_BULK_WIDTH_QSFP to original value
keboliu Aug 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions platform/mellanox/mlnx-platform-api/sonic_platform/sfp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

# definitions of the offset and width for values in XCVR info eeprom
XCVR_INTFACE_BULK_OFFSET = 0
XCVR_INTFACE_BULK_WIDTH_QSFP = 65
XCVR_INTFACE_BULK_WIDTH_QSFP = 20
Copy link
Contributor

@jleveque jleveque Aug 7, 2020 •
edited
Loading

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.

XCVR_INTFACE_BULK_WIDTH_SFP = 21
XCVR_TYPE_OFFSET = 0
XCVR_TYPE_WIDTH = 1
Expand Down Expand Up @@ -59,6 +59,8 @@
XCVR_HW_REV_WIDTH_OSFP = 2
XCVR_HW_REV_WIDTH_QSFP = 2
XCVR_HW_REV_WIDTH_SFP = 4
XCVR_EXT_SPECIFICATION_COMPLIANCE_OFFSET = 64
XCVR_EXT_SPECIFICATION_COMPLIANCE_WIDTH = 1
XCVR_VENDOR_SN_OFFSET = 68
XCVR_VENDOR_SN_WIDTH = 16
XCVR_VENDOR_DATE_OFFSET = 84
Expand Down Expand Up @@ -826,9 +828,12 @@ def get_transceiver_info(self):

for key in qsfp_compliance_code_tup:
if key in sfp_interface_bulk_data['data']['Specification compliance']['value']:
compliance_code_dict[key] = sfp_interface_bulk_data['data']['Specification compliance']['value'][key]['value']
if sfp_interface_bulk_data['data']['Extended Specification compliance']['value'] != "Unspecified":
compliance_code_dict['Extended Specification compliance'] = sfp_interface_bulk_data['data']['Extended Specification compliance']['value']
compliance_code_dict[key] = sfp_interface_bulk_data['data']['Specification compliance']['value'][key]['value']
sfp_ext_specification_compliance_raw = self._read_eeprom_specific_bytes(offset + XCVR_EXT_SPECIFICATION_COMPLIANCE_OFFSET, XCVR_EXT_SPECIFICATION_COMPLIANCE_WIDTH)
if sfp_ext_specification_compliance_raw is not None:
sfp_ext_specification_compliance_data = sfpi_obj.parse_ext_specification_compliance(sfp_ext_specification_compliance_raw[0 : 1], 0)
if sfp_ext_specification_compliance_data['data']['Extended Specification compliance']['value'] != "Unspecified":
compliance_code_dict['Extended Specification compliance'] = sfp_ext_specification_compliance_data['data']['Extended Specification compliance']['value']
transceiver_info_dict['specification_compliance'] = str(compliance_code_dict)

transceiver_info_dict['nominal_bit_rate'] = str(sfp_interface_bulk_data['data']['Nominal Bit Rate(100Mbs)']['value'])
Expand Down