Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable periodic polling of port in DomInfoUpdateTask thread during CMIS init #449

Merged

Conversation

mihirpat1
Copy link
Contributor

@mihirpat1 mihirpat1 commented Mar 18, 2024

Description

We currently need to disable periodic DOM polling of a port through DomInfoUpdateTask thread during CMIS initialization.

Motivation and Context

Disabling of DOM polling during CMIS initialization is primarily needed to disable sending CDB commands to read FW version from DomInfoUpdateTask thread during CMIS initialization.
For transceivers which do not support CDB background mode, any EEPROM access to the module can fail if a CDB command is executed at the same time.

In order to disable DOM polling during CMIS initialization, the cmis_state from CmisManagerTask thread is now being updated in the TRANSCEIVER_STATUS table of STATE_DB.
If the current cmis_state does not belong to CMIS_TERMINAL_STATES, DomInfoUpdateTask will disable DOM polling for the port to allow CmisManagerTask thread to complete CMIS initialization if required.
For platforms with CmisManagerTask disabled, the function is_port_in_cmis_initialization_process will always return False to allow DOM polling.
In case of device boot-up or transceiver insertion, the DomInfoUpdateTask thread will wait for the port to be in either of the CMIS_TERMINAL_STATES before proceeding with DOM polling. For non-CMIS transceivers, the expected cmis_state is CMIS_STATE_READY. Hence, once the corresponding port reaches CMIS_STATE_READY state, DOM polling will be enabled for such port.

Also, the cmis_state is not planned to be modified by DomInfoUpdateTask thread at any time to prevent race condition with CmisManagerTask.

How Has This Been Tested?

Following is the summary of tests performed

1 Ensure DOM thread polls for non-CMIS transceivers
2 Ensure DOM thread polls for platform with CMIS manager disabled
3 Ensure DOM thread waits during CMIS initialization
4 Ensure DOM polling is resumed after transceiver insertion
4.1 After removal
4.2 After insertion
5. Ensured `show interface transceiver status` CLI works with the current changes

Redis-db dump snippet to show `cmis_state` field in TRANSCEIVER_STATUS table
root@sonic:/home/admin# redis-cli -n 6 hgetall "TRANSCEIVER_STATUS|Ethernet0"
  1) "cmis_state"
  2) "READY"

Additional Information (Optional)

MSFT ADO - 26993372

sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved
@prgeor
Copy link
Collaborator

prgeor commented Mar 19, 2024

@keboliu @Junchao-Mellanox can you review this PR and see if it does not impact your paltform that skip's CMIS manager

sonic-xcvrd/xcvrd/xcvrd.py Outdated Show resolved Hide resolved
threading.Thread.__init__(self)
self.name = "DomInfoUpdateTask"
self.exc = None
self.task_stopping_event = threading.Event()
self.main_thread_stop_event = main_thread_stop_event
self.port_mapping = copy.deepcopy(port_mapping)
self.namespaces = namespaces
self.skip_cmis_mgr = skip_cmis_mgr
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mihirpat1 I am not sure if this is a must required because platform which skip cmis mgr will return CMIS_STATE_UNKNOWN which means is_port_in_cmis_initialization_process() will return False always (even though the fw could be initializing the CMIS module like in SN 2700)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prgeor The function is_port_in_cmis_initialization_process currently assumes that CMIS_STATE_UNKNOWN is a transient state and hence, returns True.
For platforms which have CMIS manager enabled, we assume that CMIS_STATE_UNKNOWN is the first state after starting CMIS state machine post xcvrd boot-up. Hence, the DomInfoUpdateTask thread holds off polling on the corresponding port to allow the port go through CMIS initialization if needed.

@mihirpat1
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -1252,6 +1252,10 @@ def task_worker(self):
for namespace in self.namespaces:
self.wait_for_port_config_done(namespace)

logical_port_list = self.port_mapping.logical_port_list
for lport in logical_port_list:
self.update_port_transceiver_status_table_sw_cmis_state(lport, CMIS_STATE_UNKNOWN)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mihirpat1 do we need to take care of Xcvrd restart?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to explicitly handle xcvrd restart case since the below code will set the cmis_state to CMIS_STATE_READY and prevent link flap if the module is already initialized.

need_update = self.is_cmis_application_update_required(api, appl, host_lanes_mask)

Also, the current nightly behavior is to clear cmis_state after xcvrd restart. Hence, this should not cause additional change in behavior.

