mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: [PATCH net-next v1 2/5] net: lan743x: start phylink last and stop it first
@ 2026-10-19 16:34 netdev-bot+sashiko
  0 siblings, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-19 16:34 UTC (permalink / raw)
  To: cbabroski
  Cc: bryan.whitehead, UNGLinuxDriver, andrew+netdev, davem, edumazet,
	kuba, pabeni, netdev, linux-kernel, richardcochran, linux,
	maxime.chevallier, Raju.Lakkaraju, davthompson

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

Critical: 0 · High: 2 (2) · Medium: 0 · 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] Moving phylink_stop() to the start of lan743x_netdev_close()
  relies on mac_link_down() (lan743x_phylink_mac_link_down ->…
- [High] On the suspend path, lan743x_pcidev_shutdown() calls
  netif_device_detach() (lockless netif_tx_stop_all_queues) and then…

Pre-existing issues:
- [High] On LAN7431/PCI11x1x boards with no DT node and no PHY on the
  MDIO bus, every open after the first one fails.
- [High] lan743x_pm_resume() ignores the return value of
  lan743x_netdev_open() (line 4063) and always calls…

--

net: lan743x: start phylink last and stop it first

This patch moves lan743x_phylink_connect() to the end of
lan743x_netdev_open() and lan743x_phylink_disconnect() to the start of
lan743x_netdev_close(). The goal is that mac_link_up() can no longer wake
the TX queues while the rings are missing. It also changes the open error
path, which used to leave phylink started and the PHY attached when
lan743x_ptp_open() failed.

> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 9e1276cf313c8..583f631df701b 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -3334,6 +3334,8 @@ static int lan743x_netdev_close(struct net_device *netdev)
>  	struct lan743x_adapter *adapter = netdev_priv(netdev);
>  	int index;
>  
> +	lan743x_phylink_disconnect(adapter);
> +
>  	for (index = 0; index < adapter->used_tx_channels; index++)
>  		lan743x_tx_close(&adapter->tx[index]);

[Severity: High]
Can a TX queue get woken again after this point and stay awake while the
rings are freed?

With phylink_stop() moved here, the only thing keeping the queues stopped
during teardown is the netif_tx_stop_all_queues() call in
lan743x_phylink_mac_link_down(). TX NAPI stays live until napi_disable()
in lan743x_tx_close(), and lan743x_tx_napi_poll() wakes the queue whenever
it reclaims descriptors:

lan743x_tx_napi_poll() {
    ...
		if (tx->rqd_descriptors) {
			if (tx->rqd_descriptors <=
			    lan743x_tx_get_avail_desc(tx)) {
				tx->rqd_descriptors = 0;
				netif_tx_wake_queue(txq);
			}
		} else {
			netif_tx_wake_queue(txq);
		}
    ...
}

On the suspend path the qdisc is still attached:

lan743x_pm_suspend()
  lan743x_pcidev_shutdown()
    netif_device_detach()
    lan743x_netdev_close()
      lan743x_phylink_disconnect()   /* queues stopped */
      <TX completion on a channel not yet closed>
        lan743x_tx_napi_poll()
          netif_tx_wake_queue()
      lan743x_tx_close()             /* ring freed */

Could ndo_start_xmit() then reach lan743x_tx_xmit_frame() on the freed
ring?

Before this patch, the phylink_stop() at the end of close stopped the
queues again after the rings were gone (when the link was up). After this
patch, nothing stops a re-woken queue for the rest of suspend.

This looks to be fixed later in the series by "net: lan743x: quiesce TX
before freeing the rings". That commit adds a netif_device_present() check
before the NAPI wake, and adds synchronize_net() plus netif_tx_disable()
after lan743x_phylink_disconnect() in lan743x_netdev_close().

[Severity: High]
Can a transmit that is already in flight also race with the ring
teardown here?

netif_tx_stop_all_queues() in lan743x_phylink_mac_link_down() only sets
DRV_XOFF, just like netif_device_detach(). Neither one waits for an
ndo_start_xmit() that is already running on another CPU under the txq lock
and tx->ring_lock.

lan743x_tx_close()->lan743x_tx_ring_cleanup() then frees head_cpu_ptr,
buffer_info and ring_cpu_ptr without taking tx->ring_lock or the txq lock:

CPU1                                  CPU0
sch_direct_xmit()                     lan743x_pcidev_shutdown()
  lan743x_netdev_xmit_frame()           lan743x_netdev_close()
    lan743x_tx_xmit_frame()               lan743x_tx_close()
      /* uses tx->ring_cpu_ptr */           lan743x_tx_ring_cleanup()

The commit message says that on the suspend path ndo_start_xmit() can run
on a ring that "has just been freed", but the patch only removes the
link-up wake source. Is the in-flight case meant to be covered here too?

This also appears to be fixed later in the series by "net: lan743x:
quiesce TX before freeing the rings", which adds synchronize_net() and
netif_tx_disable() right after lan743x_phylink_disconnect().
netif_tx_disable() takes each txq lock, so it waits for in-flight
transmits.

[ ... ]

> @@ -3387,6 +3383,10 @@ static int lan743x_netdev_open(struct net_device *netdev)
>  			goto close_tx;
>  	}
>  
> +	ret = lan743x_phylink_connect(adapter);
> +	if (ret)
> +		goto close_tx;
> +

