Re: [PATCH net-next v2 1/2] net: sfp: allow prefix matching in quirk lookup

[Jakub Kicinski <kuba@kernel.org>]

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: allow prefix matching in quirk lookup

This adds a prefix_match flag to struct sfp_quirk plus a
SFP_QUIRK_F_PREFIX macro, and teaches sfp_match() to compare only the
leading strlen() bytes of the quirk string when the flag is set.  The
motivation is clone SFP modules whose vendor PN field is padded with
garbage instead of spaces, which defeats the existing full-field length
comparison.

A couple of questions below.

> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 6c25b73c668a7..dfb2b883d7616 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -516,6 +516,14 @@ static void sfp_quirk_ubnt_uf_instant(const struct sfp_eeprom_id *id,
>  	{ .vendor = _v, .part = _p, .support = _s, .fixup = _f, }
>  #define SFP_QUIRK_S(_v, _p, _s) SFP_QUIRK(_v, _p, _s, NULL)
>  #define SFP_QUIRK_F(_v, _p, _f) SFP_QUIRK(_v, _p, NULL, _f)
> +/* Like SFP_QUIRK_F, but matches as a prefix. Use for clone modules
> + * that fill EEPROM trailing bytes with garbage instead of the
> + * SFF-8472-mandated space padding, so sfp_strlen can't trim the
> + * field down to the legitimate length.
> + */
> +#define SFP_QUIRK_F_PREFIX(_v, _p, _f) \
> +	{ .vendor = _v, .part = _p, .support = NULL, .fixup = _f, \
> +	  .prefix_match = true }

[Severity: Low]
Should the commit message describe what this patch alone does?

At this commit SFP_QUIRK_F_PREFIX has no users: every entry in
sfp_quirks[] still uses SFP_QUIRK/SFP_QUIRK_S/SFP_QUIRK_F, so
q->prefix_match is false for all entries and the new branch in
sfp_match() is not reachable.

The commit message describes the end result, including the user-visible
symptom:

    The kernel then honors the module's spurious TX_FAULT signal and the
    SFP state machine eventually disables the module.

That symptom is only fixed once the follow-on patch in the series, "net:
sfp: add quirks for OEM XGSPONST2001 and FS XGS-SFP-ONT-MACI", adds
SFP_QUIRK_F_PREFIX("FS", "XGS-SFP-ONT-MACI", sfp_fixup_potron) and
SFP_QUIRK_F_PREFIX("OEM", "XGSPONST2001", sfp_fixup_potron).  Splitting
mechanism from table entries is fine; the wording just reads as if this
patch fixes the modules by itself.

>  
>  static const struct sfp_quirk sfp_quirks[] = {
>  	// Alcatel Lucent G-010S-P can operate at 2500base-X, but incorrectly

[ ... ]

> @@ -629,13 +637,16 @@ static size_t sfp_strlen(const char *str, size_t maxlen)
>  	return size;
>  }
>  
> -static bool sfp_match(const char *qs, const char *str, size_t len)
> +static bool sfp_match(const char *qs, const char *str, size_t len, bool prefix)
>  {
> +	size_t qs_len;
> +
>  	if (!qs)
>  		return true;
> -	if (strlen(qs) != len)
> +	qs_len = strlen(qs);
> +	if (prefix ? qs_len > len : qs_len != len)
>  		return false;
> -	return !strncmp(qs, str, len);
> +	return !strncmp(qs, str, qs_len);
>  }
>  
>  static const struct sfp_quirk *sfp_lookup_quirk(const struct sfp_eeprom_id *id)
> @@ -648,8 +659,10 @@ static const struct sfp_quirk *sfp_lookup_quirk(const struct sfp_eeprom_id *id)
>  	ps = sfp_strlen(id->base.vendor_pn, ARRAY_SIZE(id->base.vendor_pn));
>  
>  	for (i = 0, q = sfp_quirks; i < ARRAY_SIZE(sfp_quirks); i++, q++)
> -		if (sfp_match(q->vendor, id->base.vendor_name, vs) &&
> -		    sfp_match(q->part, id->base.vendor_pn, ps))
> +		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))
>  			return q;

[Severity: Medium]
Was it intended for the prefix relaxation to apply to vendor_name as
well as vendor_pn?

sfp_lookup_quirk() passes the same q->prefix_match to both sfp_match()
calls, so an entry declared with SFP_QUIRK_F_PREFIX loosens the vendor
comparison too.  The macro comment only justifies the relaxation for the
PN field:

    /* Like SFP_QUIRK_F, but matches as a prefix. Use for clone modules
     * that fill EEPROM trailing bytes with garbage instead of the
     * SFF-8472-mandated space padding, so sfp_strlen can't trim the
     * field down to the legitimate length.
     */

With prefix set, the vendor test reduces to "vendor_name starts with
q->vendor", since sfp_match() only checks:

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

The vendor strings used by the entries added later in the series are
short, SFP_QUIRK_F_PREFIX("FS", ...) and SFP_QUIRK_F_PREFIX("OEM", ...),
so those entries accept any vendor_name beginning with "FS" or "OEM",
such as "FS.COM".

sfp_lookup_quirk() returns the first table hit, and the table already
contains vendor strings where one is a prefix of another:

	SFP_QUIRK("Hisense-Leox", "LXT-010S-H", sfp_quirk_2500basex,
		  sfp_fixup_ignore_tx_fault),
	SFP_QUIRK("Hisense", "ZNID-GPON-2311NA", sfp_quirk_2500basex,
		  sfp_fixup_ignore_tx_fault),

Can a future prefix entry then claim a module meant for a different
entry and run the wrong fixup?  sfp_fixup_potron() masks TX_FAULT and
LOS and extends T_START_UP, so a mis-match would suppress genuine fault
and LOS signalling on an unrelated module.

Would it make sense to keep the flag scoped to the part field, for
example a separate part_prefix_match, since struct sfp_quirk currently
has no way to express "prefix-match the part only"?

>  
>  	return NULL;

> diff --git a/drivers/net/phy/sfp.h b/drivers/net/phy/sfp.h
> index 879dff7afe6a4..867e45efbf3f1 100644
> --- a/drivers/net/phy/sfp.h
> +++ b/drivers/net/phy/sfp.h
> @@ -12,6 +12,7 @@ struct sfp_quirk {
>  	void (*support)(const struct sfp_eeprom_id *id,
>  			struct sfp_module_caps *caps);
>  	void (*fixup)(struct sfp *sfp);
> +	bool prefix_match;
>  };
-- 
pw-bot: cr