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

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 <redacted>
---
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

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

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

quoted

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

quoted

-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

quoted

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

+/* 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

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

Humm. Why?

      Andrew