[Severity: High]
This is a pre-existing issue, but does every open after the first one
fail on LAN7431/PCI11x1x boards that have no DT node and no PHY on the
MDIO bus?

In that case lan743x_phylink_connect() takes the fallback branch and calls
phylink_set_fixed_link() on every open:

lan743x_phylink_connect() {
    ...
			ret = phylink_set_fixed_link(adapter->phylink, &state);
			if (ret) {
				netdev_err(dev, "Could not set fixed link\n");
				return ret;
			}
    ...
}

phylink_set_fixed_link() only succeeds while the mode is still
MLO_AN_PHY, and it changes the mode for good when it succeeds:

phylink_set_fixed_link() {
	if (pl->cfg_link_an_mode != MLO_AN_PHY || !state ||
	    !test_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state))
		return -EINVAL;
    ...
	pl->cfg_link_an_mode = MLO_AN_FIXED;
    ...
}

lan743x_phylink_disconnect() only calls phylink_stop() and
phylink_disconnect_phy(), and phylink_create() only runs at probe. Nothing
resets cfg_link_an_mode.

So the second ndo_open after ifdown/ifup, and every
lan743x_pm_resume()->lan743x_netdev_open(), would fail with "Could not set
fixed link" (-EINVAL).

Boards with a DT node seem fine, because phylink_fwnode_phy_connect()
returns 0 in fixed mode. The commit message covers a different "every
later open then fails" case (the PHY left attached), not this one.

[Severity: High]
This isn't a bug introduced by this patch, but what happens on resume
when this goto close_tx is taken?

lan743x_pm_resume() ignores the return value and attaches the device
anyway:

lan743x_pm_resume() {
    ...
	if (netif_running(netdev))
		lan743x_netdev_open(netdev);

	netif_device_attach(netdev);
	rtnl_unlock();
    ...
}

The device is still running, so netif_device_attach() calls
netif_tx_wake_all_queues(). That leaves the interface IFF_UP with awake
queues and no rings. Every TX/RX NAPI is disabled and deleted, with
NAPIF_STATE_SCHED and NPSVC still set.

A later ifdown, a second suspend, or a reboot through
lan743x_pcidev_shutdown() then runs:

lan743x_netdev_close()
  lan743x_tx_close()
    napi_disable()
      napi_disable_locked()

which waits on:

		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}

__netif_napi_del_locked() does not clear SCHED, so can this loop run
forever with rtnl held?

A transmit path that bypasses the qdisc and does not check carrier, such
as netpoll, could also reach lan743x_tx_xmit_frame() while ring_cpu_ptr
and buffer_info are NULL.

Together with the fixed-link -EINVAL above, every resume on fixed-link
boards without a DT node would hit this. It still seems to be present at
the end of the series. The lan743x_netdev_stop() wrapper added later only
skips close when !netif_device_present(), and resume has already
re-attached the device by then.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1792008404.git.cbabroski%40nvidia.com

