[PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang

[PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang

[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, but TX_HEAD stops advancing. TX MSI-X stops
incrementing while RX interrupts continue to increment.

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

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

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

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

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

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

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

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

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

base-commit: 05b5019b01fb9443b09ae4ed34af1135a80a84c8
-- 
2.34.1

[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

[PATCH net-next v1 2/5] net: lan743x: start phylink last and stop it first

[Chris Babroski]

lan743x_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.

Fixes: a5f199a8d8a0 ("net: lan743x: Migrate phylib to phylink")
Assisted-by: LLM
Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

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

[PATCH net-next v1 3/5] net: lan743x: quiesce TX before freeing the rings

[Chris Babroski]

lan743x_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.

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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 583f631df701..8d256a80dac6 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -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]);
 
-- 
2.34.1

[PATCH net-next v1 4/5] net: lan743x: reset the device on TX timeout

[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_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.

Assisted-by: LLM
Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 74 ++++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  2 +
 2 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 8d256a80dac6..2545a846478d 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -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);
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index d9495cf96b41..2697f0a2a83b 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -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 <Bryan.Whitehead@microchip.com>"
@@ -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;
-- 
2.34.1

[PATCH net-next v1 5/5] net: lan743x: add byte queue limits support

[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: LLM
Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
---
 drivers/net/ethernet/microchip/lan743x_main.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 2545a846478d..7739fa49fc44 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -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;
 
-- 
2.34.1

Re: [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang

[Nicolai Buchwitz]

Hi Chris

On 15.10.2026 17:00, Chris Babroski wrote:
> 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.

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?

> [...]

I will have a look at the actual patches later today and test with
our devices.

Regards
Nicolai

Re: [PATCH net-next v1 2/5] net: lan743x: start phylink last and stop it first

[Nicolai Buchwitz]

On 15.10.2026 17:00, Chris Babroski wrote:
> lan743x_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.
> 
> Fixes: a5f199a8d8a0 ("net: lan743x: Migrate phylib to phylink")
> Assisted-by: LLM
> Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
> ---
>  drivers/net/ethernet/microchip/lan743x_main.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c 
> b/drivers/net/ethernet/microchip/lan743x_main.c
> index 9e1276cf313c..583f631df701 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -3334,6 +3334,8 @@ static int lan743x_netdev_close(struct net_device 
> *netdev)
>  	struct lan743x_adapter *adapter = netdev_priv(netdev);
>  	int index;
> 
> +	lan743x_phylink_disconnect(adapter);
> +
>  	for (index = 0; index < adapter->used_tx_channels; index++)
>  		lan743x_tx_close(&adapter->tx[index]);
> 
> @@ -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);

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net-next v1 3/5] net: lan743x: quiesce TX before freeing the rings

[Nicolai Buchwitz]

On 15.10.2026 17:00, Chris Babroski wrote:
> lan743x_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.
> 
> 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 | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c 
> b/drivers/net/ethernet/microchip/lan743x_main.c
> index 583f631df701..8d256a80dac6 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -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]);

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net-next v1 5/5] net: lan743x: add byte queue limits support

[Nicolai Buchwitz]

On 15.10.2026 17:00, Chris Babroski wrote:
> 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: LLM
> Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
> ---
>  drivers/net/ethernet/microchip/lan743x_main.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c 
> b/drivers/net/ethernet/microchip/lan743x_main.c
> index 2545a846478d..7739fa49fc44 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -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;

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

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 4/5] net: lan743x: reset the device on TX timeout

[Nicolai Buchwitz]

Hi Chris

On 15.10.2026 17:00, Chris Babroski wrote:
> 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_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.

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.

> 
> 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.
> 
> Assisted-by: LLM
> Signed-off-by: Chris Babroski <cbabroski@nvidia.com>
> ---
>  drivers/net/ethernet/microchip/lan743x_main.c | 74 ++++++++++++++++++-
>  drivers/net/ethernet/microchip/lan743x_main.h |  2 +
>  2 files changed, 73 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c 
> b/drivers/net/ethernet/microchip/lan743x_main.c
> index 8d256a80dac6..2545a846478d 100644

> [...]

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

Unrelated to this series, but lan743x_pm_resume() seems to ignore this
return value.

> [...]

Thanks,
Nicolai