@prgeor prgeor force-pushed the dom_polling_disable_during_cmis_init branch from 1b522a2 to 8f8326d Compare March 20, 2024 20:09
@prgeor prgeor merged commit 2770fd2 into sonic-net:master Mar 20, 2024
5 checks passed
mihirpat1 added a commit to mihirpat1/sonic-platform-daemons that referenced this pull request Mar 20, 2024
…MIS init (sonic-net#449)

* Disable periodic polling of port in DomInfoUpdateTask thread during CMIS init

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

* Replaced sharing of cmis_state from instance to a field in TRANSCEIVER_STATUS table

* Improved code coverage

* Moved update_port_transceiver_status_table_sw_cmis_state to CmisManagerTask class

* Initializing cmis_state to CMIS_STATE_UNKNOWN while spawning CmisManagerTask

---------

Signed-off-by: Mihir Patel <[email protected]>
mihirpat1 added a commit to mihirpat1/sonic-platform-daemons that referenced this pull request Mar 20, 2024
…MIS init (sonic-net#449)

* Disable periodic polling of port in DomInfoUpdateTask thread during CMIS init

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

* Replaced sharing of cmis_state from instance to a field in TRANSCEIVER_STATUS table

* Improved code coverage

* Moved update_port_transceiver_status_table_sw_cmis_state to CmisManagerTask class

* Initializing cmis_state to CMIS_STATE_UNKNOWN while spawning CmisManagerTask

---------

Signed-off-by: Mihir Patel <[email protected]>
StormLiangMS pushed a commit that referenced this pull request Mar 21, 2024
…MIS init (#449) (#450)

Cherry-pick for #449

Description
We currently need to disable periodic DOM polling of a port through DomInfoUpdateTask thread during CMIS initialization.

Motivation and Context
Disabling of DOM polling during CMIS initialization is primarily needed to disable sending CDB commands to read FW version from DomInfoUpdateTask thread during CMIS initialization.
For transceivers which do not support CDB background mode, any EEPROM access to the module can fail if a CDB command is executed at the same time.

In order to disable DOM polling during CMIS initialization, the cmis_state from CmisManagerTask thread is now being updated in the TRANSCEIVER_STATUS table of STATE_DB.
If the current cmis_state does not belong to CMIS_TERMINAL_STATES, DomInfoUpdateTask will disable DOM polling for the port to allow CmisManagerTask thread to complete CMIS initialization if required.
For platforms with CmisManagerTask disabled, the function is_port_in_cmis_initialization_process will always return False to allow DOM polling.
In case of device boot-up or transceiver insertion, the DomInfoUpdateTask thread will wait for the port to be in either of the CMIS_TERMINAL_STATES before proceeding with DOM polling. For non-CMIS transceivers, the expected cmis_state is CMIS_STATE_READY. Hence, once the corresponding port reaches CMIS_STATE_READY state, DOM polling will be enabled for such port.

Also, the cmis_state is not planned to be modified by DomInfoUpdateTask thread at any time to prevent race condition with CmisManagerTask.

How Has This Been Tested?
Following is the summary of tests performed

1 Ensure DOM thread polls for non-CMIS transceivers
2 Ensure DOM thread polls for platform with CMIS manager disabled
3 Ensure DOM thread waits during CMIS initialization
4 Ensure DOM polling is resumed after transceiver insertion
4.1 After removal
4.2 After insertion
5. Ensured `show interface transceiver status` CLI works with the current changes

Redis-db dump snippet to show `cmis_state` field in TRANSCEIVER_STATUS table
root@sonic:/home/admin# redis-cli -n 6 hgetall "TRANSCEIVER_STATUS|Ethernet0"
  1) "cmis_state"
  2) "READY"
Additional Information (Optional)
MSFT ADO - 26993372
yxieca pushed a commit that referenced this pull request Mar 21, 2024
…MIS init (#449) (#451)

* Disable periodic polling of port in DomInfoUpdateTask thread during CMIS init



* Replaced sharing of cmis_state from instance to a field in TRANSCEIVER_STATUS table

* Improved code coverage

* Moved update_port_transceiver_status_table_sw_cmis_state to CmisManagerTask class

* Initializing cmis_state to CMIS_STATE_UNKNOWN while spawning CmisManagerTask

---------

Signed-off-by: Mihir Patel <[email protected]>
yuazhe pushed a commit to yuazhe/sonic-platform-daemons that referenced this pull request Jul 2, 2024
…MIS init (sonic-net#449)

* Disable periodic polling of port in DomInfoUpdateTask thread during CMIS init

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

* Replaced sharing of cmis_state from instance to a field in TRANSCEIVER_STATUS table

* Improved code coverage

* Moved update_port_transceiver_status_table_sw_cmis_state to CmisManagerTask class

* Initializing cmis_state to CMIS_STATE_UNKNOWN while spawning CmisManagerTask

---------

Signed-off-by: Mihir Patel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants