Hi Thierry,
Às 7:57 PM de 4/4/2017, Thierry Reding escreveu:
> 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 <jpi...@synopsys.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 <tred...@nvidia.com>
>
> It also doesn't break on Tegra186, so
>
> Tested-by: Thierry Reding <tred...@nvidia.com>
>
Thanks for testing and for the feedback. Let's see if Corentin Labbe can test
this in his setup.
Joao
