Date:   Tue, 4 Apr 2017 20:57:35 +0200
From:   Thierry Reding <treding@...dia.com>
To:     Joao Pinto <Joao.Pinto@...opsys.com>
CC:     <davem@...emloft.net>, <clabbe.montjoie@...il.com>,
        <niklas.cassel@...s.com>, <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/4] net: stmmac: break some functions into RX and TX
 scopes

On Tue, Apr 04, 2017 at 06:54:24PM +0100, Joao Pinto wrote:
> This patch breaks several functions into RX and TX scopes, which
> will be useful when adding multiple buffers mechanism.
> 
> Signed-off-by: Joao Pinto <jpinto@...opsys.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 350 +++++++++++++++++-----
>  1 file changed, 268 insertions(+), 82 deletions(-)

A couple of small nits below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[...]
> @@ -924,16 +941,16 @@ static int stmmac_set_bfsize(int mtu, int bufsize)
>  }
>  
>  /**
> - * stmmac_clear_descriptors - clear descriptors
> + * stmmac_clear_rx_descriptors - clear RX descriptors
>   * @priv: driver private structure
> - * Description: this function is called to clear the tx and rx descriptors
> + * Description: this function is called to clear the rx descriptors

You seem to be transitioning to "RX" and "TX" everywhere, maybe do the
same in this comment for consistency?

Also, on a general note: there's no need for "Description:" here. The
kerneldoc format mandates that you leave a blank line after the block
of parameter descriptions, and the paragraph that follows becomes the
description. I know that these are static functions and are therefore
not parsed by kerneldoc, but since you already use the syntax anyway,
you might as well get it right.

>   * in case of both basic and extended descriptors are used.
>   */
> -static void stmmac_clear_descriptors(struct stmmac_priv *priv)
> +static void stmmac_clear_rx_descriptors(struct stmmac_priv *priv)
>  {
>  	int i;

This could be unsigned.

>  
> -	/* Clear the Rx/Tx descriptors */
> +	/* Clear the RX descriptors */
>  	for (i = 0; i < DMA_RX_SIZE; i++)
>  		if (priv->extend_desc)
>  			priv->hw->desc->init_rx_desc(&priv->dma_erx[i].basic,
> @@ -943,6 +960,19 @@ static void stmmac_clear_descriptors(struct stmmac_priv *priv)
>  			priv->hw->desc->init_rx_desc(&priv->dma_rx[i],
>  						     priv->use_riwt, priv->mode,
>  						     (i == DMA_RX_SIZE - 1));
> +}
> +
> +/**
> + * stmmac_clear_tx_descriptors - clear tx descriptors
> + * @priv: driver private structure
> + * Description: this function is called to clear the tx descriptors
> + * in case of both basic and extended descriptors are used.
> + */
> +static void stmmac_clear_tx_descriptors(struct stmmac_priv *priv)
> +{
> +	int i;

Same here. There are a couple of other such occurrences throughout the
file. This already exists in many places in the driver, so I don't think
this needs to be changed. Or at least it could be a follow-up patch.

> +
> +	/* Clear the TX descriptors */
>  	for (i = 0; i < DMA_TX_SIZE; i++)
>  		if (priv->extend_desc)
>  			priv->hw->desc->init_tx_desc(&priv->dma_etx[i].basic,
> @@ -955,6 +985,21 @@ static void stmmac_clear_descriptors(struct stmmac_priv *priv)
>  }
>  
>  /**
> + * stmmac_clear_descriptors - clear descriptors
> + * @priv: driver private structure
> + * Description: this function is called to clear the tx and rx descriptors
> + * in case of both basic and extended descriptors are used.
> + */
> +static void stmmac_clear_descriptors(struct stmmac_priv *priv)
> +{
> +	/* Clear the RX descriptors */
> +	stmmac_clear_rx_descriptors(priv);
> +
> +	/* Clear the TX descriptors */
> +	stmmac_clear_tx_descriptors(priv);
> +}
> +
> +/**
>   * stmmac_init_rx_buffers - init the RX descriptor buffer.
>   * @priv: driver private structure
>   * @p: descriptor pointer
> @@ -996,6 +1041,11 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv, struct dma_desc *p,
>  	return 0;
>  }
>  
> +/**
> + * stmmac_free_rx_buffers - free RX dma buffers
> + * @priv: private structure
> + * @i: buffer index.

If this operates on a single buffer, as specified by the buffer index,
maybe this should be named singular stmmac_free_rx_buffer()?

> + */
>  static void stmmac_free_rx_buffers(struct stmmac_priv *priv, int i)

The index could be unsigned.

>  {
>  	if (priv->rx_skbuff[i]) {
> @@ -1007,14 +1057,42 @@ static void stmmac_free_rx_buffers(struct stmmac_priv *priv, int i)
>  }
>  
>  /**
> - * init_dma_desc_rings - init the RX/TX descriptor rings
> + * stmmac_free_tx_buffers - free RX dma buffers
> + * @priv: private structure
> + * @i: buffer index.
> + */
> +static void stmmac_free_tx_buffers(struct stmmac_priv *priv, int i)
> +{
> +	if (priv->tx_skbuff_dma[i].buf) {
> +		if (priv->tx_skbuff_dma[i].map_as_page)
> +			dma_unmap_page(priv->device,
> +				       priv->tx_skbuff_dma[i].buf,
> +				       priv->tx_skbuff_dma[i].len,
> +				       DMA_TO_DEVICE);
> +		else
> +			dma_unmap_single(priv->device,
> +					 priv->tx_skbuff_dma[i].buf,
> +					 priv->tx_skbuff_dma[i].len,
> +					 DMA_TO_DEVICE);
> +	}
> +
> +	if (priv->tx_skbuff[i]) {
> +		dev_kfree_skb_any(priv->tx_skbuff[i]);
> +		priv->tx_skbuff[i] = NULL;
> +		priv->tx_skbuff_dma[i].buf = 0;
> +		priv->tx_skbuff_dma[i].map_as_page = false;
> +	}
> +}
> +
> +/**
> + * init_dma_rx_desc_rings - init the RX descriptor rings
>   * @dev: net device structure
>   * @flags: gfp flag.
> - * Description: this function initializes the DMA RX/TX descriptors
> + * Description: this function initializes the DMA RX descriptors
>   * and allocates the socket buffers. It supports the chained and ring
>   * modes.
>   */
> -static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
> +static int init_dma_rx_desc_rings(struct net_device *dev, gfp_t flags)
>  {
>  	int i;
>  	struct stmmac_priv *priv = netdev_priv(dev);
> @@ -1030,8 +1108,7 @@ static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
>  	priv->dma_buf_sz = bfsize;
>  
>  	netif_dbg(priv, probe, priv->dev,
> -		  "(%s) dma_rx_phy=0x%08x dma_tx_phy=0x%08x\n",
> -		  __func__, (u32)priv->dma_rx_phy, (u32)priv->dma_tx_phy);
> +		  "(%s) dma_rx_phy=0x%08x\n", __func__, (u32)priv->dma_rx_phy);
>  
>  	/* RX INITIALIZATION */
>  	netif_dbg(priv, probe, priv->dev,
> @@ -1058,17 +1135,44 @@ static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
>  
>  	/* Setup the chained descriptor addresses */
>  	if (priv->mode == STMMAC_CHAIN_MODE) {
> -		if (priv->extend_desc) {
> +		if (priv->extend_desc)
>  			priv->hw->mode->init(priv->dma_erx, priv->dma_rx_phy,
>  					     DMA_RX_SIZE, 1);
> -			priv->hw->mode->init(priv->dma_etx, priv->dma_tx_phy,
> -					     DMA_TX_SIZE, 1);
> -		} else {
> +		else
>  			priv->hw->mode->init(priv->dma_rx, priv->dma_rx_phy,
>  					     DMA_RX_SIZE, 0);
> +	}
> +
> +	return 0;
> +err_init_rx_buffers:
> +	while (--i >= 0)
> +		stmmac_free_rx_buffers(priv, i);
> +	return ret;
> +}
> +
> +/**
> + * init_dma_tx_desc_rings - init the TX descriptor rings
> + * @dev: net device structure.
> + * Description: this function initializes the DMA TX descriptors
> + * and allocates the socket buffers. It supports the chained and ring
> + * modes.
> + */
> +static int init_dma_tx_desc_rings(struct net_device *dev)
> +{
> +	struct stmmac_priv *priv = netdev_priv(dev);
> +	int i;
> +
> +	netif_dbg(priv, probe, priv->dev,
> +		  "(%s) dma_tx_phy=0x%08x\n", __func__, (u32)priv->dma_rx_phy);
> +
> +	/* Setup the chained descriptor addresses */
> +	if (priv->mode == STMMAC_CHAIN_MODE) {
> +		if (priv->extend_desc)
> +			priv->hw->mode->init(priv->dma_etx, priv->dma_tx_phy,
> +					     DMA_TX_SIZE, 1);
> +		else
>  			priv->hw->mode->init(priv->dma_tx, priv->dma_tx_phy,
>  					     DMA_TX_SIZE, 0);
> -		}
>  	}
>  
>  	/* TX INITIALIZATION */
> @@ -1099,18 +1203,42 @@ static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
>  	priv->cur_tx = 0;
>  	netdev_reset_queue(priv->dev);
>  
> +	return 0;
> +}
> +
> +/**
> + * init_dma_desc_rings - init the RX/TX descriptor rings
> + * @dev: net device structure
> + * @flags: gfp flag.
> + * Description: this function initializes the DMA RX/TX descriptors
> + * and allocates the socket buffers. It supports the chained and ring
> + * modes.
> + */
> +static int init_dma_desc_rings(struct net_device *dev, gfp_t flags)
> +{
> +	struct stmmac_priv *priv = netdev_priv(dev);
> +	int ret;
> +
> +	/* RX INITIALIZATION */
> +	ret = init_dma_rx_desc_rings(dev, flags);

That comment already exists in init_dma_rx_desc_rings(). And even there
it doesn't provide any useful information, so might as well drop it.

> +	if (ret)
> +		return ret;
> +
> +	/* TX INITIALIZATION */
> +	ret = init_dma_tx_desc_rings(dev);

Same here.

[...]
> -static void free_dma_desc_resources(struct stmmac_priv *priv)
> +/**
> + * alloc_dma_desc_resources - alloc TX/RX resources.
> + * @priv: private structure
> + * Description: according to which descriptor can be used (extend or basic)
> + * this function allocates the resources for TX and RX paths. In case of
> + * reception, for example, it pre-allocated the RX socket buffer in order to
> + * allow zero-copy mechanism.
> + */
> +static int alloc_dma_desc_resources(struct stmmac_priv *priv)
> +{
> +	/* RX Allocation */
> +	int ret = alloc_dma_rx_desc_resources(priv);

And here.

> +
> +	if (ret)
> +		return ret;
> +
> +	/* TX Allocation */
> +	ret = alloc_dma_tx_desc_resources(priv);

And here.

None of the above comments are critical and this could be cleaned up in
follow-up patches, so:

Reviewed-by: Thierry Reding <treding@...dia.com>

It also doesn't break on Tegra186, so

Tested-by: Thierry Reding <treding@...dia.com>

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists