[sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port #3485

xinyulin · 2024-08-10T00:29:27Z

What I did

This update enhances the loopback command functionality to support configuring the loopback mode for specific logical ports, instead of applying the configuration to the entire physical port.

How I did it

By calculating the lane mask according to the port's configuration and state data, the loopback mode can now be accurately set for the specified port, ensuring that other unrelated logical ports are not affected during diagnostic operations.

How to verify it

Tested under Cisco8111 with Credo C1 cable.

The sfputil debug loopback command will now only affect the relevant lanes of the logical port.

Set host input loopback mode on logical port Ethernet80, the expected value of page 0x13, byte 0xB7= 0x0F

Set media input loopback mode on logical port Ethernet84, the expected value of page 0x13, byte 0xB5= 0xF0

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

…s of the logical port Signed-off-by: xinyu <xinyu0123@gmail.com>

mihirpat1 · 2024-08-12T17:30:11Z

sfputil/main.py

@@ -1991,11 +1991,44 @@ def loopback(port_name, loopback_mode):
        click.echo("{}: This functionality is not implemented".format(port_name))
        sys.exit(ERROR_NOT_IMPLEMENTED)

+    namespaces = multi_asic.get_front_end_namespaces()
+    for namespace in namespaces:


@xinyulin Instead of using the for loop to traverse over all possible namespaces, you can directly get the namespace of the port by calling multi_asic.get_namespace_for_port(port_name)

@mihirpat1 Thanks for the example, I have modified it.

mihirpat1 · 2024-08-12T17:38:13Z

sfputil/main.py

+        config_db = ConfigDBConnector(use_unix_socket_path=True, namespace=namespace)
+        if config_db is not None:
+            config_db.connect()
+            subport = int(config_db.get(config_db.CONFIG_DB,


@xinyulin Please handle case wherein subport field is not defined in CONFIG_DB. You can assume subport to be 0 in such case.

@mihirpat1 Could we simply assign the subport to 1 if the subport is not defined in the CONFIG_DB (subport is None) or is mistakenly set to 0?
if subport is None or subport == 0: subport = 1

@xinyulin Yes, this should be fine as well

sfputil/main.py

Signed-off-by: xinyu <xinyu0123@gmail.com>

mihirpat1 · 2024-08-14T21:17:34Z

sfputil/main.py

+        subport = int(config_db.get(config_db.CONFIG_DB,
+                                    'PORT|{}'.format(port_name), 'subport'))
+
+        # If it is not defined (None) or if it is set to 0, assign a default value of 1


Suggested change

# If it is not defined (None) or if it is set to 0, assign a default value of 1

# If subport is not defined (None) or set to 0, assign a default value of 1

@mihirpat1 Thank you for correcting the wording. I also added more loopback_mode options to allow users to individually clear each mode in the latest commit.

@click.argument('loopback_mode', required=True, default="none", type=click.Choice(["none", "host-side-input", "host-side-output", "media-side-input", "media-side-output", "host-side-input-none", "host-side-output-none", "host-side-input-none", "host-side-output-none"]))

I’ve kept none as a valid loopback_mode option. Do you think it’s useful for users who want to completely clear all loopback modes on the physical port at once? Or should we remove this none option and require users to disable the loopback mode individually?

…FIG_DB

Signed-off-by: xinyu <xinyu0123@gmail.com>

mihirpat1 · 2024-08-18T17:48:38Z

sfputil/main.py

+        click.echo("Failed to connect to STATE_DB")
+        sys.exit(EXIT_FAIL)
+
+    if loopback_mode in ("host-side-input", "host-side-output",


Can we replace this block with something similar to the below?

lane_mask = 0 if "host-side" in loopback_mode: lane_mask = ((1 << host_lane_count) - 1) << ((subport - 1) * host_lane_count) elif "media-side" in loopback_mode: lane_mask = ((1 << media_lane_count) - 1) << ((subport - 1) * media_lane_count)

Thanks! It looks compact and clean!

mihirpat1 · 2024-08-19T05:53:04Z

sfputil/main.py

@@ -1469,10 +1469,10 @@ def is_fw_switch_done(port_name):
                status = -1 # Abnormal status.
            elif (ImageARunning == 1) and (ImageACommitted == 0):   # ImageA is running, but not committed.
                click.echo("FW images switch successful : ImageA is running")
-                status = 1  # run_firmware is done. 
+                status = 1  # run_firmware is done.


@xinyulin Can you please revert this line

mihirpat1 · 2024-08-19T05:53:13Z

sfputil/main.py

            elif (ImageBRunning == 1) and (ImageBCommitted == 0):   # ImageB is running, but not committed.
                click.echo("FW images switch successful : ImageB is running")
-                status = 1  # run_firmware is done. 


@xinyulin Can you please revert this line

Oops! My editor automatically trimmed the trailing whitespace. Let me revert it and disable that feature.

mihirpat1 · 2024-08-19T06:01:36Z

sfputil/main.py

+    state_db = SonicV2Connector(use_unix_socket_path=False, namespace=namespace)
+    if state_db is not None:
+        state_db.connect(state_db.STATE_DB)
+        host_lane_count = int(state_db.get(state_db.STATE_DB,


@xinyulin Wondering if we should handle ValueError exception and return error to handle case wherein subport, host_lane_count or media_lane_count are not present in the DB.

@mihirpat1 It is helpful for the user to know about the incorrect configuration. The newly added commit catches the TypeError when the variable is not present in the DB.

try: subport = int(config_db.get(config_db.CONFIG_DB, f'PORT|{port_name}', 'subport')) except TypeError: click.echo(f"{port_name}: subport is not present in CONFIG_DB") sys.exit(EXIT_FAIL)

From the formula, the lane_mask will automatically be set to 0 when the host or media lane count is zero.

if 'host-side' in loopback_mode: lane_mask = ((1 << host_lane_count) - 1) << ((subport - 1) * host_lane_count) elif 'media-side' in loopback_mode: lane_mask = ((1 << media_lane_count) - 1) << ((subport - 1) * media_lane_count)

… clean Signed-off-by: xinyu <xinyu0123@gmail.com>

… host_lane_count, or media_lane_count is not present Signed-off-by: xinyu <xinyu0123@gmail.com>

…lane count Signed-off-by: xinyu <xinyu0123@gmail.com>

mihirpat1

@xinyulin Approving with a minor comment

mihirpat1 · 2024-09-10T20:24:28Z

doc/Command-Reference.md

@@ -3106,19 +3106,19 @@ This command is the standard CMIS diagnostic control used for troubleshooting li

 - Usage:
  ```
-  sfputil debug loopback PORT_NAME LOOPBACK_MODE
+  sfputil debug loopback PORT_NAME LOOPBACK_MODE <enable/disable>

  Set the loopback mode


Suggested change

Set the loopback mode

Valid values for LOOPBACK_MODE

@mihirpat1 Thanks! It has been revised.

…ove clarity Signed-off-by: xinyu <xinyu0123@gmail.com>

prgeor · 2024-09-11T17:53:43Z

@bingwang-ms can you cherry pick to 202405? Its a new CLI for debugging.

mihirpat1 · 2024-09-11T18:04:17Z

@yxieca Can you please help to cherry pick this to 202311?
MSFT ADO - 29396307

…s of the logical port (sonic-net#3485) * [sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port Signed-off-by: xinyu <xinyu0123@gmail.com>

mssonicbld · 2024-09-11T22:38:38Z

Cherry-pick PR to 202311: #3543

…s of the logical port (#3485) * [sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port Signed-off-by: xinyu <xinyu0123@gmail.com>

…s of the logical port (sonic-net#3485) * [sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port Signed-off-by: xinyu <xinyu0123@gmail.com>

mssonicbld · 2024-09-20T21:23:11Z

Cherry-pick PR to 202405: #3552

…s of the logical port (#3485) * [sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port Signed-off-by: xinyu <xinyu0123@gmail.com>

xinyulin force-pushed the sfputil_loopback_lanemask branch 3 times, most recently from cc4e0ca to 99f5c92 Compare August 10, 2024 00:47

[sfputil] Configure the debug loopback mode only on the relevant lane…

0de47b2

…s of the logical port Signed-off-by: xinyu <xinyu0123@gmail.com>

xinyulin force-pushed the sfputil_loopback_lanemask branch from 99f5c92 to 0de47b2 Compare August 10, 2024 02:22

mihirpat1 reviewed Aug 12, 2024

View reviewed changes

mihirpat1 requested a review from prgeor August 12, 2024 20:07

mihirpat1 added Request for 202311 Branch Request for 202405 Branch labels Aug 12, 2024

xinyulin force-pushed the sfputil_loopback_lanemask branch from fb3e74b to b922081 Compare August 13, 2024 07:59

[sfputil] Get namespace instance directly from the logical port

c7010e2

Signed-off-by: xinyu <xinyu0123@gmail.com>

xinyulin force-pushed the sfputil_loopback_lanemask branch from b922081 to 33bd87c Compare August 14, 2024 01:58

mihirpat1 reviewed Aug 17, 2024

View reviewed changes

[sfputil] Assign subport to 1 if it is not defined or set to 0 in CON…

a4362be

…FIG_DB

xinyulin force-pushed the sfputil_loopback_lanemask branch from 33bd87c to 1f7112b Compare August 18, 2024 10:38

[sfputil] Update loopback_mode options to individually clear each mode

4d85d22

Signed-off-by: xinyu <xinyu0123@gmail.com>

xinyulin force-pushed the sfputil_loopback_lanemask branch from 1f7112b to 4d85d22 Compare August 18, 2024 10:50

mihirpat1 reviewed Aug 18, 2024

View reviewed changes

mihirpat1 reviewed Aug 19, 2024

View reviewed changes

[sfputil] Simplify the loopback condition to make it more compact and…

af0508c

… clean Signed-off-by: xinyu <xinyu0123@gmail.com>

xinyulin force-pushed the sfputil_loopback_lanemask branch 7 times, most recently from 2e69024 to 22d2634 Compare August 20, 2024 10:12

[sfputil] Reject the loopback command and report an error if subport,…

e1c7aa1

… host_lane_count, or media_lane_count is not present Signed-off-by: xinyu <xinyu0123@gmail.com>

xinyulin force-pushed the sfputil_loopback_lanemask branch from 22d2634 to e1c7aa1 Compare August 20, 2024 12:48

xinyulin force-pushed the sfputil_loopback_lanemask branch from 52ebfb3 to b78f125 Compare September 3, 2024 11:26

xinyulin force-pushed the sfputil_loopback_lanemask branch 3 times, most recently from aff5a53 to 9f8ae79 Compare September 10, 2024 03:47

[sfputil] Add generic function to get lane mask by given subport and …

2aaf92f

…lane count Signed-off-by: xinyu <xinyu0123@gmail.com>

xinyulin force-pushed the sfputil_loopback_lanemask branch from 9f8ae79 to 2aaf92f Compare September 10, 2024 14:30

mihirpat1 previously approved these changes Sep 10, 2024

View reviewed changes

[sfputil] Update the loopback command reference documentation to impr…

96eac89

…ove clarity Signed-off-by: xinyu <xinyu0123@gmail.com>

xinyulin dismissed mihirpat1’s stale review via 96eac89 September 11, 2024 06:06

prgeor approved these changes Sep 11, 2024

View reviewed changes

mihirpat1 approved these changes Sep 11, 2024

View reviewed changes

prgeor merged commit 2cb8cc6 into sonic-net:master Sep 11, 2024
7 checks passed

yxieca added the Approved for 202311 Branch label Sep 11, 2024

mssonicbld mentioned this pull request Sep 11, 2024

[action] [PR:3485] [sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port (#3485) #3543

Merged

mssonicbld added the Created PR to 202311 Branch label Sep 11, 2024

mssonicbld added Included in 202311 Branch and removed Created PR to 202311 Branch labels Sep 12, 2024

xinyulin deleted the sfputil_loopback_lanemask branch September 12, 2024 04:26

bingwang-ms added the Approved for 202405 Branch label Sep 20, 2024

mssonicbld mentioned this pull request Sep 20, 2024

[action] [PR:3485] [sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port (#3485) #3552

Merged

mssonicbld added the Created PR to 202405 Branch label Sep 20, 2024

mssonicbld added Included in 202405 Branch and removed Created PR to 202405 Branch labels Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port #3485

[sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port #3485

xinyulin commented Aug 10, 2024 •

edited

Loading

mihirpat1 Aug 12, 2024

xinyulin Aug 13, 2024

mihirpat1 Aug 12, 2024

xinyulin Aug 13, 2024

mihirpat1 Aug 13, 2024

mihirpat1 Aug 14, 2024

xinyulin Aug 18, 2024

mihirpat1 Aug 18, 2024

xinyulin Aug 19, 2024

mihirpat1 Aug 19, 2024

mihirpat1 Aug 19, 2024

xinyulin Aug 19, 2024

mihirpat1 Aug 19, 2024

xinyulin Aug 20, 2024

mihirpat1 left a comment

mihirpat1 Sep 10, 2024

xinyulin Sep 11, 2024

prgeor commented Sep 11, 2024

mihirpat1 commented Sep 11, 2024

mssonicbld commented Sep 11, 2024

mssonicbld commented Sep 20, 2024

	# If it is not defined (None) or if it is set to 0, assign a default value of 1
	# If subport is not defined (None) or set to 0, assign a default value of 1

[sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port #3485

[sfputil] Configure the debug loopback mode only on the relevant lanes of the logical port #3485

Conversation

xinyulin commented Aug 10, 2024 • edited Loading

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihirpat1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prgeor commented Sep 11, 2024

mihirpat1 commented Sep 11, 2024

mssonicbld commented Sep 11, 2024

mssonicbld commented Sep 20, 2024

xinyulin commented Aug 10, 2024 •

edited

Loading