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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 4 · Low: 1 (1)

- [Medium] On a W5500 whose optional LINKLED interrupt is not described
  in the device tree, the driver's two link-state interfaces permanently…
- [Medium] w5500_get_phycfgr_lnk() (line 352) converts a register-read
  failure into "link down": int ret = w5100_read(priv, W5500_PHYCFGR);…
- [Medium] w5100_remove() frees both IRQs and resets the hardware while
  the netdev is still registered and possibly UP, and uses an…
- [Medium] w5100_open() and w5100_resume() sample the PHYCFGR link bit
  and commit the carrier state *before* arming the link interrupt: if…
- [Low] The binding schema allows two `interrupts` entries for all three
  compatibles (wiznet,w5100, wiznet,w5200, wiznet,w5500) with no…

Pre-existing issues:
- [Low] The binding example specifies the main interrupt as
  `IRQ_TYPE_EDGE_FALLING` while the driver requests that same interrupt…

--

w5100: detect carrier state using link status bit and optional interrupt

This adds link detection for the w5500 by reading the Link Status bit of the
PHY Configuration register, exposes it through ethtool get_link, and wires an
optional second interrupt (LINKLED pin) to update the carrier on link
changes. It also moves register_netdev() to the end of w5100_probe() and
reworks the probe/remove unwind order.

A few questions below, plus two about the dt-bindings patch earlier in the
series.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> index 53d8dc642fbd6..d91881ecd797c 100644
> --- a/drivers/net/ethernet/wiznet/w5100.c
> +++ b/drivers/net/ethernet/wiznet/w5100.c

[ ... ]

> @@ -345,6 +349,38 @@ static void w5500_memory_configure(struct w5100_priv *priv)
>  	}
>  }
>  
> +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?

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.

> +
> +static irqreturn_t w5500_detect_link(int irq, void *ndev_instance)
> +{
> +	struct net_device *ndev = ndev_instance;
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +
> +	if (netif_running(ndev)) {
> +		if (w5500_get_phycfgr_lnk(ndev)) {
> +			netif_info(priv, link, ndev, "link is up\n");
> +			netif_carrier_on(ndev);
> +		} else {
> +			netif_info(priv, link, ndev, "link is down\n");
> +			netif_carrier_off(ndev);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int w5100_hw_reset(struct w5100_priv *priv)
>  {
>  	u32 rtr;
> @@ -414,6 +450,16 @@ static void w5100_get_drvinfo(struct net_device *ndev,
>  		sizeof(info->bus_info));
>  }
>  
> +static u32 w5100_get_link(struct net_device *ndev)
> +{
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +
> +	if (priv->ops->chip_id == W5500)
> +		return w5500_get_phycfgr_lnk(ndev);
> +
> +	return 1;
> +}
> +

[ ... ]

> @@ -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);

The binding patch earlier in this series documents the second interrupt as
optional, so a w5500 node with a single interrupts entry leaves
fwnode_irq_get(dev_fwnode(dev), 1) returning -EINVAL, which is accepted
silently (only -EPROBE_DEFER aborts probe).

On such a board with the cable unplugged, does ip link report LOWER_UP while
ethtool reports "Link detected: no"?  With no link interrupt there is no
code path left that updates the carrier after open, so is there anything
that can reconcile the two afterwards?  Consumers that use ETHTOOL_GLINK
instead of the carrier (for example bonding with use_carrier=0 through
bond_check_dev_link()) would then see the opposite state from the stack.

The same short-circuit is repeated in w5100_resume(), so the state is
re-asserted on every resume.

The commit message says the link status on the w5500 is detected "using the
Link Status bit from the PHY Configuration register (PHYCFGR)" and that only
"On w5100/w5200, which lack this register, the link is always reported as
up."  Should the carrier decision key on chip_id, the way w5100_get_link()
already does, rather than on the presence of the interrupt?

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

> +
>  	return 0;
>  }
>  
> @@ -667,6 +722,10 @@ static int w5100_stop(struct net_device *ndev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	netif_info(priv, ifdown, ndev, "shutting down\n");
> +
> +	if (priv->link_irq >= 0)
> +		disable_irq(priv->link_irq);
> +
>  	w5100_hw_close(priv);
>  	netif_carrier_off(ndev);
>  	netif_stop_queue(ndev);

[ ... ]

> @@ -745,13 +805,21 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
>  		break;
>  	default:
>  		err = -EINVAL;
> -		goto err_register;
> +		goto err_wq;
>  	}
>  
>  	priv->ndev = ndev;
>  	priv->ops = ops;
>  	priv->irq = irq;
>  
> +	priv->link_irq = ops->chip_id == W5500 ?
> +			 fwnode_irq_get(dev_fwnode(dev), 1) : -ENODEV;
> +	if (priv->link_irq == -EPROBE_DEFER) {
> +		err = dev_err_probe(dev, priv->link_irq,
> +				    "failed to get link irq\n");
> +		goto err_wq;
> +	}
> +

[ ... ]

> @@ -795,21 +859,39 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
>  	if (err)
>  		goto err_hw;
>  
> +	if (priv->link_irq >= 0) {
> +		err = request_threaded_irq(priv->link_irq, NULL,
> +					   w5500_detect_link,
> +					   IRQF_TRIGGER_RISING |
> +					   IRQF_TRIGGER_FALLING |
> +					   IRQF_ONESHOT | IRQF_NO_AUTOEN,
> +					   "w5100-link", priv->ndev);
> +		if (err < 0)
> +			goto err_link_irq;
> +	}
> +
> +	err = register_netdev(ndev);
> +	if (err < 0)
> +		goto err_register;
> +
>  	return 0;
>  
> +err_register:
> +	if (priv->link_irq >= 0)
> +		free_irq(priv->link_irq, ndev);
> +err_link_irq:
> +	free_irq(priv->irq, ndev);
>  err_hw:
>  	destroy_workqueue(priv->xfer_wq);
>  err_wq:
> -	unregister_netdev(ndev);
> -err_register:
>  	free_netdev(ndev);
>  	return err;
>  }
> @@ -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.

>  	flush_work(&priv->setrx_work);
>  	flush_work(&priv->restart_work);
>  	destroy_workqueue(priv->xfer_wq);
>  
> -	unregister_netdev(ndev);
>  	free_netdev(ndev);
>  }
>  EXPORT_SYMBOL_GPL(w5100_remove);

[ ... ]

> @@ -857,6 +948,12 @@ static int w5100_resume(struct device *dev)
>  		w5100_hw_start(priv);
>  
>  		netif_device_attach(ndev);
> +
> +		if (priv->link_irq < 0 || w5100_get_link(ndev))
> +			netif_carrier_on(ndev);
> +
> +		if (priv->link_irq >= 0)
> +			enable_irq(priv->link_irq);
>  	}
>  	return 0;
>  }

Two questions about the preceding dt-bindings patch in the series, commit
6a4ec8a759 ("dt-bindings: net: wiznet,w5100: add link status interrupt"):

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> index 601d828bae927..d0717ab45de6c 100644
> --- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> @@ -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.

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

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

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