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

[PATCH net-next v9 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>
---
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                | 220 ++++++++++++++++++---
 4 files changed, 293 insertions(+), 72 deletions(-)
---
base-commit: 4bb9710c6a68d35207f123aef55dcd50e7195ec5
change-id: 20260804-wiznet-link-gpio-796f36cd14f9

[PATCH net-next v9 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 cae5962045dd..e6a5a4e245a0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29439,6 +29439,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

Re: [PATCH net-next v9 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: 1 · 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).

- [Medium] In Documentation/devicetree/bindings/net/wiznet,w5100.yaml the
  new machine-checkable constraint 'spi-max-frequency: maximum:…
- [Low] Undisclosed MAINTAINERS change: the commit adds a new "WIZNET
  W5100 ETHERNET DRIVER" section (MAINTAINERS:29442) that is never…

--

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 from
edge-falling to level-low to match the driver.  It also adds a new
MAINTAINERS section for the binding.

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

[ ... ]

> +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: Medium]
Do the description and the constraint here disagree for wiznet,w5100?

The text says w5100 is limited to about 14 MHz, but the only
machine-checkable limit is a single unconditional maximum of 80000000 shared
by all three compatibles.  So this node passes dt_binding_check:

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

Nothing downstream clamps it either: w5100_spi_read() and w5100_spi_write()
in drivers/net/ethernet/wiznet/w5100-spi.c issue their transfers at the SPI
device's configured speed, with no per-chip ceiling, so the out-of-spec rate
is what actually gets driven for every register and packet buffer access.

Would an if-then block be preferable, keeping 80 MHz for the other two
parts?

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

Note the removed .txt scoped its 80 MHz statement to the w5500 only and had
no machine-checkable limit, so the unconditional bound is new here.  The
single maximum of 80000000 is still the only spi-max-frequency constraint at
the end of the series (9d871f2771b9), where the added allOf block constrains
only interrupts.

[ ... ]

> diff --git a/MAINTAINERS b/MAINTAINERS
> index cae5962045dd..e6a5a4e245a0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -29439,6 +29439,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]
Could the commit message mention this new MAINTAINERS section?  The
preserved log text only describes the .txt to YAML conversion and the
edge-falling to level-low trigger change; the new maintainership claim is
mentioned only in the changelog below the --- separator, which does not end
up in git history.

The section is named as a driver entry, but its only F: line matches the
binding file, so at this commit scripts/get_maintainer.pl will not route
drivers/net/ethernet/wiznet/ changes here.  That part is addressed later in
the same series: the commit "w5100: detect carrier state using link status
bit and optional interrupt" adds F: drivers/net/ethernet/wiznet/ to this
section, so the mismatch exists only between this patch and that one.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-wiznet-link-gpio-v9-0-5536f24d8d6d%40marmottus.net

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

[Arthur Crepin Leblond]

On Tue, Sep 22, 2026 at 03:43:24PM +0000, netdev-bot+sashiko@kernel.org wrote:
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index cae5962045dd..e6a5a4e245a0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -29439,6 +29439,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]
>Could the commit message mention this new MAINTAINERS section?  The
>preserved log text only describes the .txt to YAML conversion and the
>edge-falling to level-low trigger change; the new maintainership claim is
>mentioned only in the changelog below the --- separator, which does not end
>up in git history.
>
>The section is named as a driver entry, but its only F: line matches the
>binding file, so at this commit scripts/get_maintainer.pl will not route
>drivers/net/ethernet/wiznet/ changes here.  That part is addressed later in
>the same series: the commit "w5100: detect carrier state using link status
>bit and optional interrupt" adds F: drivers/net/ethernet/wiznet/ to this
>section, so the mismatch exists only between this patch and that one.
>

I'll update the commit and the MAINTAINERS file.

Arthur

