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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 7/5/26 20:54, Martino Dell'Ambrogio wrote:
> Some clone SFP modules (notably XGS-PON ONT sticks) ship malformed
> EEPROMs where the vendor PN field is filled with non-printable garbage
> past the trailing legitimate characters instead of SFF-8472 mandated
> space padding.

:(

> The current sfp_match() requires an exact full-field
> length match: sfp_strlen() returns 16 (no trailing spaces or NULs to
> strip), but strlen() of the quirk string is shorter, so the length
> comparison rejects the entry before strncmp() is even called and the
> quirk silently never applies. The kernel then honors the module's
> spurious TX_FAULT signal and the SFP state machine eventually disables
> the module.
> 
> Add a prefix_match flag to struct sfp_quirk and a SFP_QUIRK_F_PREFIX
> macro. When set, sfp_match() compares only strlen() leading bytes of
> the quirk string, ignoring trailing field bytes. Existing exact-match
> quirks are unaffected (prefix_match defaults to false via zero-init in
> the existing SFP_QUIRK macros).
> 
> Signed-off-by: Martino Dell'Ambrogio <tillo@tillo.ch>
> ---
>  drivers/net/phy/sfp.c | 22 +++++++++++++++++-----
>  drivers/net/phy/sfp.h |  1 +
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index f520206..e7ba642 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -516,6 +516,13 @@ 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 }
>  
>  static const struct sfp_quirk sfp_quirks[] = {
>  	// Alcatel Lucent G-010S-P can operate at 2500base-X, but incorrectly
> @@ -626,13 +633,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);
>  }

The variable naming qs_len and the "if(<ternary operator>)" aren't very easy
to process. It's correct but could use a few comments to explain that for
prefix match, we only compare up to the PN/Vendor string length.

But there's already a good comment on the macro definition, I'm personally
OK with this.

>  
>  static const struct sfp_quirk *sfp_lookup_quirk(const struct sfp_eeprom_id *id)
> @@ -645,8 +655,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;
>  
>  	return NULL;
> diff --git a/drivers/net/phy/sfp.h b/drivers/net/phy/sfp.h
> index 879dff7..867e45e 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;
>  };
>  
>  struct sfp_socket_ops {

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime