Re: [net-next,v6,3/3] w5100: detect carrier state using link status bit and optional interrupt

On Thu, Sep 03, 2026 at 12:56:04AM +0000, netdev-bot+sashiko@kernel.org wrote:

quoted

+static int w5500_get_phycfgr_lnk(struct net_device *ndev)
+{
+	struct w5100_priv *priv = netdev_priv(ndev);
+	int ret = w5100_read(priv, W5500_PHYCFGR);
+
+	if (ret < 0) {
+		netif_err(priv, link, ndev,
+			  "failed to read link status: %d\n", ret);
+		return 0;
+	}
+
+	return ret & PHYCFGR_LNK;
+}

[Severity: Medium]

Returning 0 on a failed read makes a bus error indistinguishable from a
genuine LNK=0 for every caller.  For the w5500, w5100_read() ends up in
w5500_spi_read():

drivers/net/ethernet/wiznet/w5100-spi.c:w5100_spi_read() {
ret = spi_write_then_read(spi, cmd, sizeof(cmd), &data, 1);

return ret ? ret : data;
}

so negative returns are reachable (spi_write_then_read() can fail with
-ENOMEM on its internal buffer, or with any controller error).

If that happens once inside w5100_open(), the else branch latches
netif_carrier_off(); inside w5100_resume() it just skips netif_carrier_on()
and the carrier stays off.  After that, the only code that ever touches the
carrier again is w5500_detect_link(), which runs on a LINKLED edge only, so
if the cable is left plugged in does the interface stay carrier-down for the
rest of its lifetime with no retry or re-read anywhere?

Would returning the error to the callers, and leaving the carrier state
unchanged on a failed read, be preferable?

Good point, I can propagate the error and handle it in the caller and
leave the carrier state unchanged.

One more note on the diagnostic: priv->msg_enable is never initialised in
w5100_probe() (the priv area comes from the zeroed alloc_etherdev()) and the
only writer is w5100_set_msglevel(), so netif_msg_link(priv) is false by
default and this netif_err() prints nothing unless the message level was
raised beforehand.

I guess it would be up to the userland to decide if they want to have
error messages enabled?

quoted

@@ -659,6 +705,15 @@ static int w5100_open(struct net_device *ndev)
 	w5100_hw_start(priv);
 	napi_enable(&priv->napi);
 	netif_start_queue(ndev);
+
+	if (priv->link_irq < 0 || w5100_get_link(ndev))
+		netif_carrier_on(ndev);
+	else
+		netif_carrier_off(ndev);

[Severity: Medium]

Since || short-circuits, w5100_get_link() is not called at all when link_irq
is negative and the carrier is unconditionally turned on.  The ethtool
callback added just above, however, keys on the chip id rather than on
link_irq:

if (priv->ops->chip_id == W5500)
	return w5500_get_phycfgr_lnk(ndev);

Again, good catch! One could setup the w5500 without the link IRQ but
still use the PHCFGR register for the carrier state.

quoted

+
+	if (priv->link_irq >= 0)
+		enable_irq(priv->link_irq);

[Severity: Medium]

The link bit is sampled and the carrier committed before the LINKLED source
is armed here.  The interrupt is requested with IRQF_NO_AUTOEN, so this
first enable_irq() is what performs irq_startup(); can an edge that arrives
between the PHYCFGR read (an SPI transfer) and enable_irq() be dropped
rather than replayed by check_irq_resend()?

w5100_resume() has the same order, right after w5100_hw_reset() /
w5100_hw_start() where the PHY renegotiates, which looks like a spot where a
link-up edge is likely to land in the window.

Since w5500_detect_link() also writes the carrier from its own PHYCFGR
sample, with no lock in common with open/resume, would swapping these two
statements be sufficient on its own, or does the "sample plus carrier
update" pair need serializing against the handler (or a re-sample after
enable_irq())?

If a transition is lost here, the carrier stays stale until the next
physical link change.

Chances are probably low but I can enable the irq after the get_link
check.

quoted

@@ -820,14 +902,20 @@ void w5100_remove(struct device *dev)
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct w5100_priv *priv = netdev_priv(ndev);

+	if (priv->link_irq >= 0) {
+		free_irq(priv->link_irq, ndev);
+		priv->link_irq = -ENODEV;
+	}
+
 	w5100_hw_reset(priv);
 	free_irq(priv->irq, ndev);

+	unregister_netdev(ndev);
+

[Severity: Medium]

Both interrupts are freed and the chip is reset here while the netdev is
still registered and possibly up, and priv->link_irq = -ENODEV is used as
the only guard against the other side.  Driver unbind
(device_release_driver() -> w5100_remove()) holds no rtnl, while
w5100_open() / w5100_stop() run under rtnl, and both read priv->link_irq
with no lock, barrier or READ_ONCE/WRITE_ONCE.

Can this interleaving happen?

CPU0 (unbind, no rtnl)			CPU1 (ip link set dev down, rtnl)
w5100_remove()				w5100_stop()
				if (priv->link_irq >= 0)  /* true */
free_irq(priv->link_irq, ndev);
priv->link_irq = -ENODEV;
				disable_irq(priv->link_irq);

disable_irq() on a descriptor with no installed action leaves desc->depth
unbalanced; after a re-bind, request_threaded_irq() with IRQF_NO_AUTOEN plus
the single enable_irq() in w5100_open() would not bring the depth back to
zero, so would link detection stay dead until reboot?

The mirror case is w5100_open() calling enable_irq() on a just-freed link
irq, and running w5100_hw_start() / w5100_get_link() against a chip that
w5100_hw_reset() has already reset and whose main irq handler has already
been released by free_irq(priv->irq, ndev).  The window covers several
sleeping SPI transfers plus a synchronize_irq(), so it is not narrow.

Would calling unregister_netdev() first, before free_irq(priv->link_irq),
free_irq(priv->irq) and w5100_hw_reset(), close this?  It closes the device
under rtnl and guarantees no further ndo_ or ethtool callbacks can start,
which would also make the priv->link_irq = -ENODEV store unnecessary.

That should work, calling unregister_netdev earlier would disable ndo_ calls.

quoted

@@ -24,7 +24,11 @@ properties:
       - wiznet,w5500

   interrupts:
-    maxItems: 1
+    minItems: 1
+    items:
+      - description: Main interrupt (INT pin on the device).
+      - description:
+          Optional interrupt for link status changes (LINKLED pin on w5500).

[Severity: Low]

This relaxes the count to two entries for all three compatibles, including
wiznet,w5100 and wiznet,w5200, even though the second item's own description
says the pin only exists on the w5500, and the driver only resolves index 1
for that chip:

priv->link_irq = ops->chip_id == W5500 ?
		 fwnode_irq_get(dev_fwnode(dev), 1) : -ENODEV;

Should this be expressed with an allOf / if-then block, so that

 if:
   properties:
     compatible:
       contains:
         enum:
           - wiznet,w5100
           - wiznet,w5200

keeps interrupts at maxItems: 1, and only wiznet,w5500 allows maxItems: 2?
As it stands a w5100/w5200 tree with a second interrupt passes
dt_binding_check while the driver ignores the line.

I think the user should also be able to read the document and know
which hardware they are using? But yes, adding allOf with a condition
would be bullet proof.

quoted

   reg:
     maxItems: 1

@@ -58,7 +62,8 @@ examples:
             pinctrl-names = "default";
             pinctrl-0 = <&eth1_pins>;
             interrupt-parent = <&gpio>;
-            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>,
+                         <24 IRQ_TYPE_EDGE_BOTH>;

[Severity: Low]

This isn't an issue introduced by this series, but the example describes the
main interrupt as IRQ_TYPE_EDGE_FALLING while the driver requests that same
line with a hard-coded IRQF_TRIGGER_LOW in both paths:

err = request_threaded_irq(priv->irq, NULL, w5100_interrupt,
			   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
			   dev_name(dev), ndev);
...
err = request_irq(priv->irq, w5100_interrupt,
		  IRQF_TRIGGER_LOW, dev_name(dev), ndev);

A non-zero IRQF_TRIGGER_ mask makes __setup_irq() call __irq_set_trigger()
and reprogram the controller, so the type from the device tree is
discarded.  The same example text came from the deleted
Documentation/devicetree/bindings/net/wiznet,w5x00.txt.  Since the file is
being touched anyway, is it worth aligning the example with what the driver
actually programs?  The second entry is consistent, IRQ_TYPE_EDGE_BOTH
matches IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.

quoted

             spi-max-frequency = <30000000>;
             local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
         };

Right, the DT binding is not aligned with the implementation.

--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-wiznet-link-gpio-v6-0-abee6c5c966e%40marmottus.net

Thank you!

Arthur