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

[PATCH net-next v11 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>
---
v10 -> v11:
 - Fix line over 80 columns
 - Update commit message
 - Enable the link irq after reporting the carrier state
 - Call w5100_hw_close() earlier before cancelling/flushing the queues
   in w5100_stop()
 - Call w5100_stop() in w5100_suspend()
 - Call w5100_open() in w5100_resume()
 - Issue a reset w5100_open()
 - Call netif_stop_queue() earlier in w5100_stop() to avoid race
   condition
 - Prevent w5100_rx_work() from re-enabling the interrupt
 - Add a retry mechanism when reading PHYCFGR
 - Protect netif_device_attach|detach behind a mutex

Link to v10: https://patch.msgid.link/20260921-wiznet-link-gpio-v10-0-5874a7a147a2@marmottus.net
Sashiko: https://sashiko.dev/#/patchset/20260921-wiznet-link-gpio-v10-0-5874a7a147a2@marmottus.net
Clashiko: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-wiznet-link-gpio-v10-0-5874a7a147a2%40marmottus.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
Clashiko: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918-wiznet-link-gpio-v9-0-5536f24d8d6d%40marmottus.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                | 234 ++++++++++++++++++---
 4 files changed, 299 insertions(+), 80 deletions(-)
---
base-commit: 528de6832b2194ae0b1d62b0925e0ac6cad1087c
change-id: 20260804-wiznet-link-gpio-796f36cd14f9

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

Add wiznet,w5100.yaml entry in MAINTAINERS.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
v11:
 - Add a line about MAINTAINERS changes in commit message
 - Describe the entry in MAINTAINERS as BINDINGS
 - Reorder example properties to pass dt-check-style in strict mode

v10:
 - No changes

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..df83f90b3919
--- /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>;
+            interrupt-parent = <&gpio>;
+            interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+            local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
+            pinctrl-0 = <&eth1_pins>;
+            pinctrl-names = "default";
+            spi-max-frequency = <30000000>;
+        };
+    };
+...
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 e3ce77c839b0..42acc7bb7bfd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29451,6 +29451,12 @@ M:	Miloslav Trmac <mitr@volny.cz>
 S:	Maintained
 F:	drivers/input/misc/wistron_btns.c
 
+WIZNET W5100 ETHERNET BINDINGS
+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 v11 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] In Documentation/devicetree/bindings/net/wiznet,w5100.yaml, `spi-
  max-frequency` has a single `maximum: 80000000` that applies to all…

--

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

This commit converts the Wiznet w5x00 SPI Ethernet controller binding
from plain text to DT schema. It also changes the example interrupt
trigger type from edge-falling to level-low to match the driver, and
adds a MAINTAINERS entry for the new YAML file.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> new file mode 100644
> index 000000000000..df83f90b3919
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml

[ ... ]

> +allOf:
> +  - $ref: /schemas/net/ethernet-controller.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - wiznet,w5100
> +      - wiznet,w5200
> +      - wiznet,w5500

[ ... ]

> +  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 gives w5100 a lower limit of about 14 MHz, but the only
constraint here is maximum: 80000000, and it applies to all three
compatibles. Should the schema enforce the w5100 limit as well?

The allOf above only holds the two $refs. Nothing like this lowers the
limit for w5100:

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

As a result, a node with compatible = "wiznet,w5100" and, for example,
the 30 MHz value from the example would pass dt_binding_check and
dtbs_check.

The driver does not clamp the rate either. w5100_spi_probe() picks
w5100_spi_ops from the match data and passes the device on to
w5100_probe(), and the SPI core uses the max_speed_hz from DT as is.

The same gap remains at the end of the series. The allOf if/then block
added by "dt-bindings: net: wiznet,w5100: add link status interrupt"
only restricts interrupts and does not touch spi-max-frequency.

The old text binding had no limit at all, so this is not a regression
from the conversion. If the 14 MHz figure is left unenforced on purpose
because it comes from user reports rather than the datasheet, could the
binding say so?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-wiznet-link-gpio-v11-0-42a332517b30%40marmottus.net

