8139cp dma-debug warning.

Submitted by David Miller on Aug. 13, 2009, 5:20 a.m.

Details

Message ID 20090812.222046.60033759.davem@davemloft.net
State Not Applicable
Headers show

Commit Message

David Miller Aug. 13, 2009, 5:20 a.m.
From: Dave Jones <davej@redhat.com>
Date: Wed, 12 Aug 2009 13:13:33 -0400

> There's another instance of the same further in the file.
> 
> Does this look right?

Dave, Francois posted the following fix to lkml (he forgot
to CC: netdev and the people reporting this bug, oh well)

Did you see it?  It should fix the bug.

8139cp: balance dma_map_single vs dma_unmap_single pair

The driver always:
1. allocate cp->rx_buf_sz + NET_IP_ALIGN
2. map cp->rx_buf_sz

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/net/8139cp.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

Comments

Dave Jones Aug. 13, 2009, 1:45 p.m.
On Wed, Aug 12, 2009 at 10:20:46PM -0700, David Miller wrote:
 > From: Dave Jones <davej@redhat.com>
 > Date: Wed, 12 Aug 2009 13:13:33 -0400
 > 
 > > There's another instance of the same further in the file.
 > > 
 > > Does this look right?
 > 
 > Dave, Francois posted the following fix to lkml (he forgot
 > to CC: netdev and the people reporting this bug, oh well)

Ah, I missed it (disk crashed on friday, so I've been backlogged
since then). Thanks for the forward.

 > Did you see it?  It should fix the bug.
 > 
 > 8139cp: balance dma_map_single vs dma_unmap_single pair
 > 
 > The driver always:
 > 1. allocate cp->rx_buf_sz + NET_IP_ALIGN
 > 2. map cp->rx_buf_sz
 > 
 > Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
 > Signed-off-by: David S. Miller <davem@davemloft.net>
 > ---
 >  drivers/net/8139cp.c |    5 ++---
 >  1 files changed, 2 insertions(+), 3 deletions(-)
 > 
 > diff --git a/drivers/net/8139cp.c b/drivers/net/8139cp.c
 > index 50efde1..d0dbbf3 100644
 > --- a/drivers/net/8139cp.c
 > +++ b/drivers/net/8139cp.c
 > @@ -515,7 +515,7 @@ rx_status_loop:
 >  		dma_addr_t mapping;
 >  		struct sk_buff *skb, *new_skb;
 >  		struct cp_desc *desc;
 > -		unsigned buflen;
 > +		const unsigned buflen = cp->rx_buf_sz;
 >  
 >  		skb = cp->rx_skb[rx_tail];
 >  		BUG_ON(!skb);
 > @@ -549,8 +549,7 @@ rx_status_loop:
 >  			pr_debug("%s: rx slot %d status 0x%x len %d\n",
 >  			       dev->name, rx_tail, status, len);
 >  
 > -		buflen = cp->rx_buf_sz + NET_IP_ALIGN;
 > -		new_skb = netdev_alloc_skb(dev, buflen);
 > +		new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN);
 >  		if (!new_skb) {
 >  			dev->stats.rx_dropped++;
 >  			goto rx_next;

Below this, we're still doing an skb_reserve(NET_IP_ALIGN) on new_skb.
Although the mapping is now constantly sized, aren't we still wastefully
bumping the data/tail of the skb twice ?

	Dave

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
fran├žois romieu Aug. 13, 2009, 7:23 p.m.
Dave Jones <davej@redhat.com> :
[...]
>  > @@ -549,8 +549,7 @@ rx_status_loop:
>  >  			pr_debug("%s: rx slot %d status 0x%x len %d\n",
>  >  			       dev->name, rx_tail, status, len);
>  >  
>  > -		buflen = cp->rx_buf_sz + NET_IP_ALIGN;
>  > -		new_skb = netdev_alloc_skb(dev, buflen);
>  > +		new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN);
>  >  		if (!new_skb) {
>  >  			dev->stats.rx_dropped++;
>  >  			goto rx_next;
> 
> Below this, we're still doing an skb_reserve(NET_IP_ALIGN) on new_skb.
> Although the mapping is now constantly sized, aren't we still wastefully
> bumping the data/tail of the skb twice ?

$ grep -n NET_IP_ALIGN drivers/net/8139cp.c
552:		new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN);
558:		skb_reserve(new_skb, NET_IP_ALIGN);
1059:		skb = netdev_alloc_skb(dev, cp->rx_buf_sz + NET_IP_ALIGN);
1063:		skb_reserve(skb, NET_IP_ALIGN);

I do not get it : netdev_alloc_skb allocates but it does not "bump" as
skb_reserve does (and skb_reserve does not allocate). Where would the
double bump come from ?

The mapping is constantly sized but we want the ethernet header following
IP data to be evenly aligned and thus the mapping to be oddly aligned.
Dave Jones Aug. 13, 2009, 7:28 p.m.
On Thu, Aug 13, 2009 at 09:23:26PM +0200, Francois Romieu wrote:
 
 > > Below this, we're still doing an skb_reserve(NET_IP_ALIGN) on new_skb.
 > > Although the mapping is now constantly sized, aren't we still wastefully
 > > bumping the data/tail of the skb twice ?
 > 
 > $ grep -n NET_IP_ALIGN drivers/net/8139cp.c
 > 552:		new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN);
 > 558:		skb_reserve(new_skb, NET_IP_ALIGN);
 > 1059:		skb = netdev_alloc_skb(dev, cp->rx_buf_sz + NET_IP_ALIGN);
 > 1063:		skb_reserve(skb, NET_IP_ALIGN);
 > 
 > I do not get it : netdev_alloc_skb allocates but it does not "bump" as
 > skb_reserve does (and skb_reserve does not allocate). Where would the
 > double bump come from ?
 > 
 > The mapping is constantly sized but we want the ethernet header following
 > IP data to be evenly aligned and thus the mapping to be oddly aligned.

Ah, now I get it. Thanks for the explanation :)

	Dave

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/net/8139cp.c b/drivers/net/8139cp.c
index 50efde1..d0dbbf3 100644
--- a/drivers/net/8139cp.c
+++ b/drivers/net/8139cp.c
@@ -515,7 +515,7 @@  rx_status_loop:
 		dma_addr_t mapping;
 		struct sk_buff *skb, *new_skb;
 		struct cp_desc *desc;
-		unsigned buflen;
+		const unsigned buflen = cp->rx_buf_sz;
 
 		skb = cp->rx_skb[rx_tail];
 		BUG_ON(!skb);
@@ -549,8 +549,7 @@  rx_status_loop:
 			pr_debug("%s: rx slot %d status 0x%x len %d\n",
 			       dev->name, rx_tail, status, len);
 
-		buflen = cp->rx_buf_sz + NET_IP_ALIGN;
-		new_skb = netdev_alloc_skb(dev, buflen);
+		new_skb = netdev_alloc_skb(dev, buflen + NET_IP_ALIGN);
 		if (!new_skb) {
 			dev->stats.rx_dropped++;
 			goto rx_next;