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

[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 .set_ringparam()

This patch modifies the ethtool set_ringparam implementation to use a
context swapping mechanism. Instead of closing and reopening the interface,
it allocates new buffers, stops the interface temporarily, swaps the active
context, and then frees the old one.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 3deab87c565b5..d1536cb1e40bb 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -3081,6 +3081,89 @@ static void macb_configure_dma(struct macb *bp)
[ ... ]
> +static void macb_context_swap_start(struct macb *bp)
> +{
[ ... ]
> +	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> +		napi_disable(&queue->napi_rx);
> +		napi_disable(&queue->napi_tx);

This isn't a regression from this patch, but does calling napi_disable()
unconditionally crash on EMAC devices?

EMAC devices appear to not initialize NAPI, which leaves the NAPI struct
zero-initialized (n->dev is NULL), potentially causing a NULL pointer
dereference in napi_disable(). Additionally, does this path also blindly
call bp->macbgem_ops.mog_init_rings() which is NULL on EMAC?

> +		netdev_tx_reset_queue(netdev_get_tx_queue(bp->netdev, q));
> +		cancel_work_sync(&queue->tx_error_task);
> +	}
[ ... ]
> +static void macb_context_swap_end(struct macb *bp,
> +				  struct macb_context *new_ctx)
> +{
[ ... ]
> +	/* Swap contexts & give buffer pointers to HW. */
> +
> +	old_ctx = bp->ctx;
> +	bp->ctx = new_ctx;
> +	macb_init_buffers(bp);

Since the new context swap doesn't call phylink_stop(), could a concurrent
macb_mac_link_up() execution set the RE and TE bits in NCR during the window
where bp->lock is dropped?

If so, macb_init_buffers() would write new DMA ring base addresses to RBQP
and TBQP while the MAC is actively enabled, which violates hardware
specifications and could cause DMA corruption.

[ ... ]
> +	if (!(bp->caps & MACB_CAPS_MACB_IS_EMAC)) {
> +		for (q = 0, queue = bp->queues; q < bp->num_queues;
> +		     ++q, ++queue) {
> +			queue_writel(queue, IER,
> +				     bp->rx_intr_mask |
> +				     MACB_TX_INT_FLAGS |
> +				     MACB_BIT(HRESP));
> +		}
> +	}

Hardware interrupts are disabled unconditionally for all devices in
macb_context_swap_start() by writing -1 to IDR.

Will this explicit check prevent EMAC devices from having their interrupts
re-enabled?

Since EMAC devices do not use NAPI and rely on hardware interrupts, leaving
them masked might silently halt all network traffic after a ring parameter
change.

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

Can this unprotected read-modify-write of the NCR register race with phylink
state changes?

If an asynchronous phylink callback like macb_mac_config() modifies NCR
while holding bp->lock, this unlocked write could overwrite and drop those
concurrent updates.

> +
> +	netif_tx_start_all_queues(bp->netdev);
> +
> +	/* Free old context. */
> +
> +	macb_free_consistent(old_ctx);

This isn't a regression from this patch, but does this context swap leak
pending TX packets and their DMA mappings?

When freeing the old context, macb_free_consistent() appears to only free
the pointer array. If it doesn't iterate from txq->tail to txq->head to
unmap the active DMA mappings and free the sk_buff structs, repeatedly
modifying ring parameters could cause a memory and DMA mapping leak.