net: lan743x: automatically recover from TX hang
Subsystems (3)
cover net: lan743x: automatically recover from TX hang
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
Signature · 1 line
2.34.1
PATCH 1/5 net: lan743x: reset DMA channels that fail to stop
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.
--- drivers/net/ethernet/microchip/lan743x_main.c | 64 +++++++++++++++---- 1 file changed, 52 insertions(+), 12 deletions(-)
drivers/net/ethernet/microchip/lan743x_main.c +52 -12
@@ -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,
Signature · 1 line
2.34.1
Re: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop
Hi Chris
Quoted 64 lines · Chris Babroski
lan743x_tx_close()andlan743x_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_CMDstill 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()andlan743x_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 LLMSigned-off-by Chris Babroski <[email protected]>--- drivers/net/ethernet/microchip/lan743x_main.c | 64 +++++++++++++++---- 1 file changed, 52 insertions(+), 12 deletions(-)drivers/net/ethernet/microchip/lan743x_main.c +12
-1@@ -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,drivers/net/ethernet/microchip/lan743x_main.c +0
-0DMAC_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 */
drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_tx_open(struct lan743x_tx · line 2308
Quoted 10 lines
drivers/net/ethernet/microchip/lan743x_main.c +5
-5-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.
Quoted 1 line
[...]
Thanks,
Nicolai
RE: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop
Hi Chris & Nicolai,
...
Quoted 18 lines
Quoted 5 lines
drivers/net/ethernet/microchip/lan743x_main.c +0
-0@@ -2271,11 +2300,11 @@ static int lan743x_tx_open(struct lan743x_tx *tx) FCT_TX_CTL_EN_(tx->channel_number));/* reset tx channel */ drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_tx_open(struct lan743x_tx · line 2308
Quoted 10 lines
drivers/net/ethernet/microchip/lan743x_main.c +5
-5-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?
Quoted 7 lines
If the channel never came out of reset,
lan743x_tx_open()andlan743x_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.
Agree with Nicolai's comment that returning error at lan743x_tx/rx_open() to make lan743x_netdev_open() returns with error.
Per Jakub's email, this patch could be a part of net and more chance to be in stable tree without patch 4 & 5, it would be better to handle eroor case along with error message.
Best regards,
Woojung
RE: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop
Hi Woojung and Nicolai,
Quoted 21 lines
Quoted 21 lines
Quoted 5 lines
drivers/net/ethernet/microchip/lan743x_main.c +0
-0@@ -2271,11 +2300,11 @@ static int lan743x_tx_open(struct lan743x_tx *tx) FCT_TX_CTL_EN_(tx->channel_number));/* reset tx channel */ drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_tx_open(struct lan743x_tx · line 2308
Quoted 10 lines
drivers/net/ethernet/microchip/lan743x_main.c +5
-5-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()andlan743x_rx_open()could return the error instead of starting it anyway.
Quoted 6 lines
Quoted 4 lines
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.Agree with Nicolai's comment that returning error at lan743x_tx/rx_open() to make
lan743x_netdev_open()returns with error.
Sounds good. I'll update V2 to handle the timeout error by cleaning up previously initialized steps and then return the error.
One thing worth noting is that every call to lan743x_csr_wait_for_bit() could technically time out and most cases are unhandled by the driver today. My goal was to avoid making too many unrelated changes as part of this patch series, but more error handling could potentially be added as part of a future series.
Thanks,
Chris
Re: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop
In reply to Chris BabroskiQuoted 46 lines · Chris Babroski
lan743x_tx_close()andlan743x_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_CMDstill 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()andlan743x_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 LLMSigned-off-by Chris Babroski <[email protected]>--- drivers/net/ethernet/microchip/lan743x_main.c | 64 +++++++++++++++---- 1 file changed, 52 insertions(+), 12 deletions(-)drivers/net/ethernet/microchip/lan743x_main.c +2
-1@@ -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);
drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_tx_ring_init(struct · line 2258
Quoted 10 lines
drivers/net/ethernet/microchip/lan743x_main.c +10
-0+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); + }
While looking something up in the registers for the other thread I stumbled over this in AN2948 about TX_SWR_x:
"This is an immediate reset and will cause any, in process, PCIe request to [be] aborted. This may likely result in a corrupt PCIe packet."
A channel that did not stop is the one case where a request is still in flight, so this resets right when the datasheet says not to. Is there a sanctioned way to recover a channel that ignores STOP_T?
Woojung, maybe you can say?
Quoted 1 line
[...]
Thanks,
Nicolai
RE: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop
Hi Nicola
Quoted 64 lines
Quoted 50 lines
lan743x_tx_close()andlan743x_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_CMDstill 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()andlan743x_rx_open()already open-code, and log there too if the reset times out.Fixes 23f0703c125b ("lan743x: Add main source files for new lan743xdriver")
Assisted-by LLMSigned-off-by Chris Babroski <[email protected]>--- drivers/net/ethernet/microchip/lan743x_main.c | 64 +++++++++++++++---- 1 file changed, 52 insertions(+), 12 deletions(-)drivers/net/ethernet/microchip/lan743x_main.c +2
-1@@ -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);drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_tx_ring_init(struct · line 2258
Quoted 10 lines
drivers/net/ethernet/microchip/lan743x_main.c +10
-0+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); + }While looking something up in the registers for the other thread I stumbled over this in AN2948 about
TX_SWR_x:
Quoted 9 lines
"This is an immediate reset and will cause any, in process, PCIe request to [be] aborted. This may likely result in a corrupt PCIe packet."
A channel that did not stop is the one case where a request is still in flight, so this resets right when the datasheet says not to. Is there a sanctioned way to recover a channel that ignores
STOP_T?Woojung, maybe you can say?
Understood your question.
Talking to hardware engineer whether there is a way beside hitting TX_SWR_x in this situation. Will update when getting answered.
Best regards,
Woojung
RE: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop
Hi Nicolai & Chris,
Quoted 72 lines
Quoted 72 lines
Quoted 40 lines
lan743x_tx_close()andlan743x_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_CMDstill 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()andlan743x_rx_open()already open-code, and log there too if the reset times out.Fixes 23f0703c125b ("lan743x: Add main source files for new lan743xdriver")
Assisted-by LLMSigned-off-by Chris Babroski <[email protected]>--- drivers/net/ethernet/microchip/lan743x_main.c | 64 +++++++++++++++---- 1 file changed, 52 insertions(+), 12 deletions(-)drivers/net/ethernet/microchip/lan743x_main.c +0
-0@@ -2221,11 +2239,22 @@ static int lan743x_tx_ring_init(struct lan743x_tx *tx)drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_tx_ring_init(struct · line 2248
Quoted 10 lines
drivers/net/ethernet/microchip/lan743x_main.c +2
-1static 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);drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_tx_ring_init(struct lan743x_tx *tx) · line 2257
Quoted 10 lines
drivers/net/ethernet/microchip/lan743x_main.c +10
-0+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); + }While looking something up in the registers for the other thread I stumbled over this in AN2948 about
TX_SWR_x:"This is an immediate reset and will cause any, in process, PCIe request to [be] aborted. This may likely result in a corrupt PCIe packet."
A channel that did not stop is the one case where a request is still in flight, so this resets right when the datasheet says not to. Is there a sanctioned way to recover a channel that ignores
STOP_T?
Quoted 8 lines
Quoted 2 lines
Woojung, maybe you can say?
Understood your question.
Talking to hardware engineer whether there is a way beside hitting
TX_SWR_xin this situation. Will update when getting answered.
Recommended way of setting TX_SWR_x is after stopping and flushing FCT to avoid in-flight frames during TX DMA reset.
lan743x_dmac_tx_reset() is already called lan743x_tx_open() with proper sequence of initializing FCT at lan743x_netdev_open().
lan743x_reset_work() in the patch 4/5 is doing lan743x_netdev_close(), ... and lan743x_netdev_open(), I think no additional lan743x_dmac_tx_reset() may be needed at failure of lan743x_dmac_tx_wait_till_stopped() in lan743x_tx_close().
Hi Chris,
Did you see calling lan743x_dmac_tx_reset() at lan743x_tx_close() is a necessary step in addition to lan743x_dmac_tx_reset() at lan743x_tx_open() when lockup happened?
Best regards,
Woojung
RE: [PATCH net-next v1 1/5] net: lan743x: reset DMA channels that fail to stop
Hi Woojung and Nicolai,
Quoted 11 lines
Recommended way of setting
TX_SWR_xis after stopping and flushing FCT to avoid in-flight frames during TX DMA reset.
lan743x_dmac_tx_reset()is already calledlan743x_tx_open()with proper sequence of initializing FCT atlan743x_netdev_open().
lan743x_reset_work()in the patch 4/5 is doinglan743x_netdev_close(), ... andlan743x_netdev_open(), I think no additionallan743x_dmac_tx_reset()may be needed at failure oflan743x_dmac_tx_wait_till_stopped()inlan743x_tx_close().
Quoted 4 lines
Hi Chris,
Did you see callinglan743x_dmac_tx_reset()atlan743x_tx_close()is a necessary step in addition tolan743x_dmac_tx_reset()atlan743x_tx_open()when lockup happened?
Thanks for clarifying the reset flow. I did not see that it is unnecessary, but is it an issue to leave the DMAC in a started state on close if the descriptor ring was freed even though the FIFO is disabled after?
I can remove the call to lan743x_dmac_tx_reset() in lan743x_tx_close(). It sounds like changing the order of lan743x_tx_close() to disable FIFO beforehand could also work, but that feels out of scope for this patch series.
I also noticed in lan743x_rx_close() the FIFO is already disabled before checking DMAC channel state. This means we can leave the call to lan743x_dmac_rx_reset() inside lan743x_rx_close() or I can remove it as well if you prefer.
Thanks,
Chris
PATCH 2/5 net: lan743x: start phylink last and stop it first
In reply to Chris Babroskilan743x_netdev_open() starts phylink before the RX and TX rings exist, and lan743x_netdev_close() only stops it after the rings are freed. The mac_link_up() callback wakes all TX queues, so it can run while there is no TX ring to transmit on.
For ndo_open and ndo_stop the qdisc is not active at that point, but the suspend and resume paths call lan743x_netdev_close() and lan743x_netdev_open() directly with the qdisc still attached. A link up event in either window lets the stack call ndo_start_xmit() on a channel whose ring is not allocated yet or has just been freed.
The error path of lan743x_netdev_open() is also wrong: a lan743x_ptp_open() failure jumps past lan743x_phylink_disconnect(), leaving phylink started and the PHY attached after open has failed. Every later open then fails because the PHY is already attached.
Connect and start phylink as the last step of open and stop it as the first step of close, as other phylink users do. If phylink fails to connect, nothing is attached, so the error path only needs to unwind the rings, PTP, MAC and interrupts.
--- drivers/net/ethernet/microchip/lan743x_main.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
drivers/net/ethernet/microchip/lan743x_main.c +6 -8
@@ -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]);
@@ -3342,8 +3344,6 @@ static int lan743x_netdev_close(struct net_device *netdev)lan743x_ptp_close (adapter); -lan743x_phylink_disconnect (adapter); -lan743x_mac_close (adapter);lan743x_intr_close (adapter);
@@ -3365,10 +3365,6 @@ static int lan743x_netdev_open(struct net_device *netdev)if (ret) goto close_intr; - ret =lan743x_phylink_connect (adapter); -if (ret) - goto close_mac; - ret =lan743x_ptp_open (adapter);if (ret) goto close_mac;
@@ -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; +if (netdev->phydev)phy_support_eee (netdev->phydev);
@@ -3414,8 +3414,6 @@ static int lan743x_netdev_open(struct net_device *netdev)lan743x_rx_close (&adapter->rx[index]); }lan743x_ptp_close (adapter); -if (adapter->phylink) -lan743x_phylink_disconnect (adapter); close_mac:lan743x_mac_close (adapter);
Signature · 1 line
2.34.1
Re: [PATCH net-next v1 2/5] net: lan743x: start phylink last and stop it first
Quoted 85 lines · Chris Babroski
lan743x_netdev_open()starts phylink before the RX and TX rings exist, andlan743x_netdev_close()only stops it after the rings are freed. Themac_link_up()callback wakes all TX queues, so it can run while there is no TX ring to transmit on.For
ndo_openandndo_stopthe qdisc is not active at that point, but the suspend and resume paths calllan743x_netdev_close()andlan743x_netdev_open()directly with the qdisc still attached. A link up event in either window lets the stack callndo_start_xmit()on a channel whose ring is not allocated yet or has just been freed.The error path of
lan743x_netdev_open()is also wrong: alan743x_ptp_open()failure jumps pastlan743x_phylink_disconnect(), leaving phylink started and the PHY attached after open has failed. Every later open then fails because the PHY is already attached.Connect and start phylink as the last step of open and stop it as the first step of close, as other phylink users do. If phylink fails to connect, nothing is attached, so the error path only needs to unwind the rings, PTP, MAC and interrupts.
Fixes a5f199a8d8a0 ("net: lan743x: Migrate phylib to phylink")Assisted-by LLMSigned-off-by Chris Babroski <[email protected]>--- drivers/net/ethernet/microchip/lan743x_main.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)drivers/net/ethernet/microchip/lan743x_main.c +2
-2@@ -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]);@@ -3342,8 +3344,6 @@ static int lan743x_netdev_close(struct net_device *netdev)lan743x_ptp_close (adapter); -lan743x_phylink_disconnect (adapter); -lan743x_mac_close (adapter);drivers/net/ethernet/microchip/lan743x_main.c +0
-4lan743x_intr_close (adapter);@@ -3365,10 +3365,6 @@ static int lan743x_netdev_open(struct net_device *netdev)if (ret) goto close_intr; - ret =lan743x_phylink_connect (adapter); -if (ret) - goto close_mac; - ret =lan743x_ptp_open (adapter);if (ret)drivers/net/ethernet/microchip/lan743x_main.c +4
-2goto close_mac;@@ -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; +if (netdev->phydev)phy_support_eee (netdev->phydev);@@ -3414,8 +3414,6 @@ static int lan743x_netdev_open(struct net_device *netdev)lan743x_rx_close (&adapter->rx[index]); }lan743x_ptp_close (adapter); -if (adapter->phylink) -lan743x_phylink_disconnect (adapter); close_mac:
Quoted 1 line
lan743x_mac_close(adapter);
Thanks,
Nicolai
PATCH 3/5 net: lan743x: quiesce TX before freeing the rings
In reply to Chris Babroskilan743x_pcidev_shutdown(), which suspend also uses, calls netif_device_detach() and then lan743x_netdev_close() with the qdisc still attached. Detaching only sets the queue stop bits. It does not wait for an ndo_start_xmit() that is already running, and the TX NAPI poll still wakes a stopped queue as soon as it has reclaimed descriptors. The TX NAPI of a channel that has not been closed yet can therefore restart its queue, and the stack then transmits on a ring that lan743x_tx_close() is freeing.
Only wake a queue from TX NAPI while the device is present. In lan743x_netdev_close(), wait for any NAPI poll that may have missed the detach with synchronize_net(), then stop all queues with netif_tx_disable(), which also waits for transmits in progress, before closing the channels. For ndo_stop the qdisc is already deactivated, so this only adds a grace period there.
--- drivers/net/ethernet/microchip/lan743x_main.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/net/ethernet/microchip/lan743x_main.c +5 -1
@@ -2125,7 +2125,8 @@ static int lan743x_tx_napi_poll(struct napi_struct *napi, int weight)/* clean up tx ring */ lan743x_tx_release_completed_descriptors (tx); txq =netdev_get_tx_queue (adapter->netdev, tx->channel_number); -if (netif_tx_queue_stopped (txq)) { +if (netif_tx_queue_stopped (txq) && +netif_device_present (adapter->netdev)) {if (tx->rqd_descriptors) {if (tx->rqd_descriptors <=lan743x_tx_get_avail_desc (tx)) {
@@ -3336,6 +3337,9 @@ static int lan743x_netdev_close(struct net_device *netdev)lan743x_phylink_disconnect (adapter); +synchronize_net (); +netif_tx_disable (netdev); +for (index =0 ; index < adapter->used_tx_channels; index++)lan743x_tx_close (&adapter->tx[index]);
Signature · 1 line
2.34.1
Re: [PATCH net-next v1 3/5] net: lan743x: quiesce TX before freeing the rings
Quoted 39 lines · Chris Babroski
lan743x_pcidev_shutdown(), which suspend also uses, callsnetif_device_detach()and thenlan743x_netdev_close()with the qdisc still attached. Detaching only sets the queue stop bits. It does not wait for anndo_start_xmit()that is already running, and the TX NAPI poll still wakes a stopped queue as soon as it has reclaimed descriptors. The TX NAPI of a channel that has not been closed yet can therefore restart its queue, and the stack then transmits on a ring thatlan743x_tx_close()is freeing.Only wake a queue from TX NAPI while the device is present. In
lan743x_netdev_close(), wait for any NAPI poll that may have missed the detach withsynchronize_net(), then stop all queues withnetif_tx_disable(), which also waits for transmits in progress, before closing the channels. Forndo_stopthe qdisc is already deactivated, so this only adds a grace period there.Fixes 23f0703c125b ("lan743x: Add main source files for new lan743x driver")Assisted-by LLMSigned-off-by Chris Babroski <[email protected]>--- drivers/net/ethernet/microchip/lan743x_main.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)drivers/net/ethernet/microchip/lan743x_main.c +2
-1@@ -2125,7 +2125,8 @@ static int lan743x_tx_napi_poll(struct napi_struct *napi, int weight)/* clean up tx ring */ lan743x_tx_release_completed_descriptors (tx ); txq =netdev_get_tx_queue (adapter->netdev, tx->channel_number); -if (netif_tx_queue_stopped (txq)) { +if (netif_tx_queue_stopped (txq) && +netif_device_present (adapter->netdev)) {if (tx->rqd_descriptors) { if (tx->rqd_descriptors <=drivers/net/ethernet/microchip/lan743x_main.c +0
-0lan743x_tx_get_avail_desc(tx)) {
drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_netdev_close(struct net_device · line 3345
Quoted 9 lines
drivers/net/ethernet/microchip/lan743x_main.c +3
-0@@ -3336,6 +3337,9 @@ static int lan743x_netdev_close(struct net_device *netdev)lan743x_phylink_disconnect (adapter); +synchronize_net (); +netif_tx_disable (netdev); +for (index =0 ; index < adapter->used_tx_channels; index++)lan743x_tx_close (&adapter->tx[index]);
Thanks,
Nicolai
RE: [PATCH net-next v1 3/5] net: lan743x: quiesce TX before freeing the rings
In reply to Chris BabroskiHi Chris,
..
drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_netdev_close(struct net_device · line 3342
Quoted 7 lines
drivers/net/ethernet/microchip/lan743x_main.c +2
-0@@ -3336,6 +3337,9 @@ static int lan743x_netdev_close(struct net_device *netdev)lan743x_phylink_disconnect (adapter); +synchronize_net (); +netif_tx_disable (netdev);
synchronize_net() is called inside of lan743x_tx_close() and lan743x_rx_close() via netif_napi_del(). Do you see current order of calling is an issue?
Quoted 6 lines
drivers/net/ethernet/microchip/lan743x_main.c +1
-1+for (index =0 ; index < adapter->used_tx_channels; index++)lan743x_tx_close (&adapter->tx[index]); --2.34.1
Thanks
Woojung
Re: [PATCH net-next v1 3/5] net: lan743x: quiesce TX before freeing the rings
Hi Woojung,
Quoted 14 lines
drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_netdev_close(struct net_device · line 3342
Quoted 7 lines
drivers/net/ethernet/microchip/lan743x_main.c +2
-0@@ -3336,6 +3337,9 @@ static int lan743x_netdev_close(struct net_device *netdev)lan743x_phylink_disconnect (adapter); +synchronize_net (); +netif_tx_disable (netdev);
synchronize_net()is called inside oflan743x_tx_close()andlan743x_rx_close()vianetif_napi_del(). Do you see current order of calling is an issue?Quoted 3 lines
drivers/net/ethernet/microchip/lan743x_main.c +1
-0+for (index =0 ; index < adapter->used_tx_channels; index++)lan743x_tx_close (&adapter->tx[index]);
This was a change introduced after AI review flagged a potential race condition:
1. On CPU A, the TX NAPI poll now checks netif_device_present() and gets true.
2. On CPU B, netif_device_detach() runs, then lan743x_netdev_close() calls netif_tx_disable() and the queue is stopped.
3. CPU A calls netif_tx_wake_queue() and the queue is running again.
4. The qdisc is still attached in the suspend and reset paths, so the stack transmits into the ring while lan743x_tx_close() frees it.
So the synchronize_net() called via lan743x_tx_close() can't prevent NAPI poll from waking the queue again.
The fix is to wait for NAPI poll to complete with synchronize_net() first to prevent unexpected queue wake before netif_tx_disable() stops the queue and waits for transmits to complete.
This is unlikely to happen as part of the TX hang scenario because the reset in that case comes after TX has stopped. The pre-existing conditions that could see this would be suspend, reboot, or kexec on a multi-core system under TX load.
Given that this is not directly related to TX hang recovery I could drop it from this patch series if you prefer.
Thanks,
Chris
RE: [PATCH net-next v1 3/5] net: lan743x: quiesce TX before freeing the rings
Hi Chris,
Quoted 71 lines
Quoted 10 lines
Quoted 9 lines
(quoted diff) +0
-0@@ -3336,6 +3337,9 @@ static int lan743x_netdev_close(struct net_device *netdev) lan743x_phylink_disconnect(adapter);Quoted 7 lines
drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_netdev_close(struct net_device *netdev)
Quoted 3 lines
drivers/net/ethernet/microchip/lan743x_main.c +2
-0+synchronize_net (); +netif_tx_disable (netdev);
synchronize_net()is called inside oflan743x_tx_close()and
lan743x_rx_close()Quoted 9 lines
via
netif_napi_del(). Do you see current order of calling is an issue?Quoted 5 lines
+
for (index = 0; index < adapter->used_tx_channels; index++) lan743x_tx_close(&adapter->tx[index]);This was a change introduced after AI review flagged a potential race
condition:
1. On CPU A, the TX NAPI poll now checks
netif_device_present()and getstrue.
2. On CPU B,
netif_device_detach()runs, thenlan743x_netdev_close()calls
netif_tx_disable()and the queue is stopped.3. CPU A calls
netif_tx_wake_queue()and the queue is running again.4. The qdisc is still attached in the suspend and reset paths, so the
stack transmits into the ring while
lan743x_tx_close()frees it.So the
synchronize_net()called vialan743x_tx_close()can't preventNAPI poll from waking the queue again.
The fix is to wait for NAPI poll to complete with
synchronize_net()first to prevent unexpected queue wake before
netif_tx_disable()stopsthe queue and waits for transmits to complete.
This is unlikely to happen as part of the TX hang scenario because the
reset in that case comes after TX has stopped. The pre-existing
conditions that could see this would be suspend, reboot, or kexec on a
Quoted 8 lines
multi-core system under TX load.
Given that this is not directly related to TX hang recovery I could
drop it from this patch series if you prefer.
Thanks for detail explanation.
IMHO, it is from Ai review than reproducible and/or failure during tests I would suggest making as separate net-next patch than net fix[1] if others are ok.
[1] https://lore.kernel.org/all/[email protected]/
Thanks
Woojung
PATCH 4/5 net: lan743x: reset the device on TX timeout
In reply to Chris BabroskiThe driver does not implement ndo_tx_timeout, so the core TX watchdog is never started. If a TX DMA channel stops completing descriptors, its queue stays stopped and the interface cannot transmit again until it is brought down and back up.
Add an ndo_tx_timeout handler that schedules a work item to reset the device. Under rtnl_lock(), the work detaches the netdev, closes it, soft-resets the DMA controller, opens it again and reattaches it. This is the sequence suspend and resume already use, with the DMA controller reset taking the place of the full hardware init.
Because everything is reopened, the RX path, PTP and the PHY link are restarted along with TX. For PTP, the reset has the same effect as bringing the interface down and up. As on suspend and resume, the PTP clock is unregistered and registered again, and its time is reset to the system time. The hardware timestamping state is also reset, so the RX filter returns to none and one-step sync insertion is disabled, while the driver keeps its TX timestamping flags. PTP applications have to set up timestamping again.
lan743x_dmac_init() did not check that the DMA controller soft reset completed, so the work could reopen a controller whose reset timed out. Return the error from the poll so the work can tell. Probe already fails when lan743x_dmac_init() returns an error, so a controller that does not come out of reset now also fails probe.
If lan743x_dmac_init() or the reopen returns an error, the interface is taken down with dev_close() so it can be brought back up later. The hardware is already closed at that point, so route ndo_stop through a wrapper that skips the hardware close while the device is detached.
ndo_stop is called with rtnl held and cannot wait for the work, so it only cancels a pending one. A work that is already waiting for rtnl returns if the device is still down when it gets the lock. If the device was opened again in the meantime, it is reset once more, which only costs an extra link flap. On remove, the work is cancelled synchronously after the netdev is unregistered.
--- drivers/net/ethernet/microchip/lan743x_main.c | 74 ++++++++++++++++++- drivers/net/ethernet/microchip/lan743x_main.h | 2 + 2 files changed, 73 insertions(+), 3 deletions(-)
drivers/net/ethernet/microchip/lan743x_main.c +71 -3
@@ -1516,10 +1516,14 @@ static void lan743x_rfe_set_multicast(struct lan743x_adapter *adapter)static int lan743x_dmac_init (struct lan743x_adapter *adapter) {u32 data =0 ; +int ret;lan743x_csr_write (adapter,DMAC_CMD ,DMAC_CMD_SWR_ ); -lan743x_csr_wait_for_bit (adapter,DMAC_CMD ,DMAC_CMD_SWR_ , -0 ,1000 ,20000 ,100 ); + ret =lan743x_csr_wait_for_bit (adapter,DMAC_CMD ,DMAC_CMD_SWR_ , +0 ,1000 ,20000 ,100 ); +if (ret) +return ret; +switch (DEFAULT_DMA_DESCRIPTOR_SPACING ) {case DMA_DESCRIPTOR_SPACING_16 : data =DMAC_CFG_MAX_DSPACE_16_ ;
@@ -3335,6 +3339,12 @@ static int lan743x_netdev_close(struct net_device *netdev)struct lan743x_adapter *adapter =netdev_priv (netdev);int index; +/* lan743x_reset_work() takes rtnl, so it cannot be waited for here. +* A running one returns if the device is still down when it gets +* rtnl, otherwise it resets the reopened device once more. +*/ +cancel_work (&adapter->reset_work); +lan743x_phylink_disconnect (adapter);synchronize_net ();
@@ -3431,6 +3441,61 @@ static int lan743x_netdev_open(struct net_device *netdev)return ret; } +static int lan743x_netdev_stop (struct net_device *netdev) +{ +if (!netif_device_present (netdev)) +return 0 ; + +return lan743x_netdev_close (netdev); +} + +static void lan743x_reset_work (struct work_struct *work) +{ +struct lan743x_adapter *adapter = + container_of(work, struct lan743x_adapter, reset_work); +struct net_device *netdev = adapter->netdev; +int ret; + +rtnl_lock (); + +if (!netif_running (netdev) || !netif_device_present (netdev)) + goto unlock; + +netdev_warn (netdev,"resetting after TX timeout\n" ); + +netif_device_detach (netdev); +lan743x_netdev_close (netdev); + + ret =lan743x_dmac_init (adapter); +if (ret) { +netdev_err (netdev,"DMA controller reset failed: %d\n" , ret); + goto close; + } + + ret =lan743x_netdev_open (netdev); +if (ret) { +netdev_err (netdev,"failed to reopen after reset: %d\n" , ret); + goto close; + } + + goto attach; + +close: +dev_close (netdev); +attach: +netif_device_attach (netdev); +unlock: +rtnl_unlock (); +} + +static void lan743x_netdev_tx_timeout (struct net_device *netdev, +unsigned int txqueue) +{ +struct lan743x_adapter *adapter =netdev_priv (netdev); + +schedule_work (&adapter->reset_work); +} +static netdev_tx_t lan743x_netdev_xmit_frame (struct sk_buff *skb,struct net_device *netdev) {
@@ -3537,8 +3602,9 @@ static int lan743x_netdev_set_mac_address(struct net_device *netdev,static const struct net_device_ops lan743x_netdev_ops = { .ndo_open = lan743x_netdev_open, - .ndo_stop = lan743x_netdev_close, + .ndo_stop = lan743x_netdev_stop, .ndo_start_xmit = lan743x_netdev_xmit_frame, + .ndo_tx_timeout = lan743x_netdev_tx_timeout, .ndo_eth_ioctl = lan743x_netdev_ioctl, .ndo_set_rx_mode = lan743x_netdev_set_multicast, .ndo_change_mtu = lan743x_netdev_change_mtu,
@@ -3567,6 +3633,7 @@ static void lan743x_destroy_phylink(struct lan743x_adapter *adapter)static void lan743x_full_cleanup (struct lan743x_adapter *adapter) {unregister_netdev (adapter->netdev); +cancel_work_sync (&adapter->reset_work);lan743x_destroy_phylink (adapter);lan743x_mdiobus_cleanup (adapter);
@@ -3761,6 +3828,7 @@ static int lan743x_pcidev_probe(struct pci_dev *pdev,NETIF_MSG_LINK |NETIF_MSG_IFUP |NETIF_MSG_IFDOWN |NETIF_MSG_TX_QUEUED ; netdev->max_mtu =LAN743X_MAX_FRAME_SIZE ; +INIT_WORK (&adapter->reset_work, lan743x_reset_work);of_get_mac_address (pdev->dev.of_node, adapter->mac_address);
drivers/net/ethernet/microchip/lan743x_main.h +2 -0
@@ -6,6 +6,7 @@#include <linux/phy.h> #include <linux/phylink.h> +#include <linux/workqueue.h> #include "lan743x_ptp.h" #define DRIVER_AUTHOR "Bryan Whitehead <[email protected]>"
@@ -1075,6 +1076,7 @@ struct lan743x_adapter {struct lan743x_phy phy;struct lan743x_tx tx[PCI11X1X_USED_TX_CHANNELS ];struct lan743x_rx rx[LAN743X_USED_RX_CHANNELS ]; +struct work_struct reset_work;bool is_pci11x1x;bool is_sgmii_en;bool is_rmii_en;
Signature · 1 line
2.34.1
Re: [PATCH net-next v1 4/5] net: lan743x: reset the device on TX timeout
Hi Chris
Quoted 11 lines · Chris Babroski
The driver does not implement
ndo_tx_timeout, so the core TX watchdog is never started. If a TX DMA channel stops completing descriptors, its queue stays stopped and the interface cannot transmit again until it is brought down and back up.Add an
ndo_tx_timeouthandler that schedules a work item to reset the device. Underrtnl_lock(), the work detaches the netdev, closes it, soft-resets the DMA controller, opens it again and reattaches it. This is the sequence suspend and resume already use, with the DMA controller reset taking the place of the full hardware init.
Quoted 8 lines
Because everything is reopened, the RX path, PTP and the PHY link are restarted along with TX. For PTP, the reset has the same effect as bringing the interface down and up. As on suspend and resume, the PTP clock is unregistered and registered again, and its time is reset to the system time. The hardware timestamping state is also reset, so the RX filter returns to none and one-step sync insertion is disabled, while the driver keeps its TX timestamping flags. PTP applications have to set up timestamping again.
Is it ok to re-register the PTP clock on a reset the driver starts by itself?
ptp_clock_unregister() makes the open /dev/ptpN return -ENODEV, and since the old index stays taken while ptp4l holds it, the reopen comes back as a different /dev/ptpN. So ptp4l has to be restarted, which is more than "set up timestamping again"...
How about moving clock registration to probe instead, so it survives the reset? Then only the hw timestamping config would need to be put back.
Quoted 49 lines
lan743x_dmac_init()did not check that the DMA controller soft reset completed, so the work could reopen a controller whose reset timed out. Return the error from the poll so the work can tell. Probe already fails whenlan743x_dmac_init()returns an error, so a controller that does not come out of reset now also fails probe.If
lan743x_dmac_init()or the reopen returns an error, the interface is taken down withdev_close()so it can be brought back up later. The hardware is already closed at that point, so routendo_stopthrough a wrapper that skips the hardware close while the device is detached.
ndo_stopis called with rtnl held and cannot wait for the work, so it only cancels a pending one. A work that is already waiting for rtnl returns if the device is still down when it gets the lock. If the device was opened again in the meantime, it is reset once more, which only costs an extra link flap. On remove, the work is cancelled synchronously after the netdev is unregistered.Assisted-by LLMSigned-off-by Chris Babroski <[email protected]>--- drivers/net/ethernet/microchip/lan743x_main.c | 74 ++++++++++++++++++- drivers/net/ethernet/microchip/lan743x_main.h | 2 + 2 files changed, 73 insertions(+), 3 deletions(-)drivers/net/ethernet/microchip/lan743x_main.c +18
-0+static void lan743x_reset_work (struct work_struct *work) +{ +struct lan743x_adapter *adapter = + container_of(work, struct lan743x_adapter, reset_work); +struct net_device *netdev = adapter->netdev; +int ret; + +rtnl_lock (); + +if (!netif_running (netdev) || !netif_device_present (netdev)) + goto unlock; + +netdev_warn (netdev,"resetting after TX timeout\n" ); + +netif_device_detach (netdev); +lan743x_netdev_close (netdev); + + ret =lan743x_dmac_init (adapter);
drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_netdev_open(struct net_device *netdev) · line 3479
Quoted 10 lines
drivers/net/ethernet/microchip/lan743x_main.c +10
-0+if (ret) { +netdev_err (netdev,"DMA controller reset failed: %d\n" , ret); + goto close; + } + + ret =lan743x_netdev_open (netdev); +if (ret) { +netdev_err (netdev,"failed to reopen after reset: %d\n" , ret); + goto close; + }
Unrelated to this series, but lan743x_pm_resume() seems to ignore this return value.
Quoted 1 line
[...]
Thanks,
Nicolai
Re: [PATCH net-next v1 4/5] net: lan743x: reset the device on TX timeout
Hi Nicolai,
Quoted 11 lines
Is it ok to re-register the PTP clock on a reset the driver starts by itself?
ptp_clock_unregister()makes the open/dev/ptpNreturn-ENODEV, and since the old index stays taken while ptp4l holds it, the reopen comes back as a different/dev/ptpN. So ptp4l has to be restarted, which is more than "set up timestamping again"...How about moving clock registration to probe instead, so it survives the reset? Then only the hw timestamping config would need to be put back.
It may be best to get Microchip's opinion about how this should be handled. Woojung, can you answer this?
Makes sense that moving clock registration to probe and remove would keep the PTP device valid across TX hang reset, but I'm not sure if there are other consequences to PTP still being disabled and re-enabled during reset.
I'll update the commit message to be more clear about ptp4l needing to be restarted and try to run some tests to see what the effect is of moving clock registration.
Quoted 8 lines
drivers/net/ethernet/microchip/lan743x_main.c static int lan743x_netdev_open(struct net_device *netdev) · line 3479
Quoted 5 lines
drivers/net/ethernet/microchip/lan743x_main.c +5
-0+ ret =lan743x_netdev_open (netdev); +if (ret) { +netdev_err (netdev,"failed to reopen after reset: %d\n" , ret); + goto close; + }Unrelated to this series, but
lan743x_pm_resume()seems to ignore this return value.
This issue has been flagged by the Sashiko AI review bot twice as well now. It would not be difficult to add similar error handling to lan743x_pm_resume() so I can add that as a separate small patch when I send the v2 series.
Thanks,
Chris
RE: [PATCH net-next v1 4/5] net: lan743x: reset the device on TX timeout
Hi Chris & Nicolai,
Quoted 15 lines
Quoted 11 lines
Is it ok to re-register the PTP clock on a reset the driver starts by itself?
ptp_clock_unregister()makes the open/dev/ptpNreturn-ENODEV, and since the old index stays taken while ptp4l holds it, the reopen comes back as a different/dev/ptpN. So ptp4l has to be restarted, which is more than "set up timestamping again"...How about moving clock registration to probe instead, so it survives the reset? Then only the hw timestamping config would need to be put back.
It may be best to get Microchip's opinion about how this should be handled. Woojung, can you answer this?
Quoted 9 lines
Makes sense that moving clock registration to probe and remove would keep the PTP device valid across TX hang reset, but I'm not sure if there are other consequences to PTP still being disabled and re-enabled during reset.
I'll update the commit message to be more clear about ptp4l needing to be restarted and try to run some tests to see what the effect is of moving clock registration.
Because TX path is already lockup, I would say that impacted PTP traffic even though timestamp function still alive.
It could look ok at workaround point of view, my opinion is keeping ptp_clock reg/unreg at *_open() and *_close().
Also, if moving out to *_probe(), PTP clock is not initialized at *_open() time, module unload/load is required to reinit PTP clock.
Thanks
Woojung
PATCH 5/5 net: lan743x: add byte queue limits support
In reply to Chris BabroskiReport the bytes queued to and completed by each TX ring to BQL.
Besides limiting how much data sits in the TX ring, this lets the stack stop a queue once the bytes in flight exceed the BQL limit. Without it, a queue is only stopped when its 128 entry ring is full, and the TX watchdog only looks at stopped queues. With light traffic it can take tens of seconds after a TX hang to fill the ring, before the watchdog timeout even starts counting.
lan743x_tx_close() frees pending descriptors without completing them, so reset the BQL state there.
--- drivers/net/ethernet/microchip/lan743x_main.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
drivers/net/ethernet/microchip/lan743x_main.c +19 -0
@@ -1708,10 +1708,23 @@ static int lan743x_tx_next_index(struct lan743x_tx *tx, int index)static void lan743x_tx_release_completed_descriptors (struct lan743x_tx *tx) { +struct lan743x_tx_buffer_info *buffer_info; +unsigned int bytes =0 ; +unsigned int pkts =0 ; +while (le32_to_cpu (*tx->head_cpu_ptr) != (tx->last_head)) { + buffer_info = &tx->buffer_info[tx->last_head]; +if (buffer_info->skb) { + bytes += buffer_info->skb->len; + pkts++; + }lan743x_tx_release_desc (tx, tx->last_head, false); tx->last_head =lan743x_tx_next_index (tx, tx->last_head); } + +netdev_tx_completed_queue (netdev_get_tx_queue (tx->adapter->netdev, + tx->channel_number), + pkts, bytes); }static void lan743x_tx_release_all_descriptors (struct lan743x_tx *tx)
@@ -2003,6 +2016,10 @@ static void lan743x_tx_frame_end(struct lan743x_tx *tx,if (ignore_sync) buffer_info->flags |=TX_BUFFER_INFO_FLAG_IGNORE_SYNC ; +netdev_tx_sent_queue (netdev_get_tx_queue (adapter->netdev, + tx->channel_number), + skb->len); + tx_descriptor = &tx->ring_cpu_ptr[tx->frame_tail]; tx_descriptor->data0 =cpu_to_le32 (tx->frame_data0); tx->frame_tail =lan743x_tx_next_index (tx, tx->frame_tail);
@@ -2276,6 +2293,8 @@ static void lan743x_tx_close(struct lan743x_tx *tx) 0, 1000, 20000, 100);lan743x_tx_release_all_descriptors (tx); +netdev_tx_reset_queue (netdev_get_tx_queue (adapter->netdev, + tx->channel_number)); tx->rqd_descriptors =0 ;
Signature · 1 line
2.34.1
Re: [PATCH net-next v1 5/5] net: lan743x: add byte queue limits support
Quoted 61 lines · Chris Babroski
Report the bytes queued to and completed by each TX ring to BQL.
Besides limiting how much data sits in the TX ring, this lets the stack stop a queue once the bytes in flight exceed the BQL limit. Without it, a queue is only stopped when its 128 entry ring is full, and the TX watchdog only looks at stopped queues. With light traffic it can take tens of seconds after a TX hang to fill the ring, before the watchdog timeout even starts counting.
lan743x_tx_close()frees pending descriptors without completing them, so reset the BQL state there.Assisted-by LLMSigned-off-by Chris Babroski <[email protected]>--- drivers/net/ethernet/microchip/lan743x_main.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)drivers/net/ethernet/microchip/lan743x_main.c +13
-0@@ -1708,10 +1708,23 @@ static int lan743x_tx_next_index(struct lan743x_tx *tx, int index)static void lan743x_tx_release_completed_descriptors (struct lan743x_tx *tx) { +struct lan743x_tx_buffer_info *buffer_info; +unsigned int bytes =0 ; +unsigned int pkts =0 ; +while (le32_to_cpu (*tx->head_cpu_ptr) != (tx->last_head)) { + buffer_info = &tx->buffer_info[tx->last_head]; +if (buffer_info->skb) { + bytes += buffer_info->skb->len; + pkts++; + }lan743x_tx_release_desc (tx, tx->last_head, false); tx->last_head =lan743x_tx_next_index (tx, tx->last_head); } + +netdev_tx_completed_queue (netdev_get_tx_queue (tx->adapter->netdev, + tx->channel_number), + pkts, bytes); }static void
lan743x_tx_release_all_descriptors(struct lan743x_tx *tx)drivers/net/ethernet/microchip/lan743x_main.c +4
-0@@ -2003,6 +2016,10 @@ static void lan743x_tx_frame_end(struct lan743x_tx *tx, if (ignore_sync) buffer_info->flags |= TX_BUFFER_INFO_FLAG_IGNORE_SYNC; +netdev_tx_sent_queue (netdev_get_tx_queue (adapter->netdev, + tx->channel_number), + skb->len); + tx_descriptor = &tx->ring_cpu_ptr[tx->frame_tail]; tx_descriptor->data0 =cpu_to_le32 (tx->frame_data0);drivers/net/ethernet/microchip/lan743x_main.c +0
-0tx->frame_tail =lan743x_tx_next_index (tx, tx->frame_tail);
drivers/net/ethernet/microchip/lan743x_main.c static void lan743x_tx_close(struct lan743x_tx · line 2300
Quoted 8 lines
drivers/net/ethernet/microchip/lan743x_main.c +2
-0@@ -2276,6 +2293,8 @@ static void lan743x_tx_close(struct lan743x_tx *tx) 0, 1000, 20000, 100);lan743x_tx_release_all_descriptors (tx); +netdev_tx_reset_queue (netdev_get_tx_queue (adapter->netdev, + tx->channel_number)); tx->rqd_descriptors =0 ;
Thanks,
Nicolai
Hi Chris
Quoted 9 lines · Chris Babroski
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, butTX_HEADstops advancing. TX MSI-X stops incrementing while RX interrupts continue to increment.
Quoted 8 lines
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.
A TX channel that says started, no error, with TX_HEAD not moving is also what you see when the MAC stops draining the TX FIFO. When it hangs, is the FCT TX FIFO full? Is the MAC in LPI, or paused by the link partner? Does it still happen with EEE and rx pause turned off?
The other place to look is the PCIe link. We had a LAN7430 stall on a BCM2712 root complex that was also platform specific. The RC driver cleared an undocumented bit in its CLKREQ#/L1 control register. That one took the whole MMIO window down, so it is probably a different failure, but it took days of uptime to show up too. Do the Device Status or AER registers on the hung device show anything (UnsupReq, CmpltTO)? Is the link running with ASPM L1 or L1 substates on that platform?
Quoted 1 line
[...]
I will have a look at the actual patches later today and test with our devices.
Regards
Nicolai
Hi Nicolai,
Thanks for taking a look at these patches; will respond to your other comments soon.
Quoted 3 lines
A TX channel that says started, no error, with
TX_HEADnot moving is also what you see when the MAC stops draining the TX FIFO. When it hangs, is the FCT TX FIFO full?
Interestingly, we are consistently seeing FCT_TX_USED=0x34 when reproducing the TX hang. We have FCT_TX_FIFO_END=0x27, default of 20kB, so it is mostly empty when we reproduce. Is there anything special about the 52 byte usage?
Quoted 2 lines
Is the MAC in LPI, or paused by the link partner? Does it still happen with EEE and rx pause turned off?
In all repro cases so far the MAC does not appear to be in LPI and EEE is disabled (MAC_CR=0x200C and EEE_EN is clear). I also tested with "ethtool -A <if> autoneg off rx off tx off" and could still reproduce the tx hang.
We have a PCIe switch connecting the NIC to our SoC and on the switch I can see that the NIC's read requests drop to ~0 implying that it stops fetching TX data and descriptors rather than the MAC stopping queue drain.
Quoted 7 lines
The other place to look is the PCIe link. We had a LAN7430 stall on a BCM2712 root complex that was also platform specific. The RC driver cleared an undocumented bit in its CLKREQ#/L1 control register. That one took the whole MMIO window down, so it is probably a different failure, but it took days of uptime to show up too. Do the Device Status or AER registers on the hung device show anything (UnsupReq, CmpltTO)? Is the link running with ASPM L1 or L1 substates on that platform?
I do think the PCIe link could be related; our current best guess is that there is some issue between the NIC and PCIe switch and we have tickets open with both vendors to help debug (along with setting up EVBs to run comparison tests on).
UnsupReq and CmpltTO appear clean. ASPM and L1 substates are disabled; the link stays in L0 at 2.5 GT/s x1.
Recently we found that after every reproduced hang we see that the NIC sent a NAK to the switch, potentially in response to a CplD from switch while reading TX data and descriptors, but I am not yet able to see the rejected TLP or NAK reason with my setup.
We also found that disabling scatter gather makes this issue significantly harder to reproduce. So it seems that a full TX descriptor ring and the NIC sending a NAK while fetching TX data/descriptors increases repro rate.
Best,
Chris
Hi Chris
Quoted 6 lines · Chris Babroski
[...]
Interestingly, we are consistently seeing FCT_TX_USED=0x34 when reproducing the TX hang. We have FCT_TX_FIFO_END=0x27, default of 20kB, so it is mostly empty when we reproduce. Is there anything special about the 52 byte usage?
Not that I know of. It looks like the head of a scatter-gather frame whose fragments never came in. Does it match the buffer_length of the first active descriptor at tx->last_head?
Quoted 9 lines
Quoted 3 lines
Is the MAC in LPI, or paused by the link partner? Does it still happen with EEE and rx pause turned off?
In all repro cases so far the MAC does not appear to be in LPI and EEE is disabled (MAC_CR=0x200C and
EEE_ENis clear). I also tested with "ethtool -A <if> autoneg off rx off tx off" and could still reproduce the tx hang.
Quoted 4 lines
We have a PCIe switch connecting the NIC to our SoC and on the switch I can see that the NIC's read requests drop to ~0 implying that it stops fetching TX data and descriptors rather than the MAC stopping queue drain.
Which one? Ours sit behind a Pericom PI7C9X2G404.
Quoted 14 lines
Quoted 8 lines
The other place to look is the PCIe link. We had a LAN7430 stall on a BCM2712 root complex that was also platform specific. The RC driver cleared an undocumented bit in its CLKREQ#/L1 control register. That one took the whole MMIO window down, so it is probably a different failure, but it took days of uptime to show up too. Do the Device Status or AER registers on the hung device show anything (UnsupReq, CmpltTO)? Is the link running with ASPM L1 or L1 substates on that platform?
I do think the PCIe link could be related; our current best guess is that there is some issue between the NIC and PCIe switch and we have tickets open with both vendors to help debug (along with setting up EVBs to run comparison tests on).
Quoted 7 lines
UnsupReq and CmpltTO appear clean. ASPM and L1 substates are disabled; the link stays in L0 at 2.5 GT/s x1.
Recently we found that after every reproduced hang we see that the NIC sent a NAK to the switch, potentially in response to a CplD from switch while reading TX data and descriptors, but I am not yet able to see the rejected TLP or NAK reason with my setup.
Then BadTLP/BadDLLP on the NIC and Rollover/Timeout on the switch port should tell what went wrong with the replay. Are those clean too?
My guess: the replay hands the NIC a completion it had already counted, and the DMA engine then waits for data that never comes, with nothing in the error registers.
Lowering MaxReadReq to 128 bytes with setpci would be a cheap way to test that?
Quoted 1 line
[...]
Regards,
Nicolai
Quoted 5 lines
Quoted 1 line
Is there anything special about the 52 byte usage?
Not that I know of. It looks like the head of a scatter-gather frame whose fragments never came in. Does it match the
buffer_lengthof the first active descriptor attx->last_head?
Yes, you're right, tx->last_head is pointing to the header of a TSO frame and the consistent 52 bytes was just an artifact of the test data I was using to reproduce this week which creates a 54-byte header. FCT_TX_USED is 52 bytes, but tx->last_head has a buffer_length of 54, so not sure where the 2 byte difference comes from. TX_HEAD stopped at the frame's payload descriptor, after the TSO extension descriptor.
Quoted 1 line
Which one? Ours sit behind a Pericom PI7C9X2G404.
Oh, we are also using a Diodes switch, the PI7C9X3G606GP.
Quoted 7 lines
Quoted 4 lines
Recently we found that after every reproduced hang we see that the NIC sent a NAK to the switch, potentially in response to a CplD from switch while reading TX data and descriptors, but I am not yet able to see the rejected TLP or NAK reason with my setup.
Then BadTLP/BadDLLP on the NIC and Rollover/Timeout on the switch port should tell what went wrong with the replay. Are those clean too?
Those all appear to be clean too (based on NIC's receive error counters and firmware AER logging). Occasionally we see replay timer timeouts reported from the switch on port connected to NIC, but they occur even without the TX hang and don't always occur when there is a TX hang.
Quoted 6 lines
My guess: the replay hands the NIC a completion it had already counted, and the DMA engine then waits for data that never comes, with nothing in the error registers.
Lowering MaxReadReq to 128 bytes with setpci would be a cheap way to test that?
Unfortunately, we can still reproduce with MaxReadReq set to 128. Using the same test as mentioned previously, the FCT_TX_USED now shows the header plus ~1 KB of payload data instead of 52 bytes.
Thanks,
Chris
Hi Chris
Quoted 6 lines · Chris Babroski
Quoted 5 lines
Quoted 1 line
Is there anything special about the 52 byte usage?
Not that I know of. It looks like the head of a scatter-gather frame whose fragments never came in. Does it match the
buffer_lengthof the first active descriptor attx->last_head?
Quoted 7 lines
Yes, you're right,
tx->last_headis pointing to the header of a TSO frame and the consistent 52 bytes was just an artifact of the test data I was using to reproduce this week which creates a 54-byte header.FCT_TX_USEDis 52 bytes, buttx->last_headhas abuffer_lengthof 54, so not sure where the 2 byte difference comes from.TX_HEADstopped at the frame's payload descriptor, after the TSO extension descriptor.
AN2948 5.2.9 says FCT_TX_USED counts the frame length rounded up to a dword plus the command words, so a 54 byte header should read 56 or more. 52 does not add up, so I would leave that to Microchip.
The same document lists TX_ERR_STS0 with TX_DATA_READ_ERR and TX_DESC_READ_ERR, set when a PCIe read failed "and all retries have been used", and FCT_TXE for a failed FCT sanity check. They land in DMAC_INT_STS bit 21, which the driver never enables or reads. What do TX_ERR_STS0, DMAC_INT_STS and FCT_INT_STS show while it hangs?
DMAC_CFG MAX_READ_REQ is not a size either. It is the number of outstanding TX data reads, 1 to 6, and the driver sets 6. Does the hang survive MAX_READ_REQ = 1?
Quoted 4 lines
Quoted 2 lines
Which one? Ours sit behind a Pericom PI7C9X2G404.
Oh, we are also using a Diodes switch, the PI7C9X3G606GP.
Same family then - and I it's known to have it quirks ...
Ours is the Gen2 part at 2.5 GT/s and has not hung so far, but that port sees little TX load. If you can share the reproducer I can run it here.
Quoted 6 lines
[...]
Quoted 4 lines
My guess: the replay hands the NIC a completion it had already counted, and the DMA engine then waits for data that never comes, with nothing in the error registers.
Quoted 6 lines
Quoted 2 lines
Lowering MaxReadReq to 128 bytes with setpci would be a cheap way to test that?
Unfortunately, we can still reproduce with MaxReadReq set to 128. Using the same test as mentioned previously, the
FCT_TX_USEDnow shows the header plus ~1 KB of payload data instead of 52 bytes.
So not the request size. What is left in the driver is the descriptor prefetch. lan743x_tx_open() sets TX_PF_THRES to 0x10 and TX_PF_PRI_THRES to 4, and the prefetcher works hardest with a full ring, which is when you see the most hangs. Does TX_PF_THRES 1 change the rate?
Quoted 1 line
[...]
Regards,
Nicolai
Hi Nicolai,
Quoted 1 line
What do
TX_ERR_STS0,DMAC_INT_STSandFCT_INT_STSshow while it hangs?
No errors; the following was captured during a hang:
* TX_ERR_STS0=0x0
* RX_ERR_STS0-3=0x0
* DMAC_INT_STS=0xF100
* FCT_INT_STS=0x100000F1
* DMAC_CFG=0x10060
* DMAC_CMD=0x1000F0
* FCT_TX_USED=0x34
* TX_HEAD=0x3C
* TX_TAIL=0x38
Quoted 3 lines
DMAC_CFGMAX_READ_REQis not a size either. It is the number of outstanding TX data reads, 1 to 6, and the driver sets 6. Does the hang surviveMAX_READ_REQ= 1?
So far I have not been able to reproduce with MAX_READ_REQ = 1, but I have reproduced with MAX_READ_REQ = 3+. It's possible I have not run long enough and the issue would still eventually reproduce with MAX_READ_REQ = 1 though.
Quoted 1 line
If you can share the reproducer I can run it here.
I have been able to reproduce consistently just by starting an infinite scp loop to copy a random 50MB file off of the SoC onto a Windows laptop. Usually will reproduce within a few minutes when on the same local network. Something about Windows scp/ssh significantly increases repro rate; we have only reproduced once this way when doing repeated scp from a Linux server.
Regarding the test data I mentioned: I do have a debug kernel module that can reproduce consistently on our custom HW and I included it below for reference.
The test configuration is based on some data we captured while reproducing with the scp loop. It requires that SG and TSO are enabled. See comment at top of file for details and change IFNAME to your device's interface name.
I have not reproduced the TX hang with a LAN743x EVB connected directly to a server using this test module or the scp loop method.
Quoted 5 lines
So not the request size. What is left in the driver is the descriptor prefetch.
lan743x_tx_open()setsTX_PF_THRESto0x10andTX_PF_PRI_THRESto 4, and the prefetcher works hardest with a full ring, which is when you see the most hangs. DoesTX_PF_THRES1 change the rate?
Unfortunately, no. I can still reproduce with TX_PF_THRES set to 1 and TX_PF_PRI_THRES set to 1 as well (tried various combinations of values). With the test module all reproduced on first NAK.
I also tried enabling DMA_CMPL_RETRY_EN and increasing the completion timeout to 17-64s with setpci and could still reproduce quickly with the test module.
Thanks,
Chris
Test module (debug only)
-------------------------
// SPDX-License-Identifier: GPL-2.0-only
/*
* LAN743x TX hang reproducer. DEBUG ONLY.
*
* Sends the same TSO frame to TX queue 0 of IFNAME as fast as the driver
* accepts it, until the module is removed. Each frame is a 54 byte
* Ethernet/IPv4/TCP header in the skb linear area plus one 23560 byte
* payload fragment at page offset 14335 (0x37FF), segmented by the NIC into
* 1240 byte TCP segments. The driver maps this to three TX descriptors:
* header, TSO extension and payload.
*
* Frames are addressed to the interface's own MAC and to 192.0.2.1
* (TEST-NET-1). Needs scatter-gather, TSO and TX checksum offload enabled. *
* If the queue stays stopped for STOP_MS, the sender reports it and stops;
* the module then idles until it is removed. Remove this module before
* unloading the network driver, it holds a reference on the netdev. */
#include <linux/delay.h>
#include <linux/etherdevice.h>
#include <linux/ip.h>
#include <linux/kthread.h>
#include <linux/module.h>
#include <linux/netdevice.h>
#include <linux/skbuff.h>
#include <linux/tcp.h>
#include <net/ip.h>
#include <net/net_namespace.h>
#include <net/tcp.h>
#define IFNAME "eth0"
#define HEADER_LEN (ETH_HLEN + sizeof(struct iphdr) + sizeof(struct tcphdr))
#define PAYLOAD_LEN 23560
#define PAYLOAD_OFFSET 14335
#define GSO_SIZE 1240
#define STOP_MS 1000
static struct task_struct *test_task;
static struct net_device *test_dev;
static struct sk_buff *tx_test_build_skb(struct net_device *dev, u32 sequence)
{
struct ethhdr *eth;
struct tcphdr *th;
struct sk_buff *skb;
struct iphdr *iph;
struct page *page;
unsigned int order;
/* headers in the linear area */
skb = alloc_skb(LL_RESERVED_SPACE(dev) + HEADER_LEN, GFP_KERNEL);
if (!skb)
return NULL;
skb_reserve(skb, LL_RESERVED_SPACE(dev));
skb_put_zero(skb, HEADER_LEN);
skb_reset_mac_header(skb);
skb_set_network_header(skb, ETH_HLEN);
skb_set_transport_header(skb, ETH_HLEN + sizeof(*iph));
eth = eth_hdr(skb);
ether_addr_copy(eth->h_source, dev->dev_addr);
ether_addr_copy(eth->h_dest, dev->dev_addr);
eth->h_proto = htons(ETH_P_IP);
iph = ip_hdr(skb);
iph->version = 4;
iph->ihl = sizeof(*iph) / 4;
iph->tot_len = htons(sizeof(*iph) + sizeof(*th) + PAYLOAD_LEN);
iph->id = htons((u16)sequence);
iph->frag_off = htons(IP_DF);
iph->ttl = 64;
iph->protocol = IPPROTO_TCP;
iph->saddr = htonl(0xc0000202); /* 192.0.2.2 */
iph->daddr = htonl(0xc0000201); /* 192.0.2.1 */
ip_send_check(iph);
th = tcp_hdr(skb);
th->source = htons(12345);
th->dest = htons(22);
th->seq = htonl(sequence * PAYLOAD_LEN);
th->ack_seq = htonl(1);
th->doff = sizeof(*th) / 4;
th->ack = 1;
th->window = htons(U16_MAX);
th->check = ~tcp_v4_check(sizeof(*th) + PAYLOAD_LEN,
iph->saddr, iph->daddr, 0);
/* payload in one page fragment, starting at an unaligned offset */
order = get_order(PAYLOAD_OFFSET + PAYLOAD_LEN);
page = alloc_pages(GFP_KERNEL | __GFP_COMP | __GFP_ZERO, order);
if (!page) {
kfree_skb(skb);
return NULL;
}
skb_add_rx_frag(skb, 0, page, PAYLOAD_OFFSET, PAYLOAD_LEN,
PAGE_SIZE << order);
/* TCP checksum and segmentation offloaded to the NIC */
skb->dev = dev;
skb->protocol = htons(ETH_P_IP);
skb->ip_summed = CHECKSUM_PARTIAL;
skb->csum_start = skb_transport_header(skb) - skb->head;
skb->csum_offset = offsetof(struct tcphdr, check);
skb_set_queue_mapping(skb, 0);
skb_shinfo(skb)->gso_size = GSO_SIZE;
skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(PAYLOAD_LEN, GSO_SIZE);
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
return skb;
}static int tx_test_thread(void *data)
{
struct netdev_queue *txq = netdev_get_tx_queue(test_dev, 0);
unsigned long stopped_since = 0;
struct sk_buff *skb;
u32 sequence = 1;
int ret;
while (!kthread_should_stop()) {
/* the driver stops the queue while its TX ring is full */
if (netif_tx_queue_stopped(txq)) {
if (!stopped_since) {
stopped_since = jiffies;
} else if (time_after(jiffies, stopped_since +
msecs_to_jiffies(STOP_MS))) {
netdev_warn(test_dev,
"tx test: queue 0 remained stopped for %u ms after %u packets\n",
STOP_MS, sequence - 1);
break;
}
usleep_range(1000, 2000);
continue;
}
stopped_since = 0;
skb = tx_test_build_skb(test_dev, sequence);
if (!skb) {
netdev_err(test_dev, "tx test: skb allocation failed\n");
break;
}
ret = dev_queue_xmit(skb);
if (ret != NET_XMIT_SUCCESS && ret != NET_XMIT_CN) {
netdev_err(test_dev,
"tx test: dev_queue_xmit failed after %u packets: %d\n",
sequence - 1, ret);
break;
}
sequence++;
cond_resched();
}
netdev_info(test_dev, "tx test: sender stopped after %u packets\n",
sequence - 1);
while (!kthread_should_stop())
msleep_interruptible(1000);
return 0;
}static int __init tx_test_init(void)
{
int ret;
test_dev = dev_get_by_name(&init_net, IFNAME);
if (!test_dev) {
pr_err("lan743x_tx_test: %s not found\n", IFNAME);
return -ENODEV;
}
if (!netif_running(test_dev) || !netif_carrier_ok(test_dev)) {
netdev_err(test_dev, "tx test: interface must be up with carrier\n");
ret = -ENETDOWN;
goto put_dev;
}
if (!(test_dev->features & NETIF_F_SG) ||
!(test_dev->features & NETIF_F_TSO) ||
!(test_dev->features & NETIF_F_HW_CSUM)) {
netdev_err(test_dev,
"tx test: needs scatter-gather, TSO and TX checksum offload (ethtool -K %s sg on tso on tx on)\n",
IFNAME);
ret = -EOPNOTSUPP;
goto put_dev;
}
netdev_info(test_dev,
"tx test: starting header=%zu payload=%u@%u gso_size=%u\n",
HEADER_LEN, PAYLOAD_LEN, PAYLOAD_OFFSET, GSO_SIZE);
test_task = kthread_run(tx_test_thread, NULL, "lan743x-tx-test");
if (IS_ERR(test_task)) {
ret = PTR_ERR(test_task);
test_task = NULL;
goto put_dev;
}
return 0;put_dev:
dev_put(test_dev); test_dev = NULL; return ret; }
static void __exit tx_test_exit(void)
{
if (test_task)
kthread_stop(test_task);
if (test_dev)
dev_put(test_dev);
}module_init(tx_test_init);module_exit(tx_test_exit);
MODULE_DESCRIPTION("LAN743x TX hang reproducer (DEBUG ONLY)"); MODULE_LICENSE("GPL");
Hi Chris,
Thanks for the patch.
We ([email protected]) are under review and plan to test with our devices of different configurations.
Best regards,
Woojung
Quoted 105 lines
-----Original Message-----
From: Chris Babroski <[email protected]>
Sent: Thursday, October 15, 2026 11:01 AM
To: Bryan Whitehead - C21958 <[email protected]>; UNGLinuxDriver <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hangEXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
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, butTX_HEADstops 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_timeoutsupport 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 newlan743x_dmac_tx_resetandlan743x_dmac_rx_resethelper 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 fulllan743x_netdev_closefollowed bylan743x_netdev_openinstead 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 thendo_tx_timeoutsupport becauselan743x_netdev_openandlan743x_netdev_closeare 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 supportdrivers/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
[email protected]
Tested-by Yuiko OshinoHi Chris,
Quoted 10 lines
Hi Chris,
Thanks for the patch.
We ([email protected]) are under review and plan to test with our devices of different configurations.Best regards,
Woojung
This series is tested with LAN7430, LAN7431+external PHY and PCI11414. No regression is observed by tests including traffic tests, sleep/wakeup, link up/down, cable in/out and etc.
Will retest when revised series is ready.
Thanks
Woojung
Quoted 107 lines
Quoted 4 lines
-----Original Message-----
From: Chris Babroski <[email protected]>
Sent: Thursday, October 15, 2026 11:01 AM
To: Bryan Whitehead - C21958 <[email protected]>;UNGLinuxDriver
Quoted 7 lines
<[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH net-next v1 0/5] net: lan743x: automatically recover fromTX
Quoted 94 lines
hang
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
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, butTX_HEADstops 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_timeoutsupport 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 newlan743x_dmac_tx_resetandlan743x_dmac_rx_resethelper 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 fulllan743x_netdev_closefollowed bylan743x_netdev_openinstead 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 thendo_tx_timeoutsupport becauselan743x_netdev_openandlan743x_netdev_closeare 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 supportdrivers/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
Jakub Kicinski
maintainerQuoted 3 lines · On Thu, 15 Oct 2026 11:00:39 -0400 Chris Babroski wrote:
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
first 3 patches are sort-of fixes so they can be resent during the merge window. The rest will have to wait for net-next to reopen
Quoted 2 lines
net: lan743x: reset the device on TX timeout
net: lan743x: add byte queue limits support
Signature · 1 line
pw-bot: cr
Quoted 3 lines
first 3 patches are sort-of fixes so they can be resent during the merge window. The rest will have to wait for net-next to reopen
Sounds good, thanks.
Chris