Skip to content

[optoe] Reset page select byte to 0 before upper memory access on page 0h#463

Merged
prgeor merged 4 commits intosonic-net:masterfrom
mihirpat1:optoe_reset_page_select
Feb 14, 2025
Merged

[optoe] Reset page select byte to 0 before upper memory access on page 0h#463
prgeor merged 4 commits intosonic-net:masterfrom
mihirpat1:optoe_reset_page_select

Conversation

@mihirpat1
Copy link
Copy Markdown
Contributor

@mihirpat1 mihirpat1 commented Feb 12, 2025

Description
The optoe kernel driver currently assumes that the page select byte (page 0, byte 127) is set to 0h before accessing the upper memory on page 0h.
This assumption causes failures when accessing the module's EEPROM in multiple scenarios.
On the contrary, the driver currently sets the page select byte if the intended page to be accessed is > 0 as evident from the below code
https://github.com/opencomputeproject/oom/blob/c32499a89dff005e7ff2acb48b33f55543ce5140/optoe/optoe.c#L344-L353

Impact
If the optoe driver accesses any page except page 0h and the code for restoring the page select byte to 0h fails, subsequent attempts to
access the upper memory of page 0h will fail because the page select byte from the previous transaction remains unchanged.
https://github.com/opencomputeproject/oom/blob/c32499a89dff005e7ff2acb48b33f55543ce5140/optoe/optoe.c#L373-L392

Steps to recreate the failure scenario
One way to recreate this issue is:

  1. Initiate CDB firmware download operation on a module supporting CDB foreground mode only
  2. Kill the process which is downloading the FW
  3. Dump the page select byte and ensure it displays a non-zero value
  4. Execute sfputil show fwversion EthernetXX CLI and you will observe a traceback. The traceback occurs because the vendor name is read
    with garbage content, as the page select byte is set to 0x9F instead of 0x0.

Fix
Before optoe accesses any address from upper page 0h, the page select byte is now set to 0.
This fix is done only for non-SFP modules to limit the scope of impact. SFP modules require additional handling since the page select byte
is on page A2h and NOT A0h unlike non-SFP modules which do not have the concept of A0h and A2h page.

Testing
The fix has been tested on a non-SFP module and it was ensured that the sfputil show fwversion EthernetXX CLI command executes successfully without any traceback.

Logs for testing the fix

