Retrieve subport from CONFIG_DB to enable breakout support #342

mihirpat1 · 2023-03-07T05:35:55Z

The changes with this PR needs to be merged along with sonic-net/sonic-platform-common#352

Description

Currently, xcvrd has a fixed logic to calculate active host lanes for a port. This needs to be changed to support ports with breakout cable.

Motivation and Context

The fixed host lane mapping will now be replaced by reading the subport from the CONFIG_DB. The subport will then be used to calculate active host lanes by reading the Application Descriptors.

Finding the desired application
The HostInterfaceID and HostLaneCount from Application Descriptors will be used to find the desired application for the port with breakout cable by comparing these with speed and lanes retrieved from CONFIG_DB.

Finding active host lane
Once the desired application is found, the HostLaneAssignmentOptions (bitmap of active host lanes) for the corresponding application and the subport from CONFIG_DB will be used to find the active host lanes.

In addition to the above, check_datapath_init_pending is now being called after ConfigStatusLane register has the value "ConfigSuccess". This has been done to be inline with the CMIS spec.

Note
Once the port_config.ini file has a new column for subport, the CONFIG_DB will be updated with the corresponding subport values (requires minigraph reload though).
The field "subport" will have the below value:
0 - This means the xcvr in non-breakout type
non-zero - The value here corresponds to the subport group (formed by a group of active lanes) to which the port belongs to

How Has This Been Tested?

Testcase summary
The value of host_lanes can be seen in the logs by finding lanemask.
400G related TCs

SFP EEPROM dump
show int status
LLDP o/p
Log dump for CMIS init

200G breakout related TCs - 200G side

SFP EEPROM dump
show int status
LLDP o/p
Log dump for CMIS init

200G breakout related TCs - 100G side

SFP EEPROM dump
show int status
LLDP o/p
Log dump for CMIS init
Shut/no shut on subports (200G and 100G sides)
config reload (of Fixed chassis) with both native and break-out optics inserted
reboot (of Fixed chassis) with both native and break-out optics inserted
Remove and re-insert 2x100G break-out optic at run-time (system running)
xcvrd restart - Ensure link flap is not seen
PMON restart - Ensure link flap is not seen
SWSS restart
SYNCD restart
CONFIG_DB for a port after adding subport to port_config.ini

For additional details, please refer to Unit-test_breakout_channel_v5.txt

The code changes have been tested on a chassis with 200G AOC breakout cable.
Also, the code changes have been tested on a chassis with 400ZR transceiver and 10GBase-SR

Additional Information (Optional)

Signed-off-by: Mihir Patel <[email protected]>

…daemons into channel_breakout_support

mihirpat1 · 2023-03-07T07:53:31Z

@prgeor @jaganbal-a @keboliu @shyam77git - It will be great if you can help in reviewing this PR

prgeor · 2023-03-07T18:13:03Z

@shyam77git @jaganbal-a please review

prgeor · 2023-03-07T18:13:16Z

@keboliu please review

jaganbal-a · 2023-03-08T20:03:05Z

how the 4x10 or 4x25 breakout is taken cared for 100G/40G module ?

…be inline with CMIS spec

mihirpat1 · 2023-03-09T04:19:12Z

how the 4x10 or 4x25 breakout is taken cared for 100G/40G module ?

The API get_cmis_application_desired will return the application ID by matching the speed and number of active host lanes from CONFIG_DB. This application ID will then be used by get_cmis_host_lanes function to find the appropriate active host lanes.

jaganbal-a · 2023-03-09T16:53:47Z

I get_cmis_application_desired will return the application ID by matching the speed and number of active host lanes from CONFIG_DB. This application ID will then be used by get_cmis_host_lanes function to find the appropriate active host lanes.

get_cmis_host_lanes() is not supposed to be invoked for non-CMIS module isn't it ?
the question is for breakout 4x25G and 4x10G on non-cmis (100G/40G modules)

sonic-xcvrd/xcvrd/xcvrd.py

mihirpat1 · 2023-03-09T21:29:28Z

hannel > host_lane_

I get_cmis_application_desired will return the application ID by matching the speed and number of active host lanes from CONFIG_DB. This application ID will then be used by get_cmis_host_lanes function to find the appropriate active host lanes.

get_cmis_host_lanes() is not supposed to be invoked for non-CMIS module isn't it ? the question is for breakout 4x25G and 4x10G on non-cmis (100G/40G modules)

