[PATCH net-next v6 0/3] w5100: restore GPIO-based link detection

[PATCH net-next v6 0/3] w5100: restore GPIO-based link detection

[Arthur Crepin Leblond]

Hi,

In the previous version of the w5100 driver, the LINKLED pin could be
wired to a GPIO to detect the link status changes via an interrupt.
This only works on w5500 as it is the only one documenting its LINKLED
pin to hold the link status (on w5100 the LINKLED pin is changing with
RX/TX activity).

This series of patches is bringing back the link status detection on
w5500 using the Link Status bit of the PHY Configuration Register.
Additionally, the LINKLED pin can also be wired to detect link status
changes and read the register in the interrupt handler.

Arthur Crepin Leblond

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
---
- Link to v5: https://patch.msgid.link/20260813-wiznet-link-gpio-v5-0-ac8046f24413@marmottus.net
- Link to v4: https://patch.msgid.link/20260811-wiznet-link-gpio-v4-0-4d5f7da885a8@marmottus.net
- Link to v3: https://patch.msgid.link/20260806-wiznet-link-gpio-v3-0-532d4a143805@marmottus.net
- Link to v2: https://patch.msgid.link/20260804-wiznet-link-gpio-v2-0-3b1d0c870f35@marmottus.net
- Link to v1: https://patch.msgid.link/20260804-wiznet-link-gpio-v1-1-b626fd4f7ccb@marmottus.net
---
Arthur Crepin Leblond (3):
      dt-bindings: net: wiznet,w5100: convert to DT schema
      dt-bindings: net: wiznet,w5100: add link status interrupt
      w5100: detect carrier state using link status bit and optional interrupt

 .../devicetree/bindings/net/wiznet,w5100.yaml      |  71 ++++++++++++
 .../devicetree/bindings/net/wiznet,w5x00.txt       |  50 ---------
 drivers/net/ethernet/wiznet/w5100.c                | 119 +++++++++++++++++++--
 3 files changed, 179 insertions(+), 61 deletions(-)

[PATCH net-next v6 1/3] dt-bindings: net: wiznet,w5100: convert to DT schema

[Arthur Crepin Leblond]

Convert the Wiznet w5x00 SPI Ethernet controller binding from
plain text to DT schema.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

---
v6:
- Put back the reviewed by tag from Krzysztof Kozlowski from v4

v5:
- Remove gpio/gpio.h include from DT example
- Make the DT interrupts property dynamic from the compatible one
- Fix DT bindings errors

v4:
- Update interrupts in DT bindings

v3:
- Fix DT binding errors

v2:
- Convert device tree binding to YAML
---
 .../devicetree/bindings/net/wiznet,w5100.yaml      | 66 ++++++++++++++++++++++
 .../devicetree/bindings/net/wiznet,w5x00.txt       | 50 ----------------
 2 files changed, 66 insertions(+), 50 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
new file mode 100644
index 000000000000..601d828bae92
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/wiznet,w5100.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Wiznet w5100/w5200/w5500 SPI Ethernet Controller
+
+maintainers:
+  - Arthur Crepin Leblond <arthur@marmottus.net>
+
+description: |
+  This is a standalone 10/100 MBit Ethernet controller with SPI interface.
+
+allOf:
+  - $ref: /schemas/net/ethernet-controller.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - wiznet,w5100
+      - wiznet,w5200
+      - wiznet,w5500
+
+  interrupts:
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    description:
+      According to the w5500 datasheet, the chip allows a maximum of 80 MHz,
+      however, board designs may need to limit this value.
+    maximum: 80000000
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    /* Example (for Raspberry Pi with pin control stuff for GPIO irq) */
+
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethernet@0 {
+            compatible = "wiznet,w5500";
+            reg = <0>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&eth1_pins>;
+            interrupt-parent = <&gpio>;
+            interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+            spi-max-frequency = <30000000>;
+            local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
+        };
+    };
+...
diff --git a/Documentation/devicetree/bindings/net/wiznet,w5x00.txt b/Documentation/devicetree/bindings/net/wiznet,w5x00.txt
deleted file mode 100644
index e9665798c4be..000000000000
--- a/Documentation/devicetree/bindings/net/wiznet,w5x00.txt
+++ /dev/null
@@ -1,50 +0,0 @@
-* Wiznet w5x00
-
-This is a standalone 10/100 MBit Ethernet controller with SPI interface.
-
-For each device connected to a SPI bus, define a child node within
-the SPI master node.
-
-Required properties:
-- compatible: Should be one of the following strings:
-	      "wiznet,w5100"
-	      "wiznet,w5200"
-	      "wiznet,w5500"
-- reg: Specify the SPI chip select the chip is wired to.
-- interrupts: Specify the interrupt index within the interrupt controller (referred
-              to above in interrupt-parent) and interrupt type. w5x00 natively
-              generates falling edge interrupts, however, additional board logic
-              might invert the signal.
-- pinctrl-names: List of assigned state names, see pinctrl binding documentation.
-- pinctrl-0: List of phandles to configure the GPIO pin used as interrupt line,
-             see also generic and your platform specific pinctrl binding
-             documentation.
-
-Optional properties:
-- spi-max-frequency: Maximum frequency of the SPI bus when accessing the w5500.
-  According to the w5500 datasheet, the chip allows a maximum of 80 MHz, however,
-  board designs may need to limit this value.
-- local-mac-address: See ethernet.txt in the same directory.
-
-
-Example (for Raspberry Pi with pin control stuff for GPIO irq):
-
-&spi {
-	ethernet@0: w5500@0 {
-		compatible = "wiznet,w5500";
-		reg = <0>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&eth1_pins>;
-		interrupt-parent = <&gpio>;
-		interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
-		spi-max-frequency = <30000000>;
-	};
-};
-
-&gpio {
-	eth1_pins: eth1_pins {
-		brcm,pins = <25>;
-		brcm,function = <0>; /* in */
-		brcm,pull = <0>; /* none */
-	};
-};