[PATCH net-next v9 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 v9 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>
---
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 | 220 ++++++++++++++++++++++++++++++++----
 2 files changed, 199 insertions(+), 22 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index e6a5a4e245a0..7e6f7a95a341 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29444,6 +29444,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..e6dc5fe7c088 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)
@@ -656,9 +747,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 +765,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 +844,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 +870,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 +897,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 +926,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 +974,19 @@ 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);
+
+	destroy_workqueue(priv->xfer_wq);
+	mutex_destroy(&priv->link_lock);
 	free_netdev(ndev);
 }
 EXPORT_SYMBOL_GPL(w5100_remove);
@@ -839,7 +998,18 @@ 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);
+
+		if (priv->link_irq > 0) {
+			mutex_lock(&priv->link_lock);
+			netif_carrier_off(ndev);
+			mutex_unlock(&priv->link_lock);
+		}
+
 		netif_device_detach(ndev);
 
 		w5100_hw_close(priv);
@@ -854,9 +1024,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 v9 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 7 potential
issue(s) to consider.

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

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 three of the four work items,
  so hardware access continues after (or concurrently with) the suspend-…
- [High] w5100_stop() newly calls flush_work(&priv->rx_work)
  (w5100.c:775) *before* w5100_hw_close() (w5100.c:778), i.e. while…
- [Medium] The patch silently fixes a long-standing user-visible bug
  without saying so and without a Fixes: tag.
- [Medium] The commit message states 'w5100_remove(), w5100_stop() and
  w5100_suspend() call cancel_work_sync()/flush_work() to make sure…
- [Medium] w5500_report_carrier_state() (w5100.c:369-386) silently
  discards a PHYCFGR read failure: w5500_get_phycfgr_lnk() returns the…
- [Medium] The driver samples PHYCFGR *before* enabling the edge-
  triggered LINKLED interrupt, so a link transition occurring in that…
- [Medium] net: w5100: sleep in atomic context in w5100_restart()

--

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, exposed through a new w5500 specific ethtool .get_link and updated
from an optional second interrupt wired to the LINKLED pin. It also reworks
the probe/remove ordering, adds a mutex around the link state, and
enables/disables the main irq in open/stop and suspend/resume.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> index 53d8dc642fbd..e6dc5fe7c088 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]
When w5500_get_phycfgr_lnk() returns a negative errno, neither branch runs
here, so the carrier is left untouched and the failure is not reported to the
caller either.

w5500_get_phycfgr_lnk() forwards the error from w5500_spi_read(), which
propagates spi_write_then_read() failures.

In w5100_open() the carrier bit has just been forced off (probe calls
netif_carrier_off() when a link irq is present, and w5100_stop() clears it
as well), so a single failed PHYCFGR read leaves the interface latched at
NO-CARRIER while w5100_open() still returns 0. The same applies to
w5100_resume() after w5100_suspend() cleared the carrier.

The link irq is requested edge triggered, so with a stable physical link no
further edge arrives. Is there any path that would correct the state short
of a cable flap or an ifdown/ifup? Should the read error be retried or
propagated to the caller?

Note ethtool .get_link re-reads the register, so it can report link detected
while operstate stays down.

> [ ... ]
>  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)
> @@ -656,9 +747,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]
w5100_restart() is not always reached from process context.
w5100_tx_timeout() only defers to restart_work when priv->ops->may_sleep
is set:

	if (priv->ops->may_sleep)
		schedule_work(&priv->restart_work);
	else
		w5100_restart(ndev);

and the memory mapped ops in this same file (w5100_mmio_direct_ops /
w5100_mmio_indirect_ops) set .may_sleep = false, so for those the
function is called directly from ndo_tx_timeout, i.e. from dev_watchdog()
in softirq context.

This hunk now puts sleeping calls on that path. disable_irq() begins with
might_sleep() and then synchronize_irq(), and when a link irq is present
w5500_report_carrier_state() takes priv->link_lock via mutex_lock().
Either one is a scheduling while atomic BUG, not just a lockdep
complaint.