[PATCH net-next v11 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 df83f90b3919..b0d1e1d699dc 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:
@@ -58,7 +77,8 @@ examples:
             compatible = "wiznet,w5500";
             reg = <0>;
             interrupt-parent = <&gpio>;
-            interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+            interrupts = <25 IRQ_TYPE_LEVEL_LOW>,
+                         <24 IRQ_TYPE_EDGE_BOTH>;
             local-mac-address = [ a0 b0 c0 d0 e0 f0 ];
             pinctrl-0 = <&eth1_pins>;
             pinctrl-names = "default";

-- 
2.55.0

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 52 bytes --]

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v11 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] The new allOf conditional in
  Documentation/devicetree/bindings/net/wiznet,w5100.yaml (`if: not:…

--

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

This patch adds an optional second interrupt to the wiznet,w5100 binding.
The second interrupt is meant to be wired to the LINKLED pin so link
status changes can be detected. A new allOf conditional limits every
compatible except wiznet,w5500 to a single interrupt.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> index df83f90b3919..b0d1e1d699dc 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).

[ ... ]

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

[Severity: Low]
Is this restriction based on a hardware difference, or does it follow the
driver implementation?

As far as I can tell, the w5200 (and the w5100) also have a LINKLED
output. The w5200 also has a readable link bit in PHYSTATUS, so the same
"LINKLED edge, then read the link register" approach looks like it would
work there too. I haven't been able to confirm this against the
datasheets, so please correct me if that's wrong.

In the follow-up driver patch, w5100_probe() only reads the second
interrupt on the W5500:

drivers/net/ethernet/wiznet/w5100.c:w5100_probe() {
    ...
	if (ops->chip_id == W5500) {
		priv->link_irq = fwnode_irq_get(dev_fwnode(dev), 1);
    ...
}

The older link_gpio / w5100_detect_link() logic that commit dacf281771a9
("w5100: remove unused gpio link detection") removed was not tied to a
particular chip, so link status wiring was not limited to the w5500
before.

With this patch, a w5200 board that routes LINKLED to a SoC interrupt
line would fail dtbs_check with "too many items". The w5100 seems to
have no link-status register, so the case is weaker there, but the w5200
case still seems to apply.

The commit message only says:

  "This second interrupt is only supported on wiznet,w5500."

The item description says "LINKLED pin on w5500". Could the commit
message name the hardware reason, or could the conditional be relaxed
to cover the w5200?

A smaller point: because the conditional uses "not: contains:", any
WIZnet compatible added later will also get maxItems: 1 by default.

> +
>  unevaluatedProperties: false
>  
>  examples:

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-wiznet-link-gpio-v11-0-42a332517b30%40marmottus.net

[PATCH net-next v11 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 (netif_carrier_(on|off) is not
called).

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, get_link() is not
implemented and the carrier state is not reported
(netif_carrier_(on|off) is not called).
The interrupt handler and the PHYCFGR register readings are
synchronized using a mutex.

A few changes are also brought for the bring-up/teardown
synchronization:

 - 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_stop() calls cancel_work_sync() and flush_work() to make sure
   there is no race condition or unfinished work
 - w5100_suspend() calls w5100_stop()
 - w5100_resume() calls w5100_open()
 - The main irq is not automatically enabled in w5100_probe() but in
   w5100_open() and disabled in w5100_stop()

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.

Set the WIZNET entry as DRIVER and add drivers/net/ethernet/wiznet in
MAINTAINERS file.

Signed-off-by: Arthur Crepin Leblond <arthur@marmottus.net>
---
v11:
 - Fix line over 80 columns
 - Update commit message
 - Enable the link irq after reporting the carrier state
 - Call w5100_hw_close() earlier before cancelling/flushing the queues
   in w5100_stop()
 - Call w5100_stop() in w5100_suspend()
 - Call w5100_open() in w5100_resume()
 - Issue a reset w5100_open()
 - Call netif_stop_queue() earlier in w5100_stop() to avoid race
   condition
 - Prevent w5100_rx_work() from re-enabling the interrupt
 - Add a retry mechanism when reading PHYCFGR
 - Protect netif_device_attach|detach behind a mutex

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                         |   3 +-
 drivers/net/ethernet/wiznet/w5100.c | 234 +++++++++++++++++++++++++++++++-----
 2 files changed, 206 insertions(+), 31 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 42acc7bb7bfd..e6028e8e564c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29451,11 +29451,12 @@ M:	Miloslav Trmac <mitr@volny.cz>
 S:	Maintained
 F:	drivers/input/misc/wistron_btns.c
 
-WIZNET W5100 ETHERNET BINDINGS
+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
+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..dff39ba8103c 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,82 @@ 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 retries = 3;
+	int ret;
+
+	do {
+		ret = w5100_read(priv, W5500_PHYCFGR);
+		if (ret < 0)
+			netif_err(priv, link, ndev,
+				  "failed to read link status: %d\n", ret);
+	} while (ret < 0 && --retries);
+
+	if (ret < 0)
+		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 +531,28 @@ 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_close(priv);
+	flush_work(&priv->rx_work);
+	flush_work(&priv->tx_work);
 	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) {
+		enable_irq(priv->link_irq);
+		w5500_report_carrier_state(ndev);
+	}
 }
 
 static void w5100_restart_work(struct work_struct *work)
@@ -468,10 +567,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)
@@ -564,7 +660,8 @@ static void w5100_rx_work(struct work_struct *work)
 	while ((skb = w5100_rx_skb(priv->ndev)))
 		netif_rx(skb);
 
-	w5100_enable_intr(priv);
+	if (netif_running(priv->ndev))
+		w5100_enable_intr(priv);
 }
 
 static int w5100_napi_poll(struct napi_struct *napi, int budget)
@@ -656,9 +753,17 @@ static int w5100_open(struct net_device *ndev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	netif_info(priv, ifup, ndev, "enabling\n");
+	w5100_hw_reset(priv);
+	enable_irq(priv->irq);
 	w5100_hw_start(priv);
 	napi_enable(&priv->napi);
 	netif_start_queue(ndev);
+
+	if (priv->link_irq > 0) {
+		enable_irq(priv->link_irq);
+		w5500_report_carrier_state(ndev);
+	}
+
 	return 0;
 }
 
@@ -667,13 +772,40 @@ static int w5100_stop(struct net_device *ndev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	netif_info(priv, ifdown, ndev, "shutting down\n");
-	w5100_hw_close(priv);
-	netif_carrier_off(ndev);
+
+	disable_irq(priv->irq);
+	if (priv->link_irq > 0)
+		disable_irq(priv->link_irq);
+
 	netif_stop_queue(ndev);
 	napi_disable(&priv->napi);
+
+	cancel_work_sync(&priv->restart_work);
+	cancel_work_sync(&priv->setrx_work);
+
+	w5100_hw_close(priv);
+
+	flush_work(&priv->rx_work);
+	flush_work(&priv->tx_work);
+
+	if (priv->link_irq > 0)  {
+		mutex_lock(&priv->link_lock);
+		netif_carrier_off(ndev);
+		mutex_unlock(&priv->link_lock);
+	}
+
 	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 +853,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 +879,27 @@ 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 = priv->link_irq > 0 ? &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 +907,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 +936,45 @@ 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 +985,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);
+	cancel_work_sync(&priv->setrx_work);
+	cancel_work_sync(&priv->restart_work);
+	flush_work(&priv->rx_work);
+	flush_work(&priv->tx_work);
 
-	unregister_netdev(ndev);
+	w5100_hw_reset(priv);
+
+	destroy_workqueue(priv->xfer_wq);
+	mutex_destroy(&priv->link_lock);
 	free_netdev(ndev);
 }
 EXPORT_SYMBOL_GPL(w5100_remove);
@@ -839,10 +1011,11 @@ static int w5100_suspend(struct device *dev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	if (netif_running(ndev)) {
-		netif_carrier_off(ndev);
+		mutex_lock(&priv->link_lock);
 		netif_device_detach(ndev);
+		mutex_unlock(&priv->link_lock);
 
-		w5100_hw_close(priv);
+		w5100_stop(ndev);
 	}
 	return 0;
 }
@@ -853,10 +1026,11 @@ static int w5100_resume(struct device *dev)
 	struct w5100_priv *priv = netdev_priv(ndev);
 
 	if (netif_running(ndev)) {
-		w5100_hw_reset(priv);
-		w5100_hw_start(priv);
+		w5100_open(ndev);
 
+		mutex_lock(&priv->link_lock);
 		netif_device_attach(ndev);
+		mutex_unlock(&priv->link_lock);
 	}
 	return 0;
 }

-- 
2.55.0

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

[netdev-bot+sashiko]

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

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

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] This patch quietly fixes a user-visible regression that commit
  dacf281771a9 ("w5100: remove unused gpio link detection") introduced.
- [Medium] In w5100_stop(), netif_stop_queue() runs before
  cancel_work_sync(&priv->restart_work).
- [Medium] w5100_open() now calls w5100_hw_reset() but ignores its return
  value.
- [Medium] w5100_suspend() now calls w5100_stop() (disable_irq(irq),
  disable_irq(link_irq), napi_disable()) but leaves __LINK_STATE_START…
- [Low] When all three PHYCFGR reads in w5500_get_phycfgr_lnk() fail,
  w5500_report_carrier_state() leaves carrier unchanged, and…

Pre-existing issues:
- [Low] Claim: making w5100_tx_timeout() always schedule restart_work
  removes the xmit exclusion that dev_watchdog() (tx_global_lock plus…

--

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

This commit reads the w5500 PHYCFGR link status bit to detect carrier. It
adds an optional second interrupt, wired to the LINKLED pin, that triggers
carrier updates. It also reworks bring-up and teardown: register_netdev()
moves to the end of probe, the main irq is only enabled in open, and
suspend/resume now call w5100_stop()/w5100_open().

> On w5100/w5200, which lack this register, get_link() is not implemented
> and the carrier state is not reported (netif_carrier_(on|off) is not
> called).

[Severity: Medium]
Does this patch also fix a regression introduced by commit dacf281771a9
("w5100: remove unused gpio link detection")?

That commit removed the only netif_carrier_on() calls in the driver, in
w5100_open() and w5100_resume(). It kept netif_carrier_off() in
w5100_stop() and w5100_suspend(). At the baseline there is no
netif_carrier_on() anywhere in drivers/net/ethernet/wiznet/.

As a result, on every chip the first ifdown or suspend leaves
__LINK_STATE_NOCARRIER set. The interface then stays NO-CARRIER after the
next ifup or resume.

This patch makes the netif_carrier_off() calls in stop and suspend
conditional on link_irq > 0, which fixes that. However, the commit message
reads as though nothing changes for devices without a link irq, and there
is no Fixes: tag.

Could the commit message describe this as a fix and add:

Fixes: dacf281771a9 ("w5100: remove unused gpio link detection")

That way stable kernels that carry dacf281771a9 would pick it up.

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

[ ... ]

> @@ -345,6 +352,82 @@ static void w5500_memory_configure(struct w5100_priv *priv)

[ ... ]

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

[Severity: Low]
What happens if all three PHYCFGR reads in w5500_get_phycfgr_lnk() fail on
the link-up edge?

In that case the carrier stays off, and w5500_detect_link_interrupt()
returns IRQ_HANDLED without scheduling a retry. A stable link produces no
further LINKLED edge, so the carrier can stay down even after SPI recovers.

w5500_get_link() might then report the link as up through ethtool, but it
doesn't update the carrier. dev_watchdog() also won't start the restart
path while the carrier is off. The only ways to recover seem to be a cable
replug or ifdown/ifup.

These errors can really happen: w5500_spi_read() passes on errors from
spi_write_then_read(), including -ENOMEM from its bounce buffer allocation.

Should a failed read schedule a later re-read of the link state?

[ ... ]

> @@ -448,12 +531,28 @@ 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_close(priv);
> +	flush_work(&priv->rx_work);
> +	flush_work(&priv->tx_work);
>  	w5100_hw_reset(priv);

[Severity: Low]
This isn't a bug introduced by this patch, but netif_stop_queue() doesn't
wait for an ndo_start_xmit that is already running. Can w5100_start_tx()
still queue tx_work after flush_work(&priv->tx_work) here?

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

If it can, tx_work could issue SPI transfers at the same time as
w5100_hw_reset() during tx timeout recovery. The baseline w5100_restart()
had no flush at all, so this patch narrows the window rather than opening
it.

Always scheduling restart_work in w5100_tx_timeout() doesn't change
behaviour for in-tree users. Every w5100_ops in w5100-spi.c already sets
may_sleep = true.

[ ... ]

> @@ -656,9 +753,17 @@ static int w5100_open(struct net_device *ndev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	netif_info(priv, ifup, ndev, "enabling\n");
> +	w5100_hw_reset(priv);
> +	enable_irq(priv->irq);
>  	w5100_hw_start(priv);

[Severity: Medium]
Should the return value of w5100_hw_reset() be checked here?

w5100_hw_reset() returns -ENODEV when the RTR read-back doesn't match. This
is its only check that the chip came out of reset and is responding:

	if (w5100_read16(priv, rtr) != RTR_DEFAULT)
		return -ENODEV;

Even if the reset fails, w5100_open() does all of the following and still
returns 0:

- enables the level-low main irq
- calls w5100_hw_start(), which opens the socket and unmasks its interrupt
- enables the link irq and reports carrier from PHYCFGR

w5100_resume() now calls w5100_open() and also ignores its result. So a
failed reset after resume isn't reported either, and netif_device_attach()
still marks the interface as present.

[ ... ]

> @@ -667,13 +772,40 @@ static int w5100_stop(struct net_device *ndev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	netif_info(priv, ifdown, ndev, "shutting down\n");
> -	w5100_hw_close(priv);
> -	netif_carrier_off(ndev);
> +
> +	disable_irq(priv->irq);
> +	if (priv->link_irq > 0)
> +		disable_irq(priv->link_irq);
> +
>  	netif_stop_queue(ndev);
>  	napi_disable(&priv->napi);
> +
> +	cancel_work_sync(&priv->restart_work);

[Severity: Medium]
Can a restart_work that is already running wake the queue again after the
netif_stop_queue() above?

w5100_restart() checks netif_running() and netif_device_present() only on
entry, and it always ends with netif_wake_queue(). w5100_tx_timeout()
schedules restart_work on system_percpu_wq, which isn't freezable:

w5100_suspend()
  netif_device_detach()
  w5100_stop()
    netif_stop_queue()
    cancel_work_sync(&priv->restart_work)   <- waits

w5100_restart() (already past its entry check)
  ...
  netif_wake_queue()                        <- clears XOFF

After suspend returns, the TX queue is awake on a detached device.

The suspend path doesn't deactivate the qdisc, and w5100_start_tx() doesn't
check netif_device_present(). A later transmit can therefore queue tx_work
on xfer_wq. xfer_wq isn't WQ_FREEZABLE, so this would issue SPI transfers
after the suspend callback has completed.

The commit message says w5100_stop() makes sure "there is no race condition
or unfinished work". Should the queue be stopped again after
cancel_work_sync() returns?

[ ... ]

> @@ -839,10 +1011,11 @@ static int w5100_suspend(struct device *dev)
>  	struct w5100_priv *priv = netdev_priv(ndev);
>  
>  	if (netif_running(ndev)) {
> -		netif_carrier_off(ndev);
> +		mutex_lock(&priv->link_lock);
>  		netif_device_detach(ndev);
> +		mutex_unlock(&priv->link_lock);
>  
> -		w5100_hw_close(priv);
> +		w5100_stop(ndev);
>  	}
>  	return 0;
>  }

[Severity: Medium]
What happens if the core calls ndo_stop between this suspend and the
matching resume?

Here w5100_stop() disables both irqs and calls napi_disable(). However,
__LINK_STATE_START stays set, and suspend takes neither rtnl nor the netdev
lock.

A kernel-initiated dev_close() can run during the suspend window, for
example from cleanup_net() on the non-freezable netns workqueue. In that
case w5100_stop() runs a second time:

__dev_close_many()
  w5100_stop()
    disable_irq(priv->irq)          <- disable depth now 2
    napi_disable(&priv->napi)
      napi_disable_locked()         <- SCHED/NPSVC still set from the
                                       first napi_disable()

napi_disable_locked() would then spin forever in usleep_range() while
holding rtnl.

After that, netif_running() is false, so w5100_resume() skips w5100_open().
The irq disable depth stays raised, and a later ifup leaves both irqs
disabled.

Before this patch, suspend only called w5100_hw_close(), which touched
neither NAPI nor the irq disable depth.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-wiznet-link-gpio-v11-0-42a332517b30%40marmottus.net