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

[PATCH net-next v8 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>
---
v7 -> v8:
- dt-bindings: net: wiznet,w5100: convert to DT schema
 - Add a myself in MAINTAINERS
 - Address Sashiko reviews
  - Change edge-falling to level-low
  - Clarify the spi max frequency for all devices
  - Add main interrupt description

- dt-bindings: net: wiznet,w5100: add link status interrupt
 - Remove Krzysztof Kozlowski reviewed-by tag after Sashiko review changes
  - Make the interrupts property conditional

- w5100: detect carrier state using link status bit and optional
  interrupt
 - Add myself in the MAINTAINERS file
 - Address Sashiko reviews from online bot and locally run
  - Add a mutex to synchronize the link state read and interrupt
  - Return last known carrier state (netif_carrier_ok) in
    w5500_get_link on SPI error or if the device is not present
  - Only set ops .get_link on w5500
  - Check the netif state, disable/enable the irq and re-check the
    carrier state in w5100_restart
  - Cancel the restart work on stop/suspend
  - Only set carrier state to off in w5100_stop/suspend  when a link
    irq is present
  - Warn on fwnode_irq_get errors for the link irq
  - Free the main irq and flush queues before resetting the hardware
    in w5100_remove

Link to v7: https://patch.msgid.link/20260904-wiznet-link-gpio-v7-0-69f01b70c96f@marmottus.net
Sashiko : https://sashiko.dev/#/patchset/20260904-wiznet-link-gpio-v7-0-69f01b70c96f%40marmottus.net
Netdev Sashiko : https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904-wiznet-link-gpio-v7-0-69f01b70c96f%40marmottus.net

v6 -> v7:
- Address Sashiko reviews
 - Propagate spi read error to the caller
 - Do not change the state of the carrier on SPI read error
 - Enable the link IRQ before reading the PHYCFGR bit
 - Call unregister_netdev before reset
 - Update commit message

Link to v6: https://patch.msgid.link/20260831-wiznet-link-gpio-v6-0-abee6c5c966e@marmottus.net

v5 -> v6:
- Fix DT error: use minItems
- Remove conditional properties
- Put back the reviewed by tag from Krzysztof Kozlowski from v4
- Revert to the reviewed v4 version by Arnd Bergmann with a few
  changes based on Sashiko's review
- Address Sashiko reviews
 - 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 re enabling 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

Link to v5: https://patch.msgid.link/20260813-wiznet-link-gpio-v5-0-ac8046f24413@marmottus.net

v4 -> v5:
- Remove gpio/gpio.h include from DT example
- Make the DT interrupts property dynamic from the compatible one
- Fix DT bindings errors
- Address Sashiko review
 - Read the link status register only on w5500 instead of checking
   link_irq 
 - 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

Link to v4: https://patch.msgid.link/20260811-wiznet-link-gpio-v4-0-4d5f7da885a8@marmottus.net

v3 -> v4:
- Update interrupts in DT bindings
- 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

Link to v3: https://patch.msgid.link/20260806-wiznet-link-gpio-v3-0-532d4a143805@marmottus.net

v2 -> v3:
- Fix DT binding errors
- Use the Link Status bit of the PHY Configuration Register
- Use the LINKLED gpio binding for change detection only

Link to v2: https://patch.msgid.link/20260804-wiznet-link-gpio-v2-0-3b1d0c870f35@marmottus.net

v1 -> v2:
- Convert device tree binding to YAML
- 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

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      |  92 ++++++++++
 .../devicetree/bindings/net/wiznet,w5x00.txt       |  50 ------
 MAINTAINERS                                        |   7 +
 drivers/net/ethernet/wiznet/w5100.c                | 192 +++++++++++++++++++--
 4 files changed, 272 insertions(+), 69 deletions(-)
---
base-commit: 272a65db243bfa34b9277632830e0e06d7e3518e
change-id: 20260804-wiznet-link-gpio-796f36cd14f9

[PATCH net-next v8 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>
---
v8:
 - Add a myself in MAINTAINERS
 - Address Sashiko reviews
  - Change edge-falling to level-low
  - Clarify the spi max frequency for all devices
  - Add main interrupt description

v7:
 - No changes

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

v5:
 - Remove gpio/gpio.h include from DT example
 - 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      | 68 ++++++++++++++++++++++
 .../devicetree/bindings/net/wiznet,w5x00.txt       | 50 ----------------
 MAINTAINERS                                        |  6 ++
 3 files changed, 74 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..25098308eb6e
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
@@ -0,0 +1,68 @@
+# 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
+    description: Main interrupt (INT pin on the device).
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    description:
+      According to the datasheets, w5500 and w5200 allow a maximum of 80 MHz
+      while w5100 is reported by users to allow a maximum of 14 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_LEVEL_LOW>;
+            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 */
-	};
-};
diff --git a/MAINTAINERS b/MAINTAINERS
index 0e04d92d1b09..624beed96f32 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29440,6 +29440,12 @@ M:	Miloslav Trmac <mitr@volny.cz>
 S:	Maintained
 F:	drivers/input/misc/wistron_btns.c
 
