From 72fc5540fbe9643c1b981ee7c3b4f66a6a38a576 Mon Sep 17 00:00:00 2001 From: Grigory Solovyev Date: Fri, 17 Jul 2026 08:08:08 +0300 Subject: [PATCH 1/4] [xcvr]: Detect coherent modules via CoherentPagesSupported bit * is_coherent_module() decided coherency purely by searching for the substring 'ZR' in the free-text module media interface name. This breaks for any coherent module whose active application advertises a media interface name without 'ZR' in it (e.g. FOIC-only names such as 'FOIC1.4-DO (G.709.3/Y.1331.3)'). * Add the CoherentPagesSupported bit (Page 01h byte 142 bit 4, OIF-CMIS 5.x+) to the Page 01h mem map, alongside the existing VdmSupported/ DiagPageSupportAdvtField bits on the same byte. * is_coherent_module() now reads this bit first, since it is defined by spec rather than inferred from a name. Falls back to matching 'ZR' or 'FOIC' in the media interface name when the module/CMIS revision does not expose the bit (read returns None). Signed-off-by: Grigory Solovyev --- .../sonic_xcvr/api/public/cmis.py | 11 ++++++++- .../sonic_xcvr/fields/consts.py | 1 + .../mem_maps/public/cmis/pages/page01.py | 1 + tests/sonic_xcvr/test_cmis.py | 23 +++++++++++++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/sonic_platform_base/sonic_xcvr/api/public/cmis.py b/sonic_platform_base/sonic_xcvr/api/public/cmis.py index 416d5b581..c2c43e7c0 100644 --- a/sonic_platform_base/sonic_xcvr/api/public/cmis.py +++ b/sonic_platform_base/sonic_xcvr/api/public/cmis.py @@ -1051,9 +1051,18 @@ def get_module_media_interface(self): def is_coherent_module(self): ''' Returns True if the module follow C-CMIS spec, False otherwise + + Prefers the CoherentPagesSupported bit (Page 01h byte 142 bit 4, + OIF-CMIS 5.x+) when the module advertises it, since it is defined by + spec rather than inferred from a free-text media interface name. + Falls back to matching 'ZR' or 'FOIC' in the media interface name for + modules/CMIS revisions that do not expose this bit. ''' + coherent_pages_supported = self.xcvr_eeprom.read(consts.COHERENT_PAGES_SUPPORTED) + if coherent_pages_supported is not None: + return bool(coherent_pages_supported) mintf = self.get_module_media_interface() - return False if 'ZR' not in mintf else True + return any(kw in mintf for kw in ('ZR', 'FOIC')) @read_only_cached_api_return def get_datapath_init_duration(self): diff --git a/sonic_platform_base/sonic_xcvr/fields/consts.py b/sonic_platform_base/sonic_xcvr/fields/consts.py index c4881da34..2ac47ca7c 100644 --- a/sonic_platform_base/sonic_xcvr/fields/consts.py +++ b/sonic_platform_base/sonic_xcvr/fields/consts.py @@ -292,6 +292,7 @@ FLAGS_ADVT_FIELD = "Supported Flags Advertisement" PAGE_SUPPORT_ADVT_FIELD = "Supported Pages Advertisement" DIAG_PAGE_SUPPORT_ADVT_FIELD = "Supported Diagnostic Pages Advertisement" +COHERENT_PAGES_SUPPORTED = "CoherentPagesSupported" TX_FLAGS_ADVT_FIELD = "Supported TX Flags Advertisement" RX_FLAGS_ADVT_FIELD = "Supported RX Flags Advertisement" LANE_MON_ADVT_FIELD = "Supported Lane Monitor Advertisement" diff --git a/sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page01.py b/sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page01.py index 76fa40725..36dcedbd9 100644 --- a/sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page01.py +++ b/sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page01.py @@ -85,6 +85,7 @@ def __init__(self, codes, page=ADVERTISING_PAGE): NumberRegField(consts.PAGE_SUPPORT_ADVT_FIELD, self.getaddr(142), RegBitField(consts.VDM_SUPPORTED, 6), RegBitField(consts.DIAG_PAGE_SUPPORT_ADVT_FIELD, 5), + RegBitField(consts.COHERENT_PAGES_SUPPORTED, 4), ), CodeRegField(consts.BANKS_SUPPORTED_FIELD, self.getaddr(142), codes.MAX_BANKS_SUPPORTED, *(RegBitField("Bit%d" % bit, bit) for bit in range(0, 2)) diff --git a/tests/sonic_xcvr/test_cmis.py b/tests/sonic_xcvr/test_cmis.py index 4b3b7314e..82b7c506d 100755 --- a/tests/sonic_xcvr/test_cmis.py +++ b/tests/sonic_xcvr/test_cmis.py @@ -965,13 +965,36 @@ def test_get_module_media_interface(self, mock_response1, mock_response2, expect @pytest.mark.parametrize("mock_response, expected", [ ('Copper cable', False), ('400ZR', True), + # FOIC-named media interfaces (e.g. 800G-ZR+ FOIC, no 'ZR' substring) + # are coherent too; only reachable when CoherentPagesSupported is + # unavailable (mocked read() below defaults to None). + ('FOIC1.4-DO (G.709.3/Y.1331.3)', True), ]) def test_is_coherent_module(self, mock_response, expected): + self.clear_cache('is_coherent_module') + # CoherentPagesSupported unavailable: force the string-matching + # fallback path regardless of what earlier tests left behind on the + # shared self.api.xcvr_eeprom mock. + self.api.xcvr_eeprom.read = MagicMock(return_value=None) self.api.get_module_media_interface = MagicMock() self.api.get_module_media_interface.return_value = mock_response result = self.api.is_coherent_module() assert result == expected + @pytest.mark.parametrize("mock_response, expected", [ + (1, True), + (0, False), + ]) + def test_is_coherent_module_coherent_pages_bit(self, mock_response, expected): + # When CoherentPagesSupported is advertised, it takes precedence over + # the media interface name (which is deliberately left un-mocked / + # not matching 'ZR' or 'FOIC', to prove the bit alone decides this). + self.clear_cache('is_coherent_module') + self.api.get_module_media_interface = MagicMock(return_value='Copper cable') + self.api.xcvr_eeprom.read = MagicMock(return_value=mock_response) + result = self.api.is_coherent_module() + assert result == expected + @pytest.mark.parametrize("mock_response1, mock_response2, expected", [ (True, '1', 0 ), (False, None, 0), From 24430ced7fd6e8a49c6e5b32779a8a1215049390 Mon Sep 17 00:00:00 2001 From: Grigory Solovyev Date: Sat, 18 Jul 2026 10:35:45 +0300 Subject: [PATCH 2/4] [xcvr]: Gate CoherentPagesSupported bit by CMIS revision * Page 01h byte 142 bit 4 is Reserved prior to CMIS 5.3 (OIF-CMIS-05.2 Table 8-41); it only became CoherentPagesSupported in 5.3 (OIF-CMIS-05.3 Table 8-46). XcvrEeprom.read() only returns None on an actual I2C/EEPROM read failure, not based on CMIS revision, so on a real CMIS <=5.2 module the previous code would read the reserved bit as a real 0 or 1 instead of None, short-circuiting past the 'ZR'/'FOIC' string-match fallback and misdetecting existing coherent modules as non-coherent. * Only trust the bit when the module reports CMIS 5.3 or later; fall back to the string match unconditionally otherwise, matching behavior prior to this bit's introduction. * Add tests covering pre-5.3 modules with the reserved bit read as 0, and unreadable CMIS revision fields. Signed-off-by: Grigory Solovyev --- .../sonic_xcvr/api/public/cmis.py | 23 ++++---- tests/sonic_xcvr/test_cmis.py | 52 +++++++++++++++++-- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/sonic_platform_base/sonic_xcvr/api/public/cmis.py b/sonic_platform_base/sonic_xcvr/api/public/cmis.py index c2c43e7c0..ff1469595 100644 --- a/sonic_platform_base/sonic_xcvr/api/public/cmis.py +++ b/sonic_platform_base/sonic_xcvr/api/public/cmis.py @@ -1052,15 +1052,20 @@ def is_coherent_module(self): ''' Returns True if the module follow C-CMIS spec, False otherwise - Prefers the CoherentPagesSupported bit (Page 01h byte 142 bit 4, - OIF-CMIS 5.x+) when the module advertises it, since it is defined by - spec rather than inferred from a free-text media interface name. - Falls back to matching 'ZR' or 'FOIC' in the media interface name for - modules/CMIS revisions that do not expose this bit. - ''' - coherent_pages_supported = self.xcvr_eeprom.read(consts.COHERENT_PAGES_SUPPORTED) - if coherent_pages_supported is not None: - return bool(coherent_pages_supported) + CoherentPagesSupported (Page 01h byte 142 bit 4) is only defined by + OIF-CMIS starting at revision 5.3; on earlier revisions that bit is + Reserved, and real modules are not guaranteed to report it as 0, so + it cannot be trusted to mean "not coherent" there. Only honor the + bit on modules that report CMIS 5.3 or later; every other module + keeps using the 'ZR'/'FOIC' substring match against the media + interface name, exactly as before this bit existed. + ''' + cmis_major = self.xcvr_eeprom.read(consts.CMIS_MAJOR_REVISION) + cmis_minor = self.xcvr_eeprom.read(consts.CMIS_MINOR_REVISION) + if cmis_major is not None and cmis_minor is not None and (cmis_major, cmis_minor) >= (5, 3): + coherent_pages_supported = self.xcvr_eeprom.read(consts.COHERENT_PAGES_SUPPORTED) + if coherent_pages_supported is not None: + return bool(coherent_pages_supported) mintf = self.get_module_media_interface() return any(kw in mintf for kw in ('ZR', 'FOIC')) diff --git a/tests/sonic_xcvr/test_cmis.py b/tests/sonic_xcvr/test_cmis.py index 82b7c506d..f77bb65cd 100755 --- a/tests/sonic_xcvr/test_cmis.py +++ b/tests/sonic_xcvr/test_cmis.py @@ -986,15 +986,59 @@ def test_is_coherent_module(self, mock_response, expected): (0, False), ]) def test_is_coherent_module_coherent_pages_bit(self, mock_response, expected): - # When CoherentPagesSupported is advertised, it takes precedence over - # the media interface name (which is deliberately left un-mocked / - # not matching 'ZR' or 'FOIC', to prove the bit alone decides this). + # On CMIS 5.3+, CoherentPagesSupported is advertised and takes + # precedence over the media interface name (which is deliberately + # left un-mocked / not matching 'ZR' or 'FOIC', to prove the bit + # alone decides this). self.clear_cache('is_coherent_module') self.api.get_module_media_interface = MagicMock(return_value='Copper cable') - self.api.xcvr_eeprom.read = MagicMock(return_value=mock_response) + def mock_read(field): + if field == consts.CMIS_MAJOR_REVISION: + return 5 + if field == consts.CMIS_MINOR_REVISION: + return 3 + if field == consts.COHERENT_PAGES_SUPPORTED: + return mock_response + return None + self.api.xcvr_eeprom.read = MagicMock(side_effect=mock_read) result = self.api.is_coherent_module() assert result == expected + @pytest.mark.parametrize("cmis_minor, mintf, expected", [ + (2, '400ZR', True), + (2, 'Copper cable', False), + ]) + def test_is_coherent_module_ignores_reserved_bit_pre_5_3(self, cmis_minor, mintf, expected): + # Byte 142 bit 4 is Reserved prior to CMIS 5.3 (OIF-CMIS-05.2 Table + # 8-41). A pre-5.3 module may report this bit as 0 (the spec's + # convention for reserved bits) or 1 (not spec-guaranteed, but not + # excluded either) while still being a real coherent module - the + # bit must never override the media interface name check for + # modules that predate the bit's definition. + self.clear_cache('is_coherent_module') + self.api.get_module_media_interface = MagicMock(return_value=mintf) + def mock_read(field): + if field == consts.CMIS_MAJOR_REVISION: + return 5 + if field == consts.CMIS_MINOR_REVISION: + return cmis_minor + if field == consts.COHERENT_PAGES_SUPPORTED: + return 0 + return None + self.api.xcvr_eeprom.read = MagicMock(side_effect=mock_read) + result = self.api.is_coherent_module() + assert result == expected + + def test_is_coherent_module_unknown_cmis_revision_falls_back(self): + # CmisMajorRevision/CmisMinorRevision reads failing (None) must not + # crash the (major, minor) >= (5, 3) comparison; behave as if the + # bit is untrustworthy and use the string-matching fallback. + self.clear_cache('is_coherent_module') + self.api.get_module_media_interface = MagicMock(return_value='400ZR') + self.api.xcvr_eeprom.read = MagicMock(return_value=None) + result = self.api.is_coherent_module() + assert result is True + @pytest.mark.parametrize("mock_response1, mock_response2, expected", [ (True, '1', 0 ), (False, None, 0), From 2acf42e2bdb0f7e334921de1a4296abdb9f07439 Mon Sep 17 00:00:00 2001 From: Grigory Solovyev Date: Tue, 21 Jul 2026 10:45:20 +0300 Subject: [PATCH 3/4] [xcvr]: Make coherent detection additive over name and bit * Rework is_coherent_module() so the 'ZR'/'FOIC' media interface name match returns True on its own and the CoherentPagesSupported bit only adds coherent modules whose name contains neither keyword. Previously the bit's value was returned directly on CMIS 5.3+, which could newly classify a coherent module that mis-advertises the bit as 0 (but names a 'ZR'/'FOIC' interface) as non-coherent. * Add a test for a CMIS 5.3+ module reporting the bit as 0 with a coherent media interface name. Signed-off-by: Grigory Solovyev --- .../sonic_xcvr/api/public/cmis.py | 31 ++++++++++--------- tests/sonic_xcvr/test_cmis.py | 23 ++++++++++++++ 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/sonic_platform_base/sonic_xcvr/api/public/cmis.py b/sonic_platform_base/sonic_xcvr/api/public/cmis.py index ff1469595..2d2e9bdfa 100644 --- a/sonic_platform_base/sonic_xcvr/api/public/cmis.py +++ b/sonic_platform_base/sonic_xcvr/api/public/cmis.py @@ -1050,24 +1050,27 @@ def get_module_media_interface(self): @read_only_cached_api_return def is_coherent_module(self): ''' - Returns True if the module follow C-CMIS spec, False otherwise - - CoherentPagesSupported (Page 01h byte 142 bit 4) is only defined by - OIF-CMIS starting at revision 5.3; on earlier revisions that bit is - Reserved, and real modules are not guaranteed to report it as 0, so - it cannot be trusted to mean "not coherent" there. Only honor the - bit on modules that report CMIS 5.3 or later; every other module - keeps using the 'ZR'/'FOIC' substring match against the media - interface name, exactly as before this bit existed. + Returns True if the module follows the C-CMIS spec, False otherwise. + + Detection is the union of two independent signals, so the bit can + only add coherent modules and never drops one that used to be + detected: + * the media interface name contains 'ZR' or 'FOIC' - works on + every CMIS revision and matches the behavior from before this + bit existed, and + * CoherentPagesSupported (Page 01h byte 142 bit 4) is set. That + bit is only defined from OIF-CMIS 5.3 on (Reserved before, where + a real module may report it as a stray 1), so it is only honored + when the module reports CMIS 5.3 or later. ''' + mintf = self.get_module_media_interface() + if any(kw in mintf for kw in ('ZR', 'FOIC')): + return True cmis_major = self.xcvr_eeprom.read(consts.CMIS_MAJOR_REVISION) cmis_minor = self.xcvr_eeprom.read(consts.CMIS_MINOR_REVISION) if cmis_major is not None and cmis_minor is not None and (cmis_major, cmis_minor) >= (5, 3): - coherent_pages_supported = self.xcvr_eeprom.read(consts.COHERENT_PAGES_SUPPORTED) - if coherent_pages_supported is not None: - return bool(coherent_pages_supported) - mintf = self.get_module_media_interface() - return any(kw in mintf for kw in ('ZR', 'FOIC')) + return bool(self.xcvr_eeprom.read(consts.COHERENT_PAGES_SUPPORTED)) + return False @read_only_cached_api_return def get_datapath_init_duration(self): diff --git a/tests/sonic_xcvr/test_cmis.py b/tests/sonic_xcvr/test_cmis.py index f77bb65cd..4f0d1400c 100755 --- a/tests/sonic_xcvr/test_cmis.py +++ b/tests/sonic_xcvr/test_cmis.py @@ -1039,6 +1039,29 @@ def test_is_coherent_module_unknown_cmis_revision_falls_back(self): result = self.api.is_coherent_module() assert result is True + @pytest.mark.parametrize("mintf", [ + '400ZR', + 'FOIC1.4-DO (G.709.3/Y.1331.3)', + ]) + def test_is_coherent_module_name_match_overrides_cleared_bit(self, mintf): + # A CMIS 5.3+ coherent module that mis-advertises + # CoherentPagesSupported as 0 while still naming a coherent media + # interface must stay coherent: the bit can only add detection, it + # must never drop a module the name match already covers. This + # guarantees no regression vs. the pre-bit ('ZR'/'FOIC') behavior. + self.clear_cache('is_coherent_module') + self.api.get_module_media_interface = MagicMock(return_value=mintf) + def mock_read(field): + if field == consts.CMIS_MAJOR_REVISION: + return 5 + if field == consts.CMIS_MINOR_REVISION: + return 3 + if field == consts.COHERENT_PAGES_SUPPORTED: + return 0 + return None + self.api.xcvr_eeprom.read = MagicMock(side_effect=mock_read) + assert self.api.is_coherent_module() is True + @pytest.mark.parametrize("mock_response1, mock_response2, expected", [ (True, '1', 0 ), (False, None, 0), From adc0dbfcce0afdd1009f44f579c6dd047014fccc Mon Sep 17 00:00:00 2001 From: Grigory Solovyev Date: Tue, 28 Jul 2026 20:10:28 +0200 Subject: [PATCH 4/4] [xcvr]: Address review comments on coherent detection * is_coherent_module(): guard get_module_media_interface() against None before the 'ZR'/'FOIC' name check. A failed media-interface read returns None, and `kw in None` raised TypeError before the code could fall through to the CoherentPagesSupported bit; now a coherent module with an unreadable name is still detected via the bit on CMIS 5.3+. * page01.py: move CoherentPagesSupported (byte 142 bit 4) out of the PAGE_SUPPORT_ADVT_FIELD NumberRegField into its own standalone RegBitField at the same offset. Folding the bit into the existing field changed that field's decoded value, because NumberRegField.decode right-shifts by the lowest sub-field bit position (5 -> 4) and widens the mask - a backward-incompatible change for any consumer reading Supported Pages Advertisement. * Add tests: is_coherent_module() with a None media-interface name and the bit set, and a decode-stability check that PAGE_SUPPORT_ADVT_FIELD is unaffected when byte 142 bit 4 is set. Signed-off-by: Grigory Solovyev --- .../sonic_xcvr/api/public/cmis.py | 2 +- .../mem_maps/public/cmis/pages/page01.py | 2 +- tests/sonic_xcvr/test_cmis.py | 31 +++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/sonic_platform_base/sonic_xcvr/api/public/cmis.py b/sonic_platform_base/sonic_xcvr/api/public/cmis.py index 2d2e9bdfa..0111a7158 100644 --- a/sonic_platform_base/sonic_xcvr/api/public/cmis.py +++ b/sonic_platform_base/sonic_xcvr/api/public/cmis.py @@ -1064,7 +1064,7 @@ def is_coherent_module(self): when the module reports CMIS 5.3 or later. ''' mintf = self.get_module_media_interface() - if any(kw in mintf for kw in ('ZR', 'FOIC')): + if mintf is not None and any(kw in mintf for kw in ('ZR', 'FOIC')): return True cmis_major = self.xcvr_eeprom.read(consts.CMIS_MAJOR_REVISION) cmis_minor = self.xcvr_eeprom.read(consts.CMIS_MINOR_REVISION) diff --git a/sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page01.py b/sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page01.py index 36dcedbd9..c61d5c5f0 100644 --- a/sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page01.py +++ b/sonic_platform_base/sonic_xcvr/mem_maps/public/cmis/pages/page01.py @@ -85,8 +85,8 @@ def __init__(self, codes, page=ADVERTISING_PAGE): NumberRegField(consts.PAGE_SUPPORT_ADVT_FIELD, self.getaddr(142), RegBitField(consts.VDM_SUPPORTED, 6), RegBitField(consts.DIAG_PAGE_SUPPORT_ADVT_FIELD, 5), - RegBitField(consts.COHERENT_PAGES_SUPPORTED, 4), ), + RegBitField(consts.COHERENT_PAGES_SUPPORTED, offset=self.getaddr(142), bitpos=4), CodeRegField(consts.BANKS_SUPPORTED_FIELD, self.getaddr(142), codes.MAX_BANKS_SUPPORTED, *(RegBitField("Bit%d" % bit, bit) for bit in range(0, 2)) ), diff --git a/tests/sonic_xcvr/test_cmis.py b/tests/sonic_xcvr/test_cmis.py index 4f0d1400c..8a03dcaed 100755 --- a/tests/sonic_xcvr/test_cmis.py +++ b/tests/sonic_xcvr/test_cmis.py @@ -1062,6 +1062,37 @@ def mock_read(field): self.api.xcvr_eeprom.read = MagicMock(side_effect=mock_read) assert self.api.is_coherent_module() is True + def test_is_coherent_module_media_interface_none_uses_bit(self): + # get_module_media_interface() returns None when the media interface + # EEPROM read fails. The name check must not crash on None (it is not + # iterable) - is_coherent_module() has to fall through to the + # CoherentPagesSupported bit on CMIS 5.3+. + self.clear_cache('is_coherent_module') + self.api.get_module_media_interface = MagicMock(return_value=None) + def mock_read(field): + if field == consts.CMIS_MAJOR_REVISION: + return 5 + if field == consts.CMIS_MINOR_REVISION: + return 3 + if field == consts.COHERENT_PAGES_SUPPORTED: + return 1 + return None + self.api.xcvr_eeprom.read = MagicMock(side_effect=mock_read) + assert self.api.is_coherent_module() is True + + def test_coherent_pages_supported_isolated_from_page_support_advt(self): + # CoherentPagesSupported (byte 142 bit 4) is its own field, so it must + # not change how the neighbouring PAGE_SUPPORT_ADVT_FIELD (bits 6, 5) + # decodes. With only bit 4 set: the coherent bit reads True and the + # advertisement field still reads 0. (Folding bit 4 into + # PAGE_SUPPORT_ADVT_FIELD would shift its decode from >>5 to >>4 and + # make it read 1 here - a backward-incompatible change.) + data = bytearray([0x10]) # only byte-142 bit 4 set + advt = self.mem_map.get_field(consts.PAGE_SUPPORT_ADVT_FIELD) + coherent = self.mem_map.get_field(consts.COHERENT_PAGES_SUPPORTED) + assert advt.decode(data) == 0 + assert bool(coherent.decode(data)) is True + @pytest.mark.parametrize("mock_response1, mock_response2, expected", [ (True, '1', 0 ), (False, None, 0),