The mutex part is confined to W5500, since priv->link_irq is only queried
for that chip and W5500 is SPI attached with may_sleep true, but the
disable_irq(priv->irq) / enable_irq(priv->irq) pair is unconditional and
is executed by every may_sleep == false user.

What is the intended split here - should w5100_tx_timeout() always queue
restart_work now that w5100_restart() can sleep, or should the irq
masking and the carrier re-check be done only in the may_sleep path?

[Severity: Medium]
The link irq is requested below with IRQF_NO_AUTOEN and
IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, so it stays masked until this
enable_irq(). PHYCFGR is sampled first and edge detection is only turned on
afterwards:

	w5500_report_carrier_state(ndev);	/* reads PHYCFGR */
	enable_irq(priv->link_irq);		/* edge detection starts */

If the PHY link comes up between those two steps, can that transition be
lost for good? On the BCM2835 controller used in the binding's own example,
masking disables edge detection and also discards latched events:

drivers/pinctrl/bcm/pinctrl-bcm2835.c:
	bcm2835_gpio_irq_config(pc, gpio, false);
	/* Clear events that were latched prior to clearing event sources */
	bcm2835_gpio_set_bit(pc, GPEDS0, gpio);

so unmasking does not replay the missed edge, and with a now stable LINKLED
level the driver keeps reporting carrier off until the next physical link
change.

w5100_restart() and w5100_resume() use the same order, and restart follows a
chip reset with PHY re-negotiation. The changelog lists this ordering as
intentional ("enable the link irq after checking PHYCFGR") - would enabling
the irq first and then reading PHYCFGR close the window?

>  
> @@ -667,13 +765,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]
flush_work(&priv->rx_work) now runs while socket 0 is still open, because
w5100_hw_close() is called after it.

w5100_rx_work() drains without a budget or a shutdown test:

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

and w5100_rx_skb() only returns NULL once the hardware receive buffer is
observed empty:

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

	if (rx_buf_len == 0)
		return NULL;

disable_irq(priv->irq) masks the host interrupt but does not stop the
controller from filling its receive buffer. Each frame is drained with
several synchronous SPI transfers, which is slower than line rate, so can
sustained incoming traffic keep RX_RSR non-zero and make this flush_work()
never return?

ndo_stop runs with rtnl held, so an ip link set down would then stall all
network configuration in an unkillable wait. w5100_remove() reaches the same
path through unregister_netdev().

Would calling w5100_hw_close() before flushing rx_work 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);
> +	}
> +
>  	netif_stop_queue(ndev);
>  	napi_disable(&priv->napi);
>  	return 0;
>  }

[Severity: Medium]
Making this netif_carrier_off() conditional on a link irq also fixes a
pre-existing user visible problem, but that is not called out and there is
no Fixes: tag.

Before this patch, w5100_stop() and w5100_suspend() called
netif_carrier_off() unconditionally, and there was no netif_carrier_on()
anywhere in drivers/net/ethernet/wiznet/. Since net_device state starts with
NOCARRIER clear at registration, an interface came up with carrier on, but
after the first

	ip link set dev ethX down
	ip link set dev ethX up

or after a suspend/resume cycle it stayed NO-CARRIER permanently.

With both carrier_off calls now guarded by priv->link_irq > 0, and link_irq
only ever queried for W5500, that behaviour disappears for w5100/w5200 and
for a W5500 without the optional LINKLED irq.

The changelog only says:

  The carrier state is also not reported using netif_carrier_on|off().

which reads as a description of the new design rather than a fix. Could this
be split into its own patch with a Fixes: tag so it can be picked up for
stable independently of the new feature?

> [ ... ]
> @@ -794,22 +926,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);
> +	}

[ ... ]

> @@ -839,7 +998,18 @@ 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);