root@dut:/home/admin# sfputil firmware download Ethernet496 FIRMWARE_FILE.bin
CDB: Starting firmware download
Downloading ...  [#-----------------------------------]    4%  00:08:07^C

Aborted!
root@dut:/home/admin#  i2cget -f -y 103 0x50 0x7f
0x9f
root@dut:/home/admin# sfputil show fwversion Ethernet496
Image A Version: 1.2.3
Image B Version: N/A
Factory Image Version: 0.0.0
Running Image: A
Committed Image: A
Active Firmware: 1.2.3
Inactive Firmware: 170.170.0

root@dut:/home/admin#

Logs for the failure scenario

Initiate CDB firmware download operation on a module supporting CDB foreground mode only
Kill the process which is downloading the FW
root@dut:/home/admin# sfputil firmware download Ethernet496 FIRMWARE_FILE.bin
CDB: Starting firmware download
Downloading ...  [##----------------------------------]    6%  00:07:57^C

Aborted!
root@dut:/home/admin#

Dump the page select byte and ensure it displays a non-zero value
root@dut:/home/admin#  i2cget -f -y 103 0x50 0x7f
0x9f

Execute sfputil show fwversion EthernetXX CLI and you will observe a traceback. The traceback is seen since the vendor name is being read with garbage content since the page select byte is set to 0x9F and NOT 0x0.

root@dut:/home/admin# sfputil show fwversion Ethernet496
Traceback (most recent call last):
  File "/usr/local/bin/sfputil", line 8, in <module>
    sys.exit(cli())
             ^^^^^
  File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/sfputil/main.py", line 1215, in fwversion
    show_firmware_version(physical_port)
  File "/usr/local/lib/python3.11/dist-packages/sfputil/main.py", line 1185, in show_firmware_version
    api = sfp.get_xcvr_api()
          ^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/sonic_platform_base/sfp_base.py", line 480, in get_xcvr_api
    self.refresh_xcvr_api()
  File "/usr/local/lib/python3.11/dist-packages/sonic_platform_base/sfp_base.py", line 470, in refresh_xcvr_api
    self._xcvr_api = self._xcvr_api_factory.create_xcvr_api()
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/sonic_platform_base/sonic_xcvr/xcvr_api_factory.py", line 79, in create_xcvr_api
    vendor_name = self._get_vendor_name()
                  ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/dist-packages/sonic_platform_base/sonic_xcvr/xcvr_api_factory.py", line 64, in _get_vendor_name
    vendor_name = name_data.decode()
                  ^^^^^^^^^^^^^^^^^^
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x80 in position 2: invalid start byte

Invalid vendor name content:
80: 01 04 00 80 04 ff 00 00 00 00 77 00 00 00 00 00    ??.??.....w.....
90: 49

MSFT ADO - 31376026

Signed-off-by: Mihir Patel patelmi@microsoft.com

@mihirpat1
[optoe] Reset page select byte to 0 before upper memory access on pag…
a9e1d19 
…e 0h

Signed-off-by: Mihir Patel <patelmi@microsoft.com>
@mihirpat1 mihirpat1 requested a review from a team as a code owner February 12, 2025 23:00
@mssonicbld
Copy link
Copy Markdown

mssonicbld commented Feb 12, 2025

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Feb 12, 2025

Azure Pipelines successfully started running 1 pipeline(s).

prgeor
prgeor previously approved these changes Feb 12, 2025
View reviewed changes
@prgeor prgeor added the bug label Feb 12, 2025
@saiarcot895
Copy link
Copy Markdown
Contributor

saiarcot895 commented Feb 13, 2025

@donboll FYI

@mssonicbld
Copy link
Copy Markdown

mssonicbld commented Feb 13, 2025

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Feb 13, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Copy Markdown

mssonicbld commented Feb 13, 2025

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Feb 13, 2025

Azure Pipelines successfully started running 1 pipeline(s).

prgeor
prgeor previously approved these changes Feb 13, 2025
View reviewed changes
@mssonicbld
Copy link
Copy Markdown

mssonicbld commented Feb 13, 2025

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Feb 13, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@prgeor prgeor merged commit 7a1f547 into sonic-net:master Feb 14, 2025
6 checks passed
@mihirpat1
Copy link
Copy Markdown
Contributor Author

mihirpat1 commented Feb 14, 2025

@kperumalbfn Can you please help to cherry-pick this to 202411 branch?
MSFT ADO - 31376026

@mssonicbld mssonicbld mentioned this pull request Feb 14, 2025
Merged
@mssonicbld
Copy link
Copy Markdown

mssonicbld commented Feb 14, 2025

Cherry-pick PR to 202411: #464

@prgeor prgeor mentioned this pull request Feb 25, 2025
Merged
mssonicbld added a commit to mssonicbld/sonic-linux-kernel that referenced this pull request Feb 26, 2025
@mssonicbld
Revert "[optoe] Reset page select byte to 0 before upper memory acces…
0ab44b9 
…s on page 0h"

Reverts sonic-net#463
@mssonicbld mssonicbld mentioned this pull request Feb 26, 2025
Merged
mssonicbld added a commit that referenced this pull request Feb 26, 2025
@mssonicbld
Revert "[optoe] Reset page select byte to 0 before upper memory acces…
8bd11c2 
…s on page 0h" (#468)

Reverts #463
liushilongbuaa pushed a commit to Azure/sonic-linux-kernel.msft that referenced this pull request Mar 18, 2025
@mssonicbld @liushilongbuaa
Revert "[optoe] Reset page select byte to 0 before upper memory acces…
e13c589 
…s on page 0h" (#468)

Reverts sonic-net/sonic-linux-kernel#463
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saiarcot895 saiarcot895 saiarcot895 approved these changes

@prgeor prgeor prgeor approved these changes

+1 more reviewer

@paulmenzel paulmenzel paulmenzel left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Approved for 202411 Branch bug Included in 202411 Branch Request for 202411 Branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mihirpat1 @mssonicbld @saiarcot895 @paulmenzel @prgeor @kperumalbfn