Re: [PATCH RFC net-next 1/6] cadence: macb/gem: Add page pool support

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

Hello Paolo,

On Wed Nov 19, 2025 at 2:53 PM CET, Paolo Valerio wrote:
> Subject: [PATCH RFC net-next 1/6] cadence: macb/gem: Add page pool support

`git log --oneline drivers/net/ethernet/cadence/` tells us all commits
in this series should be prefixed with "net: macb: ".

> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 87414a2ddf6e..dcf768bd1bc1 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -14,6 +14,8 @@
>  #include <linux/interrupt.h>
>  #include <linux/phy/phy.h>
>  #include <linux/workqueue.h>
> +#include <net/page_pool/helpers.h>
> +#include <net/xdp.h>
>  
>  #define MACB_GREGS_NBR 16
>  #define MACB_GREGS_VERSION 2
> @@ -957,6 +959,10 @@ struct macb_dma_desc_ptp {
>  /* Scaled PPM fraction */
>  #define PPM_FRACTION	16
>  
> +/* The buf includes headroom compatible with both skb and xdpf */
> +#define MACB_PP_HEADROOM		XDP_PACKET_HEADROOM
> +#define MACB_PP_MAX_BUF_SIZE(num)	(((num) * PAGE_SIZE) - MACB_PP_HEADROOM)

From my previous review, you know I think MACB_PP_MAX_BUF_SIZE() should
disappear.

I also don't see the point of MACB_PP_HEADROOM. Maybe if it was
max(XDP_PACKET_HEADROOM, NET_SKB_PAD) it would be more useful, but that
isn't useful anyway. Can we drop it and use XDP_PACKET_HEADROOM directly
in the code?

>  /* struct macb_tx_skb - data about an skb which is being transmitted
>   * @skb: skb currently being transmitted, only set for the last buffer
>   *       of the frame
> @@ -1262,10 +1268,11 @@ struct macb_queue {
>  	unsigned int		rx_tail;
>  	unsigned int		rx_prepared_head;
>  	struct macb_dma_desc	*rx_ring;
> -	struct sk_buff		**rx_skbuff;
> +	void			**rx_buff;

It would help review if the s/rx_skbuff/rx_buff/ renaming was done in a
separate commit with a commit message being "this only renames X and
implies no functional changes".

>  	void			*rx_buffers;
>  	struct napi_struct	napi_rx;
>  	struct queue_stats stats;
> +	struct page_pool	*page_pool;
>  };
>  
>  struct ethtool_rx_fs_item {
> @@ -1289,6 +1296,7 @@ struct macb {
>  	struct macb_dma_desc	*rx_ring_tieoff;
>  	dma_addr_t		rx_ring_tieoff_dma;
>  	size_t			rx_buffer_size;
> +	u16			rx_offset;

u16 makes me worried that we might do mistakes. For example the
following propagates the u16 type.

        bp->rx_offset + data - page_address(page)

We can spare the additional 6 bytes and turn it into a size_t. It'll
fall in holes anyway, at least it does for my target according to pahole.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index e461f5072884..985c81913ba6 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1250,11 +1250,28 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
>  	return packets;
>  }
>  
> -static void gem_rx_refill(struct macb_queue *queue)
> +static void *gem_page_pool_get_buff(struct page_pool *pool,
> +				    dma_addr_t *dma_addr, gfp_t gfp_mask)
> +{
> +	struct page *page;
> +
> +	if (!pool)
> +		return NULL;
> +
> +	page = page_pool_alloc_pages(pool, gfp_mask | __GFP_NOWARN);
> +	if (!page)
> +		return NULL;
> +
> +	*dma_addr = page_pool_get_dma_addr(page) + MACB_PP_HEADROOM;
> +
> +	return page_address(page);
> +}

Do we need a separate function called from only one location? Or we
could change its name to highlight it allocates and does not just "get"
a buffer.

> @@ -1267,25 +1284,17 @@ static void gem_rx_refill(struct macb_queue *queue)
>  
>  		desc = macb_rx_desc(queue, entry);
>  
> -		if (!queue->rx_skbuff[entry]) {
> +		if (!queue->rx_buff[entry]) {
>  			/* allocate sk_buff for this free entry in ring */
> -			skb = netdev_alloc_skb(bp->dev, bp->rx_buffer_size);
> -			if (unlikely(!skb)) {
> +			data = gem_page_pool_get_buff(queue->page_pool, &paddr,
> +						      napi ? GFP_ATOMIC : GFP_KERNEL);

I don't get why the gfp flags computation is spread across
gem_page_pool_get_buff() and gem_rx_refill().

> @@ -1349,12 +1344,16 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>  		  int budget)
>  {
>  	struct macb *bp = queue->bp;
> +	int			buffer_size;
>  	unsigned int		len;
>  	unsigned int		entry;
> +	void			*data;
>  	struct sk_buff		*skb;
>  	struct macb_dma_desc	*desc;
>  	int			count = 0;
>  
> +	buffer_size = DIV_ROUND_UP(bp->rx_buffer_size, PAGE_SIZE) * PAGE_SIZE;

This looks like

        buffer_size = ALIGN(bp->rx_buffer_size, PAGE_SIZE);

no? Anyway I think it should be dropped. It does get dropped next patch
in this RFC.

> @@ -1387,24 +1386,49 @@ static int gem_rx(struct macb_queue *queue, struct napi_struct *napi,
>  			queue->stats.rx_dropped++;
>  			break;
>  		}
> -		skb = queue->rx_skbuff[entry];
> -		if (unlikely(!skb)) {
> +		data = queue->rx_buff[entry];
> +		if (unlikely(!data)) {
>  			netdev_err(bp->dev,
>  				   "inconsistent Rx descriptor chain\n");
>  			bp->dev->stats.rx_dropped++;
>  			queue->stats.rx_dropped++;
>  			break;
>  		}
> +
> +		skb = napi_build_skb(data, buffer_size);
> +		if (unlikely(!skb)) {
> +			netdev_err(bp->dev,
> +				   "Unable to allocate sk_buff\n");
> +			page_pool_put_full_page(queue->page_pool,
> +						virt_to_head_page(data),
> +						false);
> +			break;

We should `queue->rx_skbuff[entry] = NULL` here no?
We free a page and keep a pointer to it.

> +		}
> +
> +		/* Properly align Ethernet header.
> +		 *
> +		 * Hardware can add dummy bytes if asked using the RBOF
> +		 * field inside the NCFGR register. That feature isn't
> +		 * available if hardware is RSC capable.
> +		 *
> +		 * We cannot fallback to doing the 2-byte shift before
> +		 * DMA mapping because the address field does not allow
> +		 * setting the low 2/3 bits.
> +		 * It is 3 bits if HW_DMA_CAP_PTP, else 2 bits.
> +		 */
> +		skb_reserve(skb, bp->rx_offset);
> +		skb_mark_for_recycle(skb);

I have a platform with RSC support and NET_IP_ALIGN=2. What is yours
like? It'd be nice if we can test different cases of this RBOF topic.

>  		/* now everything is ready for receiving packet */
> -		queue->rx_skbuff[entry] = NULL;
> +		queue->rx_buff[entry] = NULL;
>  		len = ctrl & bp->rx_frm_len_mask;
>  
>  		netdev_vdbg(bp->dev, "gem_rx %u (len %u)\n", entry, len);
>  
> +		dma_sync_single_for_cpu(&bp->pdev->dev,
> +					addr, len,
> +					page_pool_get_dma_dir(queue->page_pool));

Any reason for the call to dma_sync_single_for_cpu(), we could hardcode
it to DMA_FROM_DEVICE no?

> @@ -2477,13 +2497,13 @@ static int gem_alloc_rx_buffers(struct macb *bp)
>  
>  	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
>  		size = bp->rx_ring_size * sizeof(struct sk_buff *);

sizeof is called with the wrong type. Not that it matters because
pointers are pointers, but we can take this opportunity to move to

        sizeof(*queue->rx_buff)

> -		queue->rx_skbuff = kzalloc(size, GFP_KERNEL);
> -		if (!queue->rx_skbuff)
> +		queue->rx_buff = kzalloc(size, GFP_KERNEL);
> +		if (!queue->rx_buff)
>  			return -ENOMEM;
>  		else
>  			netdev_dbg(bp->dev,
> -				   "Allocated %d RX struct sk_buff entries at %p\n",
> -				   bp->rx_ring_size, queue->rx_skbuff);
> +				   "Allocated %d RX buff entries at %p\n",
> +				   bp->rx_ring_size, queue->rx_buff);

Opportunity to deindent this block?

Thanks,

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