[Severity: Medium]
The changelog states:

  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 cancels restart_work here. setrx_work (queued with
schedule_work() from w5100_set_rx_mode()), rx_work (queued on priv->xfer_wq
from w5100_interrupt()) and tx_work (queued from w5100_start_tx()) are
neither cancelled nor flushed, while w5100_stop() drains all four. Should
the changelog be narrowed, or the suspend path made to match stop()?

[Severity: High]
Independently of the changelog wording, can these work items still touch the
hardware after suspend has closed it?

disable_irq(priv->irq) waits for the threaded handler w5100_interrupt() to
finish, but any rx_work it already queued is still pending, and
priv->xfer_wq is created with WQ_MEM_RECLAIM | WQ_PERCPU (no WQ_FREEZABLE),
so it keeps running during suspend. w5100_rx_work() does SPI register
accesses (S0_RX_RD writes and S0_CR_RECV) and ends with:

	w5100_enable_intr(priv);

which re-arms the chip interrupt masks that w5100_hw_close() cleared, while
the host irq stays masked.

setrx_work runs on system_wq and calls w5100_hw_start(), re-issuing
S0_CR_OPEN and w5100_enable_intr() after suspend closed socket 0. tx_work
can still issue S0_CR_SEND. Any of these transfers issued after the suspend
callback returns are rejected once the SPI controller itself is suspended.

There is also a re-queue window: restart_work is cancelled here, before
netif_device_detach() below. In between, dev_watchdog() can still fire
(netif_device_present(), netif_running() and netif_carrier_ok() all hold,
and carrier_ok() is permanently true on w5100/w5200 and on a W5500 without a
link irq):

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

The re-queued w5100_restart() then passes its own netif_running() /
netif_device_present() test and runs w5100_hw_reset() plus w5100_hw_start()
against the device suspend is closing.

Should netif_device_detach() come before the cancel, and should setrx_work,
rx_work and tx_work be drained here as they are in w5100_stop()?

> +
> +		if (priv->link_irq > 0) {
> +			mutex_lock(&priv->link_lock);
> +			netif_carrier_off(ndev);
> +			mutex_unlock(&priv->link_lock);
> +		}
> +
>  		netif_device_detach(ndev);
>  
>  		w5100_hw_close(priv);
> @@ -854,9 +1024,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;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-wiznet-link-gpio-v9-0-5536f24d8d6d%40marmottus.net

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

[Arthur Crepin Leblond]

On Tue, Sep 22, 2026 at 03:43:26PM +0000, netdev-bot+sashiko@kernel.org wrote:
>[Severity: Medium]
> ...
>What is the intended split here - should w5100_tx_timeout() always queue
>restart_work now that w5100_restart() can sleep, or should the irq
>masking and the carrier re-check be done only in the may_sleep path?

schedule_work() will always be called (see v10).

>
>[Severity: Medium]
>The link irq is requested below with IRQF_NO_AUTOEN and
>IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, so it stays masked until this
>enable_irq(). PHYCFGR is sampled first and edge detection is only turned on
>afterwards:
>
>	w5500_report_carrier_state(ndev);	/* reads PHYCFGR */
>	enable_irq(priv->link_irq);		/* edge detection starts */
>
>If the PHY link comes up between those two steps, can that transition be
>lost for good? On the BCM2835 controller used in the binding's own example,
>masking disables edge detection and also discards latched events:
>
>drivers/pinctrl/bcm/pinctrl-bcm2835.c:
>	bcm2835_gpio_irq_config(pc, gpio, false);
>	/* Clear events that were latched prior to clearing event sources */
>	bcm2835_gpio_set_bit(pc, GPEDS0, gpio);
>
>so unmasking does not replay the missed edge, and with a now stable LINKLED
>level the driver keeps reporting carrier off until the next physical link
>change.
>
>w5100_restart() and w5100_resume() use the same order, and restart follows a
>chip reset with PHY re-negotiation. The changelog lists this ordering as
>intentional ("enable the link irq after checking PHYCFGR") - would enabling
>the irq first and then reading PHYCFGR close the window?