-- 
2.55.0

Re: [net-next,v6,1/3] dt-bindings: net: wiznet,w5100: convert to DT schema

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] The new schema enforces one `spi-max-frequency: maximum:
  80000000` for all three compatibles (wiznet,w5100, wiznet,w5200,…

--

dt-bindings: net: wiznet,w5100: convert to DT schema

This converts the Wiznet w5x00 SPI Ethernet controller binding from plain
text to a YAML DT schema, covering the wiznet,w5100, wiznet,w5200 and
wiznet,w5500 compatibles. The old wiznet,w5x00.txt file is removed.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> new file mode 100644
> index 0000000000000..601d828bae927
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> @@ -0,0 +1,66 @@

[ ... ]

> +properties:
> +  compatible:
> +    enum:
> +      - wiznet,w5100
> +      - wiznet,w5200
> +      - wiznet,w5500
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    description:
> +      According to the w5500 datasheet, the chip allows a maximum of 80 MHz,
> +      however, board designs may need to limit this value.
> +    maximum: 80000000

[Severity: Low]
Should the 80 MHz limit be conditional on the compatible string?

The description scopes the number to the w5500 datasheet, but the maximum
applies to all three compatibles enumerated just above, including
wiznet,w5100 and wiznet,w5200, whose SPI slave interfaces are rated lower
(the W5100 SPI timing figures are in the tens of MHz, not 80 MHz).

With a single top-level maximum, a device tree that clocks a wiznet,w5100
at 80000000 passes dt_binding_check without a complaint, and the value is
then handed straight to spi->max_speed_hz for the register accesses done by
w5100_spi_read()/w5100_spi_write() in
drivers/net/ethernet/wiznet/w5100-spi.c, which use a different opcode
protocol from the w5200/w5500 ops.

Would per-variant limits be more accurate here, for example:

    allOf:
      - if:
          properties:
            compatible:
              contains:
                enum:
                  - wiznet,w5100
        then:
          properties:
            spi-max-frequency:
              maximum: <w5100 limit>

Note the exact W5100 and W5200 fSCLK ratings are not derivable from the
tree, so this is based on the schema's own statement that the number comes
from the w5500 datasheet.

[ ... ]

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

[PATCH net-next v6 2/3] dt-bindings: net: wiznet,w5100: add link status interrupt

[Arthur Crepin Leblond]

Add an optional link status interrupt which can be connected to the
LINKLED pin on the device to detect link status changes.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>

---
v6:
- Fix DT error: use minItems
- Remove conditional properties

v5:
- Make the DT interrupts property dynamic from the compatible one
- Fix DT bindings errors

v4:
- Update interrupts in DT bindings

v3:
- Fix DT binding errors

v2:
- Convert device tree binding to YAML
---
 Documentation/devicetree/bindings/net/wiznet,w5100.yaml | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
index 601d828bae92..d0717ab45de6 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).
 
   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>;
             spi-max-frequency = <30000000>;
             local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
         };

-- 
2.55.0

Re: [PATCH net-next v6 2/3] dt-bindings: net: wiznet,w5100: add link status interrupt

[Krzysztof Kozlowski]