^ permalink raw reply	[flat|nested] 3+ messages in thread
* [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang
@ 2026-10-15 15:00 Chris Babroski
  2026-10-15 15:00 ` [PATCH net-next v1 2/5] net: lan743x: start phylink last and stop it first Chris Babroski
  0 siblings, 1 reply; 3+ messages in thread
From: Chris Babroski @ 2026-10-15 15:00 UTC (permalink / raw)
  To: bryan.whitehead, UNGLinuxDriver, andrew+netdev, davem, edumazet,
	kuba, pabeni, netdev, linux-kernel, richardcochran, linux
  Cc: maxime.chevallier, Raju.Lakkaraju, cbabroski, davthompson

Background
----------
On a custom HW platform that uses the LAN7430 NIC we have been able to
reproduce a scenario where the SoC stops receiving TX completion
interrupts, appearing as a TX hang. After dumping and decoding the
LAN7430 registers we see that the TX channel reports that it is in
started state with no error, but TX_HEAD stops advancing. TX MSI-X stops
incrementing while RX interrupts continue to increment.

This scenario has most often been reproduced in large server cluster
environments, but it's also been seen locally during repeated SCP and
iperf testing.

The root cause of this issue is still unknown and we are continuing
debug. It was found that bringing the interface down and up recovers the
network. This issue has not been reproduced with the LAN7430 EVB
connected to a standard Linux server.

What this patch series does
---------------------------
This patch series attempts to implement automatic recovery of the
network interface after a TX hang is detected. It does not fix the
underlying root cause of the TX hang.

At a high level, the patches implement the following:
1. ndo_tx_timeout support to reset and recover the interface when the
   netdev TX watchdog triggers.
2. BQL support to decrease the amount of time it takes for the watchdog
   to trigger.
3. Stability improvements and error handling around the open/close paths
   that now get executed more frequently as part of the TX hang recovery
   flow.

Potential issues and concerns
-----------------------------
* One of the patches adds new lan743x_dmac_tx_reset and
  lan743x_dmac_rx_reset helper functions. There is no cleanup handling
  introduced if these return an error, only a new error log. This
  matches pre-existing behavior: if a reset timeout occurs then it is
  possible to DMA freed memory. The datasheet states that these resets
  are performed immediately in HW so I'm not sure if these will ever
  time out, but I'm open to suggestions on how we should handle an error
  in these cases.
* The TX timeout reset flow does a full lan743x_netdev_close followed by
  lan743x_netdev_open instead of individual TX channel reset. If the
  stall is happening inside the NIC DMA engine then the theory is that a
  full reset may be needed to recover. For internal use we also wanted
  to reduce risk by putting the NIC into a clean state after recovery
  with full reset. It would be great to have Microchip/LAN743x experts
  confirm the best reset flow.
* LLM review tools flagged several possible bugs during development
  that resulted in the inclusion of three "Fixes:" tagged patches in this
  series. They are included in this series with the ndo_tx_timeout
  support because lan743x_netdev_open and lan743x_netdev_close are now
  called more frequently as a result of TX hang recovery.

Testing
-------
Testing was performed on a 7.0 based kernel using the same driver code.
Please let me know if you would like me to run any other tests.

Tests:
* Verified the TX timeout watchdog triggers and recovers the network
  interface when TX hang is reproduced. This was done as part of
  repeated SCP loop testing and is also being tested in a server cluster
  environment. dmesg was inspected for "NETDEV WATCHDOG" to verify that
  the watchdog is being triggered. No errors were seen in dmesg after
  reset.
* Performance tests using iperf3 to measure RX and TX throughput show no
  noticeable difference with the patches applied.
* Repeated module load and unload cycles produce no errors in dmesg.
  Network appears to function as expected when module is loaded.
* Repeated ifconfig down and up cycles produce no errors in dmesg.
  Network appears to function as expected when interface is back up.

Chris Babroski (5):
  net: lan743x: reset DMA channels that fail to stop
  net: lan743x: start phylink last and stop it first
  net: lan743x: quiesce TX before freeing the rings
  net: lan743x: reset the device on TX timeout
  net: lan743x: add byte queue limits support

 drivers/net/ethernet/microchip/lan743x_main.c | 177 +++++++++++++++---
 drivers/net/ethernet/microchip/lan743x_main.h |   2 +
 2 files changed, 155 insertions(+), 24 deletions(-)

base-commit: 05b5019b01fb9443b09ae4ed34af1135a80a84c8
-- 
2.34.1


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-19 16:34 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-19 16:34 [PATCH net-next v1 2/5] net: lan743x: start phylink last and stop it first netdev-bot+sashiko
  -- strict thread matches above, loose matches on Subject: below --
2026-10-15 15:00 [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang Chris Babroski
2026-10-15 15:00 ` [PATCH net-next v1 2/5] net: lan743x: start phylink last and stop it first Chris Babroski
2026-10-19 10:13   ` Nicolai Buchwitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®