Re: [net-next RFC v3 3/7] net: ravb: Refactor RX ring refill

[Paul Barker <paul.barker.ct@bp.renesas.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 7690 bytes --]

On 15/04/2024 12:57, Niklas Söderlund wrote:
> Hi Paul,
> 
> Thanks for your work, I really like this deduplication of code!
> 
> On 2024-04-15 10:48:00 +0100, Paul Barker wrote:
>> To reduce code duplication, we add a new RX ring refill function which
>> can handle both the initial RX ring population (which was split between
>> ravb_ring_init() and ravb_ring_format()) and the RX ring refill after
>> polling (in ravb_rx()).
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 141 +++++++++--------------
>>  1 file changed, 52 insertions(+), 89 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 1ac599a044b2..baa01bd81f2d 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -317,35 +317,42 @@ static void ravb_ring_free(struct net_device *ndev, int q)
>>  	priv->tx_skb[q] = NULL;
>>  }
>>  
>> -static void ravb_rx_ring_format(struct net_device *ndev, int q)
>> +static u32
>> +ravb_rx_ring_refill(struct net_device *ndev, int q, u32 count, gfp_t gfp_mask)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>> +	const struct ravb_hw_info *info = priv->info;
>>  	struct ravb_rx_desc *rx_desc;
>> -	unsigned int rx_ring_size;
>>  	dma_addr_t dma_addr;
>> -	unsigned int i;
>> +	u32 i, entry;
>>  
>> -	rx_ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
>> -	memset(priv->rx_ring[q].raw, 0, rx_ring_size);
>> -	/* Build RX ring buffer */
>> -	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>> -		/* RX descriptor */
>> -		rx_desc = ravb_rx_get_desc(priv, q, i);
>> -		rx_desc->ds_cc = cpu_to_le16(priv->info->rx_max_desc_use);
>> -		dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
>> -					  priv->info->rx_max_frame_size,
>> -					  DMA_FROM_DEVICE);
>> -		/* We just set the data size to 0 for a failed mapping which
>> -		 * should prevent DMA from happening...
>> -		 */
>> -		if (dma_mapping_error(ndev->dev.parent, dma_addr))
>> -			rx_desc->ds_cc = cpu_to_le16(0);
>> -		rx_desc->dptr = cpu_to_le32(dma_addr);
>> +	for (i = 0; i < count; i++) {
>> +		entry = (priv->dirty_rx[q] + i) % priv->num_rx_ring[q];
>> +		rx_desc = ravb_rx_get_desc(priv, q, entry);
>> +		rx_desc->ds_cc = cpu_to_le16(info->rx_max_desc_use);
>> +
>> +		if (!priv->rx_skb[q][entry]) {
>> +			priv->rx_skb[q][entry] = ravb_alloc_skb(ndev, info, gfp_mask);
>> +			if (!priv->rx_skb[q][entry])
>> +				break;
>> +			dma_addr = dma_map_single(ndev->dev.parent,
>> +						  priv->rx_skb[q][entry]->data,
>> +						  priv->info->rx_max_frame_size,
>> +						  DMA_FROM_DEVICE);
>> +			skb_checksum_none_assert(priv->rx_skb[q][entry]);
>> +			/* We just set the data size to 0 for a failed mapping
>> +			 * which should prevent DMA from happening...
>> +			 */
>> +			if (dma_mapping_error(ndev->dev.parent, dma_addr))
>> +				rx_desc->ds_cc = cpu_to_le16(0);
>> +			rx_desc->dptr = cpu_to_le32(dma_addr);
>> +		}
>> +		/* Descriptor type must be set after all the above writes */
>> +		dma_wmb();
>>  		rx_desc->die_dt = DT_FEMPTY;
>>  	}
>> -	rx_desc = ravb_rx_get_desc(priv, q, i);
>> -	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
>> -	rx_desc->die_dt = DT_LINKFIX; /* type */
>> +
>> +	return i;
>>  }
>>  
>>  /* Format skb and descriptor buffer for Ethernet AVB */
>> @@ -353,6 +360,7 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	unsigned int num_tx_desc = priv->num_tx_desc;
>> +	struct ravb_rx_desc *rx_desc;
>>  	struct ravb_tx_desc *tx_desc;
>>  	struct ravb_desc *desc;
>>  	unsigned int tx_ring_size = sizeof(*tx_desc) * priv->num_tx_ring[q] *
>> @@ -364,8 +372,6 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>>  	priv->dirty_rx[q] = 0;
>>  	priv->dirty_tx[q] = 0;
>>  
>> -	ravb_rx_ring_format(ndev, q);
>> -
>>  	memset(priv->tx_ring[q], 0, tx_ring_size);
>>  	/* Build TX ring buffer */
>>  	for (i = 0, tx_desc = priv->tx_ring[q]; i < priv->num_tx_ring[q];
>> @@ -379,6 +385,14 @@ static void ravb_ring_format(struct net_device *ndev, int q)
>>  	tx_desc->dptr = cpu_to_le32((u32)priv->tx_desc_dma[q]);
>>  	tx_desc->die_dt = DT_LINKFIX; /* type */
>>  
>> +	/* Regular RX descriptors have already been initialized by
>> +	 * ravb_rx_ring_refill(), we just need to initialize the final link
>> +	 * descriptor.
>> +	 */
>> +	rx_desc = ravb_rx_get_desc(priv, q, priv->num_rx_ring[q]);
>> +	rx_desc->dptr = cpu_to_le32((u32)priv->rx_desc_dma[q]);
>> +	rx_desc->die_dt = DT_LINKFIX; /* type */
>> +
> 
> super-nit: Should you not move this addition up to where you removed the 
> call to ravb_rx_ring_format()? Before this change the order of things 
> are,
> 
>     /* Init RX ring */
>     /* Init TX ring */
>     /* Set RX descriptor base address */
>     /* Set TX descriptor base address */
> 
> 
> While after it is,
> 
>     /* Init TX ring */
>     /* Init RX ring */
>     /* Set RX descriptor base address */
>     /* Set TX descriptor base address */
> 
> My OCD is itching ;-)

