Re: [PATCH net-next v4 10/13] net: lan966x: add PCIe FDMA MTU change support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Race condition in NAPI enable ordering causes permanent RX/TX stall.
- [High] Calling `napi_synchronize()` before `napi_disable()` can cause an infinite loop and soft lockup.
- [Medium] Unconditional queue wakeup in `lan966x_fdma_pci_wakeup_netdev` violates link state semantics.
--

commit 803b55efb87afad3a441075a2a92444e18f35bb2
Author: Daniel Machon <daniel.machon@microchip.com>

net: lan966x: add PCIe FDMA MTU change support

This commit adds MTU change support for the PCIe FDMA path. It reallocates
the contiguous ATU-mapped RX and TX buffers with the new size when the MTU
changes, falling back to reusing the reset buffers on allocation failure.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> index 2e88d211073d6..0568251a95d92 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c

[ ... ]

> @@ -361,9 +366,155 @@ static int lan966x_fdma_pci_init(struct lan966x *lan966x)
>  	return 0;
>  }

[ ... ]

> +/* Wake all TX queues on every port (undoes lan966x_fdma_pci_stop_netdev). */
> +static void lan966x_fdma_pci_wakeup_netdev(struct lan966x *lan966x)
> +{
> +	for (int i = 0; i < lan966x->num_phys_ports; ++i) {
> +		struct lan966x_port *port = lan966x->ports[i];
> +
> +		if (port)
> +			netif_tx_wake_all_queues(port->dev);
> +	}
> +}

Should this verify the link state before unconditionally waking the queues?

If a port is administratively down or has lost carrier, its queues should
likely remain stopped. Unconditionally waking them here might force the
network stack to queue packets to a disabled interface.

> +static int lan966x_fdma_pci_reload(struct lan966x *lan966x, int new_mtu)
> +{
> +	struct fdma tx_fdma_old = lan966x->tx.fdma;
> +	struct fdma rx_fdma_old = lan966x->rx.fdma;
> +	u32 old_mtu = lan966x->rx.max_mtu;
> +	int err;
> +
> +	napi_synchronize(&lan966x->napi);
> +	napi_disable(&lan966x->napi);

Does calling napi_synchronize() before napi_disable() risk an infinite loop
here?

Since the hardware RX and TX channels are still active at this point,
network traffic could continue generating interrupts that repeatedly
re-schedule the NAPI. This would continuously assert the NAPI_STATE_SCHED
bit.

Because napi_synchronize() spins as long as NAPI_STATE_SCHED is set, could
this hang indefinitely and cause a soft lockup? It looks like relying solely
on napi_disable() here would be safer.

> +	lan966x_fdma_pci_stop_netdev(lan966x);
> +	lan966x_fdma_rx_disable(&lan966x->rx);
> +	lan966x_fdma_tx_disable(&lan966x->tx);

[ ... ]

> +	/* Free and unmap old memory. */
> +	fdma_free_coherent_and_unmap(lan966x->dev, &rx_fdma_old);
> +	fdma_free_coherent_and_unmap(lan966x->dev, &tx_fdma_old);
> +
> +	/* Keep this order: rx_start, wakeup_netdev, napi_enable. */
> +	lan966x_fdma_rx_start(&lan966x->rx);
> +	lan966x_fdma_pci_wakeup_netdev(lan966x);
> +	napi_enable(&lan966x->napi);

Can this specific ordering cause a permanent stall?

napi_disable() leaves the NAPI_STATE_SCHED bit set until napi_enable() clears
it. If a hardware interrupt fires right after lan966x_fdma_rx_start() but
before napi_enable(), napi_schedule() will return false and fail to queue the
NAPI.

The interrupt handler would then mask the device interrupt. When
napi_enable() subsequently runs, it clears NAPI_STATE_SCHED, but the NAPI
remains unqueued and the hardware interrupt remains permanently masked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-lan966x-pci-fdma-v4-0-14e0c89d8d63@microchip.com?part=10