[BUG] bcmgenet tx path

[BUG] bcmgenet tx path

[Eric Dumazet]

Hi Florian

I was looking at drivers/net/ethernet/broadcom/genet/bcmgenet.c
and found TX completion (__bcmgenet_tx_reclaim()) was freeing skb before
frags were actually dma unmapped.


Are you sure this is OK ? bnx2 and bnx2x actually do the reverse (unmap
all frags before freeing skb)

A second problem is the dma_unmap_single() uses tx_cb_ptr->skb->len for
the length, while it really should be the same thing that was used in
bcmgenet_xmit_single()

skb_len = skb_headlen(skb) < ETH_ZLEN ? ETH_ZLEN : skb_headlen(skb);

So maybe something like :

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d7e01a7..9211b9c7 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1197,7 +1197,7 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
 			dev->stats.tx_bytes += tx_cb_ptr->skb->len;
 			dma_unmap_single(&dev->dev,
 					 dma_unmap_addr(tx_cb_ptr, dma_addr),
-					 tx_cb_ptr->skb->len,
+					 dma_unmap_len(tx_cb_ptr, dma_len),
 					 DMA_TO_DEVICE);
 			bcmgenet_free_cb(tx_cb_ptr);
 		} else if (dma_unmap_addr(tx_cb_ptr, dma_addr)) {
@@ -1308,7 +1308,7 @@ static int bcmgenet_xmit_single(struct net_device *dev,
 	}
 
 	dma_unmap_addr_set(tx_cb_ptr, dma_addr, mapping);
-	dma_unmap_len_set(tx_cb_ptr, dma_len, skb->len);
+	dma_unmap_len_set(tx_cb_ptr, dma_len, skb_len);
 	length_status = (skb_len << DMA_BUFLENGTH_SHIFT) | dma_desc_flags |
 			(priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT) |
 			DMA_TX_APPEND_CRC;

Re: [BUG] bcmgenet tx path

[Florian Fainelli]

Hi Eric,

On 16/03/16 07:18, Eric Dumazet wrote:
> Hi Florian
> 
> I was looking at drivers/net/ethernet/broadcom/genet/bcmgenet.c
> and found TX completion (__bcmgenet_tx_reclaim()) was freeing skb before
> frags were actually dma unmapped.
> 
> 
> Are you sure this is OK ? bnx2 and bnx2x actually do the reverse (unmap
> all frags before freeing skb)

It does not seem to be much of an issue right now because we put the skb
first, and the frags next in the order we want to transmit them, and we
reclaim in the same order, but it certainly makes more sense to revese
the operation.

> 
> A second problem is the dma_unmap_single() uses tx_cb_ptr->skb->len for
> the length, while it really should be the same thing that was used in
> bcmgenet_xmit_single()

Yes, that is indeed a bug, surprisingly the DMA-API debugging did not
seem to complain about that (should look into why). FWIW, we do not turn
on SG by default, so AFAIR we are not even hitting this path.

Can you submit a formal patch for this and we look into reversing the
mapping of fragments?

Thanks!

> 
> skb_len = skb_headlen(skb) < ETH_ZLEN ? ETH_ZLEN : skb_headlen(skb);
> 
> So maybe something like :
> 
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index d7e01a7..9211b9c7 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -1197,7 +1197,7 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
>  			dev->stats.tx_bytes += tx_cb_ptr->skb->len;
>  			dma_unmap_single(&dev->dev,
>  					 dma_unmap_addr(tx_cb_ptr, dma_addr),
> -					 tx_cb_ptr->skb->len,
> +					 dma_unmap_len(tx_cb_ptr, dma_len),
>  					 DMA_TO_DEVICE);
>  			bcmgenet_free_cb(tx_cb_ptr);
>  		} else if (dma_unmap_addr(tx_cb_ptr, dma_addr)) {
> @@ -1308,7 +1308,7 @@ static int bcmgenet_xmit_single(struct net_device *dev,
>  	}
>  
>  	dma_unmap_addr_set(tx_cb_ptr, dma_addr, mapping);
> -	dma_unmap_len_set(tx_cb_ptr, dma_len, skb->len);
> +	dma_unmap_len_set(tx_cb_ptr, dma_len, skb_len);
>  	length_status = (skb_len << DMA_BUFLENGTH_SHIFT) | dma_desc_flags |
>  			(priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT) |
>  			DMA_TX_APPEND_CRC;
> 
> 


-- 
Florian

[PATCH net] net: bcmgenet: fix dma api length mismatch

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

When un-mapping skb->data in __bcmgenet_tx_reclaim(),
we must use the length that was used in original dma_map_single(),
instead of skb->len that might be bigger (includes the frags)

We simply can store skb_len into tx_cb_ptr->dma_len and use it
at unmap time.

Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d7e01a7..6746fd0 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1197,7 +1197,7 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
 			dev->stats.tx_bytes += tx_cb_ptr->skb->len;
 			dma_unmap_single(&dev->dev,
 					 dma_unmap_addr(tx_cb_ptr, dma_addr),
-					 tx_cb_ptr->skb->len,
+					 dma_unmap_len(tx_cb_ptr, dma_len),
 					 DMA_TO_DEVICE);
 			bcmgenet_free_cb(tx_cb_ptr);
 		} else if (dma_unmap_addr(tx_cb_ptr, dma_addr)) {
@@ -1308,7 +1308,7 @@ static int bcmgenet_xmit_single(struct net_device *dev,
 	}
 
 	dma_unmap_addr_set(tx_cb_ptr, dma_addr, mapping);
-	dma_unmap_len_set(tx_cb_ptr, dma_len, skb->len);
+	dma_unmap_len_set(tx_cb_ptr, dma_len, skb_len);
 	length_status = (skb_len << DMA_BUFLENGTH_SHIFT) | dma_desc_flags |
 			(priv->hw_params->qtag_mask << DMA_TX_QTAG_SHIFT) |
 			DMA_TX_APPEND_CRC;

Re: [PATCH net] net: bcmgenet: fix dma api length mismatch

[Florian Fainelli]

On 17/03/16 11:57, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> When un-mapping skb->data in __bcmgenet_tx_reclaim(),
> we must use the length that was used in original dma_map_single(),
> instead of skb->len that might be bigger (includes the frags)
> 
> We simply can store skb_len into tx_cb_ptr->dma_len and use it
> at unmap time.
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

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

Good catch, thanks!
-- 
Florian

Re: [PATCH net] net: bcmgenet: fix dma api length mismatch

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 17 Mar 2016 11:57:06 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> When un-mapping skb->data in __bcmgenet_tx_reclaim(),
> we must use the length that was used in original dma_map_single(),
> instead of skb->len that might be bigger (includes the frags)
> 
> We simply can store skb_len into tx_cb_ptr->dma_len and use it
> at unmap time.
> 
> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied and queued up for -stable, thanks.