[PATCH net-next v1] net: phy: sfp: detect presence via I2C when no MOD_DEF0 GPIO

[PATCH net-next v1] net: phy: sfp: detect presence via I2C when no MOD_DEF0 GPIO

[Greg Patrick]

Boards that do not wire the SFP cage MOD_DEF0 signal to a GPIO (for
example the NicGiga S100-0800S-M, where the I/O expander is fully
consumed by TX_DISABLE) currently use sff_gpio_get_state(), which
unconditionally reports the module as present. An empty cage therefore
fails its probe and is parked in SFP_MOD_ERROR forever; because
SFP_F_PRESENT never deasserts there is no REMOVE event to recover the
state machine, so a module inserted after boot is never detected.

Derive presence from a throttled single-byte I2C read of the module
EEPROM instead: an ACK asserts SFP_F_PRESENT, two consecutive failures
clear it (to ride out a transient error on a live module). The existing
poll then emits SFP_E_INSERT / SFP_E_REMOVE normally, giving working
hot-plug and avoiding the boot-time -EIO spam on empty cages.

Signed-off-by: Greg Patrick <gregspatrick@hotmail.com>
---
Posting for review per a downstream discussion (OpenWrt PR #23579) on adding
the NicGiga S100-0800S-M, an RTL9303 8x SFP+ switch that wires none of the
cage sideband signals to a software-visible GPIO. I'd value your steer before
this is carried downstream.

Open design question: this changes behaviour for *all* boards without a
MOD_DEF0 GPIO, from "always present" (sff_gpio_get_state) to I2C-probed
presence. That fixes hot-plug and the empty-cage -EIO spam on boards that
simply lack a wired MOD_ABS, but it also affects boards that intentionally
carry a permanently-fitted module and relied on the always-present path.
Would you prefer this gated behind a DT opt-in property rather than changing
the default? Happy to respin either way. The 2s re-probe interval is likewise
a first guess.

Tested on a NicGiga S100-0800S-M (RTL9303, 8x SFP+, no MOD_DEF0 GPIO):
inserting a module into a cage that was empty at boot now links within ~2s,
removal is detected, and the boot-time -EIO burst on empty cages is gone. No
behaviour change observed on cages populated at boot.

 drivers/net/phy/sfp.c | 74 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 66 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 376c705a9..fd99638e9 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -206,6 +206,11 @@ static const enum gpiod_flags gpio_flags[] = {
 #define T_PROBE_RETRY_SLOW	msecs_to_jiffies(5000)
 #define R_PROBE_RETRY_SLOW	12
 
+/* Interval at which sfp_i2c_get_state() re-probes the I2C bus for module
+ * presence on boards without a MOD_DEF0 GPIO (see sfp_i2c_get_state()).
+ */
+#define T_PROBE_PRESENT		msecs_to_jiffies(2000)
+
 /* SFP modules appear to always have their PHY configured for bus address
  * 0x56 (which with mdio-i2c, translates to a PHY address of 22).
  * RollBall SFPs access phy via SFP Enhanced Digital Diagnostic Interface
@@ -249,6 +254,13 @@ struct sfp {
 
 	bool need_poll;
 
+	/* I2C-probed presence, for boards without a MOD_DEF0 GPIO.
+	 * Access rules: st_mutex held (updated from the poll/state machine).
+	 */
+	bool i2c_present;
+	u8 i2c_present_nak;
+	unsigned long i2c_present_next;
+
 	/* Access rules:
 	 * state_hw_drive: st_mutex held
 	 * state_hw_mask: st_mutex held
@@ -651,11 +663,6 @@ static unsigned int sfp_gpio_get_state(struct sfp *sfp)
 	return state;
 }
 
-static unsigned int sff_gpio_get_state(struct sfp *sfp)
-{
-	return sfp_gpio_get_state(sfp) | SFP_F_PRESENT;
-}
-
 static void sfp_gpio_set_state(struct sfp *sfp, unsigned int state)
 {
 	unsigned int drive;
@@ -863,6 +870,44 @@ static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
 	return sfp->read(sfp, a2, addr, buf, len);
 }
 
+/* Probe whether a module is physically present by attempting a single-byte
+ * I2C read of the EEPROM identifier (an empty cage NAKs). Used as the presence
+ * source on boards that do not wire MOD_DEF0 to a GPIO.
+ */
+static bool sfp_module_present_i2c(struct sfp *sfp)
+{
+	u8 id;
+
+	return sfp_read(sfp, false, SFP_PHYS_ID, &id, sizeof(id)) == sizeof(id);
+}
+
+/* get_state variant for boards without a MOD_DEF0 GPIO. Instead of assuming
+ * the module is always present, derive SFP_F_PRESENT from a throttled I2C
+ * probe so that hot-insertion and removal are detected. A single ACK asserts
+ * presence; two consecutive failures clear it, to ride out a transient I2C
+ * error on a live module.
+ */
+static unsigned int sfp_i2c_get_state(struct sfp *sfp)
+{
+	unsigned int state = sfp_gpio_get_state(sfp);
+
+	if (time_after_eq(jiffies, sfp->i2c_present_next)) {
+		if (sfp_module_present_i2c(sfp)) {
+			sfp->i2c_present = true;
+			sfp->i2c_present_nak = 0;
+		} else if (sfp->i2c_present && ++sfp->i2c_present_nak >= 2) {
+			sfp->i2c_present = false;
+			sfp->i2c_present_nak = 0;
+		}
+		sfp->i2c_present_next = jiffies + T_PROBE_PRESENT;
+	}
+
+	if (sfp->i2c_present)
+		state |= SFP_F_PRESENT;
+
+	return state;
+}
+
 static int sfp_write(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
 {
 	return sfp->write(sfp, a2, addr, buf, len);
@@ -3168,9 +3213,22 @@ static int sfp_probe(struct platform_device *pdev)
 	sfp->get_state = sfp_gpio_get_state;
 	sfp->set_state = sfp_gpio_set_state;
 
-	/* Modules that have no detect signal are always present */
-	if (!(sfp->gpio[GPIO_MODDEF0]))
-		sfp->get_state = sff_gpio_get_state;
+	/* Boards with no MOD_DEF0 GPIO have no hardware presence signal. Rather
+	 * than assume the module is always present (which traps an empty cage
+	 * in MOD_ERROR and never detects hot-insertion), derive presence from a
+	 * throttled I2C probe and poll for changes. sfp_i2c_configure() has
+	 * already set i2c_max_block_size; seed i2c_block_size so the presence
+	 * read does not issue a zero-length transfer before the first probe.
+	 * Seed i2c_present_next to jiffies so the first probe happens
+	 * immediately (a zero value would be in the past relative to the
+	 * negative INITIAL_JIFFIES at boot and delay detection).
+	 */
+	if (!(sfp->gpio[GPIO_MODDEF0])) {
+		sfp->get_state = sfp_i2c_get_state;
+		sfp->i2c_block_size = sfp->i2c_max_block_size;
+		sfp->i2c_present_next = jiffies;
+		sfp->need_poll = true;
+	}
 
 	device_property_read_u32(&pdev->dev, "maximum-power-milliwatt",
 				 &sfp->max_power_mW);
-- 
2.53.0

Re: [PATCH net-next v1] net: phy: sfp: detect presence via I2C when no MOD_DEF0 GPIO

[Andrew Lunn]

> Posting for review per a downstream discussion (OpenWrt PR #23579) on adding
> the NicGiga S100-0800S-M, an RTL9303 8x SFP+ switch that wires none of the
> cage sideband signals to a software-visible GPIO.

Are they wired to a software invisible GPIO? Or nowhere at all?

> Open design question: this changes behaviour for *all* boards without a
> MOD_DEF0 GPIO, from "always present" (sff_gpio_get_state) to I2C-probed
> presence.

You might want to differentiate between an SFF and an SFP, using the
compatible. An SFF has no need for an MOD_DEF0 GPIO. I did not look,
maybe the code already does?

	Andrew

Re: [PATCH net-next v1] net: phy: sfp: detect presence via I2C when no MOD_DEF0 GPIO

[Greg Patrick]

Thanks for the review, Andrew -- folding your three replies together.

> > ... wires none of the cage sideband signals to a software-visible GPIO.
> Are they wired to a software invisible GPIO? Or nowhere at all?

Nowhere usable that I can find. TX_DISABLE is driven via a PCA9534 I/O
expander, but no presence/MOD_ABS line reaches a readable input: the
RTL9303 gpio0 lines the sister TL-ST1008F uses for MOD_DEF0 read stuck-low
here, the single PCA9534 is fully consumed by TX_DISABLE, and there is no
RTL8231. I haven't traced the PCB or decompiled the vendor firmware, so I
can't prove it goes literally nowhere versus somewhere I can't reach -- but
there is no software-readable presence source. I'll reword the commit
message to say that precisely.

> You might want to differentiate between an SFF and an SFP, using the
> compatible. An SFF has no need for an MOD_DEF0 GPIO. I did not look,
> maybe the code already does?

It doesn't currently, and you're right that my patch wrongly changes the
sff,sff case too: sff_data has no SFP_F_PRESENT in .gpios, so a soldered
sff,sff never has a MOD_DEF0 GPIO and always hits this branch. v2 keeps the
always-present path for sff,sff and uses the I2C probe only for an sff,sfp
cage with no MOD_DEF0 GPIO, gated on (sff->gpios & SFP_F_PRESENT).

> Did you try a 0 byte read? That is what tools like i2cdetect use.

I can't on this board: the cage I2C master is the RTL9303's SMBus
controller, which advertises only I2C_FUNC_SMBUS_BYTE_DATA -- no
I2C_FUNC_I2C and, notably, no SMBus Quick Command (the zero-length probe
i2cdetect uses). sfp_i2c_configure() therefore selects sfp_smbus_byte_read()
with i2c_max_block_size = 1, and the presence probe reuses that same path.
A 1-byte read is the lightest primitive this controller supports, and it is
also the only form that works through sfp->read on both raw-I2C and
SMBus-only adapters -- a zero-length transfer is not expressible there. (I
checked on the hardware: i2ctransfer reports "Adapter does not have I2C
transfers capability", and i2cdetect -F shows Quick Command unsupported but
Receive/Read Byte supported, which is what presence detection rides on.)

> > -static unsigned int sff_gpio_get_state(struct sfp *sfp)
> Humm. Why?

That deletion was wrong, for the sff,sff reason above -- I'll keep
sff_gpio_get_state() in v2.

I'll send the v2 with these changes after the 24h window. Thanks again.

Greg

Re: [PATCH net-next v1] net: phy: sfp: detect presence via I2C when no MOD_DEF0 GPIO

[Andrew Lunn]

> > Did you try a 0 byte read? That is what tools like i2cdetect use.
> 
> I can't on this board: the cage I2C master is the RTL9303's SMBus
> controller, which advertises only I2C_FUNC_SMBUS_BYTE_DATA -- no
> I2C_FUNC_I2C and, notably, no SMBus Quick Command

O.K. So the board design is broken. To properly use an SFP you need
two byte reads :-(

Anyway, i was thinking 0 byte might be more reliable. We know SFPs get
all sorts of things wrong, even simple things like reading a word. Can
they get reading 0 bytes wrong? I suppose it is not a typical use
case, so probably yes :-)

So lets stick to 1-byte reads.

      Andrew

Re: [PATCH net-next v1] net: phy: sfp: detect presence via I2C when no MOD_DEF0 GPIO

[Andrew Lunn]

> +/* Probe whether a module is physically present by attempting a single-byte
> + * I2C read of the EEPROM identifier (an empty cage NAKs). Used as the presence
> + * source on boards that do not wire MOD_DEF0 to a GPIO.

Did you try a 0 byte read? That is what tools like i2cdetect use.

	Andrew

Re: [PATCH net-next v1] net: phy: sfp: detect presence via I2C when no MOD_DEF0 GPIO

[Andrew Lunn]

> -static unsigned int sff_gpio_get_state(struct sfp *sfp)
> -{
> -	return sfp_gpio_get_state(sfp) | SFP_F_PRESENT;
> -}

Humm. Why?

      Andrew