[PATCH net-next 1/3] net: bcmgenet: cleanup for bcmgenet_xmit()

[PATCH net-next 1/3] net: bcmgenet: cleanup for bcmgenet_xmit()

[Petri Gynther]

1. Readability: Move nr_frags assignment a few lines down in order
   to bundle index -> ring -> txq calculations together.
2. Readability: Add parentheses around nr_frags + 1.
3. Minor fix: Stop the Tx queue and throw the error message only if
   the Tx queue hasn't already been stopped.

Signed-off-by: Petri Gynther <pgynther@google.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index cf6445d..7f85a84 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1447,15 +1447,19 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff *skb, struct net_device *dev)
 	else
 		index -= 1;
 
-	nr_frags = skb_shinfo(skb)->nr_frags;
 	ring = &priv->tx_rings[index];
 	txq = netdev_get_tx_queue(dev, ring->queue);
 
+	nr_frags = skb_shinfo(skb)->nr_frags;
+
 	spin_lock_irqsave(&ring->lock, flags);
-	if (ring->free_bds <= nr_frags + 1) {
-		netif_tx_stop_queue(txq);
-		netdev_err(dev, "%s: tx ring %d full when queue %d awake\n",
-			   __func__, index, ring->queue);
+	if (ring->free_bds <= (nr_frags + 1)) {
+		if (!netif_tx_queue_stopped(txq)) {
+			netif_tx_stop_queue(txq);
+			netdev_err(dev,
+				   "%s: tx ring %d full when queue %d awake\n",
+				   __func__, index, ring->queue);
+		}
 		ret = NETDEV_TX_BUSY;
 		goto out;
 	}
-- 
2.8.0.rc3.226.g39d4020

[PATCH net-next 2/3] net: bcmgenet: cleanup for bcmgenet_xmit_frag()

[Petri Gynther]

Add frag_size = skb_frag_size(frag) and use it when needed.

Signed-off-by: Petri Gynther <pgynther@google.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 7f85a84..d77cd6d 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1331,6 +1331,7 @@ static int bcmgenet_xmit_frag(struct net_device *dev,
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct device *kdev = &priv->pdev->dev;
 	struct enet_cb *tx_cb_ptr;
+	unsigned int frag_size;
 	dma_addr_t mapping;
 	int ret;
 
@@ -1338,10 +1339,12 @@ static int bcmgenet_xmit_frag(struct net_device *dev,
 
 	if (unlikely(!tx_cb_ptr))
 		BUG();
+
 	tx_cb_ptr->skb = NULL;
 
-	mapping = skb_frag_dma_map(kdev, frag, 0,
-				   skb_frag_size(frag), DMA_TO_DEVICE);
+	frag_size = skb_frag_size(frag);
+
+	mapping = skb_frag_dma_map(kdev, frag, 0, frag_size, DMA_TO_DEVICE);
 	ret = dma_mapping_error(kdev, mapping);
 	if (ret) {
 		priv->mib.tx_dma_failed++;
@@ -1351,10 +1354,10 @@ static int bcmgenet_xmit_frag(struct net_device *dev,
 	}
 
 	dma_unmap_addr_set(tx_cb_ptr, dma_addr, mapping);
-	dma_unmap_len_set(tx_cb_ptr, dma_len, frag->size);
+	dma_unmap_len_set(tx_cb_ptr, dma_len, frag_size);
 
 	dmadesc_set(priv, tx_cb_ptr->bd_addr, mapping,
-		    (frag->size << DMA_BUFLENGTH_SHIFT) | dma_desc_flags |
+		    (frag_size << DMA_BUFLENGTH_SHIFT) | dma_desc_flags |
 		    (priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT));
 
 	return 0;
-- 
2.8.0.rc3.226.g39d4020

[PATCH net-next 3/3] net: bcmgenet: fix dmadesc_set()

[Petri Gynther]

dmadesc_set() is used for setting the Tx buffer DMA address, length,
and status bits on a Tx ring descriptor when a frame is being Tx'ed.

Always set the Tx buffer DMA address first, before updating the length
and status bits, i.e. giving the Tx descriptor to the hardware.

Signed-off-by: Petri Gynther <pgynther@google.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d77cd6d..f7b42b9 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -104,8 +104,8 @@ static inline void dmadesc_set_addr(struct bcmgenet_priv *priv,
 static inline void dmadesc_set(struct bcmgenet_priv *priv,
 			       void __iomem *d, dma_addr_t addr, u32 val)
 {
-	dmadesc_set_length_status(priv, d, val);
 	dmadesc_set_addr(priv, d, addr);
+	dmadesc_set_length_status(priv, d, val);
 }
 
 static inline dma_addr_t dmadesc_get_addr(struct bcmgenet_priv *priv,
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH net-next 3/3] net: bcmgenet: fix dmadesc_set()

[Florian Fainelli]

2016-04-04 17:10 GMT-07:00 Petri Gynther <pgynther@google.com>:
> dmadesc_set() is used for setting the Tx buffer DMA address, length,
> and status bits on a Tx ring descriptor when a frame is being Tx'ed.
>
> Always set the Tx buffer DMA address first, before updating the length
> and status bits, i.e. giving the Tx descriptor to the hardware.

Does this fix any real bug you have observed? The hardware won't
transmit anything until you start writing the correct TDMA producer
index. Also, dmadesc_set_length_status and dmadesc_set_addr both use
I/O accessors which use a volatile, so they should not be re-ordered
relative to each other.

I do agree that the change looks like how it should be done, I am just
questioning the qualification of this as a fix or not.

Thanks!

>
> Signed-off-by: Petri Gynther <pgynther@google.com>
> ---
>  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index d77cd6d..f7b42b9 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -104,8 +104,8 @@ static inline void dmadesc_set_addr(struct bcmgenet_priv *priv,
>  static inline void dmadesc_set(struct bcmgenet_priv *priv,
>                                void __iomem *d, dma_addr_t addr, u32 val)
>  {
> -       dmadesc_set_length_status(priv, d, val);
>         dmadesc_set_addr(priv, d, addr);
> +       dmadesc_set_length_status(priv, d, val);
>  }
>
>  static inline dma_addr_t dmadesc_get_addr(struct bcmgenet_priv *priv,
> --
> 2.8.0.rc3.226.g39d4020
>



-- 
Florian

Re: [PATCH net-next 1/3] net: bcmgenet: cleanup for bcmgenet_xmit()

[Florian Fainelli]

2016-04-04 17:09 GMT-07:00 Petri Gynther <pgynther@google.com>:
> 1. Readability: Move nr_frags assignment a few lines down in order
>    to bundle index -> ring -> txq calculations together.
> 2. Readability: Add parentheses around nr_frags + 1.
> 3. Minor fix: Stop the Tx queue and throw the error message only if
>    the Tx queue hasn't already been stopped.
>
> Signed-off-by: Petri Gynther <pgynther@google.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Re: [PATCH net-next 2/3] net: bcmgenet: cleanup for bcmgenet_xmit_frag()

[Florian Fainelli]

2016-04-04 17:10 GMT-07:00 Petri Gynther <pgynther@google.com>:
> Add frag_size = skb_frag_size(frag) and use it when needed.
>
> Signed-off-by: Petri Gynther <pgynther@google.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Re: [PATCH net-next 3/3] net: bcmgenet: fix dmadesc_set()

[Petri Gynther]

Hi Florian,

On Mon, Apr 4, 2016 at 5:58 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> 2016-04-04 17:10 GMT-07:00 Petri Gynther <pgynther@google.com>:
> > dmadesc_set() is used for setting the Tx buffer DMA address, length,
> > and status bits on a Tx ring descriptor when a frame is being Tx'ed.
> >
> > Always set the Tx buffer DMA address first, before updating the length
> > and status bits, i.e. giving the Tx descriptor to the hardware.
>
> Does this fix any real bug you have observed? The hardware won't
> transmit anything until you start writing the correct TDMA producer
> index. Also, dmadesc_set_length_status and dmadesc_set_addr both use
> I/O accessors which use a volatile, so they should not be re-ordered
> relative to each other.
>
> I do agree that the change looks like how it should be done, I am just
> questioning the qualification of this as a fix or not.
>
> Thanks!

You are right. Nothing is transmitted until TDMA producer index is
incremented, and that happens after dmadesc_set() has been called.

So, this is really just a cleanup. Perhaps not worth it, after all.

But I do have a question -- do we need wmb() in bcmgenet_xmit(), just
before we kick the TDMA producer index, to ensure Tx descriptor writes
have been completed?

@@ -1606,6 +1606,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff
*skb, struct net_device *dev)

        if (!skb->xmit_more || netif_xmit_stopped(txq)) {
                /* Packets are ready, update producer index */
+               wmb();
                bcmgenet_tdma_ring_writel(priv, ring->index,
                                          ring->prod_index, TDMA_PROD_INDEX);

>
>
> >
> > Signed-off-by: Petri Gynther <pgynther@google.com>
> > ---
> >  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > index d77cd6d..f7b42b9 100644
> > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> > @@ -104,8 +104,8 @@ static inline void dmadesc_set_addr(struct bcmgenet_priv *priv,
> >  static inline void dmadesc_set(struct bcmgenet_priv *priv,
> >                                void __iomem *d, dma_addr_t addr, u32 val)
> >  {
> > -       dmadesc_set_length_status(priv, d, val);
> >         dmadesc_set_addr(priv, d, addr);
> > +       dmadesc_set_length_status(priv, d, val);
> >  }
> >
> >  static inline dma_addr_t dmadesc_get_addr(struct bcmgenet_priv *priv,
> > --
> > 2.8.0.rc3.226.g39d4020
> >
>
>
>
> --
> Florian

Re: [PATCH net-next 3/3] net: bcmgenet: fix dmadesc_set()

[Florian Fainelli]

2016-04-04 18:11 GMT-07:00 Petri Gynther <pgynther@google.com>:
> Hi Florian,
>
> On Mon, Apr 4, 2016 at 5:58 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> 2016-04-04 17:10 GMT-07:00 Petri Gynther <pgynther@google.com>:
>> > dmadesc_set() is used for setting the Tx buffer DMA address, length,
>> > and status bits on a Tx ring descriptor when a frame is being Tx'ed.
>> >
>> > Always set the Tx buffer DMA address first, before updating the length
>> > and status bits, i.e. giving the Tx descriptor to the hardware.
>>
>> Does this fix any real bug you have observed? The hardware won't
>> transmit anything until you start writing the correct TDMA producer
>> index. Also, dmadesc_set_length_status and dmadesc_set_addr both use
>> I/O accessors which use a volatile, so they should not be re-ordered
>> relative to each other.
>>
>> I do agree that the change looks like how it should be done, I am just
>> questioning the qualification of this as a fix or not.
>>
>> Thanks!
>
> You are right. Nothing is transmitted until TDMA producer index is
> incremented, and that happens after dmadesc_set() has been called.
>
> So, this is really just a cleanup. Perhaps not worth it, after all.

OK, this is fine as a cleanup, and the typical logic is to write the
address first and the length/status second in case of descriptor-based
DMA-backed NICs, so why not.

>
> But I do have a question -- do we need wmb() in bcmgenet_xmit(), just
> before we kick the TDMA producer index, to ensure Tx descriptor writes
> have been completed?

Humm, that is a good question. We definitively need to make sure that
the writes to prod_index completes, since that variable is stored in
DRAM/normal memory, so subject to pre-fetching based on the memory
attributes (at least on ARM). It seems to me like there is a direct
dependency though, so by the time we use ring->prod_index, the CPU
should have ensured that all writes are complete.

>
> @@ -1606,6 +1606,7 @@ static netdev_tx_t bcmgenet_xmit(struct sk_buff
> *skb, struct net_device *dev)
>
>         if (!skb->xmit_more || netif_xmit_stopped(txq)) {
>                 /* Packets are ready, update producer index */
> +               wmb();
>                 bcmgenet_tdma_ring_writel(priv, ring->index,
>                                           ring->prod_index, TDMA_PROD_INDEX);
>
>>
>>
>> >
>> > Signed-off-by: Petri Gynther <pgynther@google.com>
>> > ---
>> >  drivers/net/ethernet/broadcom/genet/bcmgenet.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> > index d77cd6d..f7b42b9 100644
>> > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
>> > @@ -104,8 +104,8 @@ static inline void dmadesc_set_addr(struct bcmgenet_priv *priv,
>> >  static inline void dmadesc_set(struct bcmgenet_priv *priv,
>> >                                void __iomem *d, dma_addr_t addr, u32 val)
>> >  {
>> > -       dmadesc_set_length_status(priv, d, val);
>> >         dmadesc_set_addr(priv, d, addr);
>> > +       dmadesc_set_length_status(priv, d, val);
>> >  }
>> >
>> >  static inline dma_addr_t dmadesc_get_addr(struct bcmgenet_priv *priv,
>> > --
>> > 2.8.0.rc3.226.g39d4020
>> >
>>
>>
>>
>> --
>> Florian



-- 
Florian

RE: [PATCH net-next 1/3] net: bcmgenet: cleanup for bcmgenet_xmit()

[David Laight]

From: Petri Gynther
> Sent: 05 April 2016 01:10
...
> 2. Readability: Add parentheses around nr_frags + 1.
...
> -	if (ring->free_bds <= nr_frags + 1) {
...
> +	if (ring->free_bds <= (nr_frags + 1)) {

The extra () are not needed and do not improve readability.

	David

Re: [PATCH net-next 1/3] net: bcmgenet: cleanup for bcmgenet_xmit()

[Petri Gynther]

On Wed, Apr 6, 2016 at 1:57 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Petri Gynther
>> Sent: 05 April 2016 01:10
> ...
>> 2. Readability: Add parentheses around nr_frags + 1.
> ...
>> -     if (ring->free_bds <= nr_frags + 1) {
> ...
>> +     if (ring->free_bds <= (nr_frags + 1)) {
>
> The extra () are not needed and do not improve readability.
>
>         David
>

Not needed from C language point of view, but I personally like it
better that way.

Also, making it consistent with these:
bcmgenet.c:1227: if (ring->free_bds > (MAX_SKB_FRAGS + 1)) {
bcmgenet.c:1523: if (ring->free_bds <= (MAX_SKB_FRAGS + 1))

Re: [PATCH net-next 1/3] net: bcmgenet: cleanup for bcmgenet_xmit()

[David Miller]

From: Petri Gynther <pgynther@google.com>
Date: Mon,  4 Apr 2016 17:09:59 -0700

> 1. Readability: Move nr_frags assignment a few lines down in order
>    to bundle index -> ring -> txq calculations together.
> 2. Readability: Add parentheses around nr_frags + 1.
> 3. Minor fix: Stop the Tx queue and throw the error message only if
>    the Tx queue hasn't already been stopped.
> 
> Signed-off-by: Petri Gynther <pgynther@google.com>

Applied.

Re: [PATCH net-next 2/3] net: bcmgenet: cleanup for bcmgenet_xmit_frag()

[David Miller]

From: Petri Gynther <pgynther@google.com>
Date: Mon,  4 Apr 2016 17:10:00 -0700

> Add frag_size = skb_frag_size(frag) and use it when needed.
> 
> Signed-off-by: Petri Gynther <pgynther@google.com>

Applied.

Re: [PATCH net-next 3/3] net: bcmgenet: fix dmadesc_set()

[David Miller]

From: Petri Gynther <pgynther@google.com>
Date: Mon,  4 Apr 2016 17:10:01 -0700

> dmadesc_set() is used for setting the Tx buffer DMA address, length,
> and status bits on a Tx ring descriptor when a frame is being Tx'ed.
> 
> Always set the Tx buffer DMA address first, before updating the length
> and status bits, i.e. giving the Tx descriptor to the hardware.
> 
> Signed-off-by: Petri Gynther <pgynther@google.com>

Applied.