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

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Jakub,

On Tue Apr 14, 2026 at 2:56 AM CEST, Jakub Kicinski wrote:
> 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?

Yes and we already have the solution in macb_halt_tx(), used by
macb_tx_error_task(). Close and swap should also use it. Will fix.

Though from the manual's description of TE it sounds super unlikely.

Rx isn't an issue, modifying RE in NCR is the way to go.

> 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?

Wrong because napi_disable() grabs the netdev->lock mutex.

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

Yes, I have a pending patch for this. Will send soon. It is a bugfix
fixing the close codepath and we'll reuse the code in the swap
operation.

> 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?

Ignoring because we will refuse swap for EMAC devices. Their state is
too different; they have different ndo callbacks for everything and
don't use NAPI.

> 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?

Yes swap_end needs to wrap its access in the bp->lock spinlock to have
the read-modify-write operation be atomic.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com