Re: [net-next,5/8] cadence: macb: add XDP support for gem

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

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
cadence: macb: add XDP support for gem

This commit introduces XDP support for the macb/gem driver with XDP_PASS,
XDP_DROP, and XDP_REDIRECT verdicts.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 89f0c4dc3884..1f62100a4c4d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> -static int gem_create_page_pool(struct macb_queue *queue)
> +static int gem_create_page_pool(struct macb_queue *queue, int qid)
>  {
>  	struct page_pool_params pp_params = {
>  		.order = 0,
>  		.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
>  		.pool_size = queue->bp->rx_ring_size,
>  		.nid = NUMA_NO_NODE,
> -		.dma_dir = DMA_FROM_DEVICE,
> +		.dma_dir = rcu_access_pointer(queue->bp->prog)
> +				? DMA_BIDIRECTIONAL
> +				: DMA_FROM_DEVICE,

The page pool is created with DMA direction based on whether an XDP program
is attached at creation time. However, gem_xdp_setup() below allows attaching
XDP programs to a running interface without recreating the page pool.

If the interface is opened without XDP (page pool uses DMA_FROM_DEVICE), and
then an XDP program is attached via ndo_bpf, the pool will still use
DMA_FROM_DEVICE. For XDP redirect, DMA_BIDIRECTIONAL is needed for proper
cache coherency on non-cache-coherent architectures.

The stmmac driver handles this by calling stmmac_xdp_release() and
stmmac_xdp_open() when XDP program state changes on a running interface,
which recreates the page pool with the correct DMA direction.

Should gem_xdp_setup() reset the interface (or at least the page pools) when
attaching or detaching XDP programs on a running interface to ensure the DMA
direction matches?

[ ... ]

> +static int gem_xdp_setup(struct net_device *dev, struct bpf_prog *prog,
> +			 struct netlink_ext_ack *extack)
> +{
> +	struct macb *bp = netdev_priv(dev);
> +	struct bpf_prog *old_prog;
> +
> +	if (prog && !gem_xdp_valid_mtu(bp, dev->mtu)) {
> +		NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	old_prog = rcu_replace_pointer(bp->prog, prog, lockdep_rtnl_is_held());
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +
> +	return 0;
> +}

This function swaps the XDP program via RCU but does not reset the interface
or recreate the page pools. This relates to the DMA direction concern above.