mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* Re: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop
@ 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 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 (1) · 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] The patch does not fully close the bug it targets.

Pre-existing issues:
- [Medium] lan743x_tx_open() and lan743x_rx_open() now use the new
  lan743x_dmac_{tx,rx}_reset() helpers, but on -ETIMEDOUT they only call…

--

net: lan743x: reset DMA channels that fail to stop

This patch makes lan743x_tx_close() and lan743x_rx_close() check the
result of waiting for the DMA channel to stop. If the channel is still
stop-pending or still started, they now issue a channel soft reset. The
reset sequence is moved into lan743x_dmac_tx_reset() and
lan743x_dmac_rx_reset(), and the open paths use these helpers and log a
message if the reset times out.

> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 82d3ec20ba259..9e1276cf313c8 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c

[ ... ]

> @@ -2221,11 +2239,22 @@ static int lan743x_tx_ring_init(struct lan743x_tx *tx)
>  static void lan743x_tx_close(struct lan743x_tx *tx)
>  {
>  	struct lan743x_adapter *adapter = tx->adapter;
> +	int ret;
>  
>  	lan743x_csr_write(adapter,
>  			  DMAC_CMD,
>  			  DMAC_CMD_STOP_T_(tx->channel_number));
> -	lan743x_dmac_tx_wait_till_stopped(adapter, tx->channel_number);
> +	ret = lan743x_dmac_tx_wait_till_stopped(adapter, tx->channel_number);
> +	if (ret < 0 || ret == DMAC_CHANNEL_STATE_STARTED) {
> +		netdev_warn(adapter->netdev,
> +			    "TX channel %d failed to stop, resetting it\n",
> +			    tx->channel_number);
> +		ret = lan743x_dmac_tx_reset(adapter, tx->channel_number);
> +		if (ret)
> +			netdev_err(adapter->netdev,
> +				   "TX channel %d reset timed out\n",
> +				   tx->channel_number);
> +	}

[Severity: High]
What happens if this channel reset also times out?

The commit message says the soft reset is issued "and wait for it before
anything is freed". However, if lan743x_dmac_tx_reset() returns -ETIMEDOUT,
the code only logs it, and lan743x_tx_close() goes on with the normal
teardown:

lan743x_tx_close()
    ...
    lan743x_tx_release_all_descriptors(tx);  /* unmap and free skbs */
    ...
    lan743x_tx_ring_cleanup(tx);  /* dma_free_coherent() ring and head */

The commit message describes a wedged channel that may not honour the stop
request. Could that channel ignore the soft reset as well? It would then
keep fetching descriptors from, or writing the head index into, memory that
has already been freed and reused.

lan743x_rx_close() has the same pattern. After a timed-out RX reset,
lan743x_rx_ring_cleanup() unmaps and frees every RX skb, even though their
descriptors still have OWN set. It then frees the descriptor ring and the
head write-back buffer. On RX, the device could write packet data into
freed pages.

Later in the series, "net: lan743x: reset the device on TX timeout" does not
change this. It runs the global DMAC reset (DMAC_CMD_SWR_) from
lan743x_reset_work() only after lan743x_netdev_close() has already freed the
rings. A plain ifdown never reaches that reset at all.

Could the close paths act on the reset failure before freeing anything?
One option is to escalate to the global DMAC_CMD_SWR_ reset or clear PCI
bus mastering. Another is to leak the ring, the head buffer and the mapped
buffers when the channel can't be shown to be quiesced.

[ ... ]

> @@ -2271,11 +2300,11 @@ static int lan743x_tx_open(struct lan743x_tx *tx)
>  			  FCT_TX_CTL_EN_(tx->channel_number));
>  
>  	/* reset tx channel */
> -	lan743x_csr_write(adapter, DMAC_CMD,
> -			  DMAC_CMD_TX_SWR_(tx->channel_number));
> -	lan743x_csr_wait_for_bit(adapter, DMAC_CMD,
> -				 DMAC_CMD_TX_SWR_(tx->channel_number),
> -				 0, 1000, 20000, 100);
> +	ret = lan743x_dmac_tx_reset(adapter, tx->channel_number);
> +	if (ret)
> +		netdev_err(adapter->netdev,
> +			   "TX channel %d reset timed out\n",
> +			   tx->channel_number);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. The old
code also discarded the return value of lan743x_csr_wait_for_bit(), and
this change only adds a log message.

After a failed reset, lan743x_tx_open() still programs TX_BASE_ADDR,
TX_HEAD_WRITEBACK_ADDR, TX_CFG_A/B/C and TX_TAIL. It then writes
DMAC_CMD_START_T_ and returns 0.

Can the interface come up on a channel whose soft reset never finished?
If the reset completes late, the register writes made while it was pending
may be lost or wiped.

lan743x_rx_open() handles the reset the same way. There, if RX_HEAD is
non-zero after a failed RX reset, the code takes this path:

	rx->last_head = lan743x_csr_read(adapter, RX_HEAD(rx->channel_number));
	if (rx->last_head) {
		ret = -EIO;
		goto napi_delete;
	}

That reaches lan743x_rx_ring_cleanup() without the stop and reset handling
that this patch adds to lan743x_rx_close().

Should the open paths return an error when the channel reset times out?

>  
>  	/* Write TX_BASE_ADDR */
>  	lan743x_csr_write(adapter,

[ ... ]

-- 
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 1/5] net: lan743x: reset DMA channels that fail to stop 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 1/5] net: lan743x: reset DMA channels that fail to stop 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 1/5] net: lan743x: reset DMA channels that fail to stop Chris Babroski
2026-10-19 10:18   ` 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®