Since I'll need to re-spin this series anyway, I may as well tidy that
up :)

> 
>>  	/* RX descriptor base address for best effort */
>>  	desc = &priv->desc_bat[RX_QUEUE_OFFSET + q];
>>  	desc->die_dt = DT_LINKFIX; /* type */
>> @@ -408,11 +422,9 @@ static void *ravb_alloc_rx_desc(struct net_device *ndev, int q)
>>  static int ravb_ring_init(struct net_device *ndev, int q)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>> -	const struct ravb_hw_info *info = priv->info;
>>  	unsigned int num_tx_desc = priv->num_tx_desc;
>>  	unsigned int ring_size;
>> -	struct sk_buff *skb;
>> -	unsigned int i;
>> +	u32 num_filled;
>>  
>>  	/* Allocate RX and TX skb rings */
>>  	priv->rx_skb[q] = kcalloc(priv->num_rx_ring[q],
>> @@ -422,13 +434,6 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>>  	if (!priv->rx_skb[q] || !priv->tx_skb[q])
>>  		goto error;
>>  
>> -	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>> -		skb = ravb_alloc_skb(ndev, info, GFP_KERNEL);
>> -		if (!skb)
>> -			goto error;
>> -		priv->rx_skb[q][i] = skb;
>> -	}
>> -
>>  	if (num_tx_desc > 1) {
>>  		/* Allocate rings for the aligned buffers */
>>  		priv->tx_align[q] = kmalloc(DPTR_ALIGN * priv->num_tx_ring[q] +
>> @@ -443,6 +448,13 @@ static int ravb_ring_init(struct net_device *ndev, int q)
>>  
>>  	priv->dirty_rx[q] = 0;
>>  
>> +	/* Populate RX ring buffer. */
>> +	ring_size = priv->info->rx_desc_size * priv->num_rx_ring[q];
>> +	memset(priv->rx_ring[q].raw, 0, ring_size);
>> +	num_filled = ravb_rx_ring_refill(ndev, q, priv->num_rx_ring[q], GFP_KERNEL);
>> +	if (num_filled != priv->num_rx_ring[q])
>> +		goto error;
>> +
> 
> Here you also change the order, but it make sense here as you first deal 
> with all TX and then all RX ;-)

The placement here is because we can't call ravb_rx_ring_refill() until
priv->dirty_rx[q] has been zero'd.

The init order right now is actually:
  RX page pool
  RX buffers
  TX SKBs
  RX descriptors
  RX ring buffer
  TX descriptors

So maybe this should be re-ordered.

I considered breaking this all apart, so ravb_ring_init() would call
ravb_rx_ring_init()/ravb_tx_ring_init() and ravb_ring_format() would
call ravb_rx_ring_format()/ravb_tx_ring_format(). There are several
steps happening for TX & RX in both init and format stages. Does that
sound cleaner to you?

Thanks,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]