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

[Paolo Valerio <pvalerio@redhat.com>]

On 23 Feb 2026 at 04:08:44 PM, Jakub Kicinski <kuba@kernel.org> wrote:

> 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
> ---
> net: macb: add XDP support for gem
>
> This patch introduces basic XDP support for the macb/gem driver with
> XDP_PASS, XDP_DROP, and XDP_REDIRECT verdict support.
>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index a78ad00f53b1..33a963b6dd4d 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -15,6 +15,7 @@
>>  #include <linux/phy/phy.h>
>>  #include <linux/workqueue.h>
>>  #include <net/page_pool/helpers.h>
>> +#include <net/xdp.h>
>
> [ ... ]
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index 708107e47ae3..26b517ed251c 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>
> [ ... ]
>
>> @@ -1247,9 +1248,27 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>>  	return packets;
>>  }
>>
>> +static unsigned int gem_rx_headroom(struct macb *bp)
>> +{
>> +	if (rcu_access_pointer(bp->prog))
>> +		return XDP_PACKET_HEADROOM;
>> +
>> +	return NET_SKB_PAD;
>> +}
>> +
>> +static unsigned int gem_max_rx_data_size(int base_sz)
>> +{
>> +	return SKB_DATA_ALIGN(base_sz + ETH_HLEN + ETH_FCS_LEN);
>> +}
>> +
>> +static unsigned int gem_max_rx_buffer_size(int data_sz, struct macb *bp)
>> +{
>> +	return SKB_HEAD_ALIGN(data_sz + bp->rx_headroom);
>> +}
>
> [ ... ]
>
>> @@ -1340,12 +1360,61 @@ static void discard_partial_frame(struct macb_queue *queue, unsigned int begin,
>>  	 */
>>  }
>>
>> +static u32 gem_xdp_run(struct macb_queue *queue, void *buff_head,
>> +		       unsigned int len)
>> +{
>> +	struct net_device *dev;
>> +	struct bpf_prog *prog;
>> +	struct xdp_buff xdp;
>> +
>> +	u32 act = XDP_PASS;
>> +
>> +	rcu_read_lock();
>> +
>> +	prog = rcu_dereference(queue->bp->prog);
>> +	if (!prog)
>> +		goto out;
>> +
>> +	xdp_init_buff(&xdp, gem_total_rx_buffer_size(queue->bp), &queue->xdp_rxq);
>> +	xdp_prepare_buff(&xdp, buff_head, queue->bp->rx_headroom, len, false);
>> +	xdp_buff_clear_frags_flag(&xdp);
>> +	dev = queue->bp->dev;
>> +
>> +	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;
>> +	default:
>> +		bpf_warn_invalid_xdp_action(dev, prog, act);
>> +		fallthrough;
>> +	case XDP_ABORTED:
>> +		trace_xdp_exception(dev, prog, act);
>> +		fallthrough;
>> +	case XDP_DROP:
>> +		break;
>> +	}
>
> Does this function need to handle XDP_TX? The driver advertises
> NETDEV_XDP_ACT_BASIC (set in macb_probe() below), which declares support
> for XDP_ABORTED, XDP_DROP, XDP_PASS, and XDP_TX per the UAPI definition
> in include/uapi/linux/netdev.h. An XDP program returning XDP_TX will fall
> through to the default case, call bpf_warn_invalid_xdp_action(), and
> treat it as XDP_DROP. The packet won't be transmitted; it will be dropped
> with a warning instead.
>
> [ ... ]
>
>> @@ -2641,21 +2724,23 @@ static int macb_alloc_consistent(struct macb *bp)
>>  	return -ENOMEM;
>>  }
>>
>> -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,
>>  		.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,
>>  		.dev = &queue->bp->pdev->dev,
>>  		.netdev = queue->bp->dev,
>>  		.napi = &queue->napi_rx,
>>  		.max_len = PAGE_SIZE,
>>  	};
>>  	struct page_pool *pool;
>> -	int err = 0;
>> +	int err;
>>
>>  	/* This can happen in the case of HRESP error.
>>  	 * Do nothing as page pool is already existing.
>>  	 */
>>  	if (queue->page_pool)
>>  		return err;
>                        ^^^
>
> Does this return an uninitialized value? The patch changed 'int err = 0'
> to 'int err' without initialization. The early-return path for HRESP error
> recovery returns err when the page pool already exists.
>

