Re: [PATCH net-next v3 4/6] net: bcmgenet: add XDP_TX support

[Justin Chen <justin.chen@broadcom.com>]

On 3/19/26 4:53 AM, Nicolai Buchwitz wrote:
> Implement XDP_TX using ring 16 (DESC_INDEX), the hardware default
> descriptor ring, dedicated to XDP TX for isolation from SKB TX queues.
> 
> Ring 16 gets 32 BDs carved from ring 0's allocation. TX completion is
> piggybacked on RX NAPI poll since ring 16's INTRL2_1 bit collides with
> RX ring 0, similar to how bnxt, ice, and other XDP drivers handle TX
> completion within the RX poll path.
> 
> The GENET MAC has TBUF_64B_EN set globally, requiring every TX buffer
> to start with a 64-byte struct status_64 (TSB). For local XDP_TX, the
> TSB is prepended by backing xdp->data into the RSB area (unused after
> BPF execution) and zeroing it. For foreign frames (ndo_xdp_xmit), the
> TSB is written into the xdp_frame headroom.
> 
> The page_pool DMA direction is changed from DMA_FROM_DEVICE to
> DMA_BIDIRECTIONAL to allow TX reuse of the existing DMA mapping.
> 
> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
> ---
>   .../net/ethernet/broadcom/genet/bcmgenet.c    | 195 ++++++++++++++++--
>   .../net/ethernet/broadcom/genet/bcmgenet.h    |   4 +-
>   2 files changed, 181 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index e8f90d6b16a2..54df1694f1cd 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -48,8 +48,10 @@
>   
>   #define GENET_Q0_RX_BD_CNT	\
>   	(TOTAL_DESC - priv->hw_params->rx_queues * priv->hw_params->rx_bds_per_q)
> +#define GENET_Q16_TX_BD_CNT	64
>   #define GENET_Q0_TX_BD_CNT	\
> -	(TOTAL_DESC - priv->hw_params->tx_queues * priv->hw_params->tx_bds_per_q)
> +	(TOTAL_DESC - priv->hw_params->tx_queues * priv->hw_params->tx_bds_per_q \
> +	 - GENET_Q16_TX_BD_CNT)
>   
>   #define RX_BUF_LENGTH		2048
>   #define SKB_ALIGNMENT		32
> @@ -1893,6 +1895,14 @@ static struct sk_buff *bcmgenet_free_tx_cb(struct device *dev,
>   		if (cb == GENET_CB(skb)->last_cb)
>   			return skb;
>   
> +	} else if (cb->xdpf) {
> +		if (cb->xdp_dma_map)
> +			dma_unmap_single(dev, dma_unmap_addr(cb, dma_addr),
> +					 dma_unmap_len(cb, dma_len),
> +					 DMA_TO_DEVICE);
> +		dma_unmap_addr_set(cb, dma_addr, 0);
> +		xdp_return_frame(cb->xdpf);
> +		cb->xdpf = NULL;
>   	} else if (dma_unmap_addr(cb, dma_addr)) {
>   		dma_unmap_page(dev,
>   			       dma_unmap_addr(cb, dma_addr),
> @@ -1927,8 +1937,13 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
>   	unsigned int c_index;
>   	struct sk_buff *skb;
>   
> -	/* Clear status before servicing to reduce spurious interrupts */
> -	bcmgenet_intrl2_1_writel(priv, (1 << ring->index), INTRL2_CPU_CLEAR);
> +	/* Clear status before servicing to reduce spurious interrupts.
> +	 * Ring DESC_INDEX (XDP TX) has no interrupt; skip the clear to
> +	 * avoid clobbering RX ring 0's bit at the same position.
> +	 */
> +	if (ring->index != DESC_INDEX)
> +		bcmgenet_intrl2_1_writel(priv, BIT(ring->index),
> +					 INTRL2_CPU_CLEAR);
>   
>   	/* Compute how many buffers are transmitted since last xmit call */
>   	c_index = bcmgenet_tdma_ring_readl(priv, ring->index, TDMA_CONS_INDEX)
> @@ -1964,8 +1979,11 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
>   	u64_stats_add(&stats->bytes, bytes_compl);
>   	u64_stats_update_end(&stats->syncp);
>   
> -	netdev_tx_completed_queue(netdev_get_tx_queue(dev, ring->index),
> -				  pkts_compl, bytes_compl);
> +	/* Ring DESC_INDEX (XDP TX) has no netdev TX queue; skip BQL */
> +	if (ring->index != DESC_INDEX)
> +		netdev_tx_completed_queue(netdev_get_tx_queue(dev,
> +							      ring->index),
> +					  pkts_compl, bytes_compl);
>   
>   	return txbds_processed;
>   }
> @@ -2042,6 +2060,9 @@ static void bcmgenet_tx_reclaim_all(struct net_device *dev)
>   	do {
>   		bcmgenet_tx_reclaim(dev, &priv->tx_rings[i++], true);
>   	} while (i <= priv->hw_params->tx_queues && netif_is_multiqueue(dev));
> +
> +	/* Also reclaim XDP TX ring */
> +	bcmgenet_tx_reclaim(dev, &priv->tx_rings[DESC_INDEX], true);
>   }
>   
>   /* Reallocate the SKB to put enough headroom in front of it and insert
> @@ -2299,10 +2320,96 @@ static struct sk_buff *bcmgenet_xdp_build_skb(struct bcmgenet_rx_ring *ring,
>   	return skb;
>   }
>   
> +static bool

Same as last patch. Perfer to remove newline here.

> +bcmgenet_xdp_xmit_frame(struct bcmgenet_priv *priv,
> +			struct xdp_frame *xdpf, bool dma_map)
> +{
> +	struct bcmgenet_tx_ring *ring = &priv->tx_rings[DESC_INDEX];
> +	struct device *kdev = &priv->pdev->dev;
> +	struct enet_cb *tx_cb_ptr;
> +	dma_addr_t mapping;
> +	unsigned int dma_len;
> +	u32 len_stat;
> +
> +	spin_lock(&ring->lock);
> +
> +	if (ring->free_bds < 1) {
> +		spin_unlock(&ring->lock);
> +		return false;
> +	}
> +
> +	tx_cb_ptr = bcmgenet_get_txcb(priv, ring);
> +
> +	if (dma_map) {
> +		void *tsb_start;
> +
> +		/* The GENET MAC has TBUF_64B_EN set globally, so hardware
> +		 * expects a 64-byte TSB prefix on every TX buffer.  For
> +		 * redirected frames (ndo_xdp_xmit) we prepend a zeroed TSB
> +		 * using the frame's headroom.
> +		 */
> +		if (unlikely(xdpf->headroom < sizeof(struct status_64))) {
> +			bcmgenet_put_txcb(priv, ring);
> +			spin_unlock(&ring->lock);
> +			return false;
> +		}
> +
> +		tsb_start = xdpf->data - sizeof(struct status_64);
> +		memset(tsb_start, 0, sizeof(struct status_64));
> +
> +		dma_len = xdpf->len + sizeof(struct status_64);
> +		mapping = dma_map_single(kdev, tsb_start, dma_len,
> +					 DMA_TO_DEVICE);
> +		if (dma_mapping_error(kdev, mapping)) {
> +			tx_cb_ptr->skb = NULL;
> +			tx_cb_ptr->xdpf = NULL;
> +			bcmgenet_put_txcb(priv, ring);
> +			spin_unlock(&ring->lock);
> +			return false;
> +		}
> +	} else {
> +		struct page *page = virt_to_page(xdpf->data);
> +
> +		/* For local XDP_TX the caller already prepended the TSB
> +		 * into xdpf->data/len, so dma_len == xdpf->len.
> +		 */
> +		dma_len = xdpf->len;
> +		mapping = page_pool_get_dma_addr(page) +
> +			  sizeof(*xdpf) + xdpf->headroom;
> +		dma_sync_single_for_device(kdev, mapping, dma_len,
> +					   DMA_BIDIRECTIONAL);
> +	}
> +
> +	dma_unmap_addr_set(tx_cb_ptr, dma_addr, mapping);
> +	dma_unmap_len_set(tx_cb_ptr, dma_len, dma_len);
> +	tx_cb_ptr->skb = NULL;
> +	tx_cb_ptr->xdpf = xdpf;
> +	tx_cb_ptr->xdp_dma_map = dma_map;
> +
> +	len_stat = (dma_len << DMA_BUFLENGTH_SHIFT) |
> +		   (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT) |
> +		   DMA_TX_APPEND_CRC | DMA_SOP | DMA_EOP;
> +
> +	dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping, len_stat);
> +
> +	ring->free_bds--;
> +	ring->prod_index++;
> +	ring->prod_index &= DMA_P_INDEX_MASK;
> +
> +	bcmgenet_tdma_ring_writel(priv, ring->index, ring->prod_index,
> +				  TDMA_PROD_INDEX);
> +
> +	spin_unlock(&ring->lock);
> +
> +	return true;
> +}
> +
>   static unsigned int
>   bcmgenet_run_xdp(struct bcmgenet_rx_ring *ring, struct bpf_prog *prog,
>   		 struct xdp_buff *xdp, struct page *rx_page)
>   {
> +	struct bcmgenet_priv *priv = ring->priv;
> +	struct xdp_frame *xdpf;
>   	unsigned int act;
>   
>   	act = bpf_prog_run_xdp(prog, xdp);
> @@ -2310,14 +2417,33 @@ bcmgenet_run_xdp(struct bcmgenet_rx_ring *ring, struct bpf_prog *prog,
>   	switch (act) {
>   	case XDP_PASS:
>   		return XDP_PASS;
> +	case XDP_TX:
> +		/* Prepend a zeroed TSB (Transmit Status Block).  The GENET
> +		 * MAC has TBUF_64B_EN set globally, so hardware expects every
> +		 * TX buffer to begin with a 64-byte struct status_64.  Back
> +		 * up xdp->data into the RSB area (which is no longer needed
> +		 * after the BPF program ran) and zero it.
> +		 */
> +		xdp->data -= sizeof(struct status_64);
> +		xdp->data_meta -= sizeof(struct status_64);
> +		memset(xdp->data, 0, sizeof(struct status_64));
> +
> +		xdpf = xdp_convert_buff_to_frame(xdp);
> +		if (unlikely(!xdpf) ||
> +		    unlikely(!bcmgenet_xdp_xmit_frame(priv, xdpf, false))) {
> +			page_pool_put_full_page(ring->page_pool, rx_page,
> +						true);
> +			return XDP_DROP;
> +		}
> +		return XDP_TX;
>   	case XDP_DROP:
>   		page_pool_put_full_page(ring->page_pool, rx_page, true);
>   		return XDP_DROP;
>   	default:
> -		bpf_warn_invalid_xdp_action(ring->priv->dev, prog, act);
> +		bpf_warn_invalid_xdp_action(priv->dev, prog, act);
>   		fallthrough;
>   	case XDP_ABORTED:
> -		trace_xdp_exception(ring->priv->dev, prog, act);
> +		trace_xdp_exception(priv->dev, prog, act);
>   		page_pool_put_full_page(ring->page_pool, rx_page, true);
>   		return XDP_ABORTED;
>   	}
> @@ -2555,9 +2681,15 @@ static int bcmgenet_rx_poll(struct napi_struct *napi, int budget)
>   {
>   	struct bcmgenet_rx_ring *ring = container_of(napi,
>   			struct bcmgenet_rx_ring, napi);
> +	struct bcmgenet_priv *priv = ring->priv;
>   	struct dim_sample dim_sample = {};
>   	unsigned int work_done;
>   
> +	/* Reclaim completed XDP TX frames (ring 16 has no interrupt) */
> +	if (priv->xdp_prog)
> +		bcmgenet_tx_reclaim(priv->dev,
> +				    &priv->tx_rings[DESC_INDEX], false);
> +
>   	work_done = bcmgenet_desc_rx(ring, budget);
>   
>   	if (work_done < budget && napi_complete_done(napi, work_done))
> @@ -2832,8 +2964,11 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv,
>   	bcmgenet_tdma_ring_writel(priv, index, end_ptr * words_per_bd - 1,
>   				  DMA_END_ADDR);
>   
> -	/* Initialize Tx NAPI */
> -	netif_napi_add_tx(priv->dev, &ring->napi, bcmgenet_tx_poll);
> +	/* Initialize Tx NAPI for priority queues only; ring DESC_INDEX
> +	 * (XDP TX) has its completions handled inline in RX NAPI.
> +	 */
> +	if (index != DESC_INDEX)
> +		netif_napi_add_tx(priv->dev, &ring->napi, bcmgenet_tx_poll);
>   }
>   
>   static int bcmgenet_rx_ring_create_pool(struct bcmgenet_priv *priv,
> @@ -2845,7 +2980,7 @@ static int bcmgenet_rx_ring_create_pool(struct bcmgenet_priv *priv,
>   		.pool_size = ring->size,
>   		.nid = NUMA_NO_NODE,
>   		.dev = &priv->pdev->dev,
> -		.dma_dir = DMA_FROM_DEVICE,
> +		.dma_dir = DMA_BIDIRECTIONAL,
>   		.offset = GENET_XDP_HEADROOM,
>   		.max_len = RX_BUF_LENGTH,
>   	};
> @@ -2977,6 +3112,7 @@ static int bcmgenet_tdma_disable(struct bcmgenet_priv *priv)
>   
>   	reg = bcmgenet_tdma_readl(priv, DMA_CTRL);
>   	mask = (1 << (priv->hw_params->tx_queues + 1)) - 1;
> +	mask |= BIT(DESC_INDEX);
>   	mask = (mask << DMA_RING_BUF_EN_SHIFT) | DMA_EN;
>   	reg &= ~mask;
>   	bcmgenet_tdma_writel(priv, reg, DMA_CTRL);
> @@ -3022,14 +3158,18 @@ static int bcmgenet_rdma_disable(struct bcmgenet_priv *priv)
>    * with queue 1 being the highest priority queue.
>    *
>    * Queue 0 is the default Tx queue with
> - * GENET_Q0_TX_BD_CNT = 256 - 4 * 32 = 128 descriptors.
> + * GENET_Q0_TX_BD_CNT = 256 - 4 * 32 - 32 = 96 descriptors.
> + *
> + * Ring 16 (DESC_INDEX) is used for XDP TX with
> + * GENET_Q16_TX_BD_CNT = 32 descriptors.
>    *
>    * The transmit control block pool is then partitioned as follows:
> - * - Tx queue 0 uses tx_cbs[0..127]
> - * - Tx queue 1 uses tx_cbs[128..159]
> - * - Tx queue 2 uses tx_cbs[160..191]
> - * - Tx queue 3 uses tx_cbs[192..223]
> - * - Tx queue 4 uses tx_cbs[224..255]
> + * - Tx queue 0 uses tx_cbs[0..95]
> + * - Tx queue 1 uses tx_cbs[96..127]
> + * - Tx queue 2 uses tx_cbs[128..159]
> + * - Tx queue 3 uses tx_cbs[160..191]
> + * - Tx queue 4 uses tx_cbs[192..223]
> + * - Tx ring 16 uses tx_cbs[224..255]

Can keep the term tx queue? Unless we are trying to highlight something 
different here.

>    */
>   static void bcmgenet_init_tx_queues(struct net_device *dev)
>   {
> @@ -3050,13 +3190,18 @@ static void bcmgenet_init_tx_queues(struct net_device *dev)
>   			<< DMA_PRIO_REG_SHIFT(i);
>   	}
>   
> +	/* Initialize ring 16 (descriptor ring) for XDP TX */
> +	bcmgenet_init_tx_ring(priv, DESC_INDEX, GENET_Q16_TX_BD_CNT,
> +			      TOTAL_DESC - GENET_Q16_TX_BD_CNT, TOTAL_DESC);
> +
>   	/* Set Tx queue priorities */
>   	bcmgenet_tdma_writel(priv, dma_priority[0], DMA_PRIORITY_0);
>   	bcmgenet_tdma_writel(priv, dma_priority[1], DMA_PRIORITY_1);
>   	bcmgenet_tdma_writel(priv, dma_priority[2], DMA_PRIORITY_2);
>   
> -	/* Configure Tx queues as descriptor rings */
> +	/* Configure Tx queues as descriptor rings, including ring 16 */
>   	ring_mask = (1 << (priv->hw_params->tx_queues + 1)) - 1;
> +	ring_mask |= BIT(DESC_INDEX);
>   	bcmgenet_tdma_writel(priv, ring_mask, DMA_RING_CFG);
>   
>   	/* Enable Tx rings */
> @@ -3770,6 +3915,21 @@ static void bcmgenet_get_stats64(struct net_device *dev,
>   		stats->tx_dropped += tx_dropped;
>   	}
>   
> +	/* Include XDP TX ring (DESC_INDEX) stats */
> +	tx_stats = &priv->tx_rings[DESC_INDEX].stats64;
> +	do {
> +		start = u64_stats_fetch_begin(&tx_stats->syncp);
> +		tx_bytes = u64_stats_read(&tx_stats->bytes);
> +		tx_packets = u64_stats_read(&tx_stats->packets);
> +		tx_errors = u64_stats_read(&tx_stats->errors);
> +		tx_dropped = u64_stats_read(&tx_stats->dropped);
> +	} while (u64_stats_fetch_retry(&tx_stats->syncp, start));
> +
> +	stats->tx_bytes += tx_bytes;
> +	stats->tx_packets += tx_packets;
> +	stats->tx_errors += tx_errors;
> +	stats->tx_dropped += tx_dropped;
> +
>   	for (q = 0; q <= priv->hw_params->rx_queues; q++) {
>   		rx_stats = &priv->rx_rings[q].stats64;
>   		do {
> @@ -4273,6 +4433,7 @@ static int bcmgenet_probe(struct platform_device *pdev)
>   		u64_stats_init(&priv->rx_rings[i].stats64.syncp);
>   	for (i = 0; i <= priv->hw_params->tx_queues; i++)
>   		u64_stats_init(&priv->tx_rings[i].stats64.syncp);
> +	u64_stats_init(&priv->tx_rings[DESC_INDEX].stats64.syncp);
>   
>   	/* libphy will determine the link state */
>   	netif_carrier_off(dev);
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> index 1459473ac1b0..fce598c29a96 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
> @@ -472,6 +472,8 @@ struct bcmgenet_rx_stats64 {
>   
>   struct enet_cb {
>   	struct sk_buff      *skb;
> +	struct xdp_frame    *xdpf;
> +	bool                xdp_dma_map;
>   	struct page         *rx_page;
>   	unsigned int        rx_page_offset;
>   	void __iomem *bd_addr;
> @@ -610,7 +612,7 @@ struct bcmgenet_priv {
>   	struct enet_cb *tx_cbs;
>   	unsigned int num_tx_bds;
>   
> -	struct bcmgenet_tx_ring tx_rings[GENET_MAX_MQ_CNT + 1];
> +	struct bcmgenet_tx_ring tx_rings[DESC_INDEX + 1];

This creates a bunch of tx_rings that aren't used. I don't see why we 
need them in the same array. The logic above loops through the queue 
rings, then hits the DESC_INDEX ring last. So not too much value of 
keeping them in the same array.

Thanks,
Justin

>   
>   	/* receive variables */
>   	void __iomem *rx_bds;