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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 5 (1) · Low: 0

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

- [Medium] The commit message says lan743x_dmac_init() now returns the
  soft-reset poll error so that the work cannot reopen a controller…
- [Medium] Before this patch, lan743x_hardware_init() could never fail.
- [Medium] On boards that use the driver's fixed-link fallback (LAN7431
  or PCI11x1x with no DT node and no PHY on the MDIO bus), the reopen…
- [Medium] lan743x_reset_work() holds rtnl and keeps the device detached
  from netif_device_detach() until netif_device_attach().
- [Medium] lan743x_reset_work() runs the full lan743x_netdev_close() on a
  controller it already treats as wedged.

Pre-existing issues:
- [Medium] After the automatic TX-timeout reset, tx->ts_flags still
  contains TX_TS_FLAG_ONE_STEP_SYNC.

--

net: lan743x: reset the device on TX timeout

This adds an ndo_tx_timeout handler. The handler schedules a work item
that, under rtnl, detaches the netdev, closes it, soft-resets the DMA
controller, reopens it and reattaches it. It also makes
lan743x_dmac_init() return the soft-reset poll error, and routes
ndo_stop through a wrapper that skips the hardware close while the device
is detached.

> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 8d256a80dac64..2545a846478d6 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;

[Severity: Medium]
The commit message only talks about probe here. However,
lan743x_hardware_init() is also called from lan743x_pm_resume().

Before this change lan743x_hardware_init() could not fail in practice,
so the resume error branch was dead code. Does this new return make that
branch reachable?

lan743x_pm_resume() {
    ...
	ret = lan743x_hardware_init(adapter, pdev);
	if (ret) {
		netif_err(adapter, probe, adapter->netdev,
			  "lan743x_hardware_init returned %d\n", ret);
		lan743x_pci_cleanup(adapter);
		return ret;
	}
    ...
}

If that branch runs, the netdev stays registered and detached. A later
unbind then runs:

lan743x_pcidev_remove()
  lan743x_full_cleanup()
    lan743x_hardware_cleanup()   <- INT_EN_CLR write to a disabled device
    lan743x_pci_cleanup()        <- second region release and disable

Would that release the PCI regions twice and trigger the "disabling
already-disabled device" warning?

> +
>  	switch (DEFAULT_DMA_DESCRIPTOR_SPACING) {
>  	case DMA_DESCRIPTOR_SPACING_16:
>  		data = DMAC_CFG_MAX_DSPACE_16_;

[ ... ]

> @@ -3431,6 +3441,61 @@ static int lan743x_netdev_open(struct net_device *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);

[Severity: Medium]
Here lan743x_netdev_close() runs on a controller that is already assumed
to be wedged. The controller-wide DMAC_CMD_SWR_ reset only happens later,
in lan743x_dmac_init().

In lan743x_tx_close(), suppose the channel does not stop and the
per-channel reset also times out. The error is only logged, and the ring
is freed anyway:

lan743x_tx_close() {
    ...
		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_tx_release_all_descriptors(tx);
    ...
	lan743x_tx_ring_cleanup(tx);
}

lan743x_rx_close() does the same for the RX ring, the head write-back
buffer and the RX skbs.

If the engine is still running at that point, could it write head indices
or received frames into memory that was already freed and maybe reused?
Should the DMAC soft reset happen before the rings are torn down?

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. The same
close/open sequence leaves the TX one-step state inconsistent, and with
this patch the watchdog can trigger that sequence automatically.

lan743x_ptp_open() calls lan743x_ptp_reset() and sets the RX filter back
to HWTSTAMP_FILTER_NONE. However, tx->ts_flags still has
TX_TS_FLAG_ONE_STEP_SYNC, and nothing turns one-step Sync insertion back
on in the hardware.

After the reset, lan743x_ptp_hwtstamp_get() still reports:

	if (tx->ts_flags & TX_TS_FLAG_ONE_STEP_SYNC)
		config->tx_type = HWTSTAMP_TX_ONESTEP_SYNC;

and lan743x_tx_xmit_frame() still sets ignore_sync for Sync frames:

		if (tx->ts_flags & TX_TS_FLAG_ONE_STEP_SYNC)
			ignore_sync = true;

Can Sync frames go out without a hardware-inserted origin timestamp until
the application sets up timestamping again?

Admin down/up and suspend/resume already have the same mismatch. The
commit message also documents it ("one-step sync insertion is disabled,
while the driver keeps its TX timestamping flags"). The PHC is
unregistered and registered again, so applications have to reconfigure.
Reconfiguring applies one-step mode again through
lan743x_ptp_hwtstamp_set().

> +
> +	ret = lan743x_dmac_init(adapter);
> +	if (ret) {
> +		netdev_err(netdev, "DMA controller reset failed: %d\n", ret);
> +		goto close;
> +	}

[Severity: Medium]
According to the commit message, lan743x_dmac_init() now returns the poll
error so the work does not reopen a controller whose reset timed out. It
also says the interface is taken down "so it can be brought back up
later".

lan743x_netdev_open() never calls lan743x_dmac_init(). That function is
only called from lan743x_hardware_init() (probe and resume) and from this
work.

So if the user runs "ip link set up" after this failure, does
lan743x_netdev_open() reopen the same controller that never came out of
reset?

The early return in lan743x_dmac_init() also skips the DMAC_CFG,
DMAC_COAL_CFG and DMAC_OBFF_CFG writes. Nothing on the open path writes
them again.

Should the open path retry the DMAC reset, or should the device be kept
from coming back up in this case?

> +
> +	ret = lan743x_netdev_open(netdev);
> +	if (ret) {
> +		netdev_err(netdev, "failed to reopen after reset: %d\n", ret);
> +		goto close;
> +	}

[Severity: Medium]
Some boards use the fixed-link fallback in lan743x_phylink_connect():
LAN7431 or PCI11x1x with no DT node and no PHY on the MDIO bus. Can this
reopen ever succeed on those boards?

On the first open, phylink_set_fixed_link() sets
pl->cfg_link_an_mode = MLO_AN_FIXED. Neither phylink_stop() nor
phylink_disconnect_phy() resets it. So on the reopen:

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

lan743x_phylink_connect() then logs "Could not set fixed link" and
lan743x_netdev_open() fails. The work jumps to close: and calls
dev_close(). Every later attempt to bring the interface up fails the same
way.

The fixed-link handling is existing code. On these boards, though, the
new recovery turns a stalled TX queue into an interface that cannot come
back up until the driver is probed again.

> +
> +	goto attach;
> +
> +close:
> +	dev_close(netdev);
> +attach:
> +	netif_device_attach(netdev);

[Severity: Medium]
While the device is detached under rtnl here, multicast list writers that
do not take rtnl can still change dev->mc. The IPv6 socket join path is
one example:

ipv6_sock_mc_join()
  igmp6_group_added()
    dev_mc_add()
      __dev_set_rx_mode()
	if (!netif_device_present(dev))
		return;

These changes never reach the hardware. After netif_device_attach(),
nothing reprograms the filter. lan743x_rfe_open() only programs RSS, and
lan743x_rfe_set_multicast() is only called from ndo_set_rx_mode.

The normal open path resyncs because __dev_open() calls dev_set_rx_mode()
after ndo_open. This path has nothing equivalent.

Can the RFE filter get out of sync with dev->mc? Groups joined during the
reset would not be received, and groups left would stay in the filter,
until the next rx_mode change. Would a call to dev_set_rx_mode() after
the reopen fix this?

> +unlock:
> +	rtnl_unlock();
> +}

[ ... ]

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

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

[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