diff --git a/sonic-xcvrd/tests/test_xcvrd.py b/sonic-xcvrd/tests/test_xcvrd.py index a25d742ea..d575b50e4 100644 --- a/sonic-xcvrd/tests/test_xcvrd.py +++ b/sonic-xcvrd/tests/test_xcvrd.py @@ -3800,6 +3800,57 @@ def get_application(lane): assert task.is_cmis_application_update_required(mock_xcvr_api, app_new, host_lanes_mask) == expected + @pytest.mark.parametrize("appl, host_lanes_mask, active_apsel, expected", [ + # The module activated the desired application + (1, 0x0F, {'ActiveAppSelLane1': 1, 'ActiveAppSelLane2': 1, + 'ActiveAppSelLane3': 1, 'ActiveAppSelLane4': 1}, True), + # The module is running a different application than the desired one + (12, 0x0F, {'ActiveAppSelLane1': 4, 'ActiveAppSelLane2': 4, + 'ActiveAppSelLane3': 4, 'ActiveAppSelLane4': 4}, False), + # Only a subset of the host lanes failed to activate the desired application + (12, 0x0F, {'ActiveAppSelLane1': 12, 'ActiveAppSelLane2': 12, + 'ActiveAppSelLane3': 12, 'ActiveAppSelLane4': 4}, False), + # Host lanes outside of the mask are ignored + (12, 0x03, {'ActiveAppSelLane1': 12, 'ActiveAppSelLane2': 12, + 'ActiveAppSelLane3': 4, 'ActiveAppSelLane4': 4}, True), + # The active application code is not readable + (12, 0x0F, {'ActiveAppSelLane1': 'N/A', 'ActiveAppSelLane2': 'N/A', + 'ActiveAppSelLane3': 'N/A', 'ActiveAppSelLane4': 'N/A'}, True), + (12, 0x0F, {}, True), + ]) + def test_CmisManagerTask_is_active_apsel_matching_desired(self, appl, host_lanes_mask, active_apsel, expected): + mock_xcvr_api = MagicMock() + mock_xcvr_api.get_active_apsel_hostlane = MagicMock(return_value=active_apsel) + + port_mapping = PortMapping() + stop_event = threading.Event() + task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event, platform_chassis=MagicMock()) + + assert task.is_active_apsel_matching_desired(mock_xcvr_api, appl, host_lanes_mask) == expected + + def test_CmisManagerTask_is_cmis_application_update_required_active_apsel_mismatch(self): + """ + A module can accept the staged application and report ConfigSuccess without + ever updating the Active Control Set. The staged application then matches the + desired one and the datapath looks healthy, while the module keeps running the + previous application, so an update still has to be forced. + """ + mock_xcvr_api = MagicMock() + mock_xcvr_api.is_flat_memory = MagicMock(return_value=False) + mock_xcvr_api.get_application = MagicMock(return_value=12) + mock_xcvr_api.get_datapath_state = MagicMock(return_value=self.DEFAULT_DP_STATE) + mock_xcvr_api.get_config_datapath_hostlane_status = MagicMock(return_value=self.DEFAULT_CONFIG_STATUS) + mock_xcvr_api.get_active_apsel_hostlane = MagicMock(return_value={ + 'ActiveAppSelLane1': 4, 'ActiveAppSelLane2': 4, + 'ActiveAppSelLane3': 4, 'ActiveAppSelLane4': 4 + }) + + port_mapping = PortMapping() + stop_event = threading.Event() + task = CmisManagerTask(DEFAULT_NAMESPACE, port_mapping, stop_event, platform_chassis=MagicMock()) + + assert task.is_cmis_application_update_required(mock_xcvr_api, 12, 0x0F) == True + @pytest.mark.parametrize("ifname, expected", [ ('1.6TBASE-CR8 (Clause179)', 1600000), ('1.6TAUI-8 (Annex176E)', 1600000), diff --git a/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py b/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py index b39992fa7..54293fd5b 100644 --- a/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py +++ b/sonic-xcvrd/xcvrd/cmis/cmis_manager_task.py @@ -572,9 +572,58 @@ def is_cmis_application_update_required(self, api, app_new, host_lanes_mask): if conf_state[name] != 'ConfigSuccess': skip = False break + if skip and not self.is_active_apsel_matching_desired(api, app_new, host_lanes_mask): + self.log_notice("Forcing application update since the module has not " + "activated the desired application {}...".format(app_new)) + skip = False return (not skip) return True + def is_active_apsel_matching_desired(self, api, appl, host_lanes_mask): + """ + Check whether the module has actually activated the desired application. + + get_application() reads the staged control set, which reflects what was + requested rather than what the module is running. A module can accept a + staged application and report ConfigSuccess while leaving the Active + Control Set unchanged, in which case the media side keeps running the + previous application. Comparing against the active application select + code detects that case. + + Args: + api: + XcvrApi object + appl: + Integer, the desired transceiver-specific application code + host_lanes_mask: + Integer, a bitmask of the lanes on the host side + e.g. 0x5 for lane 0 and lane 2. + + Returns: + Boolean, true if the active application matches the desired one on + every host lane of the mask, or if the active application code + cannot be read. + """ + active_apsel = api.get_active_apsel_hostlane() + if not active_apsel: + return True + + for lane in range(self.CMIS_MAX_HOST_LANES): + if ((1 << lane) & host_lanes_mask) == 0: + continue + active = active_apsel.get("ActiveAppSelLane{}".format(lane + 1)) + # The active application code is not always readable, e.g. on flat + # memory modules it is reported as 'N/A'. Skip the check instead of + # forcing a needless datapath reinitialization. + if not isinstance(active, int): + return True + if active != appl: + self.log_error("Active application {} on host lane {} does not match " + "the desired application {}".format(active, lane + 1, appl)) + return False + + return True + def force_cmis_reinit(self, lport, retries=0): """ Try to force the restart of CMIS state machine @@ -1240,6 +1289,15 @@ def process_cmis_state_machine(self, lport): self.force_cmis_reinit(lport, retries + 1) return + # A module can report ConfigSuccess and activate its datapath while + # still running a different application than the one requested. Verify + # the active application before declaring the port ready, otherwise the + # module silently stays on the wrong media interface. + if not self.is_active_apsel_matching_desired(api, appl, host_lanes_mask): + self.log_error("{}: module did not activate application {}".format(lport, appl)) + self.force_cmis_reinit(lport, retries + 1) + return + self.log_notice("{}: READY".format(lport)) self.update_port_transceiver_status_table_sw_cmis_state(lport, CMIS_STATE_READY) self.post_port_active_apsel_to_db(api, lport, host_lanes_mask)