Re: [PATCH net-next v3 14/15] net: macb: use context swapping in .set_ringparam()

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

On Thu Jul 2, 2026 at 12:37 PM CEST, Nicolai Buchwitz wrote:
> On 1.7.2026 17:59, Théo Lebrun wrote:
>> ethtool_ops.set_ringparam() is implemented using the primitive close /
>> update ring size / reopen sequence. Under memory pressure this does not
>> fly: we free our buffers at close and cannot reallocate new ones at
>> open. Also, it triggers a slow PHY reinit.
>> 
>> Instead, exploit the new context mechanism and improve our sequence to:
>>  - allocate a new context (including buffers) first
>>  - if it fails, early return without any impact to the interface
>>  - stop interface
>>  - update global state (bp, netdev, etc)
>>  - pass buffer pointers to the hardware
>>  - start interface
>>  - free old context.
>> 
>> The HW disable sequence is inspired by macb_reset_hw() but avoids
>> (1) setting NCR bit CLRSTAT and (2) clearing register PBUFRXCUT.
>> 
>> The HW re-enable sequence is inspired by macb_mac_link_up(), skipping
>> over register writes which would be redundant (because values have not
>> changed).
>> 
>> The generic context swapping parts are isolated into helper functions
>> macb_context_swap_start|end(), reusable by other operations 
>> (change_mtu,
>> set_channels, etc).
>> 
>> Introduce a new locking primitive (mac_cfg_lock mutex) to serialise 
>> swap
>> with phylink MAC callbacks. Avoid stopping phylink to avoid a slow PHY
>> retrain. Those callbacks grab phydev->lock if it exists so we could
>> imagine grabbing that from the swap op, but phydev->lock doesn't exist
>> in the SFP case.
>> 
>> AT91 EMAC is handled differently as their buffer management is separate
>> and they don't do NAPI. We refuse them (-EBUSY) to avoid implementing
>> context swapping for them.
>> 
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
>>  drivers/net/ethernet/cadence/macb.h      |   2 +
>>  drivers/net/ethernet/cadence/macb_main.c | 142 
>> +++++++++++++++++++++++++++++--
>
>> [...]
>
>> +static void macb_context_swap_start(struct macb *bp)
>> +{
>> +	struct macb_queue *queue;
>> +	unsigned long flags;
>> +	unsigned int q;
>> +	u32 ctrl;
>> +
>> +	mutex_lock(&bp->mac_cfg_lock);
>> +
>> +	/* Mask interrupts before disabling BH features. */
>> +	spin_lock_irqsave(&bp->lock, flags);
>> +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> +		queue_writel(queue, IDR, -1);
>> +		queue_readl(queue, ISR);
>> +		macb_queue_isr_clear(bp, queue, -1);
>> +	}
>> +	spin_unlock_irqrestore(&bp->lock, flags);
>> +
>> +	/* Drain BH features. HW is still active and usable at this point. */
>> +
>> +	cancel_work_sync(&bp->hresp_err_bh_work);
>> +	cancel_delayed_work_sync(&bp->tx_lpi_work);
>> +
>> +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>> +		napi_disable(&queue->napi_rx);
>> +		napi_disable(&queue->napi_tx);
>> +		cancel_work_sync(&queue->tx_error_task);
>> +		netdev_tx_reset_queue(netdev_get_tx_queue(bp->netdev, q));
>> +	}
>
> Can this deadlock against a pending tx_error_task?
>
> AFAIU macb_tx_error_task() does napi_disable(&queue->napi_tx) and later
> napi_enable() on the same napi, and it can already be queued 
> (macb_interrupt()
> schedules it on a TX error) by the time the swap runs:
>
>    swap_start: napi_disable(napi_tx)          /* sets SCHED, returns */
>    worker:     tx_error_task: napi_disable(napi_tx)  /* spins on SCHED */
>    swap_start: cancel_work_sync(tx_error_task)       /* waits on worker 
> */
>
> napi_disable() spins until napi_enable() clears SCHED, but here the swap 
> won't
> re-enable until macb_context_swap_end(), and cancel_work_sync() is 
> what's
> holding it up. Nothing clears it.
>
> Maybe cancel_work_sync() before the napi_disable() calls would work 
> instead? IRQs
> are masked just above, so AFAICT nothing can reschedule tx_error_task by 
> then.

Ah yes, good catch. macb_tx_error_task() should never enter if NAPI is
disabled, else its napi_disable() will hang.

The fix sounds easy enough, as you indicated. We'll move the
cancel_work_sync(queue->tx_error_task) call to be above our swap start
napi_disable().

Thanks,

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