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

[PATCH net-next v10 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>
---
v9 -> v10:
 - Address Sashiko review
  - Always schedule the restart work to avoid atomic context
  - Move netif_device_detach before disabling irq in w5100_suspend
  - Flush rx/tx queues in w5100_remove

Link to v9: https://patch.msgid.link/20260918-wiznet-link-gpio-v9-0-5536f24d8d6d@marmottus.net
Sashiko: https://sashiko.dev/#/patchset/20260918-wiznet-link-gpio-v9-0-5536f24d8d6d@marmottus.net

v8 -> v9:
 - Address Sashiko review
  - Rework commit message
  - Disable the link irq first in w5100_stop()
  - Call w5100_hw_close earlier in w5100_stop()
  - Flush rx/tx work and cancel restart in w5100_stop()
  - Call cancel_work_sync() after unregister_netdev in w5100_remove()
  - Call cancel_work_sync before netif_carrier_off in w5100_suspend()
  - Enable/Disable main irq in open()/stop() and suspend()/resume()
  - Check the number of irqs defined in DT
  - enable the link irq after checking PHYCFGR
  - Ignore fwnode_irq_get errors other than EPROBE_DEFER

 - DT binding: Simplify the interrupts description:
  - Describe the 2 interrupts in the top-level properties
  - Limit the maximum number to 1 on non-w5500 with a condition
  - Somehow only adding minItems in the top-level and then describing
    2 items in the if block led to dt checks errors.

Link to v8: https://patch.msgid.link/20260915-wiznet-link-gpio-v8-0-d173622474cc@marmottus.net
Sashiko: https://sashiko.dev/#/patchset/20260915-wiznet-link-gpio-v8-0-d173622474cc%40marmottus.net
Clashiko: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-wiznet-link-gpio-v8-0-d173622474cc%40marmottus.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      |  88 ++++++++
 .../devicetree/bindings/net/wiznet,w5x00.txt       |  50 -----
 MAINTAINERS                                        |   7 +
 drivers/net/ethernet/wiznet/w5100.c                | 227 ++++++++++++++++++---
 4 files changed, 296 insertions(+), 76 deletions(-)
---
base-commit: 8830e65ed46de41f849eefb8ba227d4852c460f6
change-id: 20260804-wiznet-link-gpio-796f36cd14f9

[PATCH net-next v10 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.

Change the trigger type from edge-falling to low to match the driver
implementation.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
v9:
 - Add edge-falling->low in commit message

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 df8ab9b82402..9f84e4c8163e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29450,6 +29450,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 v10 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>
---
v9:
 - Simplify the interrupts description:
  - Describe the 2 interrupts in the top-level properties
  - Limit the maximum number to 1 on non-w5500 with a condition
  - Somehow only adding minItems in the top-level and then describing
    2 items in the if block led to dt checks errors.

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      | 34 +++++++++++++++++-----
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
index 25098308eb6e..ac2ee725af5d 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,16 @@ properties:
       - wiznet,w5500
 
   interrupts:
-    maxItems: 1
-    description: Main interrupt (INT pin on the device).
+    minItems: 1
+    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.
+    items:
+      - description: Main interrupt (INT pin on the device).
+      - description:
+          Optional interrupt for link status changes (LINKLED pin on w5500).
 
   reg:
     maxItems: 1
@@ -42,6 +46,21 @@ required:
   - reg
   - interrupts
 
+allOf:
+  - $ref: /schemas/net/ethernet-controller.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - wiznet,w5500
+    then:
+      properties:
+        interrupts:
+          maxItems: 1
+
 unevaluatedProperties: false
 
 examples:
@@ -60,7 +79,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 v10 3/3] w5100: detect carrier state using link status bit and optional interrupt

[Arthur Crepin Leblond]

Detect the carrier state on the w5500 Ethernet controller 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. The carrier state is also not
reported using netif_carrier_on|off().

For the w5500, add an optional interrupt, wired to the LINKLED pin of
the device, 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, netif_carrier_on|off() is not called. The
interrupt handler and the PHYCFGR register read are synchronized using
a mutex.

A few changes are brought to the bring-up teardown:

 - The net device is registered last in the probe() function so no
   operation can be performed before the probing is finished
 - the net device is unregistered first in remove()
 - dev_name is now used instead of netdev_name
 - w5100_remove(), w5100_stop() and w5100_suspend() call
   cancel_work_sync()/flush_work() to make sure there is no pending
   work
 - the main irq is enabled/disabled in open()/stop() and
   suspend()/resume()
 - enable the link irq after checking PHYCFGR

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>
---
v10:
 - Address Sashiko review
  - Always schedule the restart work to avoid atomic context
  - Move netif_device_detach before disabling irq in w5100_suspend
  - Flush rx/tx queues in w5100_remove

v9:
 - Address Sashiko review
  - Rework commit message
  - Disable the link irq first in w5100_stop()
  - Call w5100_hw_close earlier in w5100_stop()
  - Flush rx/tx work and cancel restart in w5100_stop()
  - Call cancel_work_sync() after unregister_netdev in w5100_remove()
  - Call cancel_work_sync before netif_carrier_off in w5100_suspend()
  - Enable/Disable main irq in open()/stop() and suspend()/resume()
  - Check the number of irqs defined in DT
  - Ignore fwnode_irq_get errors other than EPROBE_DEFER

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 | 227 +++++++++++++++++++++++++++++++-----
 2 files changed, 202 insertions(+), 26 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9f84e4c8163e..946b5094bba7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29455,6 +29455,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..203a2099aa7b 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,77 @@ 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;
+	bool link;
+
+	mutex_lock(&priv->link_lock);
+
+	if (!netif_device_present(ndev)) {
+		link = netif_carrier_ok(ndev);
+		goto out;
+	}
+
+	state = w5500_get_phycfgr_lnk(ndev);
+
+	if (state < 0) {
+		link = netif_carrier_ok(ndev);
+		goto out;
+	}
+
+	link = state > 0;
+
+out:
+	mutex_unlock(&priv->link_lock);
+
+	return link;
+}
+
 static int w5100_hw_reset(struct w5100_priv *priv)
 {
 	u32 rtr;
@@ -448,12 +526,25 @@ static void w5100_restart(struct net_device *ndev)
 {
 	struct w5100_priv *priv = netdev_priv(ndev);
 
+	if (!netif_running(ndev) || !netif_device_present(ndev))
+		return;
+
+	disable_irq(priv->irq);
+	if (priv->link_irq > 0)
+		disable_irq(priv->link_irq);
+
 	netif_stop_queue(ndev);
 	w5100_hw_reset(priv);
+	enable_irq(priv->irq);
 	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)
@@ -468,10 +559,7 @@ static void w5100_tx_timeout(struct net_device *ndev, unsigned int txqueue)
 {
 	struct w5100_priv *priv = netdev_priv(ndev);
 
-	if (priv->ops->may_sleep)
-		schedule_work(&priv->restart_work);
-	else
-		w5100_restart(ndev);
+	schedule_work(&priv->restart_work);
 }
 
 static void w5100_tx_skb(struct net_device *ndev, struct sk_buff *skb)
@@ -656,9 +744,16 @@ static int w5100_open(struct net_device *ndev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	netif_info(priv, ifup, ndev, "enabling\n");
-	w5100_hw_start(priv);
 	napi_enable(&priv->napi);
+	enable_irq(priv->irq);
+	w5100_hw_start(priv);
 	netif_start_queue(ndev);
+
+	if (priv->link_irq > 0) {
+		w5500_report_carrier_state(ndev);
+		enable_irq(priv->link_irq);
+	}
+
 	return 0;
 }
 
@@ -667,13 +762,38 @@ static int w5100_stop(struct net_device *ndev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	netif_info(priv, ifdown, ndev, "shutting down\n");
+
+	disable_irq(priv->irq);
+	if (priv->link_irq > 0)
+		disable_irq(priv->link_irq);
+
+	cancel_work_sync(&priv->restart_work);
+	cancel_work_sync(&priv->setrx_work);
+	flush_work(&priv->rx_work);
+	flush_work(&priv->tx_work);
+
 	w5100_hw_close(priv);
-	netif_carrier_off(ndev);
+
+	if (priv->link_irq > 0)  {
+		mutex_lock(&priv->link_lock);
+		netif_carrier_off(ndev);
+		mutex_unlock(&priv->link_lock);
+	}
+
 	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 +841,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 +867,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 = -EINVAL;
+	if (ops->chip_id == W5500) {
+		priv->link_irq = fwnode_irq_get(dev_fwnode(dev), 1);
+		if (priv->link_irq == -EPROBE_DEFER) {
+			err = dev_err_probe(dev, priv->link_irq,
+					    "failed to get link irq\n");
+			goto err_mutex;
+		}
+	}
+
 	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 +894,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);
@@ -794,22 +923,44 @@ 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);
+					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
+					   IRQF_NO_AUTOEN,
+					   dev_name(dev), ndev);
 	} else {
 		err = request_irq(priv->irq, w5100_interrupt,
-				  IRQF_TRIGGER_LOW, netdev_name(ndev), ndev);
+				  IRQF_TRIGGER_LOW | IRQF_NO_AUTOEN, 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 +971,21 @@ 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);
+
+	if (priv->link_irq > 0)
+		free_irq(priv->link_irq, ndev);
 	free_irq(priv->irq, ndev);
 
-	flush_work(&priv->setrx_work);
-	flush_work(&priv->restart_work);
-	destroy_workqueue(priv->xfer_wq);
+	w5100_hw_reset(priv);
 
-	unregister_netdev(ndev);
+	cancel_work_sync(&priv->setrx_work);
+	cancel_work_sync(&priv->restart_work);
+	flush_work(&priv->rx_work);
+	flush_work(&priv->tx_work);
+
+	destroy_workqueue(priv->xfer_wq);
+	mutex_destroy(&priv->link_lock);
 	free_netdev(ndev);
 }
 EXPORT_SYMBOL_GPL(w5100_remove);
@@ -839,9 +997,20 @@ static int w5100_suspend(struct device *dev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	if (netif_running(ndev)) {
-		netif_carrier_off(ndev);
+		disable_irq(priv->irq);
+		if (priv->link_irq > 0)
+			disable_irq(priv->link_irq);
+
+		cancel_work_sync(&priv->restart_work);
+
 		netif_device_detach(ndev);
 
+		if (priv->link_irq > 0) {
+			mutex_lock(&priv->link_lock);
+			netif_carrier_off(ndev);
+			mutex_unlock(&priv->link_lock);
+		}
+
 		w5100_hw_close(priv);
 	}
 	return 0;
@@ -854,9 +1023,15 @@ static int w5100_resume(struct device *dev)
 
 	if (netif_running(ndev)) {
 		w5100_hw_reset(priv);
+		enable_irq(priv->irq);
 		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 v10 3/3] w5100: detect carrier state using link status bit and optional interrupt

[Arthur Crepin Leblond]

On Mon, Sep 21, 2026 at 12:52:51PM +0200, Arthur Crepin Leblond wrote:
>@@ -794,22 +923,44 @@ 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);
>+					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
>+					   IRQF_NO_AUTOEN,
>+					   dev_name(dev), ndev);
> 	} else {
> 		err = request_irq(priv->irq, w5100_interrupt,
>-				  IRQF_TRIGGER_LOW, netdev_name(ndev), ndev);
>+				  IRQF_TRIGGER_LOW | IRQF_NO_AUTOEN, dev_name(dev), ndev);
> 	}

