[CMIS] Add lane_mask parameter to set_loopback_mode() to enable setti… #490

xinyulin · 2024-08-05T08:22:35Z

Description

This commit introduces two additional input parameters, lane_mask and enable, to the set_loopback_mode() function. By specifying lane_mask, users can enable or disable the loopback mode on individual lanes rather than applying it to the entire physical port

Motivation and Context

Previously, the set_loopback_mode() function affected all lanes of a physical port simultaneously, limiting flexibility. By adding the lane_mask parameter, this update allows callers to target specific lanes, thereby enabling more precise testing, debugging, and configuration of network equipment.

How Has This Been Tested?

Only the corresponding lanes of the logical port will be manipulated with the newly updated sfputil debug loopback command.

Test log for the following reject cases:loopback_error_case_test_log.txt

Simultaneous host media loopback is not supported
Host/media output loopback is not supported
Per-lane/host host loopback is not supported

…ng loopback mode for individual lanes rather than the entire physical port Signed-off-by: xinyu <[email protected]>

mihirpat1

@xinyulin Can we have a check to ensure that for modules which do not support SimultaneousHostAndMediaSideLoopbacks (13h, Byte 128, bit 6), this API rejects such config wherein caller is trying to configure simultaneous host and media side loopbacks?

mihirpat1 · 2024-08-12T17:12:32Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

        1. "none" (default)
        2. "host-side-input"
        3. "host-side-output"
        4. "media-side-input"
        5. "media-side-output"