Yes, get_cmis_host_lanes() will not be invoked for non-CMIS module.
Since 4x25G and 4x10G are non-CMIS modules, datapath initialization from xcvrd is not required and hence, the current changes are not needed for such modules.
The expectation is that existing bring-up sequence outside of xcvrd should take care of initial reset for such modules and then, it should work as is without the intervention of xcvrd.

shyam77git · 2023-03-10T01:55:49Z

hannel > host_lane_

I get_cmis_application_desired will return the application ID by matching the speed and number of active host lanes from CONFIG_DB. This application ID will then be used by get_cmis_host_lanes function to find the appropriate active host lanes.

get_cmis_host_lanes() is not supposed to be invoked for non-CMIS module isn't it ? the question is for breakout 4x25G and 4x10G on non-cmis (100G/40G modules)

Yes, get_cmis_host_lanes() will not be invoked for non-CMIS module. Since 4x25G and 4x10G are non-CMIS modules, datapath initialization from xcvrd is not required and hence, the current changes are not needed for such modules. The expectation is that existing bring-up sequence outside of xcvrd should take care of initial reset for such modules and then, it should work as is without the intervention of xcvrd.

@mihirpat1 , @prgeor
For non-CMIS compliant modules (i.e. SFF* spec based ones), one cannot expect them to work seamlessly.
Beside xcvrd, following areas to be checked from code-flow and validation standpoint:

syncd vertical (syncd-> SAI-> SDK) w.r.t handling the port brea-out
use case: shut, no-shut on a particular channel (broken-down/logical port) of an interface
though xcvrd vertical seems to have no role (per above-mentioned explanation), still its part of end-to-end validation to ensure no open item/issue
I'd suggest to file a separate git issue for this

shyam77git · 2023-03-10T17:23:00Z

@mihirpat1
Besides the UT cases you mentioned in description section and attached UT logs, could you validate the following

config reload (of Fixed chassis) with both native and break-out optics inserted
reboot (of Fixed chassis) with both native and break-out optics inserted
Mode change: Change existing (inserted) optics to 4x100G or 2x100G breakout mode
Mode change: Change/revert from breakout mode to native mode
Insert break-out optics at run-time (system running): 4x100G or 2x100G break-out optics

shyam77git · 2023-03-10T17:28:55Z

Wanted to keep following comment separate from above UT comment:
How about port break-out validation on Modular chassis's LC?
IMO, should be covered here to ensure no issue

shyam77git · 2023-03-10T17:49:49Z

@mihirpat1
Don't see mention and/or validation of SONiC breakout CLI !
Could you configure and validate following SONiC breakout CLIs?

#config interface breakout <interface_name>
show interfaces breakout
show interfaces breakout current-mode
show interfaces breakout current-mode <interface_name>

prgeor · 2023-03-18T02:06:12Z

sonic-xcvrd/xcvrd/xcvrd.py

@@ -1121,7 +1159,7 @@ def is_cmis_application_update_required(self, api, channel, speed):
        if speed == 0 or channel == 0 or api.is_flat_memory():
            return False

-        app_new = self.get_cmis_application_desired(api, channel, speed)
+        app_new = self.get_cmis_application_desired(api, host_lane_count, speed)


current logic checks for all 8 host lanes whereas we should care for the requested host lanes only

Also we should make use of the selected/desired application during init state and compare it with the current active application

Also we should check for DP != Activated early in the function

I have handled #2 now. As discussed, we are already handling #1 and #3 and have changed CMIS_NUM_CHANNELS to CMIS_MAX_HOST_LANES.

2. Removed unwanted parameter for is_cmis_application_update_required 3. Changed channel to host_lanes_mask in multiple functions to avoid confusion 4. Reading application code and host_lanes_mask as part of just CMIS_STATE_INSERTED

keboliu · 2023-03-23T02:38:57Z

In the Notes, "Once the port_config.ini file has a new column for channel, the CONFIG_DB will be updated with the corresponding channel values (requires minigraph reload though)."

I am a bit of concern that to extend "port_config.ini" for this purpose, AFAIK, "platform.json" was introduced for the dynamic port breakout, I think we should consider extending it instead of "port_config.ini".
by the way, have you already had a PR for this part?

prgeor · 2023-03-23T05:07:02Z

In the Notes, "Once the port_config.ini file has a new column for channel, the CONFIG_DB will be updated with the corresponding channel values (requires minigraph reload though)."

