[sfp-refactoring] xcvrd: add initial support for CMIS application initialization by ds952811 · Pull Request #217 · sonic-net/sonic-platform-daemons

ds952811 · 2021-10-04T12:39:20Z

Signed-off-by: Dante Su [email protected]

Why I did it

Add initial support for CMIS application initialization

How I did it

Add CmisManagerTask to handle the per-port CMIS application initialization in parallel

How to verify it

Bring up Quanta IX9 with the default configuration and CMIS 4.0/5.0 QSFPDD modules
Issue 'show logging xcvrd' to check if the application initialization succeeded
Issue 'show interfaces transceiver eeprom --dom' to verify both the static and DOM information parsers
Issue 'sudo sfputil show eeprom --dom' to verify both the static and DOM information parsers

Which release branch to backport (provide reason below if selected)

Description for the changelog

Repo	PR title	State
SONiC	[sfp-refactoring] Initial support for CMIS application initialization
sonic-platform-common	[sfp-refactoring] Initial support for CMIS application initialization
sonic-platform-daemon	[sfp-refactoring] Initial support for CMIS application initialization

A picture of a cute animal (not mandatory but encouraged)

lgtm-com · 2021-10-04T12:48:22Z

This pull request introduces 3 alerts when merging f2b0adc into 6e4cf6f - view on LGTM.com

new alerts:

3 for Except block handles 'BaseException'

sonic-xcvrd/xcvrd/xcvrd.py

lgtm-com · 2021-11-05T09:44:33Z

This pull request introduces 1 alert when merging 999e156 into ddb22b9 - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

lgtm-com · 2021-11-09T11:42:15Z

This pull request introduces 1 alert when merging a61387c into 1fd3d16 - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

lgtm-com · 2021-11-12T07:48:42Z

This pull request introduces 1 alert when merging abe64de into 7b95211 - view on LGTM.com

new alerts:

1 for Except block handles 'BaseException'

aravindmani-1 · 2021-11-12T07:56:39Z

sonic-xcvrd/xcvrd/xcvrd.py

+    vendor_pn_str = transceiver_dict['model']
+    vendor_key = (vendor_name_str.strip() + '#' + vendor_pn_str.strip()).upper()
+
+    for k, v in g_dict[CMIS_MEDIA_SETTINGS_KEY].items():


I could see that this could is optic specific(vendor_name_str + part number)?.
Can it be made it as generic?.
This code skips application code for flat memory/media types other than QSFP_DD, so can we apply the settings for other media types directly?.

Does all the CMIS vendors also need to specify "CMIS_MEDIA_SETTINGS" in media_settings.json in their platforms?.

If the attached xcvr is not specified in CMIS_MEDIA_SETTINGS, or the CMIS_MEDIA_SETTINGS is missing, the CMIS application selection defaults to be enabled

The CMIS registers of application selection are at page 0x10 and 0x11, hence it's not available on modules with Flat Memory.

The idea for module-specific controls is for certain modules that have paged memory support while application selection is not necessary

you may include a condition to return failure for flat memory media. you can make use of the function defined in SFP refactored code. https://github.com/Azure/sonic-platform-common/blob/c8eceec53625b6c021b7c6619b2555d0c7a1b38d/sonic_platform_base/sonic_xcvr/api/public/cmis.py#L291

"> 3. The idea for module-specific controls is for certain modules that have paged memory support while application selection is not necessary
"
How do you determine whether the specific media needs application selection?.

Thanks for feedback, although we already have a checker for flat media in the xcvrd, I agree it's better to have this in the cmis.py as well.

Actually, the CMIS_MEDIA_SETTINGS is a backup plan for the faulty transceiver if any. And as of now, we don't really need this, please feel free to let me know if we should have it removed, thanks

Just for a record, the CMIS_MEDIA_SETTINGS is already removed.

mikeberesford · 2021-11-12T23:00:14Z

sonic-xcvrd/xcvrd/xcvrd.py