Re: [PATCH net-next v1 2/5] net: lan743x: start phylink last and stop it first

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (2) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Moving phylink_stop() to the start of lan743x_netdev_close()
  relies on mac_link_down() (lan743x_phylink_mac_link_down ->…
- [High] On the suspend path, lan743x_pcidev_shutdown() calls
  netif_device_detach() (lockless netif_tx_stop_all_queues) and then…

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

--

net: lan743x: start phylink last and stop it first

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

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

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

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

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

On the suspend path the qdisc is still attached:

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

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

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

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

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

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

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

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

which waits on:

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

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

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

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

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

RE: [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang

[Woojung.Huh]

Hi Chris,

Thanks for the patch.
We (UNGLinuxDriver@microchip.com) are under review and
plan to test with our devices of different configurations.

Best regards,
Woojung

> -----Original Message-----
> From: Chris Babroski <cbabroski@nvidia.com>
> Sent: Thursday, October 15, 2026 11:01 AM
> To: Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; andrew+netdev@lunn.ch; davem@davemloft.net;
> edumazet@kernel.org; kuba@kernel.org; pabeni@redhat.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> richardcochran@gmail.com; linux@armlinux.org.uk
> Cc: maxime.chevallier@bootlin.com; Raju.Lakkaraju@microchip.com;
> cbabroski@nvidia.com; davthompson@nvidia.com
> Subject: [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX
> 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, but TX_HEAD stops advancing. TX MSI-X stops
> incrementing while RX interrupts continue to increment.
> 
> This scenario has most often been reproduced in large server cluster
> environments, but it's also been seen locally during repeated SCP and
> iperf testing.
> 
> The root cause of this issue is still unknown and we are continuing
> debug. It was found that bringing the interface down and up recovers the
> network. This issue has not been reproduced with the LAN7430 EVB
> connected to a standard Linux server.
> 
> What this patch series does
> ---------------------------
> This patch series attempts to implement automatic recovery of the
> network interface after a TX hang is detected. It does not fix the
> underlying root cause of the TX hang.
> 
> At a high level, the patches implement the following:
> 1. ndo_tx_timeout support to reset and recover the interface when the
>    netdev TX watchdog triggers.
> 2. BQL support to decrease the amount of time it takes for the watchdog
>    to trigger.
> 3. Stability improvements and error handling around the open/close paths
>    that now get executed more frequently as part of the TX hang recovery
>    flow.
> 
> Potential issues and concerns
> -----------------------------
> * One of the patches adds new lan743x_dmac_tx_reset and
>   lan743x_dmac_rx_reset helper functions. There is no cleanup handling
>   introduced if these return an error, only a new error log. This
>   matches pre-existing behavior: if a reset timeout occurs then it is
>   possible to DMA freed memory. The datasheet states that these resets
>   are performed immediately in HW so I'm not sure if these will ever
>   time out, but I'm open to suggestions on how we should handle an error
>   in these cases.
> * The TX timeout reset flow does a full lan743x_netdev_close followed by
>   lan743x_netdev_open instead of individual TX channel reset. If the
>   stall is happening inside the NIC DMA engine then the theory is that a
>   full reset may be needed to recover. For internal use we also wanted
>   to reduce risk by putting the NIC into a clean state after recovery
>   with full reset. It would be great to have Microchip/LAN743x experts
>   confirm the best reset flow.
> * LLM review tools flagged several possible bugs during development
>   that resulted in the inclusion of three "Fixes:" tagged patches in this
>   series. They are included in this series with the ndo_tx_timeout
>   support because lan743x_netdev_open and lan743x_netdev_close are now
>   called more frequently as a result of TX hang recovery.
> 
> Testing
> -------
> Testing was performed on a 7.0 based kernel using the same driver code.
> Please let me know if you would like me to run any other tests.
> 
> Tests:
> * Verified the TX timeout watchdog triggers and recovers the network
>   interface when TX hang is reproduced. This was done as part of
>   repeated SCP loop testing and is also being tested in a server cluster
>   environment. dmesg was inspected for "NETDEV WATCHDOG" to verify that
>   the watchdog is being triggered. No errors were seen in dmesg after
>   reset.
> * Performance tests using iperf3 to measure RX and TX throughput show no
>   noticeable difference with the patches applied.
> * Repeated module load and unload cycles produce no errors in dmesg.
>   Network appears to function as expected when module is loaded.
> * Repeated ifconfig down and up cycles produce no errors in dmesg.
>   Network appears to function as expected when interface is back up.
> 
> Chris Babroski (5):
>   net: lan743x: reset DMA channels that fail to stop
>   net: lan743x: start phylink last and stop it first
>   net: lan743x: quiesce TX before freeing the rings
>   net: lan743x: reset the device on TX timeout
>   net: lan743x: add byte queue limits support
> 
>  drivers/net/ethernet/microchip/lan743x_main.c | 177 +++++++++++++++---
>  drivers/net/ethernet/microchip/lan743x_main.h |   2 +
>  2 files changed, 155 insertions(+), 24 deletions(-)
> 
> base-commit: 05b5019b01fb9443b09ae4ed34af1135a80a84c8
> --
> 2.34.1

Re: [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang

[Chris Babroski]

Hi Nicolai,

Thanks for taking a look at these patches; will respond to your other
comments soon.

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

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?

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

> 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

Re: [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang

[Jakub Kicinski]

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

>   net: lan743x: reset the device on TX timeout
>   net: lan743x: add byte queue limits support
-- 
pw-bot: cr

Re: [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang

[Nicolai Buchwitz]

Hi Chris

On 20.10.2026 22:36, Chris Babroski wrote:
> [...]

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

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

Which one? Ours sit behind a Pericom PI7C9X2G404.

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

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?

> [...]

Regards,
Nicolai

RE: [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang

[Chris Babroski]

> 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

RE: [PATCH net-next v1 3/5] net: lan743x: quiesce TX before freeing the rings

[Woojung.Huh]

Hi Chris,

..

> @@ -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?

> +
>         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 0/5] net: lan743x: automatically recover from TX hang

[Chris Babroski]

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

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.

> Which one? Ours sit behind a Pericom PI7C9X2G404.

Oh, we are also using a Diodes switch, the PI7C9X3G606GP.

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

> 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

Re: [PATCH net-next v1 0/5] net: lan743x: automatically recover from TX hang

[Nicolai Buchwitz]

Hi Chris

On 21.10.2026 21:23, Chris Babroski wrote:
>>> 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?
> 
> 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.

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?

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

> [...]

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

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?

> [...]

Regards,
Nicolai

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

[Nicolai Buchwitz]

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

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?

> [...]

Thanks,
Nicolai

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

[Woojung.Huh]

Hi Nicola

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

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

[Woojung.Huh]

Hi Chris & Nicolai,

...

> > @@ -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.
> 
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

[Woojung.Huh]

Hi Nicolai & Chris,

> > > 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);
> > > +     }
> >
> > 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?
> >
> 
> 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.
> 

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 0/5] net: lan743x: automatically recover from TX hang

[Chris Babroski]

Hi Nicolai,

> What do TX_ERR_STS0, DMAC_INT_STS and FCT_INT_STS show 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

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

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.

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

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

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");