[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