+        lane_mask: A bitmask representing which lanes to apply the loopback mode to.
+        The default value of 0xFF indicates that the mode should be applied to all lanes.
+
        The function will look at 13h:128 to check advertized loopback capabilities.
        Return True if the provision succeeds, False if it fails
        '''


@xinyulin As part of clearing the loopback (line 1136-1140), can we make sure that the loopback is cleared only for the lane_mask lanes rather than all lanes?
Also, should we check if the module advertises per lane loopback support and then use lane_mask accordingly. This will be helpful to clear loopback on all lanes if per lane loopback is not supported.

Sure! we'd better to reject the request if simultaneous host media loopback mode is not supported.

To clear specific lanes corresponding to the logical port for the none mode, we will need to have two lane masks (host_lane_mask and media_lane_mask for host and media individually) because the host and media lane masks might not be identical (e.g., host_lane_mask = 0x3, media_lane_mask = 0x1).

If per-lane loopback mode is not supported, should we reject the request or configure the entire physical port instead?

Do you think it is useful to log some messages for the reject cases?

@xinyulin
For 2., we can expect loopback_mode to include the type of loopback to be cleared i.e. have host-side-input-none, host-side-output-none etc. In this case, the caller can pass the lane_mask accordingly.
For 3., I think we should configure entire physical port and display a message. @prgeor Can you please confirm the expected behavior in this case?
For 4., Yes, I think it is helpful to log messages in reject cases.

@mihirpat1 Thanks for the comments. I've updated it accordingly. Let's check if Prince has any comments on the changes.

@xinyulin I discussed with @prgeor regarding 3. and concluded to reject the config if per lane loopback is not supported (i.e. we should not configure unrelated lanes when loopback is intended to configure specific lanes).

@mihirpat1 Thanks for the confirmation. It has been updated!

mihirpat1

@xinyulin Can you please attach an error log showing that simultaneous loopback is not supported when loopback is performed on a module which does not support simultaneous host and media side loopback.

…pback modes and log errors for all rejection cases Signed-off-by: xinyu <[email protected]>

…a loopback is not supported or per-lane loopback is not supported Signed-off-by: xinyu <[email protected]>

Signed-off-by: xinyu <[email protected]>

xinyulin · 2024-08-21T07:46:14Z

@xinyulin Can you please attach an error log showing that simultaneous loopback is not supported when loopback is performed on a module which does not support simultaneous host and media side loopback.

I have attached it in the PR description, it covers the various unsupported rejection cases. Please let me know if we need to provide more information or make it clearer to read

prgeor · 2024-08-21T15:34:43Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+
+        if 'none' in loopback_mode:
+            if loopback_mode == 'none':
+                status_host_input = self.xcvr_eeprom.write(consts.HOST_INPUT_LOOPBACK, 0)
+                status_host_output = self.xcvr_eeprom.write(consts.HOST_OUTPUT_LOOPBACK, 0)
+                status_media_input = self.xcvr_eeprom.write(consts.MEDIA_INPUT_LOOPBACK, 0)
+                status_media_output = self.xcvr_eeprom.write(consts.MEDIA_OUTPUT_LOOPBACK, 0)
+                return all([status_host_input, status_host_output, status_media_input, status_media_output])
+
+            if loopback_mode == 'host-side-input-none':
+                return self.xcvr_eeprom.write(consts.HOST_INPUT_LOOPBACK, host_input_val & ~lane_mask)
+
+            if loopback_mode == 'host-side-output-none':
+                return self.xcvr_eeprom.write(consts.HOST_OUTPUT_LOOPBACK, host_output_val & ~lane_mask)
+
+            if loopback_mode == 'media-side-input-none':
+                return self.xcvr_eeprom.write(consts.MEDIA_INPUT_LOOPBACK, media_input_val & ~lane_mask)
+
+            if loopback_mode == 'media-side-output-none':
+                return self.xcvr_eeprom.write(consts.MEDIA_OUTPUT_LOOPBACK, media_output_val & ~lane_mask)
        else:
-            return False
+            if loopback_capability['simultaneous_host_media_loopback_supported'] is False:
+                if any(['host' in loopback_mode and (media_input_val or media_output_val),
+                        'media' in loopback_mode and (host_input_val or host_output_val)]):
+                    txt = 'Simultaneous host media loopback is not supported\n'
+                    txt += f'host_input_val:{host_input_val:02x}, host_output_val:{host_output_val:02x}, '
+                    txt += f'media_input_val:{media_input_val:02x}, media_output_val:{media_output_val:02x}\n'
+                    logger.error(txt)
+                    return False
+
+            if loopback_mode == 'host-side-input' and host_input_support:
+                return self.xcvr_eeprom.write(consts.HOST_INPUT_LOOPBACK, host_input_val | lane_mask)
+
+            if loopback_mode == 'host-side-output' and host_output_support:
+                return self.xcvr_eeprom.write(consts.HOST_OUTPUT_LOOPBACK, host_output_val | lane_mask)
+
+            if loopback_mode == 'media-side-input' and media_input_support:
+                return self.xcvr_eeprom.write(consts.MEDIA_INPUT_LOOPBACK, media_input_val | lane_mask)
+
+            if loopback_mode == 'media-side-output' and media_output_support:
+                return self.xcvr_eeprom.write(consts.MEDIA_OUTPUT_LOOPBACK, media_output_val | lane_mask)
+
+        txt = f'Failed to set {loopback_mode} loopback, lane_mask:{lane_mask:02x}\n'
+        txt += f'host_input_support:{host_input_support}, host_output_support:{host_output_support}, '
+        txt += f'media_input_support:{media_input_support}, media_output_support:{media_output_support}\n'
+        txt += f'host_input_val:{host_input_val:02x}, host_output_val:{host_output_val:02x}, '
+        txt += f'media_input_val:{media_input_val:02x}, media_output_val:{media_output_val:02x}\n'


@mihirpat1 @xinyulin can we write 4 different functions
set_host_input_loopback()
set_host_ouput_loopback()
set_media_output_loopback()
set_media_input_loopback()

And use these functions to make code more readable

@prgeor Yes, that would be much clearer to read. Do you think we should keep the set_loopback_mode() function since it has been there for years, or should we remove it and replace it with the four functions you proposed?

set_host_input_loopback(lane_mask, enable)
set_host_ouput_loopback(lane_mask, enable)
set_media_output_loopback(lane_mask, enable)
set_media_input_loopback(lane_mask, enable)

@xinyulin

def set_loopback_mode(self, loopback_mode, lane_mask = 0xff): if (loopback_mode == host-side-ouput") : set_host_ouput_loopback(lane_mask, enable) else if (loopback_mode == host-side-input") : set_host_input_loopback(lane_mask, enable) else if (loopback_mode == media-side-input") : set_media_output_loopback(lane_mask, enable)

@prgeor Thank you for the suggestions. It looks more clear and readable now.

…host, media, input, and output loopback modes Signed-off-by: xinyu <[email protected]>

prgeor · 2024-08-31T19:55:47Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+        else:
+            return self.xcvr_eeprom.write(consts.MEDIA_OUTPUT_LOOPBACK, media_output_val & ~lane_mask)
+
+    def set_loopback_mode(self, loopback_mode, lane_mask = 0xff):


@xinyulin can you add an argument enable to this function. Enable can be True or False.

prgeor · 2024-08-31T19:58:02Z

sonic_platform_base/sonic_xcvr/api/public/cmis.py

+            2. "host-side-input-none"
+            3. "host-side-output-none"
+            4. "media-side-input-none"
+            5. "media-side-output-none"


@xinyulin please see my comments below. if you add one argument enable we don't need "none"

@prgeor Yes, the enable input parameter has been added, and all loopback modes with the none suffix have been removed in the latest commit.
I've also updated the PR3485, could you please help me review it

prgeor · 2024-09-02T21:33:26Z

@mihirpat1 can you review?

…es with the -none suffix Signed-off-by: xinyu <[email protected]>

mihirpat1 · 2024-09-10T23:35:37Z

@xinyulin Can you please help to cherry-pick this manually to 202311 branch since there is a merge conflict?
MSFT ADO - 28898451

@yxieca @prgeor for viz

yxieca · 2024-09-10T23:59:12Z

@mihirpat1 @xinyulin please help raise 202311 PR. This RP cannot cherry-picked cleanly. Please test on 202311 branch before raising the PR. Thanks.

xinyulin · 2024-09-12T04:22:21Z

@mihirpat1 @yxieca PR #499 has been created and tested. Please review and merge it, thank you!

mssonicbld · 2024-10-17T17:35:21Z

Cherry-pick PR to 202405: #505

[CMIS] Add lane_mask parameter to set_loopback_mode() to enable setti…

36a0294

…ng loopback mode for individual lanes rather than the entire physical port Signed-off-by: xinyu <[email protected]>

mihirpat1 requested review from mihirpat1 and prgeor August 12, 2024 17:14

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

mihirpat1 reviewed Aug 12, 2024

View reviewed changes

xinyulin force-pushed the set_loopback_mode branch 8 times, most recently from f339ec3 to a65614b Compare August 18, 2024 12:35

mihirpat1 reviewed Aug 20, 2024

View reviewed changes

xinyulin added 3 commits August 21, 2024 12:25

[CMIS] Add options to individually clear host/media, input/output loo…

b2ac81c

…pback modes and log errors for all rejection cases Signed-off-by: xinyu <[email protected]>

[CMIS] Reject the loopback mode request if simultaneous host and medi…

6801fa0

…a loopback is not supported or per-lane loopback is not supported Signed-off-by: xinyu <[email protected]>

[CMIS] Log errors for all set loopback mode rejection cases

7105a22

Signed-off-by: xinyu <[email protected]>

xinyulin force-pushed the set_loopback_mode branch from a65614b to 7105a22 Compare August 21, 2024 04:33

prgeor reviewed Aug 21, 2024

View reviewed changes

xinyulin force-pushed the set_loopback_mode branch 4 times, most recently from 4e9dbc5 to 143c8ad Compare August 26, 2024 06:34

mssonicbld added the Cherry Pick Conflict_202311 label Aug 26, 2024

xinyulin force-pushed the set_loopback_mode branch from 143c8ad to d4b6832 Compare August 26, 2024 07:07

[CMIS] Improve readability by using individual functions for setting …

dd39936

…host, media, input, and output loopback modes Signed-off-by: xinyu <[email protected]>

xinyulin force-pushed the set_loopback_mode branch from d4b6832 to dd39936 Compare August 26, 2024 07:51

xinyulin requested a review from prgeor August 26, 2024 10:02

xinyulin requested a review from mihirpat1 August 26, 2024 10:02

prgeor reviewed Aug 31, 2024

View reviewed changes

prgeor previously approved these changes Sep 2, 2024

View reviewed changes

[CMIS] Add an enable parameter to eliminate the need for loopback mod…

9faee1a

…es with the -none suffix Signed-off-by: xinyu <[email protected]>

xinyulin dismissed prgeor’s stale review via 9faee1a September 3, 2024 05:46

mihirpat1 approved these changes Sep 4, 2024

View reviewed changes

prgeor approved these changes Sep 8, 2024

View reviewed changes

prgeor merged commit ccea995 into sonic-net:master Sep 8, 2024
5 checks passed

xinyulin deleted the set_loopback_mode branch September 12, 2024 04:25

yxieca added the Included in 202311 Branch label Sep 17, 2024

bingwang-ms added the Approved for 202405 Branch label Oct 17, 2024

mssonicbld mentioned this pull request Oct 17, 2024

[action] [PR:490] [CMIS] Add lane_mask parameter to set_loopback_mode() to enable setti… (#490) #505

Merged

mssonicbld added the Created PR to 202405 Branch label Oct 17, 2024

mssonicbld added Included in 202405 Branch and removed Created PR to 202405 Branch labels Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CMIS] Add lane_mask parameter to set_loopback_mode() to enable setti… #490

[CMIS] Add lane_mask parameter to set_loopback_mode() to enable setti… #490

xinyulin commented Aug 5, 2024 •

edited

Loading

mihirpat1 left a comment

mihirpat1 Aug 12, 2024

xinyulin Aug 13, 2024 •

edited

Loading

mihirpat1 Aug 13, 2024

xinyulin Aug 18, 2024

mihirpat1 Aug 20, 2024

xinyulin Aug 21, 2024

mihirpat1 left a comment

xinyulin commented Aug 21, 2024

prgeor Aug 21, 2024

xinyulin Aug 22, 2024

prgeor Aug 24, 2024

xinyulin Aug 26, 2024

prgeor Aug 31, 2024

prgeor Aug 31, 2024

xinyulin Sep 4, 2024 •

edited

Loading

prgeor commented Sep 2, 2024

mihirpat1 commented Sep 10, 2024

yxieca commented Sep 10, 2024

xinyulin commented Sep 12, 2024

mssonicbld commented Oct 17, 2024

[CMIS] Add lane_mask parameter to set_loopback_mode() to enable setti… #490

[CMIS] Add lane_mask parameter to set_loopback_mode() to enable setti… #490

Conversation

xinyulin commented Aug 5, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

mihirpat1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinyulin Aug 13, 2024 • edited Loading

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

xinyulin commented Aug 21, 2024

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

xinyulin Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

prgeor commented Sep 2, 2024

mihirpat1 commented Sep 10, 2024

yxieca commented Sep 10, 2024

xinyulin commented Sep 12, 2024

mssonicbld commented Oct 17, 2024

xinyulin commented Aug 5, 2024 •

edited

Loading

xinyulin Aug 13, 2024 •

edited

Loading

xinyulin Sep 4, 2024 •

edited

Loading