+WIZNET W5100 ETHERNET DRIVER
+M:	Arthur Crepin Leblond <arthur@marmottus.net>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/wiznet,w5100.yaml
+
 WMI BINARY MOF DRIVER
 M:	Armin Wolf <W_Armin@gmx.de>
 R:	Thomas Weißschuh <linux@weissschuh.net>

-- 
2.55.0

[PATCH net-next v8 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. This second
interrupt is only supported on wiznet,w5500.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
---
v8:
 - Remove Krzysztof Kozlowski reviewed-by tag after Sashiko review
   changes
  - Make the interrupts property conditional

v7:
 - No changes

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
---
 .../devicetree/bindings/net/wiznet,w5100.yaml      | 38 ++++++++++++++++++----
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
index 25098308eb6e..da03f4e07f29 100644
--- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
+++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
@@ -12,10 +12,6 @@ maintainers:
 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:
@@ -24,8 +20,11 @@ properties:
       - wiznet,w5500
 
   interrupts:
-    maxItems: 1
-    description: Main interrupt (INT pin on the device).
+    description:
+      The main interrupt (INT pin) is mandatory on all devices. The w5500
+      additionally supports an optional second interrupt for link status changes
+      (LINKLED pin). If it is not specified, link state changes will not be
+      reported.
 
   reg:
     maxItems: 1
@@ -42,6 +41,30 @@ required:
   - reg
   - interrupts
 
+allOf:
+  - $ref: /schemas/net/ethernet-controller.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - wiznet,w5500
+    then:
+      properties:
+        interrupts:
+          minItems: 1
+          items:
+            - description: Main interrupt (INT pin on the device).
+            - description:
+                Optional interrupt for link status changes (LINKLED pin on
+                w5500).
+    else:
+      properties:
+        interrupts:
+          maxItems: 1
+          description: Main interrupt (INT pin on the device).
+
 unevaluatedProperties: false
 
 examples:
@@ -60,7 +83,8 @@ examples:
             pinctrl-names = "default";
             pinctrl-0 = <&eth1_pins>;
             interrupt-parent = <&gpio>;
-            interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+            interrupts = <25 IRQ_TYPE_LEVEL_LOW>,
+                         <24 IRQ_TYPE_EDGE_BOTH>;
             spi-max-frequency = <30000000>;
             local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
         };

-- 
2.55.0

