Restore optoe page to default 0 for active cables #548
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
PR Overview
This PR restores the optoe page to zero before performing EEPROM reads for offsets greater than 127, preventing potential read errors and crashes when the module is busy with CDB commands.
- Introduces a new constant, SFP_OPTOE_PAGE_OFFSET.
- Adds methods to get the current page and restore the page to zero.
- Modifies the EEPROM read function to check and restore the page if necessary.
Reviewed Changes
| File | Description |
|---|---|
| sonic_platform_base/sonic_xcvr/sfp_optoe_base.py | Adds new constants and methods to manage the optoe page, and updates read_eeprom accordingly |
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
sonic_platform_base/sonic_xcvr/sfp_optoe_base.py:284
- Comparing the bytearray returned by get_optoe_current_page() directly to 0 may not work as intended; consider checking the first element (e.g., self.get_optoe_current_page()[0] != 0) to correctly verify if the page is zero.
if offset > 127 and offset < 256 and self.get_optoe_current_page() != 0:
sonic_platform_base/sonic_xcvr/sfp_optoe_base.py:284
- [nitpick] Consider using the SFP_OPTOE_PAGE_OFFSET constant instead of the literal 127 in the condition to maintain consistency within the code.
if offset > 127 and offset < 256 and self.get_optoe_current_page() != 0:
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@prgeor Wondering if the current changeset requires special handling for SFF-8472 based transceivers since these transceivers have page select byte at byte 127, A2h instead of A0h. |
Co-authored-by: mihirpat1 <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@mihirpat1 Good question. I think we should be good in that case because 0xA0 address space offset > 127 is a reserved space. We don't have a use case today
In other words, all Access for offset > 127 will be in 0xA2 address space in SFP case. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Why do you only need to reset the page code in 128-255 offset range? do you also need to reset the page code when offset is greater than 255? |
@yxieca The optoe kernel driver before r/w to the page >=1 already sets the page selection register. |
|
@kperumalbfn could you help cherry pick to 202411 |
|
Cherry-pick PR to 202405: #549 |
|
@kperumalbfn @bingwang-ms |
|
Cherry-pick PR to 202411: #550 |
Description
Restore the optoe's page to page ZERO before reading the Lower page > 127
Motivation and Context
When the module is busy with CDB command, the optoe kernel driver at the end of user page read request, tries to restore the page to Page ZERO and if this write fails because the module could be busy with CDB command (since these modules donot support background CDB mode). Because of this when Xcvrd tries to read offset (> 127) in Lower page, since optoe does not restore the page to 0 for any Lower page access, the page read to these offset will result in wrong page to be read (because of previous page select write failure) and Xcvrd parsing fails causing crash.
Now in optoe EEPROM read API we ensure to restore the Page to ZERO page if the Page select byte is non-zero
How Has This Been Tested?
TBD:
Additional Information (Optional)