Re: [PATCH v2 4/4] net: ethernet: altera: rename functions and their prototypes

[Simon Horman]

On Thu, Dec 21, 2023 at 09:40:41PM +0800, deepakx.nagaraju@xxxxxxxxx wrote:
> From: Nagaraju DeepakX <deepakx.nagaraju@xxxxxxxxx>
> 
> Move standard DMA interface for sgdma and msgdma and rename them
> from tse_private to dma_private.
> 
> Signed-off-by: Nagaraju DeepakX <deepakx.nagaraju@xxxxxxxxx>
> Reviewed-by: Andy Schevchenko <andriy.schevchenko@xxxxxxxxxxxxxxx>

...

> diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c

...

> @@ -252,42 +256,42 @@ static void tse_free_tx_buffer(struct altera_tse_private *priv,
> 
>  static int alloc_init_skbufs(struct altera_tse_private *priv)
>  {
> -	unsigned int rx_descs = priv->rx_ring_size;
> -	unsigned int tx_descs = priv->tx_ring_size;
> +	struct altera_dma_private *dma = &priv->dma_priv;
> +	unsigned int rx_descs = dma->rx_ring_size;
> +	unsigned int tx_descs = dma->tx_ring_size;
>  	int ret = -ENOMEM;
>  	int i;
> 
>  	/* Create Rx ring buffer */
> -	priv->rx_ring = kcalloc(rx_descs, sizeof(struct tse_buffer), GFP_KERNEL);
> -	if (!priv->rx_ring)
> +	dma->rx_ring = kcalloc(rx_descs, sizeof(struct altera_dma_private), GFP_KERNEL);

Hi Nagaraju,

Sorry, I didn't notice this until after I sent my previous review
to this patch.

Is struct altera_dma_private correct on the line above?
It seems to me that it should, rather, be struct altera_dma_buffer.
Likewise a few lines below.

Flagged by Smatch.

> +	if (!dma->rx_ring)
>  		goto err_rx_ring;
> 
>  	/* Create Tx ring buffer */
> -	priv->tx_ring = kcalloc(tx_descs, sizeof(struct tse_buffer), GFP_KERNEL);
> -	if (!priv->tx_ring)
> +	dma->tx_ring = kcalloc(tx_descs, sizeof(struct altera_dma_private), GFP_KERNEL);
> +	if (!dma->tx_ring)
>  		goto err_tx_ring;

> 
> -	priv->tx_cons = 0;
> -	priv->tx_prod = 0;
> +	dma->tx_cons = 0;
> +	dma->tx_prod = 0;
> 
>  	/* Init Rx ring */
>  	for (i = 0; i < rx_descs; i++) {
> -		ret = tse_init_rx_buffer(priv, &priv->rx_ring[i],
> -					 priv->rx_dma_buf_sz);
> +		ret = tse_init_rx_buffer(priv, &priv->dma_priv.rx_ring[i], dma->rx_dma_buf_sz);
>  		if (ret)
>  			goto err_init_rx_buffers;
>  	}
> 
> -	priv->rx_cons = 0;
> -	priv->rx_prod = 0;
> +	dma->rx_cons = 0;
> +	dma->rx_prod = 0;
> 
>  	return 0;
>  err_init_rx_buffers:
>  	while (--i >= 0)
> -		tse_free_rx_buffer(priv, &priv->rx_ring[i]);
> -	kfree(priv->tx_ring);
> +		tse_free_rx_buffer(priv, &priv->dma_priv.rx_ring[i]);
> +	kfree(dma->tx_ring);
>  err_tx_ring:
> -	kfree(priv->rx_ring);
> +	kfree(dma->rx_ring);
>  err_rx_ring:
>  	return ret;
>  }

...