[xcvrd] Verify that the module activated the requested CMIS application

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),

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)