Oh, I exceeded 80 columns here...

Arthur

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch silently adds a new MAINTAINERS section ("WIZNET W5100
  ETHERNET DRIVER") naming the submitter as maintainer, but the commit…
- [Low] In Documentation/devicetree/bindings/net/wiznet,w5100.yaml the
  new schema states a single, unconditional 'spi-max-frequency: maximum:…

--

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, and changes the documented interrupt trigger type
from edge-falling to level-low to match the driver. It also adds a new
MAINTAINERS section for the binding.

Two questions below.

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

[ ... ]

> +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 here says the w5100 tops out around 14 MHz, but the only
machine-checkable constraint is the loosest value, 80000000, and it applies
to all three compatibles.

Should the w5100 limit be expressed as a conditional instead, so that it is
actually validated?

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

As written, a node with:

    compatible = "wiznet,w5100";
    spi-max-frequency = <80000000>;

passes dt_binding_check and dtbs_check with no diagnostic. Nothing in
drivers/net/ethernet/wiznet/ looks at spi->max_speed_hz either, so the
14 MHz figure is not enforced anywhere.

This is still the case at the end of the series: the allOf if/then block
added later by "dt-bindings: net: wiznet,w5100: add link status interrupt"
only constrains interrupts, and spi-max-frequency keeps the unconditional
80 MHz ceiling.

Separately, the old wiznet,w5x00.txt documented the 80 MHz figure for the
w5500 only:

    - spi-max-frequency: Maximum frequency of the SPI bus when accessing
      the w5500.

The new schema extends that ceiling to w5100 and w5200 as well. Could the
commit message mention that change?

[ ... ]

> diff --git a/MAINTAINERS b/MAINTAINERS
> index df8ab9b824024..9f84e4c8163e4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -29450,6 +29450,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
> +

[Severity: Low]
The commit message describes only the .txt to .yaml conversion and the
trigger type change, but this hunk also creates a new maintainership entry.
The "v8: Add a myself in MAINTAINERS" note is below the --- line and will
not land in git history.

Could the commit message describe the MAINTAINERS addition too?

Also, at this commit the section is titled "WIZNET W5100 ETHERNET DRIVER"
while the only F: pattern matches the YAML binding, so get_maintainer.pl
still reports no maintainer for drivers/net/ethernet/wiznet/. That gap is
closed later in the series: "w5100: detect carrier state using link status
bit and optional interrupt" adds F: drivers/net/ethernet/wiznet/ to the
same block. Would it make sense to add that F: line here, where the section
is introduced?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-wiznet-link-gpio-v10-0-5874a7a147a2%40marmottus.net

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Concern (INVALIDATED by later patch in this series): the binding
  documents an optional second, LINKLED interrupt for wiznet,w5500 and…

--

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

This adds an optional second interrupt to the wiznet,w5100 binding for the
LINKLED pin, described as being supported only on wiznet,w5500.  The
interrupts property is moved to a two-item list with an allOf conditional
restricting non-w5500 compatibles to a single interrupt, and the example is
updated to list both interrupts.

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

[ ... ]

> @@ -24,8 +20,16 @@ properties:
>        - wiznet,w5500
>  
>    interrupts:
> -    maxItems: 1
> -    description: Main interrupt (INT pin on the device).
> +    minItems: 1
> +    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.
> +    items:
> +      - description: Main interrupt (INT pin on the device).
> +      - description:
> +          Optional interrupt for link status changes (LINKLED pin on w5500).

[Severity: Low]
At this point in the series, is there anything that consumes the second
interrupt?  The description promises that link state changes are reported
when the LINKLED interrupt is present, but w5100_probe() in
drivers/net/ethernet/wiznet/w5100.c only uses spi->irq (interrupt index 0)
and there is no netif_carrier_on() call in the driver yet.

For the record, this resolves later in the same series: the final patch
("w5100: detect carrier state using link status bit and optional
interrupt") adds

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

and w5500_report_carrier_state() which calls
netif_carrier_on()/netif_carrier_off().  So the documentation and the
implementation only disagree between this patch and that one, which is
nothing more than a transient bisect-window mismatch.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-wiznet-link-gpio-v10-0-5874a7a147a2%40marmottus.net

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] w5100_suspend() does not quiesce the deferred hardware-access
  paths before closing the controller: it disables the IRQs, cancels…
- [High] w5100_stop() now waits synchronously for the receive worker
  *before* stopping the hardware that produces receive traffic:…
- [Medium] The patch silently repairs a live, user-visible regression
  introduced by dacf281771a9 ("w5100: remove unused gpio link…
- [Medium] Commit message claim is not implemented: the message states
  "w5100_remove(), w5100_stop() and w5100_suspend() call…
- [Medium] w5500_report_carrier_state() (w5100.c:369-386) silently
  discards a failed PHYCFGR read: w5500_get_phycfgr_lnk() returns a…
- [Medium] Lost link-state edge because the PHYCFGR sample is taken
  before the edge-triggered link IRQ is (re-)enabled.
- [Medium] w5100_suspend() calls cancel_work_sync(&priv->restart_work)
  (line 1004) BEFORE netif_device_detach() (line 1006).
- [Low] For a w5500 whose DT does not supply the optional second
  (LINKLED) interrupt, the driver installs w5500_get_link() in…

--

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

This adds carrier detection for the w5500 by reading the Link Status bit of
PHYCFGR, exposes it through a new w5500-only .get_link callback, and adds an
optional second interrupt (LINKLED) to update the carrier with
netif_carrier_on|off().  It also reworks probe/remove ordering, IRQ
enable/disable across open/stop/suspend/resume and work cancellation.

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

[ ... ]

> @@ -345,6 +352,77 @@ 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]
What happens to the carrier state when the PHYCFGR read fails here?

w5500_get_phycfgr_lnk() returns a negative errno on SPI failure, but
w5500_report_carrier_state() only acts on state > 0 and state == 0, and it
returns void, so the error is neither acted on nor propagated.

The carrier is forced off beforehand (netif_carrier_off() in w5100_probe()
for the link_irq case, and in w5100_stop()/w5100_suspend()), and
w5100_open() samples the state exactly once:

	if (priv->link_irq > 0) {
		w5500_report_carrier_state(ndev);
		enable_irq(priv->link_irq);
	}

	return 0;

Since w5100_open() still returns 0 and the link IRQ is edge triggered
(IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING), no further event arrives while
the link stays up.  Can a single failed read at open (or resume, or restart)
therefore leave the netdev permanently without carrier, with dev_activate()
refusing to leave noop_qdisc and dev_watchdog() skipping the device, until
the cable is toggled or the interface is cycled?  Meanwhile w5500_get_link()
re-reads the register and keeps reporting "Link detected: yes".

> +
> +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;
> +	bool link;
> +
> +	mutex_lock(&priv->link_lock);
> +
> +	if (!netif_device_present(ndev)) {
> +		link = netif_carrier_ok(ndev);
> +		goto out;
> +	}
> +
> +	state = w5500_get_phycfgr_lnk(ndev);
> +
> +	if (state < 0) {
> +		link = netif_carrier_ok(ndev);
> +		goto out;
> +	}
> +
> +	link = state > 0;
> +
> +out:
> +	mutex_unlock(&priv->link_lock);
> +
> +	return link;
> +}

