Re: [PATCH RFC net-next 4/6] cadence: macb/gem: add XDP support for gem

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

On Wed Nov 19, 2025 at 2:53 PM CET, Paolo Valerio wrote:
> @@ -1273,6 +1275,7 @@ struct macb_queue {
>  	struct queue_stats stats;
>  	struct page_pool	*page_pool;
>  	struct sk_buff		*skb;
> +	struct xdp_rxq_info	xdp_q;
>  };

Those are always named `xdp_rxq` inside the kernel, we should stick with
the calling convention no?

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 5829c1f773dd..53ea1958b8e4 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1344,10 +1344,51 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
>  	 */
>  }
>  
> +static u32 gem_xdp_run(struct macb_queue *queue, struct xdp_buff *xdp,
> +		       struct net_device *dev)

Why pass `struct net_device` explicitly? It is in queue->bp->dev.

> +{
> +	struct bpf_prog *prog;
> +	u32 act = XDP_PASS;
> +
> +	rcu_read_lock();
> +
> +	prog = rcu_dereference(queue->bp->prog);
> +	if (!prog)
> +		goto out;
> +
> +	act = bpf_prog_run_xdp(prog, xdp);
> +	switch (act) {
> +	case XDP_PASS:
> +		goto out;
> +	case XDP_REDIRECT:
> +		if (unlikely(xdp_do_redirect(dev, xdp, prog))) {
> +			act = XDP_DROP;
> +			break;
> +		}
> +		goto out;

Why the `unlikely()`?

> +	default:
> +		bpf_warn_invalid_xdp_action(dev, prog, act);
> +		fallthrough;
> +	case XDP_ABORTED:
> +		trace_xdp_exception(dev, prog, act);
> +		fallthrough;
> +	case XDP_DROP:
> +		break;
> +	}
> +
> +	page_pool_put_full_page(queue->page_pool,
> +				virt_to_head_page(xdp->data), true);

Maybe move that to the XDP_DROP, it is the only `break` in the above
switch statement. It will be used by the default and XDP_ABORTED cases
through fallthrough. We can avoid the out label and its two gotos that
way.

>  static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>  		  int budget)
>  {
>  	struct macb *bp = queue->bp;
> +	bool			xdp_flush = false;
>  	unsigned int		len;
>  	unsigned int		entry;
>  	void			*data;
> @@ -1356,9 +1397,11 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>  	int			count = 0;
>  
>  	while (count < budget) {
> -		u32 ctrl;
> -		dma_addr_t addr;
>  		bool rxused, first_frame;
> +		struct xdp_buff xdp;
> +		dma_addr_t addr;
> +		u32 ctrl;
> +		u32 ret;
>  
>  		entry = macb_rx_ring_wrap(bp, queue->rx_tail);
>  		desc = macb_rx_desc(queue, entry);
> @@ -1403,6 +1446,22 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>  			data_len = SKB_WITH_OVERHEAD(bp->rx_buffer_size) - bp->rx_offset;
>  		}
>  
> +		if (!(ctrl & MACB_BIT(RX_SOF) && ctrl & MACB_BIT(RX_EOF)))
> +			goto skip_xdp;
> +
> +		xdp_init_buff(&xdp, bp->rx_buffer_size, &queue->xdp_q);
> +		xdp_prepare_buff(&xdp, data, bp->rx_offset, len,
> +				 false);
> +		xdp_buff_clear_frags_flag(&xdp);

You prepare the XDP buffer before checking an XDP program is attached.
Could we avoid this work? We'd move the xdp_buff preparation into
gem_xdp_run(), after the RCU pointer dereference.

> -static void gem_create_page_pool(struct macb_queue *queue)
> +static void 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,

Ah, that is the reason for page_pool_get_dma_dir() calls!

>  static int macb_change_mtu(struct net_device *dev, int new_mtu)
>  {
> +	int frame_size = new_mtu + ETH_HLEN + ETH_FCS_LEN + MACB_MAX_PAD;
> +	struct macb *bp = netdev_priv(dev);
> +	struct bpf_prog *prog = bp->prog;

No fancy RCU macro?

> +static int macb_xdp(struct net_device *dev, struct netdev_bpf *xdp)
> +{
> +	struct macb *bp = netdev_priv(dev);
> +
> +	if (!macb_is_gem(bp))
> +		return 0;

Returning 0 sounds like a mistake, -EOPNOTSUPP sounds more appropriate.

> +	switch (xdp->command) {
> +	case XDP_SETUP_PROG:
> +		return gem_xdp_setup(dev, xdp->prog, xdp->extack);
> +	default:
> +		return -EINVAL;

Same here: we want -EOPNOTSUPP. Otherwise caller cannot dissociate an
unsupported call from one that is supported but failed.

> +	}
> +}
> +
>  static void gem_update_stats(struct macb *bp)
>  {
>  	struct macb_queue *queue;
> @@ -4390,6 +4529,7 @@ static const struct net_device_ops macb_netdev_ops = {
>  	.ndo_hwtstamp_set	= macb_hwtstamp_set,
>  	.ndo_hwtstamp_get	= macb_hwtstamp_get,
>  	.ndo_setup_tc		= macb_setup_tc,
> +	.ndo_bpf		= macb_xdp,

We want it to be "gem_" prefixed as it does not support MACB.

Thanks,

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