Re: [PATCH net-next v2 2/2] net: sfp: add quirks for OEM XGSPONST2001 and FS XGS-SFP-ONT-MACI

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: sfp: add quirks for OEM XGSPONST2001 and FS XGS-SFP-ONT-MACI

This adds two sfp_quirks entries applying sfp_fixup_potron to XGS-PON ONT
sticks that report broken TX_FAULT/LOS and need a longer T_START_UP. Both
entries use SFP_QUIRK_F_PREFIX so the vendor PN field is matched as a
prefix rather than by exact length.

> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index dfb2b883d7616..4d8219e5ed599 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -556,6 +556,13 @@ static const struct sfp_quirk sfp_quirks[] = {
>  	SFP_QUIRK("FS", "GPON-ONU-34-20BI", sfp_quirk_2500basex,
>  		  sfp_fixup_ignore_tx_fault),
>  
> +	// Fiberstore XGS-SFP-ONT-MACI is a MAC-mode XGS-PON ONT stick with
> +	// ONT-class serial-passthrough TX_FAULT/LOS wiring and slow startup;
> +	// mask both signals and extend T_START_UP via the potron fixup. The
> +	// EEPROM vendor PN field is not space-padded past the legitimate
> +	// string, so match it as a prefix.
> +	SFP_QUIRK_F_PREFIX("FS", "XGS-SFP-ONT-MACI", sfp_fixup_potron),

[Severity: Medium]
Can the prefix behaviour make any difference for this particular part
string?  "XGS-SFP-ONT-MACI" is 16 characters, which is the full width of
the field it is compared against:

include/linux/sfp.h:
	char vendor_pn[16];

sfp_lookup_quirk() bounds the computed length to that width:

	ps = sfp_strlen(id->base.vendor_pn, ARRAY_SIZE(id->base.vendor_pn));

and sfp_match() then does:

	if (prefix ? qs_len > len : qs_len != len)
		return false;
	return !strncmp(qs, str, qs_len);

With qs_len == 16 and ps <= 16, "qs_len > len" and "qs_len != len" accept
exactly the same input (ps == 16), and the strncmp() covers all 16 bytes.
So there are no EEPROM bytes left "past the legitimate string" for this
entry, and the part comparison is identical to what plain SFP_QUIRK_F()
would perform.

That leaves two possibilities for this module.  Either the PN field really
does read "XGS-SFP-ONT-MACI" and SFP_QUIRK_F() would have matched just as
well, so the commit message statement

	"a plain SFP_QUIRK_F entry would silently never apply and the
	kernel would honor the spurious TX_FAULT and eventually disable
	the module"

does not hold for this entry, or the field really does contain garbage,
which then falls inside the 16 compared bytes and neither prefix nor exact
matching can ever fire, making the new entry dead code.

Which of the two is it, and could the comment and the commit message be
adjusted to describe what the code actually does here?

>  	SFP_QUIRK_F("HALNy", "HL-GSFP", sfp_fixup_halny_gsfp),
>  
>  	SFP_QUIRK_F("H-COM", "SPP425H-GAB4", sfp_fixup_potron),
> @@ -616,6 +623,14 @@ static const struct sfp_quirk sfp_quirks[] = {
>  	SFP_QUIRK_S("OEM", "SFP-2.5G-LH20-A", sfp_quirk_2500basex),
>  	SFP_QUIRK_F("OEM", "RTSFP-10", sfp_fixup_rollball_cc),
>  	SFP_QUIRK_F("OEM", "RTSFP-10G", sfp_fixup_rollball_cc),
> +
> +	// OEM XGSPONST2001 is an XGS-PON ONT stick with broken TX_FAULT and
> +	// LOS indicators and slow startup, just like potron. The EEPROM
> +	// vendor PN field is filled with non-printable garbage past the
> +	// legitimate string instead of space padding, so match it as a
> +	// prefix.
> +	SFP_QUIRK_F_PREFIX("OEM", "XGSPONST2001", sfp_fixup_potron),

[Severity: Low]
This isn't a bug, but both new comments and the SFP_QUIRK_F_PREFIX macro
comment justify prefix matching purely in terms of the vendor PN field,
while struct sfp_quirk carries a single flag:

drivers/net/phy/sfp.h:
	bool prefix_match;

and sfp_lookup_quirk() hands that same flag to the vendor comparison:

	if (sfp_match(q->vendor, id->base.vendor_name, vs,
		      q->prefix_match) &&
	    sfp_match(q->part, id->base.vendor_pn, ps,
		      q->prefix_match))

So for these two entries the vendor test also becomes a prefix test on
very short strings, and a module reporting vendor_name "OEMTEK" or "FSCOM"
now passes the vendor check.  The PN still has to prefix-match the full
model string, so the practical widening is limited to close clones, but
since sfp_fixup_potron() masks both TX_FAULT and LOS, an unintended match
silently hides genuine fault reporting.

Would it be worth either noting in the comments that the vendor field is
matched as a prefix too, or splitting the flag so prefix semantics only
apply to the part field?

>  	SFP_QUIRK_F("Turris", "RTSFP-2.5G", sfp_fixup_rollball),
>  	SFP_QUIRK_F("Turris", "RTSFP-10", sfp_fixup_rollball),
>  	SFP_QUIRK_F("Turris", "RTSFP-10G", sfp_fixup_rollball),