+
+        has_cmis = False
+        for sfp in platform_chassis.get_all_sfps():
+            if sfp.get_port_type() in [sfp.SFP_PORT_TYPE_QSFPDD]:


more than just QSFPDD supports CMIS

As of now, only QSFPDD (type=18h) is verified, we could circle back on OSFP(type=19h) once the sample is available

Just for a record, the get_port_type() checker is already removed in the latest commit

mikeberesford · 2021-11-12T23:00:38Z

sonic-xcvrd/xcvrd/xcvrd.py

+                has_cmis = True
+                break
+
+        if not has_cmis:


modules may be plugged in or removed dynamically

The check here is to validate the cage type rather than the media type of the attached transceiver, hence it does not matter what's the module attached to right now.
The main purpose is to reduce the CPU/memory usage when the service is not appliable to the target unit.
e.g. For a box with only SFP and QSFP28 cages, the CMIS Manager will not be activated.

Just for a record, the get_port_type() checker is already removed in the latest commit

mikeberesford · 2021-11-12T23:03:50Z

sonic-xcvrd/xcvrd/xcvrd.py

+                    if conf.get('application_selection', True):
+                        self.log_notice("{}: application update for {}G, {}-lanes".format(
+                            lport, int(speed/1000), len(lanes)))
+                        ret = sfp.set_cmis_application(speed, len(lanes))


How do we ensure that this is done after the ASIC SerDes taps are configured?

If you're referring to the pre-emphasis configuration on the MAC, it's handled by the orchagent when the serdes tap parameters are uploaded to the APPL_DB, hence it's outside the scope of this PR, the serdes tap parameters are independent with this PR, and the framework of media_settings.json needs to be revised to support speed/mode switches between PAM4 and NRZ if necessary.
If you're referring to the pre-emphasis configuration on the PHY, it's not supported as of now, we could circle back on this when the module-specific parameters are available and needs to be applied to the CMIS PHY registers.

Note: In the case of Broadocm ASIC, we have different value range for PAM4 and NRZ, hence having NRZ/PAM4 pre-emphasise applied to SerDes in PAM4/NRZ mode may or may not lead to checker failure in SAI, and in turn, the orchagent may or may not crash.

My concern is that (per my understanding) the ASIC SerDes pre/main/post setitngs should be configured before the CMIS ApSel sequence is started. There doesn't seem to be anything that enforces this sequencing here.

Yes, we need a synchronization mechanism between syncd/SAI and the xcvrd to gracefully switch in between the applications. Prince @ MSFT and I had this discussion yesterday, and he will be working on this, and we could deal with this in another PR

lgtm-com · 2021-11-18T12:55:20Z

This pull request introduces 4 alerts when merging a1d8e10 into e53ff26 - view on LGTM.com

new alerts:

4 for Unused local variable

sonic-xcvrd/xcvrd/xcvrd.py

ds952811 · 2021-11-18T13:05:37Z

sonic-xcvrd/xcvrd/xcvrd_utilities/port_mapping.py


+def subscribe_port_config_change(db_list=['CONFIG_DB']):
+    port_tbl_map = {
+        'APPL_DB': swsscommon.APP_PORT_TABLE_NAME,


We'll listen to APPL_DB for the PORT CONFIG changes, and STATE_DB for XCVR INSERTION/REMOVAL

I think we should listen to PORT_TABLE in STATE_TB instead of PORT_TABLE in APP_DB because, when you get SET event for a port in APP_DB, the orchagent has still not called the sai api to create the port yet. When SET event is received for a port in STATE_DB then its confirmed the PORT create has completed.

The trick is, not all the fields in APPL_DB will be updated to STATE_DB by the orchagent as of now, it's part of synchronization issue between syncd and orchagent, it's outside the scope of this PR

sonic-xcvrd/xcvrd/xcvrd.py

lgtm-com · 2021-11-19T03:25:22Z

This pull request introduces 4 alerts when merging 2bf4580 into e53ff26 - view on LGTM.com

new alerts:

4 for Unused local variable

keboliu · 2021-12-02T06:08:27Z

sonic-xcvrd/xcvrd/xcvrd_utilities/port_mapping.py

    PORT_REMOVE = 1
-    def __init__(self, port_name, port_index, asic_id, event_type):
+    PORT_SET = 2
+    PORT_DEL = 3


it hard to differentiate the four actions here from the name, would you mind rephrasing?

No, I could update the namings, but do you have any suggestions?

While I could surely update these namings, but I don't know which one is better, could you please help me on this?

keboliu · 2021-12-02T06:10:59Z

sonic-xcvrd/xcvrd/xcvrd.py

+
+        self.dbg_print("Stopped")
+
+    def task_run(self, y_cable_presence):


what's the purpose of having "y_cable_presence" here?

Yep, you're right, I'll remove it

prgeor · 2021-11-22T06:25:08Z

sonic-xcvrd/xcvrd/xcvrd_utilities/port_mapping.py


-
-def handle_port_config_change(sel, asic_context, stop_event, port_mapping, logger, port_change_event_handler):
+def handle_port_config_change(sel, asic_context, stop_event, port_mapping, logger, port_change_event_handler, promiscuous=False):


please add a separate handler for handling PORT_TABLE update events in APP_DB

Please check the latest commit, the requested changes are already done

prgeor · 2021-11-22T06:26:13Z

sonic-xcvrd/xcvrd/xcvrd_utilities/port_mapping.py

            else:
                return None

+def subscribe_port_config_change(db_list=['CONFIG_DB']):


please add another function to subscribe

subscribe_port_table_update

It's subscribe_port_update_events() in the latest commit, do we need to have it rephrased to subscribe_port_table_update()?

prgeor · 2021-11-22T06:26:56Z

sonic-xcvrd/xcvrd/xcvrd_utilities/port_mapping.py

-                                                        port_mapping.get_logical_to_physical(key)[0], 
-                                                        asic_context[port_tbl], 
-                                                        PortChangeEvent.PORT_REMOVE)
+                if promiscuous:


what is the promiscuous check for?

This is a dead code that's already removed, please check the latest commit

prgeor · 2021-11-22T06:44:36Z

sonic-xcvrd/xcvrd/xcvrd.py

+        has_cmis = False
+        for sfp in platform_chassis.get_all_sfps():
+            try:
+                ptype = sfp.get_port_type()


please remove this check for checking the cage type. Don't want platforms to implement this new API for enabling CMIS datapath initialization. Its OK for one thread running and doing nothing (CMIS state will be in failed state for those platform having only SFP).

It's already removed in the earlier commit, please check it out

prgeor · 2021-12-08T04:49:56Z

sonic-xcvrd/xcvrd/xcvrd_utilities/port_mapping.py


