Message-ID: <87bjkgzt52.fsf@redhat.com>
Date: Tue, 02 Dec 2025 18:32:41 +0100
From: Paolo Valerio <pvalerio@...hat.com>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
 netdev@...r.kernel.org
Cc: Nicolas Ferre <nicolas.ferre@...rochip.com>, Claudiu Beznea
 <claudiu.beznea@...on.dev>, Andrew Lunn <andrew+netdev@...n.ch>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Lorenzo
 Bianconi <lorenzo@...nel.org>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, Grégory Clement
 <gregory.clement@...tlin.com>, Benoît Monin
 <benoit.monin@...tlin.com>
Subject: Re: [PATCH RFC net-next 4/6] cadence: macb/gem: add XDP support for
 gem

On 27 Nov 2025 at 03:41:56 PM, Théo Lebrun <theo.lebrun@...tlin.com> wrote:

> 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?
>

sure

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

let's avoid this

>> +{
>> +	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()`?
>

just expecting the err path to be the exception, although this is not
consistend with the XDP_TX path.
Do you prefer to remove it?

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

We'd not put to page pool in case the redirect fails or am I missing
something?

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

ack, makes sense

>> -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?
>

right, guess an rcu_access_pointer() call is needed here

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

agreed

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

ack

>> +	}
>> +}
>> +
>>  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.
>

ack

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

Powered by blists - more mailing lists