Probably very unlikely to happen and more of an edge case but I can
enable link irq first.

>[Severity: High]
>flush_work(&priv->rx_work) now runs while socket 0 is still open, because
>w5100_hw_close() is called after it.
>
>w5100_rx_work() drains without a budget or a shutdown test:
>
>	while ((skb = w5100_rx_skb(priv->ndev)))
>		netif_rx(skb);
>
>and w5100_rx_skb() only returns NULL once the hardware receive buffer is
>observed empty:
>
>	u16 rx_buf_len = w5100_read16(priv, W5100_S0_RX_RSR(priv));
>
>	if (rx_buf_len == 0)
>		return NULL;
>
>disable_irq(priv->irq) masks the host interrupt but does not stop the
>controller from filling its receive buffer. Each frame is drained with
>several synchronous SPI transfers, which is slower than line rate, so can
>sustained incoming traffic keep RX_RSR non-zero and make this flush_work()
>never return?
>
>ndo_stop runs with rtnl held, so an ip link set down would then stall all
>network configuration in an unkillable wait. w5100_remove() reaches the same
>path through unregister_netdev().
>
>Would calling w5100_hw_close() before flushing rx_work 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);
>> +	}
>> +
>>  	netif_stop_queue(ndev);
>>  	napi_disable(&priv->napi);
>>  	return 0;
>>  }
>

Agreed, w5100_hw_close() should be called earlier.

>[Severity: Medium]
>The changelog states:
>
>  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 cancels restart_work here. setrx_work (queued with
>schedule_work() from w5100_set_rx_mode()), rx_work (queued on priv->xfer_wq
>from w5100_interrupt()) and tx_work (queued from w5100_start_tx()) are
>neither cancelled nor flushed, while w5100_stop() drains all four. Should
>the changelog be narrowed, or the suspend path made to match stop()?
>
>[Severity: High]
>Independently of the changelog wording, can these work items still touch the
>hardware after suspend has closed it?
>
>disable_irq(priv->irq) waits for the threaded handler w5100_interrupt() to
>finish, but any rx_work it already queued is still pending, and
>priv->xfer_wq is created with WQ_MEM_RECLAIM | WQ_PERCPU (no WQ_FREEZABLE),
>so it keeps running during suspend. w5100_rx_work() does SPI register
>accesses (S0_RX_RD writes and S0_CR_RECV) and ends with:
>
>	w5100_enable_intr(priv);
>
>which re-arms the chip interrupt masks that w5100_hw_close() cleared, while
>the host irq stays masked.
>
>setrx_work runs on system_wq and calls w5100_hw_start(), re-issuing
>S0_CR_OPEN and w5100_enable_intr() after suspend closed socket 0. tx_work
>can still issue S0_CR_SEND. Any of these transfers issued after the suspend
>callback returns are rejected once the SPI controller itself is suspended.
>
>There is also a re-queue window: restart_work is cancelled here, before
>netif_device_detach() below. In between, dev_watchdog() can still fire
>(netif_device_present(), netif_running() and netif_carrier_ok() all hold,
>and carrier_ok() is permanently true on w5100/w5200 and on a W5500 without a
>link irq):
>
>w5100_tx_timeout()
>	if (priv->ops->may_sleep)
>		schedule_work(&priv->restart_work);
>
>The re-queued w5100_restart() then passes its own netif_running() /
>netif_device_present() test and runs w5100_hw_reset() plus w5100_hw_start()
>against the device suspend is closing.
>
>Should netif_device_detach() come before the cancel, and should setrx_work,
>rx_work and tx_work be drained here as they are in w5100_stop()?

suspend() and resume() will call w5100_stop() and w5100_open()
directly.
And yes, netif_device_detach() will be called before.

Arthur