+def subscribe_port_config_change(db_list=['CONFIG_DB']):
+    port_tbl_map = {
+        'APPL_DB': swsscommon.APP_PORT_TABLE_NAME,


I think we should listen to PORT_TABLE in STATE_TB instead of PORT_TABLE in APP_DB because, when you get SET event for a port in APP_DB, the orchagent has still not called the sai api to create the port yet. When SET event is received for a port in STATE_DB then its confirmed the PORT create has completed.

prgeor · 2021-12-08T08:13:44Z

sonic-xcvrd/xcvrd/xcvrd.py

+
+                        self.port_dict[lport]['cmis_state'] = self.CMIS_STATE_DP_DEINIT
+                    elif state == self.CMIS_STATE_DP_DEINIT:
+                        if api.get_module_state() != 'ModuleReady':


we need timeout, if the module does NOT settle for 'ModuleReady' see page 1 byte 167

prgeor · 2021-12-08T08:20:34Z

sonic-xcvrd/xcvrd/xcvrd.py

+                                done = False
+                                continue
+                        if not done:
+                            continue


please bail out if not ConfigSuccess, otherwise we will keep continuing even though the configuration has failed.

prgeor · 2021-12-08T08:21:31Z

sonic-xcvrd/xcvrd/xcvrd.py

+                                done = False
+                                continue
+                        if not done:
+                            continue


please bail out if datapath init failed. There is a timeout in page 1, byte 144

This information is not available in the mem map as of now, hence we'll use a default expiration time for now, let's do the fine-grained expiration in phase 2 (DPB support)

prgeor · 2021-12-08T08:41:12Z

sonic-xcvrd/xcvrd/xcvrd.py

+
+                        # D.2.2 Software Deinitialization
+                        api.set_datapath_deinit(host_lanes)
+                        api.set_lpmode(True)


please make sure module indeed transitioned to low power.

Done, but it looks like one of my CMIS4 optics will never be transitioned to ModuleLowPwr, hence I add 'ModuleReady' in the following checker

prgeor · 2021-12-08T08:59:48Z

sonic-xcvrd/xcvrd/xcvrd.py

+                        if not done:
+                            continue


please bail out if the datapath has not transitioned to DataPathActivated . see page 1 , byte 168 for timeout for completing datapath tx turn ON.

commit a1d8e10 Author: Dante Su <[email protected]> Date: Thu Nov 18 12:45:01 2021 +0000 okay Signed-off-by: Dante Su <[email protected]> commit 11b6850 Author: Dante Su <[email protected]> Date: Fri Nov 12 07:39:34 2021 +0000 Add custom media setting for CMIS Signed-off-by: Dante Su <[email protected]> commit 6dd3608 Author: Dante Su <[email protected]> Date: Tue Nov 9 11:34:10 2021 +0000 add missing unittest for CMIS Signed-off-by: Dante Su <[email protected]> commit ceea223 Author: Dante Su <[email protected]> Date: Tue Nov 9 03:34:59 2021 +0000 fix port remove event Signed-off-by: Dante Su <[email protected]> commit 9be5135 Author: Dante Su <[email protected]> Date: Tue Nov 9 01:46:27 2021 +0000 port_mapping: drop the hacks for CMIS Signed-off-by: Dante Su <[email protected]> commit 501134b Author: Dante Su <[email protected]> Date: Fri Nov 5 09:08:59 2021 +0000 [sfp-refactoring] xcvrd: add initial support for CMIS application initialization Signed-off-by: Dante Su <[email protected]>

Signed-off-by: Dante Su <[email protected]>

… type checker Signed-off-by: Dante Su <[email protected]>

Signed-off-by: Dante Su <[email protected]>

judyjoseph · 2022-01-17T06:01:10Z

@prgeor are we planning to to take this SFP refractoring into 202111, Is this needed for 400G in Chassis ?

prgeor · 2022-01-17T08:08:25Z

This change is not needed if the default 400G application is needed.

…e 1.2 (sonic-net#217) This release goes in sync with the following firmware version of Broadcom Y cable, which is consistent with release 6 { "version_nic_active": "D203.9.D103.1", "version_nic_inactive": "D203.9.D103.1", "version_nic_next": "D203.9.D103.1", "version_peer_active": "D302.8", "version_peer_inactive": "D302.8", "version_peer_next": "D302.8", "version_self_active": "D302.8", "version_self_inactive": "D302.8", "version_self_next": "D302.8" } Signed-off-by: vaibhav-dahiya [email protected] Description Basically a vendor specific implementation of abstract YCableBase class . detailed design discussion can be found https://github.com/Azure/SONiC/pull/757/files Motivation and Context to support the Y-Cable API required to support Broadcom's Y-Cable. Signed-off-by: vaibhav-dahiya <[email protected]>

keboliu reviewed Oct 11, 2021

View reviewed changes

sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved

ds952811 changed the title ~~sfp: add initial support for CMIS~~ [sfp-refactoring] xcvrd: add initial support for CMIS application initialization Nov 3, 2021

ds952811 force-pushed the sfp-cmis branch from f2b0adc to 999e156 Compare November 5, 2021 09:37

ds952811 mentioned this pull request Nov 8, 2021

[sfp-refactoring] Initial support for CMIS application initialization sonic-net/sonic-buildimage#9157

Closed

5 tasks

ds952811 force-pushed the sfp-cmis branch from c5590a0 to 39c91ce Compare November 9, 2021 03:35

aravindmani-1 reviewed Nov 12, 2021

View reviewed changes

mikeberesford suggested changes Nov 12, 2021

View reviewed changes

ds952811 force-pushed the sfp-cmis branch from abe64de to a1d8e10 Compare November 18, 2021 12:48

ds952811 commented Nov 18, 2021

View reviewed changes

sonic-xcvrd/xcvrd/xcvrd.py Show resolved Hide resolved

ds952811 commented Nov 18, 2021

View reviewed changes

sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved

ds952811 force-pushed the sfp-cmis branch from a1d8e10 to 2bf4580 Compare November 19, 2021 03:17

keboliu requested a review from Junchao-Mellanox December 2, 2021 05:41

keboliu reviewed Dec 2, 2021

View reviewed changes

ds952811 force-pushed the sfp-cmis branch from f1618a9 to 771e567 Compare December 2, 2021 11:41

prgeor reviewed Dec 8, 2021

View reviewed changes

ds952811 added 3 commits December 9, 2021 08:20

unittest okay

d778266

Signed-off-by: Dante Su <[email protected]>

Drop unused local variable

95637b8

Signed-off-by: Dante Su <[email protected]>

ds952811 added 11 commits December 9, 2021 08:20

Add the READY message in case that ap update is skipped

a1eb712

Signed-off-by: Dante Su <[email protected]>

Address the review comments

af01207

Signed-off-by: Dante Su <[email protected]>

fix UT failures

3fcbaa4

Signed-off-by: Dante Su <[email protected]>

cosmetic updates

dc421d1

Signed-off-by: Dante Su <[email protected]>

drop pmon_daemon_control.json and add more comments for the cage/port…

143014f

… type checker Signed-off-by: Dante Su <[email protected]>

Stop deactivating the CmisManager based on the port/cage type

2afd61e

Signed-off-by: Dante Su <[email protected]>

address review comments

3c90138

Signed-off-by: Dante Su <[email protected]>

drop debug print

5f807c4

Signed-off-by: Dante Su <[email protected]>

address review comments

a258f2a

Signed-off-by: Dante Su <[email protected]>

add CMIS expiration and retries

8951bbf

Signed-off-by: Dante Su <[email protected]>

fix the CMIS unittestfailure

de65013

Signed-off-by: Dante Su <[email protected]>

ds952811 force-pushed the sfp-cmis branch from ffa6807 to de65013 Compare December 9, 2021 09:12

prgeor approved these changes Dec 9, 2021

View reviewed changes

prgeor merged commit 2b0acfb into sonic-net:master Dec 9, 2021


		self.dbg_print("Stopped")

		def task_run(self, y_cable_presence):



		def handle_port_config_change(sel, asic_context, stop_event, port_mapping, logger, port_change_event_handler):
		def handle_port_config_change(sel, asic_context, stop_event, port_mapping, logger, port_change_event_handler, promiscuous=False):

Conversation

ds952811 commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

Uh oh!

lgtm-com bot commented Oct 4, 2021

Uh oh!

Uh oh!

lgtm-com bot commented Nov 5, 2021

Uh oh!

lgtm-com bot commented Nov 9, 2021

Uh oh!

lgtm-com bot commented Nov 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ds952811 Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aravindmani-1 Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ds952811 Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ds952811 Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ds952811 Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Nov 18, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgtm-com bot commented Nov 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ds952811 Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ds952811 commented Oct 4, 2021 •

edited

Loading

ds952811 Nov 15, 2021 •

edited

Loading

aravindmani-1 Nov 15, 2021 •

edited

Loading

ds952811 Nov 15, 2021 •

edited

Loading

ds952811 Nov 15, 2021 •

edited

Loading

ds952811 Nov 15, 2021 •

edited

Loading

ds952811 Dec 2, 2021 •

edited

Loading