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

[Paolo Valerio <pvalerio@redhat.com>]

On 27 Nov 2025 at 03:49:13 PM, Théo Lebrun <theo.lebrun@bootlin.com> wrote:

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

sure

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

yeah, I'll revisit this and the ones above

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

good catch

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