Re: [PATCH net-next v2 14/14] net: macb: use context swapping in .ndo_change_mtu()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: macb: use context swapping in .ndo_change_mtu()

This commit introduces a mechanism to perform context swapping when changing
the MTU while the interface is running. It allocates a new context,
reconfigures the hardware via a start and end swap process, and then frees
the old context to avoid PHY reinitialization and gracefully handle memory
pressure.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d1536cb1e40bb..79371718f8392 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3430,11 +3430,25 @@ static int macb_close(struct net_device *netdev)
>  
>  static int macb_change_mtu(struct net_device *netdev, int new_mtu)
>  {
> -	if (netif_running(netdev))
> -		return -EBUSY;
> +	struct macb *bp = netdev_priv(netdev);
> +	bool running = netif_running(netdev);
> +	struct macb_context *new_ctx;
> +
> +	if (running) {
> +		new_ctx = macb_context_alloc(bp, new_mtu,
> +					     bp->configured_rx_ring_size,
> +					     bp->configured_tx_ring_size);
> +		if (IS_ERR(new_ctx))
> +			return PTR_ERR(new_ctx);
> +
> +		macb_context_swap_start(bp);

Does macb_context_swap_start() gracefully halt the transmitter before the
context is freed?

It appears to attempt disabling the hardware transmitter by clearing the
TE and RE bits in the NCR register, but does not poll the hardware to ensure
it has reached an idle state.

Immediately after, macb_context_swap_end() frees the descriptor rings and data
buffers via macb_free_consistent().

If the MAC DMA engine is still mid-transfer, could it read or write to kernel
memory that has already been freed or reallocated?

Can this code cause an AB-BA deadlock between napi_disable() and
cancel_work_sync()?

In macb_context_swap_start(), the code calls napi_disable() for all queues,
and then immediately calls cancel_work_sync() on the error tasks.

Both of these work items internally invoke napi_disable().

If either error task runs concurrently, it will block forever inside its
napi_disable() waiting for the NAPI state bits (which were already set by the
first call in macb_context_swap_start()) to clear.

Consequently, could cancel_work_sync() hang indefinitely waiting for the
blocked work task to finish?

>  	}
>  
>  	WRITE_ONCE(netdev->mtu, new_mtu);
>  
> +	if (running)
> +		macb_context_swap_end(bp, new_ctx);

Does macb_context_swap_end() leak pending TX buffers and their DMA mappings?

When changing the MTU while the interface is running, macb_context_swap_end()
calls macb_free_consistent() to free the old DMA context. While RX buffers are
properly freed, for the TX queues it appears to only free the tracking array
pointer without iterating through the ring to unmap the DMA mappings and free
the actual buffers.

Any packets queued in the hardware ring at the moment of the MTU swap might
be permanently leaked, which could lead to memory and IOMMU space exhaustion
under active traffic.

Will MTU changes on EMAC devices permanently disable hardware interrupts?

macb_context_swap_start() disables all interrupts unconditionally by writing
-1 to the IDR register.

However, macb_context_swap_end() restores interrupts via the IER register
inside an explicit check:

    if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
        ...
    }

Since there is no else branch to re-enable interrupts for EMAC devices,
could changing the MTU on these devices leave interrupts permanently masked
and cause the network interface to silently stall?

Is there a missing spinlock when modifying the IER and NCR registers in
macb_context_swap_end()?

Modifications to the NCR and interrupt registers in macb_context_swap_start()
are protected by the spinlock. However, macb_context_swap_end() performs
lockless read-modify-write operations on both IER and NCR:

    ctrl = macb_readl(bp, NCR);
    macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));

If concurrent hardware events or background tasks take the lock and modify
NCR during this window, could their updates be silently overwritten?

> +
>  	return 0;
>  }