[xcvr]: Detect coherent modules via CoherentPagesSupported bit by gs1571 · Pull Request #720 · sonic-net/sonic-platform-common

gs1571 · 2026-07-17T05:19:02Z

Description

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)').

Added the CoherentPagesSupported bit — Page 01h byte 142 bit 4, defined in
OIF-CMIS-05.3, section 8.4.4 "Supported Pages Advertisement", Table 8-46
"Supported Pages Advertising (Page 01h)" ("Banked Pages 30h-4Fh (partially)
supported") — to the Page 01h mem map, alongside the existing
VdmSupported/DiagPageSupportAdvtField bits already defined on that same
byte. Spec: https://www.oiforum.com/wp-content/uploads/OIF-CMIS-05.3.pdf

is_coherent_module() now reads this bit first, since it is defined by spec
rather than inferred from a name, and falls back to matching 'ZR'/'FOIC'
in the media interface name when the module/CMIS revision does not expose
the bit (read returns None).

Motivation and Context

Found bringing up an 800G ZR+ coherent module (CMIS 5.3) whose default active
Application advertises a media interface name containing 'FOIC' but not
'ZR' — is_coherent_module() returned False, so CCmisApi was never
instantiated and the module's laser could never be configured.

* 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 <gs1571@gmail.com>

mssonicbld · 2026-07-17T05:19:09Z

/azp run

azure-pipelines · 2026-07-17T05:19:10Z

Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run.

azure-pipelines · 2026-07-17T05:19:18Z

Azure Pipelines: Successfully started running 1 pipeline(s).

mssonicbld · 2026-07-17T11:23:12Z

Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks!

_{---Powered by SONiC BuildBot}

aditya-nexthop

Please can you post in the testing section whether your change still correctly detects coherent (ZR or otherwise) modules advertising CMIS 5.2 or earlier with the Coherent pages supported bit set to both 0 and 1 ?

aditya-nexthop · 2026-07-17T17:27:04Z

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


Should we also be checking the CMIS revision (byte 1) here, since this field was reserved in CMIS 5.2 and earlier?

Yes, you're right — pushed a fix that gates the CoherentPagesSupported check on the module's actual reported CMIS revision. It now reads CmisMajorRevision/CmisMinorRevision (byte 1) first and only trusts the bit when (major, minor) >= (5, 3); every module below that unconditionally falls back to the 'ZR'/'FOIC' string match, exactly as it worked before this bit existed.

aditya-nexthop · 2026-07-17T17:29:33Z

+        # 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)


On a live system, I think the read returns None only upon an EEPROM read failure. We will still read byte 142, the value in a CMIS 5.2 or earlier coherent module will likely be 0 as it is reserved and we might end up breaking coherent detection altogether.

Confirmed, and that's exactly the regression this exposed — XcvrEeprom.read() only returns None on an actual EEPROM/I2C failure, not based on CMIS revision, so a real CMIS 5.2-or-earlier module would read the reserved bit as a genuine 0 and short-circuit straight to False, skipping the fallback and breaking detection for currently-supported coherent modules. Fixed by requiring CMIS 5.3+ before trusting the bit (see reply on the other comment). Added test cases: CMIS 5.2 module with the reserved bit read as 0 and as 1 (both now correctly fall through to string matching), plus unreadable CMIS revision fields.

* 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 <gs1571@gmail.com>

mssonicbld · 2026-07-18T07:36:31Z

/azp run

azure-pipelines · 2026-07-18T07:36:40Z

Azure Pipelines: Successfully started running 1 pipeline(s).

aditya-nexthop · 2026-07-20T18:49:24Z

+        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):


we can just use get_cmis_rev here?

Also, being pedantic here, but if a current SONiC release runs a CMIS 5.3+ coherent module that does not correctly advertise COHERENT_PAGES_SUPPORTED bit, such a module would be detected as coherent before, but will not be detected as coherent after this change.

We could claim though, that in that case, that the module is not CMIS-compliant due to the mis-advertisement. It is a change of behavior though.

You're right that
returning bool(CoherentPagesSupported) directly on 5.3+ was a regression: a
coherent module that reports the bit as 0 while still naming a ZR/FOIC
media interface would flip from coherent to non-coherent.

Reworked is_coherent_module() so detection is the union of the two signals
rather than the bit overriding the name. The name match returns True on its own (same as before the bit existed), and the bit now only adds coherent modules whose name contains neither keyword.
It can no longer drop a module the name match already covered, so the behavior
you flagged is gone. Added a test for a 5.3+ module reporting the bit as 0
with a coherent media interface name.

get_cmis_rev. I looked at reusing it, but it returns a formatted string
("5.3", or "None.None" when the read fails), so it can't feed the numeric
(major, minor) >= (5, 3) comparison without parsing it back — and a
lexicographic version compare would be unsafe ("5.10" < "5.3"). I kept the two
explicit int reads for that reason. Happy to switch to a small numeric helper
if you'd prefer that instead.

* 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 <gs1571@gmail.com>

mssonicbld · 2026-07-21T07:45:32Z

/azp run

azure-pipelines · 2026-07-21T07:45:41Z

Azure Pipelines: Successfully started running 1 pipeline(s).

aditya-nexthop reviewed Jul 17, 2026

View reviewed changes

aditya-nexthop reviewed Jul 20, 2026

View reviewed changes

Uh oh!

Conversation

gs1571 commented Jul 17, 2026

Description

Motivation and Context

Uh oh!

mssonicbld commented Jul 17, 2026

Uh oh!

azure-pipelines Bot commented Jul 17, 2026

Uh oh!

azure-pipelines Bot commented Jul 17, 2026

Uh oh!

mssonicbld commented Jul 17, 2026

Uh oh!

aditya-nexthop left a comment

Choose a reason for hiding this comment

Uh oh!

aditya-nexthop Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gs1571 Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

aditya-nexthop Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gs1571 Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Jul 18, 2026

Uh oh!

azure-pipelines Bot commented Jul 18, 2026

Uh oh!

aditya-nexthop Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

gs1571 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Jul 21, 2026

Uh oh!

azure-pipelines Bot commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants