* [BUG] bcmgenet tx path
@ 2016-03-16 14:18 Eric Dumazet
2016-03-17 17:00 ` Florian Fainelli
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2016-03-16 14:18 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev
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;
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [BUG] bcmgenet tx path
2016-03-16 14:18 [BUG] bcmgenet tx path Eric Dumazet
@ 2016-03-17 17:00 ` Florian Fainelli
2016-03-17 18:57 ` [PATCH net] net: bcmgenet: fix dma api length mismatch Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2016-03-17 17:00 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, opendmb
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH net] net: bcmgenet: fix dma api length mismatch
2016-03-17 17:00 ` Florian Fainelli
@ 2016-03-17 18:57 ` Eric Dumazet
2016-03-17 23:16 ` Florian Fainelli
2016-03-19 3:13 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2016-03-17 18:57 UTC (permalink / raw)
To: Florian Fainelli, David Miller; +Cc: netdev, opendmb
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;
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] net: bcmgenet: fix dma api length mismatch
2016-03-17 18:57 ` [PATCH net] net: bcmgenet: fix dma api length mismatch Eric Dumazet
@ 2016-03-17 23:16 ` Florian Fainelli
2016-03-19 3:13 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: Florian Fainelli @ 2016-03-17 23:16 UTC (permalink / raw)
To: Eric Dumazet, David Miller; +Cc: netdev, opendmb
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net: bcmgenet: fix dma api length mismatch
2016-03-17 18:57 ` [PATCH net] net: bcmgenet: fix dma api length mismatch Eric Dumazet
2016-03-17 23:16 ` Florian Fainelli
@ 2016-03-19 3:13 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2016-03-19 3:13 UTC (permalink / raw)
To: eric.dumazet; +Cc: f.fainelli, netdev, opendmb
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-19 3:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-16 14:18 [BUG] bcmgenet tx path Eric Dumazet
2016-03-17 17:00 ` Florian Fainelli
2016-03-17 18:57 ` [PATCH net] net: bcmgenet: fix dma api length mismatch Eric Dumazet
2016-03-17 23:16 ` Florian Fainelli
2016-03-19 3:13 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).