[Severity: Low]
This isn't a bug as such, since the commit message documents the intent, but
for a w5500 board whose DT has no second (LINKLED) interrupt the two link
uAPIs end up permanently disagreeing.

w5100_probe() installs the new ops for every w5500:

	ndev->ethtool_ops = ops->chip_id == W5500 ? &w5500_ethtool_ops :
						    &w5100_ethtool_ops;

so ethtool reports the real PHYCFGR bit, while every netif_carrier_on|off()
call in this patch (including the netif_carrier_off() in probe) sits inside
an if (priv->link_irq > 0) block.

With the carrier bit never touched, netif_carrier_ok() stays true (nothing
in net/core sets __LINK_STATE_NOCARRIER at registration).  Would it be
better to only publish .get_link when the driver can also maintain the
carrier, so that ip link and ETHTOOL_GLINK cannot report IFF_LOWER_UP and
"Link detected: no" at the same time?

[ ... ]

> @@ -656,9 +744,16 @@ static int w5100_open(struct net_device *ndev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	netif_info(priv, ifup, ndev, "enabling\n");
> -	w5100_hw_start(priv);
>  	napi_enable(&priv->napi);
> +	enable_irq(priv->irq);
> +	w5100_hw_start(priv);
>  	netif_start_queue(ndev);
> +
> +	if (priv->link_irq > 0) {
> +		w5500_report_carrier_state(ndev);
> +		enable_irq(priv->link_irq);
> +	}
> +
>  	return 0;
>  }

[Severity: Medium]
Can a link transition be missed here because PHYCFGR is sampled before the
edge triggered IRQ is enabled?

The same read-then-enable order is used in w5100_restart() and
w5100_resume().  The IRQ is requested with

	IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
	IRQF_NO_AUTOEN

so at open the enable_irq() is the initial irq_startup(): the flow handler
has never run, no IRQS_PENDING state exists in the irq_desc for the core to
resend, and replay of an edge that happened while the line was masked
depends on the irqchip latching it (GPIO controllers such as bcm2835 clear
the per-line edge enable bits on mask, so nothing is recorded).

