Re: [PATCH net-next v6 7/7] net: macb: introduce ndo_xdp_xmit support

[Jakub Kicinski <kuba@kernel.org>]

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: introduce ndo_xdp_xmit support

This commit introduces ndo_xdp_xmit support for the macb driver to
enable XDP redirection. It adds the gem_xdp_xmit function and updates
macb_xdp_submit_frame to handle DMA mapping for redirected frames.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 537d02264851..38b15d1e7127 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -1708,8 +1723,8 @@ static int macb_xdp_submit_frame(struct macb *bp, struct xdp_frame *xdpf,
>  	desc = macb_tx_desc(queue, queue->tx_head);
>  	tx_buff = macb_tx_buff(queue, queue->tx_head);
>  	tx_buff->ptr = xdpf;
> -	tx_buff->type = MACB_TYPE_XDP_TX;
> -	tx_buff->mapping = 0;
> +	tx_buff->type = buff_type;
> +	tx_buff->mapping = dma_map ? mapping : 0;
>  	tx_buff->size = xdpf->len;

Is a bounds check needed on the frame length here?

Since ndo_xdp_xmit can receive frames redirected from other devices,
the frame length could potentially be up to ~64KB on architectures with
64KB pages.

macb_xdp_submit_frame() casts this length to a 32-bit integer and
bitwise-ORs it into the descriptor's control word, which has a 14-bit
length field (maximum 16383 bytes). Frame lengths exceeding this limit
could silently overflow into adjacent control bits like TX_LAST,
corrupting the descriptor ring.

[ ... ]

> +static int gem_xdp_xmit(struct net_device *dev, int num_frame,
> +			struct xdp_frame **frames, u32 flags)
> +{
> +	struct macb *bp = netdev_priv(dev);
> +	u32 xmitted = 0;
> +	int i;
> +
> +	if (!macb_is_gem(bp))
> +		return -EOPNOTSUPP;
> +
> +	if (unlikely(!netif_carrier_ok(dev)))
> +		return -ENETDOWN;
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +		return -EINVAL;
> +
> +	for (i = 0; i < num_frame; i++) {
> +		if (macb_xdp_submit_frame(bp, frames[i], dev, true, 0))
> +			break;

Should this loop batch the doorbell rings based on the XDP_XMIT_FLUSH flag?

Currently, the code calls macb_xdp_submit_frame() for each frame, which
independently acquires the global bp->lock and performs an MMIO write to the
NCR register to ring the hardware doorbell per packet.

Doing this per packet ignores the batching semantics of the bulk transmission
API and could cause severe lock contention and MMIO overhead.

> +
> +		xmitted++;
> +	}
> +
> +	return xmitted;
> +}