I am a bit of concern that to extend "port_config.ini" for this purpose, AFAIK, "platform.json" was introduced for the dynamic port breakout, I think we should consider extending it instead of "port_config.ini". by the way, have you already had a PR for this part?

@keboliu we can remove the reference to port_config.ini as its not relevant to this Xcvrd changes. Please see my email.

keboliu · 2023-03-23T13:01:10Z

As I commented in PR, sonic-net/SONiC#1290

Using 'channel' to refer to a 'sub-port' is misleading. XCVRD is mainly dealing with optical cables. In the optical cables context, 'channel' is a concept which already widely used and has a clear definition, we even have SONiC platform APIs defined to handle optical cable 'channels'. I suggest to use use another name to avoid confusion.

Another point is that not sure the 'channel' number should be provided explicitly from some configuration, can it be deduced by some logic? It's shouldn't be too complicated.

rajann · 2023-03-29T17:02:51Z

I believe that instead of explicitly giving the subport/channel info in port_config.ini it should be derived from the existing information in port_config.ini (i.e.) we have defined the rules for "name" , "alias" and "index". Same convention should be following in DPB as well. If there is difference in DPB with the above rules, it should be addressed in the DPB document to match it. If we diverge here then we will have to do the same additional information in DPB as well.

mihirpat1 · 2023-04-01T18:35:47Z

I believe that instead of explicitly giving the subport/channel info in port_config.ini it should be derived from the existing information in port_config.ini (i.e.) we have defined the rules for "name" , "alias" and "index". Same convention should be following in DPB as well. If there is difference in DPB with the above rules, it should be addressed in the DPB document to match it. If we diverge here then we will have to do the same additional information in DPB as well.

As discussed, having a logic in xcvrd to calculate subport info introduces dependencies on neighboring subports belonging to the same physical group. This also relies on the fact that redis-db will always populate info for all subports in a physical port group before xcvrd actually starts using info for a subport.
In addition to this, the current physical to logical port list is not in sorted order and will need to be sorted in appropriate order before using it.

prgeor · 2023-04-04T01:55:24Z

sonic-xcvrd/xcvrd/xcvrd.py

+            host_lane_count:
+                Integer, number of lanes on the host side
+            subport:
+                Integer, subport id of the group which the host lanes belong to (1 based),


@mihirpat1 subport:
1-based logical port number of the physical port after breakout

Addressed in the new diff now.

prgeor · 2023-04-04T16:16:51Z

@yxieca please help cherry pick to 202205
@StormLiangMS please help cherry pick to 202211

Retrieve subport from CONFIG_DB to enable breakout support --------- Signed-off-by: Mihir Patel <[email protected]>

Based on the port breakout HLD, we are now using subport instead of channel in the CONFIG_DB PORT table to handle port breakout. The yang schema needs to be modified accordingly to handle the corresponding change. The corresponding code changes have been merged through sonic-net/sonic-platform-daemons/pull/342 merged Signed-off-by: Mihir Patel <[email protected]>

Retrieve subport from CONFIG_DB to enable breakout support --------- Signed-off-by: Mihir Patel <[email protected]>

…14519) Based on the port breakout HLD, we are now using subport instead of channel in the CONFIG_DB PORT table to handle port breakout. The yang schema needs to be modified accordingly to handle the corresponding change. The corresponding code changes have been merged through sonic-net/sonic-platform-daemons/pull/342 merged Signed-off-by: Mihir Patel <[email protected]>

mihirpat1 added 2 commits March 7, 2023 05:27

Retrieve channel from CONFIG_DB to enable breakout support

c6bb9a7

Signed-off-by: Mihir Patel <[email protected]>

Merge branch 'master' of https://github.com/sonic-net/sonic-platform-…

c13de6a

…daemons into channel_breakout_support

mihirpat1 changed the title ~~Channel breakout support~~ Retrieve channel from CONFIG_DB to enable breakout support Mar 7, 2023

mihirpat1 mentioned this pull request Mar 7, 2023

Modify get_host_lane_assignment_option to return value based on application id sonic-net/sonic-platform-common#352

Merged

Enhanced test cases

ed772fc

mihirpat1 requested a review from prgeor March 7, 2023 07:50

mihirpat1 marked this pull request as ready for review March 7, 2023 07:50

prgeor requested a review from keboliu March 7, 2023 18:09

prgeor added the CMIS label Mar 7, 2023

Moved datapath init pending status check after ConfigSuccess read to …

cbc7382

…be inline with CMIS spec

prgeor reviewed Mar 9, 2023

View reviewed changes