[xcvr]: Detect coherent modules via CoherentPagesSupported bit#720
[xcvr]: Detect coherent modules via CoherentPagesSupported bit#720gs1571 wants to merge 2 commits into
Conversation
* 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>
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
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
left a comment
aditya-nexthop
left a comment
There was a problem hiding this comment.
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 ?
| 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) |
There was a problem hiding this comment.
Should we also be checking the CMIS revision (byte 1) here, since this field was reserved in CMIS 5.2 and earlier?
There was a problem hiding this comment.
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.
| # 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
3 participants
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
CoherentPagesSupportedbit — Page 01h byte 142 bit 4, defined inOIF-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/DiagPageSupportAdvtFieldbits already defined on that samebyte. 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 specrather 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()returnedFalse, soCCmisApiwas neverinstantiated and the module's laser could never be configured.