On Mon, Aug 31, 2026 at 02:53:35PM +0200, Arthur Crepin Leblond wrote:
> Add an optional link status interrupt which can be connected to the
> LINKLED pin on the device to detect link status changes.
> 
> Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
> 
> ---
> v6:
> - Fix DT error: use minItems
> - Remove conditional properties

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: [net-next,v6,2/3] dt-bindings: net: wiznet,w5100: add link status interrupt

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] Incomplete per-variant schema constraint in
  Documentation/devicetree/bindings/net/wiznet,w5100.yaml: the new…
- [Low] Binding advertises a second (link-status/LINKLED) interrupt that
  no driver consumes.

--

dt-bindings: net: wiznet,w5100: add link status interrupt

This changes the interrupts property in the wiznet,w5100 binding from a
single entry to a two entry tuple with minItems: 1, so an optional second
interrupt tied to the LINKLED pin can be described.  The example is updated
to list two interrupts.

> 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: Medium]
The second item description says the LINKLED pin exists on the w5500, but
the interrupts definition here is unconditional, so should w5100 and w5200
nodes also be allowed to carry a second interrupt?

The compatible enum in this file covers all three parts:

  compatible:
    enum:
      - wiznet,w5100
      - wiznet,w5200
      - wiznet,w5500

and the only allOf entries are the two $ref lines:

  allOf:
    - $ref: /schemas/net/ethernet-controller.yaml#
    - $ref: /schemas/spi/spi-peripheral-props.yaml#

With no if/then block restricting interrupts by compatible string, a node
with compatible = "wiznet,w5100" and two interrupt entries passes
dt_binding_check even though the schema's own prose says the pin is w5500
only.

On the driver side, w5100_probe() only asks for the second interrupt on the
w5500:

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

so a w5100 or w5200 device tree written with two interrupts validates
cleanly and the second entry is silently ignored with no diagnostic.

Would it make sense to add an if/then block constraining interrupts to
maxItems: 1 for wiznet,w5100 and wiznet,w5200, or alternatively to reword
the description so it no longer claims the capability is w5500 specific?
The v6 changelog mentions the conditional properties were removed, so was
dropping the per-compatible conditional here intentional?

[ ... ]

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

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

[Arthur Crepin Leblond]

Detect the link status on the w5500 using the Link Status bit from the
PHY Configuration register (PHYCFGR).

On w5100/w5200, which lack this register, the link is always reported
as up.

Also add an optional interrupt, wired to the LINKLED pin of
the w5500, to detect link status changes and read the PHYCFGR register.

Make sure that the netdev is registered last in the probe function so
no operation can be performed before the probing is finished.

The w5500 interrupt is only enabled when the netdev is opened and
disabled on stop. Same behavior for suspend/resume.

Commit dacf281771a9 ("w5100: remove unused gpio link detection")
dropped the link_gpio/link_irq handling on the grounds that no
devicetree user passed a "link" gpio at the time and that it used the
old gpio interface.
This isn't a plain revert of that removal: link detection is now done
using a second interrupt rather than a gpio with a documented DT binding.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>

---
v6:
- Revert to the reviewed v4 version by Arnd Bergmann with a few
changes based on Sashiko's review:
 - call netif_carrier_off in open if there is no link_irq or  get_link
 returns false
 - call register_netdev at the very end of probe
 - call unregister_netdev before destroying work queue
 - handle link irq probe defer error
 - call netif_device_attach in resume before reenabling the link irq
 - disable the link irq first in suspend
 - log netif_err when PHYCFGR cannot be read
 - enable link irq in open
 - disable link irq in stop
 - free irq and invalidate it in remove

v5:
- Read the link status register only on w5500 instead of checking link_irq
- Address sashiko review
 - call netif_carrier_off on w5500 link status off in open
 - enable/disable the link irq in open/stop
 - enable/disable the link irq in resume/suspend
 - handle link irq probe defer error
 - register the netdev last

v4:
- Use directly an interrupt line instead of gpio -> irq
- Address sashiko reviews
 - drop devm_ on request_threaded_irq to avoid use after free
 - disable/enable the link_irq in the suspend/resume
 - only call netif_carrier_on|off if the link interrupt is present

v3:
- Use the Link Status bit of the PHY Configuration Register
- Use the LINKLED gpio binding for change detection only

v2:
- Use devm_request_threaded_irq instead of request_any_context_irq
- Use devm_gpiod_get_optional instead of gpiod_get_optional
- Call dev_err_probe on gpiod_to_irq failure
- Remove link_irq from priv
- Use a fixed string for the IRQ name
- Remove empty new lines
---
 drivers/net/ethernet/wiznet/w5100.c | 119 ++++++++++++++++++++++++++++++++----
 1 file changed, 108 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