[PATCH net-next v8 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, .get_link is not
implemented and the carrier state is not reported.

Add an optional interrupt, wired to the LINKLED pin of the w5500, to
detect link status changes with a read of the PHYCFGR register and
update of the carrier state with netif_carrier_on|off.
If this interrupt is not set in the DT binding, only .get_link can be
used to get the carrier state. The interrupt handler and the PHYCFGR
register read are synchronized using a mutex.

The carrier state (netif_carrier_on|off) is not called anymore on
w5100/w5200 or w5500 without the link irq.

Make sure that the netdev is registered last in the probe function so
no operation can be performed before the probing is finished. dev_name
is now used instead of netdev_name.

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>
---
v8:
 - Add myself in the MAINTAINERS file
 - Address Sashiko reviews from online bot and locally run
  - Add a mutex to synchronize the link state read and interrupt
  - Return last known carrier state (netif_carrier_ok) in
    w5500_get_link on SPI error or if the device is not present
  - Only set ops .get_link on w5500
  - Check the netif state, disable/enable the irq and re-check the
    carrier state in w5100_restart
  - Cancel the restart work on stop/suspend
  - Only set carrier state to off in w5100_stop/suspend  when a link
    irq is present
  - Warn on fwnode_irq_get errors for the link irq
  - Free the main irq and flush queues before resetting the hardware
    in w5100_remove

v7:
 - Address Sashiko reviews
  - Propagate spi read error to the caller
  - Do not change the state of the carrier on SPI read error
  - Enable the link IRQ before reading the PHYCFGR bit
  - Call unregister_netdev before reset
  - Update commit message

v6:
 - Revert to the reviewed v4 version by Arnd Bergmann with a few
   changes based on Sashiko's review
 - Address Sashiko reviews
  - 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 re enabling 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:
 - Address Sashiko review
  - Read the link status register only on w5500 instead of checking
    link_irq
  - 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
---
 MAINTAINERS                         |   1 +
 drivers/net/ethernet/wiznet/w5100.c | 192 ++++++++++++++++++++++++++++++++----
 2 files changed, 174 insertions(+), 19 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 624beed96f32..2a52f5cbe218 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29445,6 +29445,7 @@ M:	Arthur Crepin Leblond <arthur@marmottus.net>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/net/wiznet,w5100.yaml
+F:	drivers/net/ethernet/wiznet/
 
 WMI BINARY MOF DRIVER
 M:	Armin Wolf <W_Armin@gmx.de>
diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
index 53d8dc642fbd..335bafbc9fa4 100644
--- a/drivers/net/ethernet/wiznet/w5100.c
+++ b/drivers/net/ethernet/wiznet/w5100.c
@@ -18,9 +18,11 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/mutex.h>
 #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 +126,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 +158,9 @@ struct w5100_priv {
 	u16 s0_rx_buf_size;
 
 	int irq;
+	int link_irq;
+	/* Protects link state and carrier updates */
+	struct mutex link_lock;
 
 	struct napi_struct napi;
 	struct net_device *ndev;
@@ -345,6 +352,67 @@ 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 ret;
+	}
+
+	return ret & PHYCFGR_LNK;
+}
+
+static void w5500_report_carrier_state(struct net_device *ndev)
+{
+	struct w5100_priv *priv = netdev_priv(ndev);
+	int state;
+
+	mutex_lock(&priv->link_lock);
+
+	state = w5500_get_phycfgr_lnk(ndev);
+	if (state > 0) {
+		netif_info(priv, link, ndev, "link is up\n");
+		netif_carrier_on(ndev);
+	} else if (state == 0) {
+		netif_info(priv, link, ndev, "link is down\n");
+		netif_carrier_off(ndev);
+	}
+
+	mutex_unlock(&priv->link_lock);
+}
+
+static irqreturn_t w5500_detect_link_interrupt(int irq, void *ndev_instance)
+{
+	struct net_device *ndev = ndev_instance;
+
+	if (netif_running(ndev))
+		w5500_report_carrier_state(ndev);
+
+	return IRQ_HANDLED;
+}
+
+static u32 w5500_get_link(struct net_device *ndev)
+{
+	struct w5100_priv *priv = netdev_priv(ndev);
+	int state;
+
+	if (!netif_device_present(ndev))
+		return netif_carrier_ok(ndev);
+
+	mutex_lock(&priv->link_lock);
+	state = w5500_get_phycfgr_lnk(ndev);
+	mutex_unlock(&priv->link_lock);
+
+	if (state < 0)
+		return netif_carrier_ok(ndev);
+
+	return state > 0;
+}
+
 static int w5100_hw_reset(struct w5100_priv *priv)
 {
 	u32 rtr;
@@ -448,12 +516,23 @@ static void w5100_restart(struct net_device *ndev)
 {
 	struct w5100_priv *priv = netdev_priv(ndev);
 
+	if (!netif_running(ndev))
+		return;
+
+	if (priv->link_irq > 0)
+		disable_irq(priv->link_irq);
+
 	netif_stop_queue(ndev);
 	w5100_hw_reset(priv);
 	w5100_hw_start(priv);
 	ndev->stats.tx_errors++;
 	netif_trans_update(ndev);
 	netif_wake_queue(ndev);
+
+	if (priv->link_irq > 0) {
+		w5500_report_carrier_state(ndev);
+		enable_irq(priv->link_irq);
+	}
 }
 
 static void w5100_restart_work(struct work_struct *work)
@@ -659,6 +738,12 @@ 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) {
+		w5500_report_carrier_state(ndev);
+		enable_irq(priv->link_irq);
+	}
+
 	return 0;
 }
 
@@ -667,13 +752,31 @@ static int w5100_stop(struct net_device *ndev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	netif_info(priv, ifdown, ndev, "shutting down\n");
+
+	cancel_work_sync(&priv->restart_work);
+
+	if (priv->link_irq > 0) {
+		disable_irq(priv->link_irq);
+		mutex_lock(&priv->link_lock);
+		netif_carrier_off(ndev);
+		mutex_unlock(&priv->link_lock);
+	}
+
 	w5100_hw_close(priv);
-	netif_carrier_off(ndev);
 	netif_stop_queue(ndev);
 	napi_disable(&priv->napi);
 	return 0;
 }
 
