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

[Nicolai Buchwitz <nb@tipi-net.de>]

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