[PATCH net-next v2 0/2] net: sfp: quirk support for XGS-PON ONT sticks with unclean EEPROMs

[PATCH net-next v2 0/2] net: sfp: quirk support for XGS-PON ONT sticks with unclean EEPROMs

[Martino Dell'Ambrogio]

Some clone XGS-PON ONT sticks ship EEPROMs where the vendor PN field is
filled with non-printable garbage past the legitimate string instead of
the SFF-8472 mandated space padding. sfp_strlen() then can't trim the
field, the exact-length check in sfp_match() rejects the quirk entry
before the string comparison runs, and the quirk silently never applies
— so the kernel honors the module's spurious TX_FAULT and eventually
disables it.

Patch 1 adds an opt-in prefix-matching flag to the quirk table so such
modules can still be matched; existing exact-match entries behave
exactly as before. Patch 2 adds the two entries that need it: the
"OEM" XGSPONST2001 and the Fiberstore XGS-SFP-ONT-MACI, both wired to
the existing potron fixup.

Both quirks are in production use on a Bananapi BPI-R4 (MT7988A)
router on an XGS-PON uplink, backported onto 6.12.

v2, reposted as requested after the July patch-queue flush [1]:
 - collected Maxime's Reviewed-by tags
 - wrapped the SFP_QUIRK_F_PREFIX macro body to fit in 80 columns
   (checkpatch warning flagged by NIPA on v1)
No functional change since v1 [2].

[1] https://lore.kernel.org/netdev/20260720173156.41189431@kernel.org/
[2] https://lore.kernel.org/netdev/20260705185440.136496-1-tillo@tillo.ch/

Martino Dell'Ambrogio (2):
  net: sfp: allow prefix matching in quirk lookup
  net: sfp: add quirks for OEM XGSPONST2001 and FS XGS-SFP-ONT-MACI

 drivers/net/phy/sfp.c | 38 +++++++++++++++++++++++++++++++++-----
 drivers/net/phy/sfp.h |  1 +
 2 files changed, 34 insertions(+), 5 deletions(-)

base-commit: b0057c68df711bf6a62033c072ac61c4f9d3cbc1
-- 
2.47.3

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

[Martino Dell'Ambrogio]

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>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/sfp.c | 23 ++++++++++++++++++-----
 drivers/net/phy/sfp.h |  1 +
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 6c25b73..dfb2b88 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 }
 
 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;
 
 	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 {
-- 
2.47.3

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

[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: 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

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

[Martino Dell'Ambrogio]

Cheap XGS-PON ONT sticks identifying as vendor "OEM", PN "XGSPONST2001"
have broken TX_FAULT and LOS indicators (driven by the ONU serial
passthrough wires) and need a longer T_START_UP than the SFF-8472
default. The Fiberstore XGS-SFP-ONT-MACI MAC-mode ONT stick has the
same ONT-class TX_FAULT/LOS wiring and startup behaviour. Apply the
existing sfp_fixup_potron handler to both, which masks both signals
and bumps T_START_UP to T_START_UP_BAD_GPON.

Both modules fail to space-pad the EEPROM vendor PN field past the
legitimate string as SFF-8472 mandates (the XGSPONST2001 fills it with
non-printable garbage), which defeats exact-length matching:
sfp_strlen() cannot trim the field, so a plain SFP_QUIRK_F entry would
silently never apply and the kernel would honor the spurious TX_FAULT
and eventually disable the module. Match both entries as prefixes
using SFP_QUIRK_F_PREFIX.

Signed-off-by: Martino Dell'Ambrogio <tillo@tillo.ch>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/phy/sfp.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index dfb2b88..4d8219e 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),
+
 	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),
+
 	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),
-- 
2.47.3

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