+static const struct ethtool_ops w5500_ethtool_ops = {
+	.get_drvinfo		= w5100_get_drvinfo,
+	.get_msglevel		= w5100_get_msglevel,
+	.set_msglevel		= w5100_set_msglevel,
+	.get_link		= w5500_get_link,
+	.get_regs_len		= w5100_get_regs_len,
+	.get_regs		= w5100_get_regs,
+};
+
 static const struct ethtool_ops w5100_ethtool_ops = {
 	.get_drvinfo		= w5100_get_drvinfo,
 	.get_msglevel		= w5100_get_msglevel,
@@ -721,6 +824,8 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
 	dev_set_drvdata(dev, ndev);
 	priv = netdev_priv(ndev);
 
+	mutex_init(&priv->link_lock);
+
 	switch (ops->chip_id) {
 	case W5100:
 		priv->s0_regs = W5100_S0_REGS;
@@ -745,15 +850,26 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
 		break;
 	default:
 		err = -EINVAL;
-		goto err_register;
+		goto err_mutex;
 	}
 
 	priv->ndev = ndev;
 	priv->ops = ops;
 	priv->irq = irq;
 
+	priv->link_irq = ops->chip_id == W5500 ?
+			 fwnode_irq_get(dev_fwnode(dev), 1) : -EINVAL;
+	if (priv->link_irq == -EPROBE_DEFER) {
+		err = dev_err_probe(dev, priv->link_irq,
+				    "failed to get link irq\n");
+		goto err_mutex;
+	} else if (priv->link_irq < 0 && priv->link_irq != -EINVAL) {
+		dev_warn(dev, "invalid link irq: %d\n", priv->link_irq);
+	}
+
 	ndev->netdev_ops = &w5100_netdev_ops;
-	ndev->ethtool_ops = &w5100_ethtool_ops;
+	ndev->ethtool_ops = ops->chip_id == W5500 ? &w5500_ethtool_ops :
+						    &w5100_ethtool_ops;
 	netif_napi_add_weight(ndev, &priv->napi, w5100_napi_poll, 16);
 
 	/* This chip doesn't support VLAN packets with normal MTU,
@@ -761,15 +877,11 @@ 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;
+		goto err_mutex;
 	}
 
 	INIT_WORK(&priv->rx_work, w5100_rx_work);
@@ -795,21 +907,42 @@ 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_interrupt,
+					   IRQF_TRIGGER_RISING |
+					   IRQF_TRIGGER_FALLING |
+					   IRQF_ONESHOT | IRQF_NO_AUTOEN,
+					   "w5100-link", ndev);
+		if (err < 0)
+			goto err_irq;
+
+		netif_carrier_off(ndev);
+	}
+
+	err = register_netdev(ndev);
+	if (err < 0)
+		goto err_link_irq;
+
 	return 0;
 
+err_link_irq:
+	if (priv->link_irq > 0)
+		free_irq(priv->link_irq, ndev);
+err_irq:
+	free_irq(priv->irq, ndev);
 err_hw:
 	destroy_workqueue(priv->xfer_wq);
-err_wq:
-	unregister_netdev(ndev);
-err_register:
+err_mutex:
+	mutex_destroy(&priv->link_lock);
 	free_netdev(ndev);
 	return err;
 }
@@ -820,14 +953,22 @@ void w5100_remove(struct device *dev)
 	struct net_device *ndev = dev_get_drvdata(dev);
 	struct w5100_priv *priv = netdev_priv(ndev);
 
-	w5100_hw_reset(priv);
+	unregister_netdev(ndev);
+
+	cancel_work_sync(&priv->rx_work);
+	cancel_work_sync(&priv->tx_work);
+	cancel_work_sync(&priv->setrx_work);
+	cancel_work_sync(&priv->restart_work);
+
+	if (priv->link_irq > 0)
+		free_irq(priv->link_irq, ndev);
+
 	free_irq(priv->irq, ndev);
+	w5100_hw_reset(priv);
 
-	flush_work(&priv->setrx_work);
-	flush_work(&priv->restart_work);
 	destroy_workqueue(priv->xfer_wq);
+	mutex_destroy(&priv->link_lock);
 
-	unregister_netdev(ndev);
 	free_netdev(ndev);
 }
 EXPORT_SYMBOL_GPL(w5100_remove);
@@ -839,9 +980,17 @@ static int w5100_suspend(struct device *dev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	if (netif_running(ndev)) {
-		netif_carrier_off(ndev);
+		if (priv->link_irq > 0) {
+			disable_irq(priv->link_irq);
+			mutex_lock(&priv->link_lock);
+			netif_carrier_off(ndev);
+			mutex_unlock(&priv->link_lock);
+		}
+
 		netif_device_detach(ndev);
 
+		cancel_work_sync(&priv->restart_work);
+
 		w5100_hw_close(priv);
 	}
 	return 0;
@@ -857,6 +1006,11 @@ static int w5100_resume(struct device *dev)
 		w5100_hw_start(priv);
 
 		netif_device_attach(ndev);
+
+		if (priv->link_irq > 0) {
+			w5500_report_carrier_state(ndev);
+			enable_irq(priv->link_irq);
+		}
 	}
 	return 0;
 }

-- 
2.55.0

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

[Krzysztof Kozlowski]

On Tue, Sep 15, 2026 at 10:06:03AM +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. This second
> interrupt is only supported on wiznet,w5500.
> 
> Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
> ---
> v8:
>  - Remove Krzysztof Kozlowski reviewed-by tag after Sashiko review
>    changes
>   - Make the interrupts property conditional
> 
> v7:
>  - No changes
> 
> 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
> ---
>  .../devicetree/bindings/net/wiznet,w5100.yaml      | 38 ++++++++++++++++++----
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> index 25098308eb6e..da03f4e07f29 100644
> --- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> @@ -12,10 +12,6 @@ maintainers:
>  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:
> @@ -24,8 +20,11 @@ properties:
>        - wiznet,w5500
>  
>    interrupts:
> -    maxItems: 1

Broadest constraints always stay here.

I already asked for proper syntax here:
https://lore.kernel.org/all/688f57cd-a098-4904-b8cd-965470a89386@kernel.org/

https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127

Best regards,
Krzysztof

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

[Arthur Crepin Leblond]

On Thu, Sep 17, 2026 at 12:04:12PM +0200, Krzysztof Kozlowski wrote:
>>  properties:
>>    compatible:
>>      enum:
>> @@ -24,8 +20,11 @@ properties:
>>        - wiznet,w5500
>>
>>    interrupts:
>> -    maxItems: 1
>
>Broadest constraints always stay here.
>
>I already asked for proper syntax here:
>https://lore.kernel.org/all/688f57cd-a098-4904-b8cd-965470a89386@kernel.org/
>
>https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127
>
>Best regards,
>Krzysztof
>

Oh, my bad again... I'll fix it.

Thanks again for your review.

Arthur

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] In Documentation/devicetree/bindings/net/wiznet,w5100.yaml the
  new schema applies one global `spi-max-frequency: maximum: 80000000`…
- [Low] This patch is not a pure format conversion: the removed
  Documentation/devicetree/bindings/net/wiznet,w5x00.txt explicitly…

--

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

This converts the Wiznet w5x00 SPI Ethernet controller binding from plain
text to a YAML schema covering wiznet,w5100, wiznet,w5200 and wiznet,w5500,
and adds a MAINTAINERS entry for the new file.

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

[ ... ]

> +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
> +    description: Main interrupt (INT pin on the device).
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    description:
> +      According to the datasheets, w5500 and w5200 allow a maximum of 80 MHz
> +      while w5100 is reported by users to allow a maximum of 14 MHz. However,
> +      board designs may need to limit this value.
> +    maximum: 80000000

[Severity: Low]
The description says the 14 MHz limit applies only to w5100 while 80 MHz
applies to w5200/w5500, but the single maximum: 80000000 is applied to all
three compatibles.  Should the w5100 limit be expressed as a conditional so
dt_binding_check can enforce it?

With this schema as written, a node like:

    ethernet@0 {
        compatible = "wiznet,w5100";
        reg = <0>;
        spi-max-frequency = <80000000>;
        ...
    };

passes validation even though it is roughly 5.7x the documented w5100
ceiling.  Would something like this in the allOf block match the intent of
the description?

    allOf:
      - if:
          properties:
            compatible:
              contains:
                const: wiznet,w5100
        then:
          properties:
            spi-max-frequency:
              maximum: 14000000

Per-compatible spi-max-frequency constraints are already expressed this way
in other net bindings, for example
Documentation/devicetree/bindings/net/nfc/marvell,nci.yaml and
Documentation/devicetree/bindings/net/nfc/st,st-nci.yaml.

The same unconditional maximum: 80000000 is still present at the end of the
series (commit ebaf57af, "w5100: detect carrier state using link status bit
and optional interrupt"), so no later patch adjusts this.

[ ... ]

> +        ethernet@0 {
> +            compatible = "wiznet,w5500";
> +            reg = <0>;
> +            pinctrl-names = "default";
> +            pinctrl-0 = <&eth1_pins>;
> +            interrupt-parent = <&gpio>;
> +            interrupts = <25 IRQ_TYPE_LEVEL_LOW>;

[Severity: Low]
This changes the recommended trigger type from IRQ_TYPE_EDGE_FALLING in the
removed .txt binding to IRQ_TYPE_LEVEL_LOW, and the deleted text also
claimed "w5x00 natively generates falling edge interrupts".  Could the
commit message mention this correction?

The new value agrees with the driver, which requests the device interrupt
level-low in w5100_probe():

    err = request_threaded_irq(priv->irq, NULL, w5100_interrupt,
                               IRQF_TRIGGER_LOW | IRQF_ONESHOT,
                               netdev_name(ndev), ndev);

The commit message body only says the binding is converted from plain text
to DT schema.  The only record of the trigger change is the v8 changelog
line "Change edge-falling to level-low" below the --- separator, which git
does not keep.

Would a Fixes: 0114214eca21 ("dt-bindings: net: wiznet: add w5x00 support")
tag be appropriate here as well?

> +            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 e9665798c4be6..0000000000000
> --- a/Documentation/devicetree/bindings/net/wiznet,w5x00.txt
> +++ /dev/null
> @@ -1,50 +0,0 @@

[ ... ]

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

[ ... ]

> -		interrupts = <25 IRQ_TYPE_EDGE_FALLING>;

[ ... ]

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

Re: [PATCH net-next v8 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: 0 · Low: 2

- [Low] The new `allOf` `if/then/else` conditional in
  Documentation/devicetree/bindings/net/wiznet,w5100.yaml encodes a…
- [Low] Concern (assessed as NOT valid): the binding documents an
  optional second (LINKLED) interrupt for wiznet,w5500 and the semantics…

--

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

This documents an optional second interrupt on the wiznet SPI ethernet
binding, sourced from the LINKLED pin, for reporting link status changes.
The interrupts property is turned into a conditional list that allows two
entries for wiznet,w5500 and keeps a single entry for the other compatibles.

No Link: tags present in the commit header.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> index 25098308eb6e5..da03f4e07f29a 100644
> --- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml

[ ... ]

> @@ -24,8 +20,11 @@ properties:
>        - wiznet,w5500
>  
>    interrupts:
> -    maxItems: 1
> -    description: Main interrupt (INT pin on the device).
> +    description:
> +      The main interrupt (INT pin) is mandatory on all devices. The w5500
> +      additionally supports an optional second interrupt for link status changes
> +      (LINKLED pin). If it is not specified, link state changes will not be
> +      reported.

[Severity: Low]
At this commit, drivers/net/ethernet/wiznet/w5100.c only consumes spi->irq
and does not report carrier state, so the sentence "If it is not specified,
link state changes will not be reported" describes behaviour that has no
consumer yet.

Checking the rest of the series, the driver side does land in the following
patch, where w5100_probe() picks up the second entry:

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

and w5500_report_carrier_state() calls netif_carrier_on()/
netif_carrier_off().  So this looks fine as a binding-before-driver
ordering, and is only noted here for completeness.

>  
>    reg:
>      maxItems: 1
> @@ -42,6 +41,30 @@ required:
>    - reg
>    - interrupts
>  
> +allOf:
> +  - $ref: /schemas/net/ethernet-controller.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - wiznet,w5500
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 1
> +          items:
> +            - description: Main interrupt (INT pin on the device).
> +            - description:
> +                Optional interrupt for link status changes (LINKLED pin on
> +                w5500).
> +    else:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +          description: Main interrupt (INT pin on the device).

[Severity: Low]
Is this else branch describing the hardware, or the current Linux driver?

The LINKLED output exists on the w5100 and w5200 packages as well, so a
board that routes w5100/w5200 LINKLED to a host GPIO cannot be described
here:

  compatible = "wiznet,w5200";
  interrupts = <25 IRQ_TYPE_LEVEL_LOW>, <24 IRQ_TYPE_EDGE_BOTH>;

The second entry is rejected by maxItems: 1 during dt_binding_check.

The only in-tree justification found for the w5500 restriction is the
driver gate added later in this series, in w5100_probe():

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

which is a software limitation (register readback of link state via w5500
PHYCFGR) rather than absence of the pin.  Would it be preferable to allow
two interrupts for all three compatibles and note in the interrupts
description which parts can actually report link state, so this constraint
does not have to be relaxed later?

>  
>  unevaluatedProperties: false
>  
>  examples:

[ ... ]

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

Re: [PATCH net-next v8 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 6 potential
issue(s) to consider:

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

- [High] w5100_remove() now calls unregister_netdev() (line 956) *before*
  draining rx_work/tx_work (lines 958-961) and before…
- [Medium] The commit message body describes only the link-detection
  feature plus three restructuring notes (netdev registered last,…
- [Medium] The patch silently repairs a real, user-visible carrier
  regression introduced by dacf281771a9 ("w5100: remove unused gpio link…
- [Medium] In w5100_suspend(), cancel_work_sync(&priv->restart_work)
  (line 992) runs only *after* netif_carrier_off() (line 986) and…
- [Medium] The newly added cancel_work_sync(&priv->tx_work) in
  w5100_remove() (line 959) can cancel a tx_work item that has not…
- [Medium] w5500_report_carrier_state() deliberately leaves the carrier
  untouched when the PHYCFGR read fails (state < 0), and there is no…

--

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
PHYCFGR, exposes it through a w5500 specific ethtool get_link callback, and
optionally requests a second (LINKLED) interrupt to update the carrier
state.  It also reorders the probe and remove paths and adds work
cancellation in the stop, suspend and remove paths.

A few questions about the changes below.

The commit message describes the link detection feature plus three
restructuring notes:

  The carrier state (netif_carrier_on|off) is not called anymore on
  w5100/w5200 or w5500 without the link irq.

  Make sure that the netdev is registered last in the probe function so
  no operation can be performed before the probing is finished. dev_name
  is now used instead of netdev_name.

[Severity: Medium]
Should the message also mention the teardown changes that apply to all chip
variants, not just w5500 with a link interrupt?

Specifically, w5100_remove() now calls unregister_netdev() first, adds
cancel_work_sync() for rx_work and tx_work (which were never drained
explicitly before), converts the flush_work() of setrx_work/restart_work
into cancel_work_sync(), and moves w5100_hw_reset() to after free_irq().

w5100_stop() and w5100_suspend() gain cancel_work_sync(&priv->restart_work),
and w5100_restart() gains an unconditional early return on
!netif_running(ndev) which changes tx-timeout recovery on w5100/w5200 and
the MMIO parts that can never have a link interrupt.

These items appear only in the v8 changelog below the --- separator, which
is dropped when the patch is applied.

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

[ ... ]

> @@ -345,6 +352,67 @@ 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 ret;
> +	}
> +
> +	return ret & PHYCFGR_LNK;
> +}
> +
> +static void w5500_report_carrier_state(struct net_device *ndev)
> +{
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +	int state;
> +
> +	mutex_lock(&priv->link_lock);
> +
> +	state = w5500_get_phycfgr_lnk(ndev);
> +	if (state > 0) {
> +		netif_info(priv, link, ndev, "link is up\n");
> +		netif_carrier_on(ndev);
> +	} else if (state == 0) {
> +		netif_info(priv, link, ndev, "link is down\n");
> +		netif_carrier_off(ndev);
> +	}
> +
> +	mutex_unlock(&priv->link_lock);
> +}

[Severity: Medium]
Can a single failed PHYCFGR read leave the interface stuck with the carrier
off while the PHY is linked?

The negative case here intentionally leaves the carrier untouched, and there
is no retry, delayed re-read or polling fallback.  The previous value at the
two initial synchronisation points is off: w5100_probe() calls
netif_carrier_off() before register_netdev(), and w5100_suspend() forces the
carrier off.

The errors do reach here.  w5500_spi_read() in w5100-spi.c propagates SPI
failures:

	ret = spi_write_then_read(spi, cmd, sizeof(cmd), &data, 1);

	return ret ? ret : data;

so for example an -ENOMEM from the fallback allocation in
spi_write_then_read() ends up as a negative return of
w5500_get_phycfgr_lnk().

The only other refresh comes from LINKLED edges, requested with
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING.  If the link is already up and
stays up, the pin holds a constant level and no further edge is produced.

So if the read in w5100_open() or w5100_resume() fails once, does the carrier
stay off until the interface is manually taken down and brought back up,
with linkwatch keeping the qdisc deactivated in the meantime?  Would
re-arming a delayed PHYCFGR read on read failure address this?

[ ... ]

> @@ -448,12 +516,23 @@ static void w5100_restart(struct net_device *ndev)
>  {
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
> +	if (!netif_running(ndev))
> +		return;
> +
> +	if (priv->link_irq > 0)
> +		disable_irq(priv->link_irq);
> +
>  	netif_stop_queue(ndev);
>  	w5100_hw_reset(priv);
>  	w5100_hw_start(priv);
>  	ndev->stats.tx_errors++;
>  	netif_trans_update(ndev);
>  	netif_wake_queue(ndev);
> +
> +	if (priv->link_irq > 0) {
> +		w5500_report_carrier_state(ndev);
> +		enable_irq(priv->link_irq);
> +	}
>  }

[ ... ]

> @@ -667,13 +752,31 @@ static int w5100_stop(struct net_device *ndev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	netif_info(priv, ifdown, ndev, "shutting down\n");
> +
> +	cancel_work_sync(&priv->restart_work);
> +
> +	if (priv->link_irq > 0) {
> +		disable_irq(priv->link_irq);
> +		mutex_lock(&priv->link_lock);
> +		netif_carrier_off(ndev);
> +		mutex_unlock(&priv->link_lock);
> +	}
> +
>  	w5100_hw_close(priv);
> -	netif_carrier_off(ndev);
>  	netif_stop_queue(ndev);
>  	napi_disable(&priv->napi);
>  	return 0;
>  }

[Severity: Medium]
Making netif_carrier_off() conditional on priv->link_irq here (and the same
change in w5100_suspend()) looks like it also fixes an existing carrier
regression.  Should this carry a Fixes: tag for dacf281771a9 ("w5100: remove
unused gpio link detection")?

dacf281771a9 removed the only netif_carrier_on() calls, which lived in
w5100_open() and w5100_resume(), but left the unconditional
netif_carrier_off() in w5100_stop() and w5100_suspend().  On the baseline,
w5100_open() has no carrier call at all, so:

	ip link set dev eth0 down
	ip link set dev eth0 up

leaves every w5100/w5200/w5500 reporting no carrier, and linkwatch keeps the
qdisc deactivated.  A suspend/resume cycle has the same effect.

Without a Fixes: tag this fix will not be picked up for stable, so released
kernels keep the unusable-after-ifdown/ifup behaviour.

[ ... ]

> @@ -820,14 +953,22 @@ void w5100_remove(struct device *dev)
>  	struct net_device *ndev = dev_get_drvdata(dev);
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
> -	w5100_hw_reset(priv);
> +	unregister_netdev(ndev);
> +
> +	cancel_work_sync(&priv->rx_work);
> +	cancel_work_sync(&priv->tx_work);
> +	cancel_work_sync(&priv->setrx_work);
> +	cancel_work_sync(&priv->restart_work);
> +
> +	if (priv->link_irq > 0)
> +		free_irq(priv->link_irq, ndev);
> +
>  	free_irq(priv->irq, ndev);
> +	w5100_hw_reset(priv);

[Severity: High]
Can rx_work still run after unregister_netdev() here and hand an skb to
netif_rx() with a dangling ndev?

w5100_stop(), which runs from dev_close() inside unregister_netdev(), does
not cancel or flush rx_work:

	cancel_work_sync(&priv->restart_work);
	...
	w5100_hw_close(priv);
	netif_stop_queue(ndev);
	napi_disable(&priv->napi);

so an rx_work item queued just before the close can run afterwards:

w5100_rx_work()
	while ((skb = w5100_rx_skb(priv->ndev)))
		netif_rx(skb);

	w5100_enable_intr(priv);

netif_rx() takes no device reference, and unregister_netdev() has already
done flush_all_backlogs() and netdev_run_todo().  If that skb sits in a
deferred or RPS-remote backlog, is it dereferenced after the free_netdev() a
few lines below?

The main interrupt is also still registered while the cancel_work_sync()
calls run:

w5100_interrupt()
	if (ir & S0_IR_RECV) {
		w5100_disable_intr(priv);

		if (priv->ops->may_sleep)
			queue_work(priv->xfer_wq, &priv->rx_work);

so an in-flight handler can re-queue rx_work after the cancellation.  That
late work then touches chip registers concurrently with the w5100_hw_reset()
above and calls w5100_enable_intr() after the handler has been freed.

Before this patch, destroy_workqueue() drained rx_work while the netdev was
still registered and after free_irq(), so neither case could happen.  Would
freeing and synchronizing both interrupts first, then draining all work they
can produce, and only then resetting the hardware and
unregistering/freeing the netdev, be the right order?

[Severity: Medium]
Does the new cancel_work_sync(&priv->tx_work) leak priv->tx_skb?

w5100_start_tx() takes ownership of the skb and defers the free entirely to
the work item:

	if (priv->ops->may_sleep) {
		WARN_ON(priv->tx_skb);
		priv->tx_skb = skb;
		queue_work(priv->xfer_wq, &priv->tx_work);

and w5100_tx_work() is the only consumer that clears priv->tx_skb and frees
it via w5100_tx_skb() -> dev_kfree_skb():

	struct sk_buff *skb = priv->tx_skb;

	priv->tx_skb = NULL;

	if (WARN_ON(!skb))
		return;
	w5100_tx_skb(priv->ndev, skb);

If cancel_work_sync() cancels a work item that has not started yet, nothing
releases priv->tx_skb, and remove() continues to destroy_workqueue() and
free_netdev() with it still set.  The previous code reached the same state
through destroy_workqueue(), which drains queued work instead of cancelling
it, so the skb was always freed.  Would freeing priv->tx_skb explicitly
after the cancellation, or flushing tx_work instead, be preferable?

>  
> -	flush_work(&priv->setrx_work);
> -	flush_work(&priv->restart_work);
>  	destroy_workqueue(priv->xfer_wq);
> +	mutex_destroy(&priv->link_lock);
>  
> -	unregister_netdev(ndev);
>  	free_netdev(ndev);
>  }
>  EXPORT_SYMBOL_GPL(w5100_remove);
> @@ -839,9 +980,17 @@ static int w5100_suspend(struct device *dev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	if (netif_running(ndev)) {
> -		netif_carrier_off(ndev);
> +		if (priv->link_irq > 0) {
> +			disable_irq(priv->link_irq);
> +			mutex_lock(&priv->link_lock);
> +			netif_carrier_off(ndev);
> +			mutex_unlock(&priv->link_lock);
> +		}
> +
>  		netif_device_detach(ndev);
>  
> +		cancel_work_sync(&priv->restart_work);
> +
>  		w5100_hw_close(priv);
>  	}
>  	return 0;

[Severity: Medium]
Should the cancel_work_sync() come before netif_carrier_off() and
netif_device_detach() rather than after them, as it does in w5100_stop()?

restart_work can be queued at any time on w5500/SPI:

w5100_tx_timeout()
	if (priv->ops->may_sleep)
		schedule_work(&priv->restart_work);

A work item that is pending or already running when suspend starts still
passes the new guard in w5100_restart(), because netif_running() stays true
through the suspend callback and the guard does not test
netif_device_present():

	if (!netif_running(ndev))
		return;

It then re-runs w5100_hw_reset()/w5100_hw_start(), re-enabling chip
interrupts and re-opening socket 0, calls netif_wake_queue() which clears the
__QUEUE_STATE_DRV_XOFF that netif_device_detach() just set, and calls
w5500_report_carrier_state() which can turn the carrier back on right after
suspend cleared it.

With the tx queue runnable again and __dev_queue_xmit() not testing
netif_device_present(), can the stack then reach w5100_start_tx() and issue
SPI transfers to a chip whose socket w5100_hw_close() has closed, or to an
already suspended SPI controller?  Would moving the cancellation ahead of the
state changes, and/or widening the guard in w5100_restart(), close this?

>  
> @@ -857,6 +1006,11 @@ static int w5100_resume(struct device *dev)
>  		w5100_hw_start(priv);
>  
>  		netif_device_attach(ndev);
> +
> +		if (priv->link_irq > 0) {
> +			w5500_report_carrier_state(ndev);
> +			enable_irq(priv->link_irq);
> +		}
>  	}
>  	return 0;
>  }

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