netdev - Re: [PATCH RFC net-next 5/6] cadence: macb/gem: make tx path skb agnostic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <DEJK6OLX6FL7.2SV8LF9U4S0VU@bootlin.com>
Date: Thu, 27 Nov 2025 15:49:13 +0100
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Paolo Valerio" <pvalerio@...hat.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 5/6] cadence: macb/gem: make tx path skb
 agnostic

On Wed Nov 19, 2025 at 2:53 PM CET, Paolo Valerio wrote:
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 2f665260a84d..67bb98d3cb00 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -964,19 +964,27 @@ struct macb_dma_desc_ptp {
>  #define MACB_PP_HEADROOM	XDP_PACKET_HEADROOM
>  #define MACB_MAX_PAD		(MACB_PP_HEADROOM + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
>  
> -/* 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
> +enum macb_tx_buff_type {
> +	MACB_TYPE_SKB,
> +	MACB_TYPE_XDP_TX,
> +	MACB_TYPE_XDP_NDO,
> +};
> +
> +/* struct macb_tx_buff - data about an skb or xdp frame which is being transmitted
> + * @data: pointer to skb or xdp frame being transmitted, only set
> + *        for the last buffer for sk_buff
>   * @mapping: DMA address of the skb's fragment buffer
>   * @size: size of the DMA mapped buffer
>   * @mapped_as_page: true when buffer was mapped with skb_frag_dma_map(),
>   *                  false when buffer was mapped with dma_map_single()
> + * @type: type of buffer (MACB_TYPE_SKB, MACB_TYPE_XDP_TX, MACB_TYPE_XDP_NDO)
>   */
> -struct macb_tx_skb {
> -	struct sk_buff		*skb;
> -	dma_addr_t		mapping;
> -	size_t			size;
> -	bool			mapped_as_page;
> +struct macb_tx_buff {
> +	void				*data;
> +	dma_addr_t			mapping;
> +	size_t				size;
> +	bool				mapped_as_page;
> +	enum macb_tx_buff_type		type;
>  };

Here as well, reviewing would be helped by moving the tx_skb to tx_buff
renaming to a separate commit that has no functional change.

As said in [0], I am not a fan of the field name `data`.
Let's discuss it there.

[0]: https://lore.kernel.org/all/DEITSIO441QL.X81MVLL3EIV4@bootlin.com/

> -static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb, int budget)
> +static void macb_tx_unmap(struct macb *bp, struct macb_tx_buff *tx_buff,
> +			  int budget)
>  {
> -	if (tx_skb->mapping) {
> -		if (tx_skb->mapped_as_page)
> -			dma_unmap_page(&bp->pdev->dev, tx_skb->mapping,
> -				       tx_skb->size, DMA_TO_DEVICE);
> +	if (tx_buff->mapping) {
> +		if (tx_buff->mapped_as_page)
> +			dma_unmap_page(&bp->pdev->dev, tx_buff->mapping,
> +				       tx_buff->size, DMA_TO_DEVICE);
>  		else
> -			dma_unmap_single(&bp->pdev->dev, tx_skb->mapping,
> -					 tx_skb->size, DMA_TO_DEVICE);
> -		tx_skb->mapping = 0;
> +			dma_unmap_single(&bp->pdev->dev, tx_buff->mapping,
> +					 tx_buff->size, DMA_TO_DEVICE);
> +		tx_buff->mapping = 0;
>  	}
>  
> -	if (tx_skb->skb) {
> -		napi_consume_skb(tx_skb->skb, budget);
> -		tx_skb->skb = NULL;
> +	if (tx_buff->data) {
> +		if (tx_buff->type != MACB_TYPE_SKB)
> +			netdev_err(bp->dev, "BUG: Unexpected tx buffer type while unmapping (%d)",
> +				   tx_buff->type);
> +		napi_consume_skb(tx_buff->data, budget);
> +		tx_buff->data = NULL;
>  	}

This code does not make much sense by itself. We check `tx_buff->type !=
MACB_TYPE_SKB` but call napi_consume_skb() in all cases. I remember it
changes in the next commit.

> @@ -1069,16 +1073,23 @@ static void macb_tx_error_task(struct work_struct *work)
>  
>  		desc = macb_tx_desc(queue, tail);
>  		ctrl = desc->ctrl;
> -		tx_skb = macb_tx_skb(queue, tail);
> -		skb = tx_skb->skb;
> +		tx_buff = macb_tx_buff(queue, tail);
> +
> +		if (tx_buff->type != MACB_TYPE_SKB)
> +			netdev_err(bp->dev, "BUG: Unexpected tx buffer type (%d)",
> +				   tx_buff->type);
> +		skb = tx_buff->data;

Same here: `tx_buff->type != MACB_TYPE_SKB` does not make sense if we
keep on going with the SKB case anyways.

>  
>  		if (ctrl & MACB_BIT(TX_USED)) {
>  			/* skb is set for the last buffer of the frame */
>  			while (!skb) {
> -				macb_tx_unmap(bp, tx_skb, 0);
> +				macb_tx_unmap(bp, tx_buff, 0);
>  				tail++;
> -				tx_skb = macb_tx_skb(queue, tail);
> -				skb = tx_skb->skb;
> +				tx_buff = macb_tx_buff(queue, tail);
> +				if (tx_buff->type != MACB_TYPE_SKB)
> +					netdev_err(bp->dev, "BUG: Unexpected tx buffer type (%d)",
> +						   tx_buff->type);
> +				skb = tx_buff->data;

Same.

> @@ -5050,7 +5066,7 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
>  		netif_stop_queue(dev);
>  
>  		/* Store packet information (to free when Tx completed) */
> -		lp->rm9200_txq[desc].skb = skb;
> +		lp->rm9200_txq[desc].data = skb;
>  		lp->rm9200_txq[desc].size = skb->len;
>  		lp->rm9200_txq[desc].mapping = dma_map_single(&lp->pdev->dev, skb->data,
>  							      skb->len, DMA_TO_DEVICE);

We might want to assign `lp->rm9200_txq[desc].type` here, to ensure all
`struct macb_tx_buff` instances are fully initialised.

Thanks,

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