argh, I missed this patch removing the initialization while rebasing.

> gem_init_rings() calls this function during HRESP error recovery:
>
> drivers/net/ethernet/cadence/macb_main.c:gem_init_rings()
> 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> 		...
> 		err = gem_create_page_pool(queue, q);
> 		if (err) {
> 			last_err = err;
> 			if (fail_early)
> 				return err;
> 			continue;
> 		}
>
> When err is non-zero, gem_init_rings() skips gem_rx_refill() for that
> queue via continue, potentially leaving the RX queue without buffers.
>
> The unmodified macb_hresp_error_task() calls mog_init_rings(bp, false)
> which calls gem_init_rings() to recover from HRESP errors. During
> recovery, page pools already exist from the initial open, so the
> early-return is taken. If err happens to be non-zero, gem_init_rings()
> skips gem_rx_refill(), breaking packet reception on affected queues after
> HRESP error recovery.
>
> [ ... ]
>
>> @@ -3167,11 +3275,27 @@ static int macb_close(struct net_device *dev)
>>  	return 0;
>>  }
>>
>> +static bool gem_xdp_valid_mtu(struct macb *bp, int mtu)
>> +{
>> +	int max_frame_size;
>> +
>> +	max_frame_size = gem_max_rx_buffer_size(gem_max_rx_data_size(mtu), bp);
>> +
>> +	return max_frame_size <= PAGE_SIZE;
>> +}
>
> Looking at the XDP setup flow, is this MTU validation using the correct
> headroom value?
>
> When gem_xdp_setup() calls gem_xdp_valid_mtu():
>
> drivers/net/ethernet/cadence/macb_main.c:gem_xdp_setup()
> 	if (prog && !gem_xdp_valid_mtu(bp, dev->mtu)) {
> 		NL_SET_ERR_MSG_MOD(extack, "MTU too large for XDP");
> 		return -EOPNOTSUPP;
> 	}
>
> 	running = netif_running(dev);
> 	need_update = !!bp->prog != !!prog;
> 	if (running && need_update)
> 		macb_close(dev);
>
> 	old_prog = rcu_replace_pointer(bp->prog, prog, lockdep_rtnl_is_held());
>
> The validation happens before bp->prog is updated. gem_max_rx_buffer_size()
> uses bp->rx_headroom, which is cached during macb_init_rx_buffer_size().
> At validation time, bp->rx_headroom still holds the non-XDP value
> (NET_SKB_PAD, typically 64 bytes) rather than the XDP value
> (XDP_PACKET_HEADROOM = 256 bytes). This makes the validation 192 bytes too
> permissive.
>

I guess gem_xdp_valid_mtu() can depend only on XDP_PACKET_HEADROOM and
just check against it and drop the dependency with bp->rx_headroom.


> An MTU close to the PAGE_SIZE limit could pass validation but then cause
> gem_total_rx_buffer_size() to exceed PAGE_SIZE once the correct XDP
> headroom is applied in macb_open()->macb_init_rx_buffer_size().
>
> [ ... ]
>
>> @@ -5744,13 +5911,9 @@ static int macb_probe(struct platform_device *pdev)
>>  	if (err)
>>  		goto err_out_phy_exit;
>>
>> -	if (macb_is_gem(bp)) {
>> -		bp->rx_headroom = NET_SKB_PAD;
>> -		if (!(bp->caps & MACB_CAPS_RSC)) {
>> -			bp->rx_ip_align = NET_IP_ALIGN;
>> -			bp->rx_headroom += NET_IP_ALIGN;
>> -		}
>> -	}
>> +	if (macb_is_gem(bp))
>> +		dev->xdp_features = NETDEV_XDP_ACT_BASIC |
>> +				    NETDEV_XDP_ACT_REDIRECT;
>
> Is NETDEV_XDP_ACT_BASIC accurate here? Looking at gem_xdp_run() above,
> XDP_TX falls through to the default case and is treated as XDP_DROP with
> a warning. NETDEV_XDP_ACT_BASIC includes XDP_TX support per the UAPI
> definition, but the driver doesn't implement it.

This makes sense. Will require some rearrangement in the way the series
is split.