index 53d8dc642fbd..d91881ecd797 100644
--- a/drivers/net/ethernet/wiznet/w5100.c
+++ b/drivers/net/ethernet/wiznet/w5100.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/interrupt.h>
+#include <linux/property.h>
 #include <linux/irq.h>
 
 #include "w5100.h"
@@ -124,6 +125,8 @@ MODULE_LICENSE("GPL");
  */
 #define W5500_SIMR		0x0018 /* Socket Interrupt Mask Register */
 #define W5500_RTR		0x0019 /* Retry Time-value Register */
+#define W5500_PHYCFGR		0x002e /* PHY Configuration Register */
+#define   PHYCFGR_LNK		  0x01 /* Link status */
 
 #define W5500_S0_REGS		0x10000
 
@@ -154,6 +157,7 @@ struct w5100_priv {
 	u16 s0_rx_buf_size;
 
 	int irq;
+	int link_irq;
 
 	struct napi_struct napi;
 	struct net_device *ndev;
@@ -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;
+}
+
+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;
+}
+
 static u32 w5100_get_msglevel(struct net_device *ndev)
 {
 	struct w5100_priv *priv = netdev_priv(ndev);
@@ -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);
+
+	if (priv->link_irq >= 0)
+		enable_irq(priv->link_irq);
+
 	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);
@@ -678,6 +737,7 @@ static const struct ethtool_ops w5100_ethtool_ops = {
 	.get_drvinfo		= w5100_get_drvinfo,
 	.get_msglevel		= w5100_get_msglevel,
 	.set_msglevel		= w5100_set_msglevel,
+	.get_link		= w5100_get_link,
 	.get_regs_len		= w5100_get_regs_len,
 	.get_regs		= w5100_get_regs,
 };
@@ -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;
+	}
+
 	ndev->netdev_ops = &w5100_netdev_ops;
 	ndev->ethtool_ops = &w5100_ethtool_ops;
 	netif_napi_add_weight(ndev, &priv->napi, w5100_napi_poll, 16);
@@ -761,12 +829,8 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
 	 */
 	ndev->features |= NETIF_F_VLAN_CHALLENGED;
 
-	err = register_netdev(ndev);
-	if (err < 0)
-		goto err_register;
-
 	priv->xfer_wq = alloc_workqueue("%s", WQ_MEM_RECLAIM | WQ_PERCPU, 0,
-					netdev_name(ndev));
+					dev_name(dev));
 	if (!priv->xfer_wq) {
 		err = -ENOMEM;
 		goto err_wq;
@@ -795,21 +859,39 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
 	if (ops->may_sleep) {
 		err = request_threaded_irq(priv->irq, NULL, w5100_interrupt,
 					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-					   netdev_name(ndev), ndev);
+					   dev_name(dev), ndev);
 	} else {
 		err = request_irq(priv->irq, w5100_interrupt,
-				  IRQF_TRIGGER_LOW, netdev_name(ndev), ndev);
+				  IRQF_TRIGGER_LOW, dev_name(dev), ndev);
 	}
 	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);
+
 	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);
@@ -839,6 +927,9 @@ static int w5100_suspend(struct device *dev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	if (netif_running(ndev)) {
+		if (priv->link_irq >= 0)
+			disable_irq(priv->link_irq);
+
 		netif_carrier_off(ndev);
 		netif_device_detach(ndev);
 
@@ -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;
 }

-- 
2.55.0

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

[netdev-bot+sashiko]

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

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

[Arthur Crepin Leblond]

On Thu, Sep 03, 2026 at 12:56:04AM +0000, netdev-bot+sashiko@kernel.org wrote:
>> +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?

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

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

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

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

>>
>>    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 ];
>>          };
>

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

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

[Arthur Crepin Leblond]

On Thu, Sep 03, 2026 at 03:08:24PM +0200, Arthur Crepin Leblond wrote:
>On Thu, Sep 03, 2026 at 12:56:04AM +0000, netdev-bot+sashiko@kernel.org wrote:
>>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.

One more time about this one, if we use w5500 without an interrupt the
carrier state will be the one read from PHYCFGR bit and will never
change.

w5100_open:
	w5100_get_link -> 0
		netif_carrier_off      // stays off forever?

There is probably no good solution, the user can only get the real
state with .get_link.

I think netif_carrier_on should initially be called if there
is no interrupt configured, that way the state stays on like on
w5100/w5200.

Arthur