[PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop

[PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop

[Chris Babroski]

lan743x_tx_close() and lan743x_rx_close() ask the DMA controller to stop
the channel and wait for it, but ignore the result. If the channel is
still stop-pending when the wait gives up, the descriptor ring, the head
write-back buffer and the packet buffers are unmapped and freed while
the channel may still be fetching from or writing to them.

A TX channel has been seen to stop making progress, with its head index
no longer advancing while DMAC_CMD still reports it as started. A
channel in that state may not honour the stop request either, and
closing the interface would then free memory the engine can still
access.

If the channel is still stop-pending after the wait, or still reads as
started, issue a channel soft reset and wait for it before anything is
freed, and log when that happens.

Use the same helpers for the channel resets that lan743x_tx_open() and
lan743x_rx_open() already open-code, and log there too if the reset
times out.

Fixes: 23f0703c125b ("lan743x: Add main source files for new lan743x driver")
Assisted-by: LLM
Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 64 +++++++++++++++----
 1 file changed, 52 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 82d3ec20ba25..9e1276cf313c 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1584,6 +1584,15 @@ static int lan743x_dmac_tx_wait_till_stopped(struct lan743x_adapter *adapter,
 	return result;
 }
 
+static int lan743x_dmac_tx_reset(struct lan743x_adapter *adapter,
+				 int tx_channel)
+{
+	lan743x_csr_write(adapter, DMAC_CMD, DMAC_CMD_TX_SWR_(tx_channel));
+	return lan743x_csr_wait_for_bit(adapter, DMAC_CMD,
+					DMAC_CMD_TX_SWR_(tx_channel),
+					0, 1000, 20000, 100);
+}
+
 static int lan743x_dmac_rx_get_state(struct lan743x_adapter *adapter,
 				     int rx_channel)
 {
@@ -1613,6 +1622,15 @@ static int lan743x_dmac_rx_wait_till_stopped(struct lan743x_adapter *adapter,
 	return result;
 }
 
+static int lan743x_dmac_rx_reset(struct lan743x_adapter *adapter,
+				 int rx_channel)
+{
+	lan743x_csr_write(adapter, DMAC_CMD, DMAC_CMD_RX_SWR_(rx_channel));
+	return lan743x_csr_wait_for_bit(adapter, DMAC_CMD,
+					DMAC_CMD_RX_SWR_(rx_channel),
+					0, 1000, 20000, 100);
+}
+
 static void lan743x_tx_release_desc(struct lan743x_tx *tx,
 				    int descriptor_index, bool cleanup)
 {
@@ -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);
+	}
 
 	lan743x_csr_write(adapter,
 			  DMAC_INT_EN_CLR,
@@ -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);
 
 	/* Write TX_BASE_ADDR */
 	lan743x_csr_write(adapter,
@@ -2775,6 +2804,7 @@ static int lan743x_rx_ring_init(struct lan743x_rx *rx)
 static void lan743x_rx_close(struct lan743x_rx *rx)
 {
 	struct lan743x_adapter *adapter = rx->adapter;
+	int ret;
 
 	lan743x_csr_write(adapter, FCT_RX_CTL,
 			  FCT_RX_CTL_DIS_(rx->channel_number));
@@ -2784,7 +2814,17 @@ static void lan743x_rx_close(struct lan743x_rx *rx)
 
 	lan743x_csr_write(adapter, DMAC_CMD,
 			  DMAC_CMD_STOP_R_(rx->channel_number));
-	lan743x_dmac_rx_wait_till_stopped(adapter, rx->channel_number);
+	ret = lan743x_dmac_rx_wait_till_stopped(adapter, rx->channel_number);
+	if (ret < 0 || ret == DMAC_CHANNEL_STATE_STARTED) {
+		netdev_warn(adapter->netdev,
+			    "RX channel %d failed to stop, resetting it\n",
+			    rx->channel_number);
+		ret = lan743x_dmac_rx_reset(adapter, rx->channel_number);
+		if (ret)
+			netdev_err(adapter->netdev,
+				   "RX channel %d reset timed out\n",
+				   rx->channel_number);
+	}
 
 	lan743x_csr_write(adapter, DMAC_INT_EN_CLR,
 			  DMAC_INT_BIT_RXFRM_(rx->channel_number));
@@ -2810,11 +2850,11 @@ static int lan743x_rx_open(struct lan743x_rx *rx)
 
 	netif_napi_add(adapter->netdev, &rx->napi, lan743x_rx_napi_poll);
 
-	lan743x_csr_write(adapter, DMAC_CMD,
-			  DMAC_CMD_RX_SWR_(rx->channel_number));
-	lan743x_csr_wait_for_bit(adapter, DMAC_CMD,
-				 DMAC_CMD_RX_SWR_(rx->channel_number),
-				 0, 1000, 20000, 100);
+	ret = lan743x_dmac_rx_reset(adapter, rx->channel_number);
+	if (ret)
+		netdev_err(adapter->netdev,
+			   "RX channel %d reset timed out\n",
+			   rx->channel_number);
 
 	/* set ring base address */
 	lan743x_csr_write(adapter,
-- 
2.34.1

Re: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop

[Nicolai Buchwitz]

Hi Chris

On 15.10.2026 17:00, Chris Babroski wrote:
> lan743x_tx_close() and lan743x_rx_close() ask the DMA controller to 
> stop
> the channel and wait for it, but ignore the result. If the channel is
> still stop-pending when the wait gives up, the descriptor ring, the 
> head
> write-back buffer and the packet buffers are unmapped and freed while
> the channel may still be fetching from or writing to them.
> 
> A TX channel has been seen to stop making progress, with its head index
> no longer advancing while DMAC_CMD still reports it as started. A
> channel in that state may not honour the stop request either, and
> closing the interface would then free memory the engine can still
> access.
> 
> If the channel is still stop-pending after the wait, or still reads as
> started, issue a channel soft reset and wait for it before anything is
> freed, and log when that happens.
> 
> Use the same helpers for the channel resets that lan743x_tx_open() and
> lan743x_rx_open() already open-code, and log there too if the reset
> times out.
> 
> Fixes: 23f0703c125b ("lan743x: Add main source files for new lan743x 
> driver")
> Assisted-by: LLM
> Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
> ---
>  drivers/net/ethernet/microchip/lan743x_main.c | 64 +++++++++++++++----
>  1 file changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c 
> b/drivers/net/ethernet/microchip/lan743x_main.c
> index 82d3ec20ba25..9e1276cf313c 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);
> +	}
> 
>  	lan743x_csr_write(adapter,
>  			  DMAC_INT_EN_CLR,
> @@ -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);

Not a bug, but what's the benefit of keeping going here?

If the channel never came out of reset, lan743x_tx_open() and
lan743x_rx_open() could return the error instead of starting it anyway.

With the reset work from 4/5 a failed reopen then ends in dev_close(),
which seems like the better place to end up. It would also settle the
open question from the cover letter.

> [...]

Thanks,
Nicolai

Re: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop

[netdev-bot+sashiko]

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