A transition inside the window between the read and enable_irq() would then
be seen by neither, leaving the carrier stale until the next physical
change.  Would enabling the IRQ first and reading PHYCFGR afterwards be
safe, given that w5500_report_carrier_state() is idempotent and serialized
by priv->link_lock?  The v7 changelog entry ("Enable the link IRQ before
reading the PHYCFGR bit") suggests this order was intentional at some point.

>  
> @@ -667,13 +762,38 @@ static int w5100_stop(struct net_device *ndev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	netif_info(priv, ifdown, ndev, "shutting down\n");
> +
> +	disable_irq(priv->irq);
> +	if (priv->link_irq > 0)
> +		disable_irq(priv->link_irq);
> +
> +	cancel_work_sync(&priv->restart_work);
> +	cancel_work_sync(&priv->setrx_work);
> +	flush_work(&priv->rx_work);
> +	flush_work(&priv->tx_work);
> +
>  	w5100_hw_close(priv);

[Severity: High]
Can flush_work(&priv->rx_work) block here indefinitely, since it now runs
before w5100_hw_close() stops the producer of receive traffic?

disable_irq() stops interrupt handling, but socket 0 is still open and the
chip keeps filling its 16 KB RX buffer.  The worker drains without a budget
or a shutdown check:

drivers/net/ethernet/wiznet/w5100.c:w5100_rx_work() {
	while ((skb = w5100_rx_skb(priv->ndev)))
		netif_rx(skb);

	w5100_enable_intr(priv);
}

and w5100_rx_skb() only returns NULL once the hardware buffer reads empty:

	u16 rx_buf_len = w5100_read16(priv, W5100_S0_RX_RSR(priv));

	if (rx_buf_len == 0)
		return NULL;

The SPI drain rate is well below the wire rate, so an adjacent sender
flooding the link can keep RSR non-zero for as long as it likes.

ndo_stop runs under RTNL, so would "ip link set dev down" (and
unregister_netdev() from w5100_remove(), which now runs first) stall with
RTNL held, blocking all other network configuration?  Closing the hardware
before flushing rx_work, as the previous code did, would avoid this.

> -	netif_carrier_off(ndev);
> +
> +	if (priv->link_irq > 0)  {
> +		mutex_lock(&priv->link_lock);
> +		netif_carrier_off(ndev);
> +		mutex_unlock(&priv->link_lock);
> +	}
> +

[Severity: Medium]
Making this netif_carrier_off() conditional, together with the new carrier
sampling in w5100_open()/w5100_resume(), looks like it also repairs an
existing user visible breakage.  Should that be split out with a Fixes: tag?

In the baseline tree, dacf281771a9 ("w5100: remove unused gpio link
detection") removed w5100_get_link()/w5100_detect_link() and both
netif_carrier_on() call sites, but kept the unconditional
netif_carrier_off() in w5100_stop() and w5100_suspend().  git grep
netif_carrier_on on the baseline returns no match in this driver.

A freshly registered netdev has __LINK_STATE_NOCARRIER clear, so the first
ifup works, but after the first ifdown (or suspend cycle) the bit is set and
nothing ever clears it again, and dev_activate() takes the "Delay activation
until next carrier-on event" path, so the interface cannot transmit until
the module is reloaded.

As written, the fix is buried in a feature patch that also rewrites probe
ordering, IRQ enable/disable and work cancellation, which makes it hard to
pick up for stable.

>  	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,
> +};
> +

[ ... ]

> @@ -839,9 +997,20 @@ static int w5100_suspend(struct device *dev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	if (netif_running(ndev)) {
> -		netif_carrier_off(ndev);
> +		disable_irq(priv->irq);
> +		if (priv->link_irq > 0)
> +			disable_irq(priv->link_irq);
> +
> +		cancel_work_sync(&priv->restart_work);
> +
>  		netif_device_detach(ndev);

[Severity: Medium]
The commit message says:

    w5100_remove(), w5100_stop() and w5100_suspend() call
    cancel_work_sync()/flush_work() to make sure there is no pending
    work

but w5100_suspend() only drains restart_work.  w5100_stop() and
w5100_remove() drain all four (restart_work, setrx_work, rx_work, tx_work).
Could the changelog be adjusted, or the suspend path made to match?

[Severity: High]
Does leaving setrx_work, rx_work and tx_work pending here allow the chip to
be re-armed after w5100_hw_close()?

disable_irq()/synchronize_irq() waits for the threaded handler only, not for
work already queued on priv->xfer_wq:

w5100_interrupt()      -> queue_work(priv->xfer_wq, &priv->rx_work)
w5100_start_tx()       -> queue_work(priv->xfer_wq, &priv->tx_work)
w5100_set_rx_mode()    -> schedule_work(&priv->setrx_work)

A pending w5100_rx_work() ends with w5100_enable_intr(), re-arming
W5500_SIMR / W5100_IMR; w5100_setrx_work() calls w5100_hw_start(), which
issues S0_CR_OPEN and enables interrupts again; w5100_tx_work() can program
S0_TX_WR and issue S0_CR_SEND.

Any of these can run concurrently with, or after, the w5100_hw_close() a few
lines below, so the controller can stay open with socket interrupts armed
for the whole suspend while the CPU IRQ is disabled, and the SPI transfers
are issued after the device was marked absent.  All in-tree ops set
may_sleep = true, so this is the live path.  Would replicating the
w5100_stop() sequence here be appropriate?

[Severity: Medium]
Can restart_work be re-queued after this cancel_work_sync(), because the
cancel happens before netif_device_detach()?

In that window the device is still running, still present, and (for the
link IRQ case) still carrier-ok, and the tx watchdog timer is not lowered on
the suspend path.  On the dev_close() path, dev_deactivate_many() calls
netdev_watchdog_down() before ndo_stop, which is what makes the same
ordering safe in w5100_stop().

dev_watchdog() only requires:

net/sched/sch_generic.c:dev_watchdog() {
		if (netif_device_present(dev) &&
		    netif_running(dev) &&
		    netif_carrier_ok(dev)) {
	...
}

and w5100_tx_timeout() is now unconditional:

	schedule_work(&priv->restart_work);

The re-queued w5100_restart() would then pass its

	if (!netif_running(ndev) || !netif_device_present(ndev))
		return;

test, since nothing serializes that check with netif_device_detach(), and
proceed to w5100_hw_reset(), enable_irq(priv->irq), w5100_hw_start() and the
PHYCFGR read, concurrently with the w5100_hw_close() below.  Would detaching
first and cancelling afterwards close this?

>  
> +		if (priv->link_irq > 0) {
> +			mutex_lock(&priv->link_lock);
> +			netif_carrier_off(ndev);
> +			mutex_unlock(&priv->link_lock);
> +		}
> +
>  		w5100_hw_close(priv);
>  	}
>  	return 0;

[Severity: Medium]
Same observation as in w5100_stop(): this suspend path previously called
netif_carrier_off() with no matching netif_carrier_on() anywhere in the
driver, so a suspend/resume cycle left the interface stuck without carrier.
The repair is welcome, but it is not called out in the changelog and there
is no Fixes: tag referencing dacf281771a9.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-wiznet-link-gpio-v10-0-5874a7a147a2%40marmottus.net

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

[Arthur Crepin Leblond]

On Thu, Sep 24, 2026 at 01:55:53AM +0000, netdev-bot+sashiko@kernel.org wrote:
>[Severity: Low]
>The description here says the w5100 tops out around 14 MHz, but the only
>machine-checkable constraint is the loosest value, 80000000, and it applies
>to all three compatibles.
>
>Should the w5100 limit be expressed as a conditional instead, so that it is
>actually validated?
>
>  allOf:
>    - if:
>        properties:
>          compatible:
>            contains:
>              const: wiznet,w5100
>      then:
>        properties:
>          spi-max-frequency:
>            maximum: 14000000
>
>As written, a node with:
>
>    compatible = "wiznet,w5100";
>    spi-max-frequency = <80000000>;
>
>passes dt_binding_check and dtbs_check with no diagnostic. Nothing in
>drivers/net/ethernet/wiznet/ looks at spi->max_speed_hz either, so the
>14 MHz figure is not enforced anywhere.
>
>This is still the case at the end of the series: the allOf if/then block
>added later by "dt-bindings: net: wiznet,w5100: add link status interrupt"
>only constrains interrupts, and spi-max-frequency keeps the unconditional
>80 MHz ceiling.
>
>Separately, the old wiznet,w5x00.txt documented the 80 MHz figure for the
>w5500 only:
>
>    - spi-max-frequency: Maximum frequency of the SPI bus when accessing
>      the w5500.
>
>The new schema extends that ceiling to w5100 and w5200 as well. Could the
>commit message mention that change?
I don't think it is really an issue here, the user must put a relevant
max spi frequency when creating their DT.

>
>[ ... ]
>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index df8ab9b824024..9f84e4c8163e4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -29450,6 +29450,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
>> +
>
>[Severity: Low]
>The commit message describes only the .txt to .yaml conversion and the
>trigger type change, but this hunk also creates a new maintainership entry.
>The "v8: Add a myself in MAINTAINERS" note is below the --- line and will
>not land in git history.
>
>Could the commit message describe the MAINTAINERS addition too?
>
>Also, at this commit the section is titled "WIZNET W5100 ETHERNET DRIVER"
>while the only F: pattern matches the YAML binding, so get_maintainer.pl
>still reports no maintainer for drivers/net/ethernet/wiznet/. That gap is
>closed later in the series: "w5100: detect carrier state using link status
>bit and optional interrupt" adds F: drivers/net/ethernet/wiznet/ to the
>same block. Would it make sense to add that F: line here, where the section
>is introduced?
>
>--
Will be fixed in v11

Arthur

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

[Arthur Crepin Leblond]

On Thu, Sep 24, 2026 at 01:55:54AM +0000, netdev-bot+sashiko@kernel.org wrote:
>Thank you for your contribution! Sashiko AI review found more severe
>issue(s) in other patches of this series. This mail is only an FYI, in case
>you want to address the nit picks below when you respin.
>
>Low priority / nit picks for your consideration:
>
>- [Low] Concern (INVALIDATED by later patch in this series): the binding
>  documents an optional second, LINKLED interrupt for wiznet,w5500 and…
>
>--
>
>dt-bindings: net: wiznet,w5100: add link status interrupt
>
>This adds an optional second interrupt to the wiznet,w5100 binding for the
>LINKLED pin, described as being supported only on wiznet,w5500.  The
>interrupts property is moved to a two-item list with an allOf conditional
>restricting non-w5500 compatibles to a single interrupt, and the example is
>updated to list both interrupts.
>
>> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
>> index 25098308eb6e5..ac2ee725af5d5 100644
>> --- a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
>> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
>
>[ ... ]
>
>> @@ -24,8 +20,16 @@ properties:
>>        - wiznet,w5500
>>
>>    interrupts:
>> -    maxItems: 1
>> -    description: Main interrupt (INT pin on the device).
>> +    minItems: 1
>> +    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.
>> +    items:
>> +      - description: Main interrupt (INT pin on the device).
>> +      - description:
>> +          Optional interrupt for link status changes (LINKLED pin on w5500).
>
>[Severity: Low]
>At this point in the series, is there anything that consumes the second
>interrupt?  The description promises that link state changes are reported
>when the LINKLED interrupt is present, but w5100_probe() in
>drivers/net/ethernet/wiznet/w5100.c only uses spi->irq (interrupt index 0)
>and there is no netif_carrier_on() call in the driver yet.
>
>For the record, this resolves later in the same series: the final patch
>("w5100: detect carrier state using link status bit and optional
>interrupt") adds
>
>	priv->link_irq = -EINVAL;
>	if (ops->chip_id == W5500) {
>		priv->link_irq = fwnode_irq_get(dev_fwnode(dev), 1);
>
>and w5500_report_carrier_state() which calls
>netif_carrier_on()/netif_carrier_off().  So the documentation and the
>implementation only disagree between this patch and that one, which is
>nothing more than a transient bisect-window mismatch.
>
>[ ... ]
>

https://elixir.bootlin.com/linux/v7.2/source/Documentation/devicetree/bindings/submitting-patches.rst#L54

   The Documentation/ portion of the patch should come in the series before
   the code implementing the binding.

Arthur

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

[Arthur Crepin Leblond]

On Thu, Sep 24, 2026 at 01:55:55AM +0000, netdev-bot+sashiko@kernel.org wrote:
>[Severity: Medium]
>What happens to the carrier state when the PHYCFGR read fails here?
>
>w5500_get_phycfgr_lnk() returns a negative errno on SPI failure, but
>w5500_report_carrier_state() only acts on state > 0 and state == 0, and it
>returns void, so the error is neither acted on nor propagated.
>
>The carrier is forced off beforehand (netif_carrier_off() in w5100_probe()
>for the link_irq case, and in w5100_stop()/w5100_suspend()), and
>w5100_open() samples the state exactly once:
>
>	if (priv->link_irq > 0) {
>		w5500_report_carrier_state(ndev);
>		enable_irq(priv->link_irq);
>	}
>
>	return 0;
>
>Since w5100_open() still returns 0 and the link IRQ is edge triggered
>(IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING), no further event arrives while
>the link stays up.  Can a single failed read at open (or resume, or restart)
>therefore leave the netdev permanently without carrier, with dev_activate()
>refusing to leave noop_qdisc and dev_watchdog() skipping the device, until
>the cable is toggled or the interface is cycled?  Meanwhile w5500_get_link()
>re-reads the register and keeps reporting "Link detected: yes".

I'll add a retry loop.


>[Severity: Low]
>This isn't a bug as such, since the commit message documents the intent, but
>for a w5500 board whose DT has no second (LINKLED) interrupt the two link
>uAPIs end up permanently disagreeing.
>
>w5100_probe() installs the new ops for every w5500:
>
>	ndev->ethtool_ops = ops->chip_id == W5500 ? &w5500_ethtool_ops :
>						    &w5100_ethtool_ops;
>
>so ethtool reports the real PHYCFGR bit, while every netif_carrier_on|off()
>call in this patch (including the netif_carrier_off() in probe) sits inside
>an if (priv->link_irq > 0) block.
>
>With the carrier bit never touched, netif_carrier_ok() stays true (nothing
>in net/core sets __LINK_STATE_NOCARRIER at registration).  Would it be
>better to only publish .get_link when the driver can also maintain the
>carrier, so that ip link and ETHTOOL_GLINK cannot report IFF_LOWER_UP and
>"Link detected: no" at the same time?
>

No but that's a fair point, .get_link() should probably be
implemented only on w5500 with a link irq.

>[ ... ]
>
>> @@ -656,9 +744,16 @@ static int w5100_open(struct net_device *ndev)
>>  	struct w5100_priv *priv = netdev_priv(ndev);
>>
>>  	netif_info(priv, ifup, ndev, "enabling\n");
>> -	w5100_hw_start(priv);
>>  	napi_enable(&priv->napi);
>> +	enable_irq(priv->irq);
>> +	w5100_hw_start(priv);
>>  	netif_start_queue(ndev);
>> +
>> +	if (priv->link_irq > 0) {
>> +		w5500_report_carrier_state(ndev);
>> +		enable_irq(priv->link_irq);
>> +	}
>> +
>>  	return 0;
>>  }
>
>[Severity: Medium]
>Can a link transition be missed here because PHYCFGR is sampled before the
>edge triggered IRQ is enabled?
>
>The same read-then-enable order is used in w5100_restart() and
>w5100_resume().  The IRQ is requested with
>
>	IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
>	IRQF_NO_AUTOEN
>
>so at open the enable_irq() is the initial irq_startup(): the flow handler
>has never run, no IRQS_PENDING state exists in the irq_desc for the core to
>resend, and replay of an edge that happened while the line was masked
>depends on the irqchip latching it (GPIO controllers such as bcm2835 clear
>the per-line edge enable bits on mask, so nothing is recorded).
>
>A transition inside the window between the read and enable_irq() would then
>be seen by neither, leaving the carrier stale until the next physical
>change.  Would enabling the IRQ first and reading PHYCFGR afterwards be
>safe, given that w5500_report_carrier_state() is idempotent and serialized
>by priv->link_lock?  The v7 changelog entry ("Enable the link IRQ before
>reading the PHYCFGR bit") suggests this order was intentional at some
>point.

I'll enable the irq before reporting the state.

>
>>
>> @@ -667,13 +762,38 @@ static int w5100_stop(struct net_device *ndev)
>>  	struct w5100_priv *priv = netdev_priv(ndev);
>>
>>  	netif_info(priv, ifdown, ndev, "shutting down\n");
>> +
>> +	disable_irq(priv->irq);
>> +	if (priv->link_irq > 0)
>> +		disable_irq(priv->link_irq);
>> +
>> +	cancel_work_sync(&priv->restart_work);
>> +	cancel_work_sync(&priv->setrx_work);
>> +	flush_work(&priv->rx_work);
>> +	flush_work(&priv->tx_work);
>> +
>>  	w5100_hw_close(priv);
>
>[Severity: High]
>Can flush_work(&priv->rx_work) block here indefinitely, since it now runs
>before w5100_hw_close() stops the producer of receive traffic?
>
>disable_irq() stops interrupt handling, but socket 0 is still open and the
>chip keeps filling its 16 KB RX buffer.  The worker drains without a budget
>or a shutdown check:
>
>drivers/net/ethernet/wiznet/w5100.c:w5100_rx_work() {
>	while ((skb = w5100_rx_skb(priv->ndev)))
>		netif_rx(skb);
>
>	w5100_enable_intr(priv);
>}
>
>and w5100_rx_skb() only returns NULL once the hardware buffer reads empty:
>
>	u16 rx_buf_len = w5100_read16(priv, W5100_S0_RX_RSR(priv));
>
>	if (rx_buf_len == 0)
>		return NULL;
>
>The SPI drain rate is well below the wire rate, so an adjacent sender
>flooding the link can keep RSR non-zero for as long as it likes.
>
>ndo_stop runs under RTNL, so would "ip link set dev down" (and
>unregister_netdev() from w5100_remove(), which now runs first) stall with
>RTNL held, blocking all other network configuration?  Closing the hardware
>before flushing rx_work, as the previous code did, would avoid this.

Yeah, I've had this suggestion when running Sashiko locally too. This
patch is a Pandora box or race conditions! But hopefully I figured it
out and it's fixed in v11.
I am really learning a lot along the way regarding the synchronization
of ndos and how the calls are made.

>
>> -	netif_carrier_off(ndev);
>> +
>> +	if (priv->link_irq > 0)  {
>> +		mutex_lock(&priv->link_lock);
>> +		netif_carrier_off(ndev);
>> +		mutex_unlock(&priv->link_lock);
>> +	}
>> +
>
>[Severity: Medium]
>Making this netif_carrier_off() conditional, together with the new carrier
>sampling in w5100_open()/w5100_resume(), looks like it also repairs an
>existing user visible breakage.  Should that be split out with a Fixes: tag?
>
>In the baseline tree, dacf281771a9 ("w5100: remove unused gpio link
>detection") removed w5100_get_link()/w5100_detect_link() and both
>netif_carrier_on() call sites, but kept the unconditional
>netif_carrier_off() in w5100_stop() and w5100_suspend().  git grep
>netif_carrier_on on the baseline returns no match in this driver.
>
>A freshly registered netdev has __LINK_STATE_NOCARRIER clear, so the first
>ifup works, but after the first ifdown (or suspend cycle) the bit is set and
>nothing ever clears it again, and dev_activate() takes the "Delay activation
>until next carrier-on event" path, so the interface cannot transmit until
>the module is reloaded.
>
>As written, the fix is buried in a feature patch that also rewrites probe
>ordering, IRQ enable/disable and work cancellation, which makes it hard to
>pick up for stable.

It still don't think it is a fix. The patch is bringing back the
interrupt and improves it but it is too much of a change to be called
a fix.

>[Severity: Medium]
>The commit message says:
>
>    w5100_remove(), w5100_stop() and w5100_suspend() call
>    cancel_work_sync()/flush_work() to make sure there is no pending
>    work
>
>but w5100_suspend() only drains restart_work.  w5100_stop() and
>w5100_remove() drain all four (restart_work, setrx_work, rx_work, tx_work).
>Could the changelog be adjusted, or the suspend path made to match?
>
>[Severity: High]
>Does leaving setrx_work, rx_work and tx_work pending here allow the chip to
>be re-armed after w5100_hw_close()?
>
>disable_irq()/synchronize_irq() waits for the threaded handler only, not for
>work already queued on priv->xfer_wq:
>
>w5100_interrupt()      -> queue_work(priv->xfer_wq, &priv->rx_work)
>w5100_start_tx()       -> queue_work(priv->xfer_wq, &priv->tx_work)
>w5100_set_rx_mode()    -> schedule_work(&priv->setrx_work)
>
>A pending w5100_rx_work() ends with w5100_enable_intr(), re-arming
>W5500_SIMR / W5100_IMR; w5100_setrx_work() calls w5100_hw_start(), which
>issues S0_CR_OPEN and enables interrupts again; w5100_tx_work() can program
>S0_TX_WR and issue S0_CR_SEND.
>
>Any of these can run concurrently with, or after, the w5100_hw_close() a few
>lines below, so the controller can stay open with socket interrupts armed
>for the whole suspend while the CPU IRQ is disabled, and the SPI transfers
>are issued after the device was marked absent.  All in-tree ops set
>may_sleep = true, so this is the live path.  Would replicating the
>w5100_stop() sequence here be appropriate?
>
>[Severity: Medium]
>Can restart_work be re-queued after this cancel_work_sync(), because the
>cancel happens before netif_device_detach()?
>
>In that window the device is still running, still present, and (for the
>link IRQ case) still carrier-ok, and the tx watchdog timer is not lowered on
>the suspend path.  On the dev_close() path, dev_deactivate_many() calls
>netdev_watchdog_down() before ndo_stop, which is what makes the same
>ordering safe in w5100_stop().
>
>dev_watchdog() only requires:
>
>net/sched/sch_generic.c:dev_watchdog() {
>		if (netif_device_present(dev) &&
>		    netif_running(dev) &&
>		    netif_carrier_ok(dev)) {
>	...
>}
>
>and w5100_tx_timeout() is now unconditional:
>
>	schedule_work(&priv->restart_work);
>
>The re-queued w5100_restart() would then pass its
>
>	if (!netif_running(ndev) || !netif_device_present(ndev))
>		return;
>
>test, since nothing serializes that check with netif_device_detach(), and
>proceed to w5100_hw_reset(), enable_irq(priv->irq), w5100_hw_start() and the
>PHYCFGR read, concurrently with the w5100_hw_close() below.  Would detaching
>first and cancelling afterwards close this?

I reworked all those in the upcoming v11. I really tried to figure out
all the edge cases.

Arthur