[PATCH 00/11] DMA sync errors

[PATCH 00/11] DMA sync errors

[Stephen Hemminger]

According to the DMA-API.txt
  Synchronise a single contiguous or scatter/gather mapping.  All the
  parameters must be the same as those passed into the single mapping
  API.

Reviewed the following drivers with potentail for this problem;
those with - are ok, and the ones with + have a patch in this
series.

+ drivers/infiniband/ulp/ipoib/ipoib_cm.c
- drivers/message/fusion/mptlan.c
- drivers/net/b44.c
- drivers/net/bnx2.c
+ drivers/net/cassini.c
- drivers/net/chelsio/sge.c
+ drivers/net/cxgb3/sge.c
- drivers/net/irda/vlsi_ir.c
- drivers/net/myri_sbus.c
+ drivers/net/r8169.c
+ drivers/net/rrunner.c
+ drivers/net/skge.c
+ drivers/net/sky2.c
+ drivers/net/sungem.c
+ drivers/net/sunhme.c
+ drivers/net/tg3.c
- drivers/net/tokenring/3c359.c
- drivers/net/tokenring/olympic.c
+ drivers/net/tulip/de2104x.c
- drivers/net/via-velocity.c
- drivers/net/wireless/ipw2x00/ipw2100.c
- drivers/net/wireless/ipw2x00/ipw2200.c

[PATCH 01/11] tg3: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: tg3-dma-fix.patch --]
[-- Type: text/plain, Size: 951 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/tg3.c	2010-01-19 21:38:07.680163220 -0800
+++ b/drivers/net/tg3.c	2010-01-20 10:03:52.598977275 -0800
@@ -4653,9 +4653,13 @@ static int tg3_rx(struct tg3_napi *tnapi
 
 			skb_reserve(copy_skb, TG3_RAW_IP_ALIGN);
 			skb_put(copy_skb, len);
-			pci_dma_sync_single_for_cpu(tp->pdev, dma_addr, len, PCI_DMA_FROMDEVICE);
+			pci_dma_sync_single_for_cpu(tp->pdev, dma_addr,
+						    tp->rx_pkt_map_sz,
+						    PCI_DMA_FROMDEVICE);
 			skb_copy_from_linear_data(skb, copy_skb->data, len);
-			pci_dma_sync_single_for_device(tp->pdev, dma_addr, len, PCI_DMA_FROMDEVICE);
+			pci_dma_sync_single_for_device(tp->pdev, dma_addr,
+						       tp->rx_pkt_map_sz,
+						       PCI_DMA_FROMDEVICE);
 
 			/* We'll reuse the original ring buffer. */
 			skb = copy_skb;

-- 

[PATCH 02/11] sky2: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: sky2-dmar-error.patch --]
[-- Type: text/plain, Size: 1544 bytes --]

From: Jarek Poplawski <jarkao2@gmail.com>

Using pci_unmap_len(), with the same length as pci_map_single(), with
pci_dma_sync_single_for_cpu()/_device() fixes this warning (2.6.32.4):

> Jan 19 10:43:50 mail kernel: WARNING: at lib/dma-debug.c:902 
> check_sync+0xc1/0x43f()
> Jan 19 10:43:50 mail kernel: Hardware name: System Product Name
> Jan 19 10:43:50 mail kernel: sky2 0000:04:00.0: DMA-API: device driver 
> tries to sync DMA memory it has not allocated [device 
> address=0x0000000320a0b022] [size=60 bytes]  

Reported-by: Michael Breuer <mbreuer@majjas.com>
Tested-by: Michael Breuer <mbreuer@majjas.com>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
Acked-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sky2.c	2010-01-20 10:04:08.629387601 -0800
+++ b/drivers/net/sky2.c	2010-01-20 10:05:39.333941031 -0800
@@ -2252,12 +2252,14 @@ static struct sk_buff *receive_copy(stru
 	skb = netdev_alloc_skb_ip_align(sky2->netdev, length);
 	if (likely(skb)) {
 		pci_dma_sync_single_for_cpu(sky2->hw->pdev, re->data_addr,
-					    length, PCI_DMA_FROMDEVICE);
+					    pci_unmap_len(re, data_size),
+					    PCI_DMA_FROMDEVICE);
 		skb_copy_from_linear_data(re->skb, skb->data, length);
 		skb->ip_summed = re->skb->ip_summed;
 		skb->csum = re->skb->csum;
 		pci_dma_sync_single_for_device(sky2->hw->pdev, re->data_addr,
-					       length, PCI_DMA_FROMDEVICE);
+					       pci_unmap_len(re, data_size),
+					       PCI_DMA_FROMDEVICE);
 		re->skb->ip_summed = CHECKSUM_NONE;
 		skb_put(skb, length);
 	}

-- 

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[Jarek Poplawski]

On Wed, Jan 20, 2010 at 12:45:01PM -0800, Stephen Hemminger wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> 
> Using pci_unmap_len(), with the same length as pci_map_single(), with
> pci_dma_sync_single_for_cpu()/_device() fixes this warning (2.6.32.4):
> 
> > Jan 19 10:43:50 mail kernel: WARNING: at lib/dma-debug.c:902 
> > check_sync+0xc1/0x43f()
> > Jan 19 10:43:50 mail kernel: Hardware name: System Product Name
> > Jan 19 10:43:50 mail kernel: sky2 0000:04:00.0: DMA-API: device driver 
> > tries to sync DMA memory it has not allocated [device 
> > address=0x0000000320a0b022] [size=60 bytes]  
> 
> Reported-by: Michael Breuer <mbreuer@majjas.com>
> Tested-by: Michael Breuer <mbreuer@majjas.com>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> Acked-by: Stephen Hemminger <shemminger@vyatta.com>

Thanks for acking and completing this, Stephen!

Jarek P.

> 
> --- a/drivers/net/sky2.c	2010-01-20 10:04:08.629387601 -0800
> +++ b/drivers/net/sky2.c	2010-01-20 10:05:39.333941031 -0800
> @@ -2252,12 +2252,14 @@ static struct sk_buff *receive_copy(stru
>  	skb = netdev_alloc_skb_ip_align(sky2->netdev, length);
>  	if (likely(skb)) {
>  		pci_dma_sync_single_for_cpu(sky2->hw->pdev, re->data_addr,
> -					    length, PCI_DMA_FROMDEVICE);
> +					    pci_unmap_len(re, data_size),
> +					    PCI_DMA_FROMDEVICE);
>  		skb_copy_from_linear_data(re->skb, skb->data, length);
>  		skb->ip_summed = re->skb->ip_summed;
>  		skb->csum = re->skb->csum;
>  		pci_dma_sync_single_for_device(sky2->hw->pdev, re->data_addr,
> -					       length, PCI_DMA_FROMDEVICE);
> +					       pci_unmap_len(re, data_size),
> +					       PCI_DMA_FROMDEVICE);
>  		re->skb->ip_summed = CHECKSUM_NONE;
>  		skb_put(skb, length);
>  	}
> 
> -- 
> 

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 20 Jan 2010 22:32:59 +0100

> On Wed, Jan 20, 2010 at 12:45:01PM -0800, Stephen Hemminger wrote:
>> From: Jarek Poplawski <jarkao2@gmail.com>
>> 
>> Using pci_unmap_len(), with the same length as pci_map_single(), with
>> pci_dma_sync_single_for_cpu()/_device() fixes this warning (2.6.32.4):
>> 
>> > Jan 19 10:43:50 mail kernel: WARNING: at lib/dma-debug.c:902 
>> > check_sync+0xc1/0x43f()
>> > Jan 19 10:43:50 mail kernel: Hardware name: System Product Name
>> > Jan 19 10:43:50 mail kernel: sky2 0000:04:00.0: DMA-API: device driver 
>> > tries to sync DMA memory it has not allocated [device 
>> > address=0x0000000320a0b022] [size=60 bytes]  
>> 
>> Reported-by: Michael Breuer <mbreuer@majjas.com>
>> Tested-by: Michael Breuer <mbreuer@majjas.com>
>> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>> Acked-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> Thanks for acking and completing this, Stephen!

It's not a bug, lib/dma-debug.c and the DMA API documentation
are both buggy.

I'm not applying any of this, the fix belongs in the infrastructure
debugging and documentation not in the drivers, they are doing the
correct and only reasonable thing.

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[Jarek Poplawski]

On Wed, Jan 20, 2010 at 02:52:18PM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Wed, 20 Jan 2010 22:32:59 +0100
> 
> > On Wed, Jan 20, 2010 at 12:45:01PM -0800, Stephen Hemminger wrote:
> >> From: Jarek Poplawski <jarkao2@gmail.com>
> >> 
> >> Using pci_unmap_len(), with the same length as pci_map_single(), with
> >> pci_dma_sync_single_for_cpu()/_device() fixes this warning (2.6.32.4):
> >> 
> >> > Jan 19 10:43:50 mail kernel: WARNING: at lib/dma-debug.c:902 
> >> > check_sync+0xc1/0x43f()
> >> > Jan 19 10:43:50 mail kernel: Hardware name: System Product Name
> >> > Jan 19 10:43:50 mail kernel: sky2 0000:04:00.0: DMA-API: device driver 
> >> > tries to sync DMA memory it has not allocated [device 
> >> > address=0x0000000320a0b022] [size=60 bytes]  
> >> 
> >> Reported-by: Michael Breuer <mbreuer@majjas.com>
> >> Tested-by: Michael Breuer <mbreuer@majjas.com>
> >> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> >> Acked-by: Stephen Hemminger <shemminger@vyatta.com>
> > 
> > Thanks for acking and completing this, Stephen!
> 
> It's not a bug, lib/dma-debug.c and the DMA API documentation
> are both buggy.
> 
> I'm not applying any of this, the fix belongs in the infrastructure
> debugging and documentation not in the drivers, they are doing the
> correct and only reasonable thing.

You might be 100% right, but why do you want users to pay for this
even one day longer than necessary?

1) The warning was called "oops" by the reporter at the beginning,
   and it's meaningful; it frightens people at least.
2) Fixing this temporarily e.g. in sky2 looks safe and simple, while
   checking if it's more than a buggy warning needs time; and this:

"dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
                      unsigned long offset, size_t size,
                      enum dma_data_direction direction)

Does a partial sync, starting at offset and continuing for size.  You
must be careful to observe the cache alignment and width when doing
anything like this.  You must also be extra careful about accessing
memory you intend to sync partially." [Documentation/DMA-API.txt]

looks like even if implemented might be very tricky, especially if
untested.

Jarek P.

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 21 Jan 2010 00:21:02 +0100

> "dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
>                       unsigned long offset, size_t size,
>                       enum dma_data_direction direction)
> 
> Does a partial sync, starting at offset and continuing for size.  You
> must be careful to observe the cache alignment and width when doing
> anything like this.  You must also be extra careful about accessing
> memory you intend to sync partially." [Documentation/DMA-API.txt]
> 
> looks like even if implemented might be very tricky, especially if
> untested.

This is an absurd set of requirements.  What driver author is
going to get this right?

Any alignment issues are of the domain of the implementation and as
Alan said should be handled there.

Back to the main issue, these localized fixes in the driver leave all
of the other cases in the tree unhandled.  There are likely to be
SCSI, USB host controller, SOUND, and other drivers with the same
issue.

At what point do we understand that peppering workarounds in the
drivers is the wrong way to go about this?

Whereas 1 patch in the 1 place in the DMA API implementation would fix
everything for everybody.

It's great that we had this report to track down the issue, now let's
fix it correctly so nobody else (regardless of device they have) has
to see it.

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 20 Jan 2010 15:34:51 -0800 (PST)

> Whereas 1 patch in the 1 place in the DMA API implementation would fix
> everything for everybody.

For the record I just checked DMAR and Intel-IOMMU and they
don't even implement the SYNC ops, they are NULL and thus
can't be effected by the changes being discussed here.

The generic DMA SYNC operations check for a NULL function
pointer in the DMA operations, and do nothing if it is NULL.

Thus, so far it is proven that only the DMA debugging code 
actually has this SYNC length requirement.

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[Jarek Poplawski]

On Wed, Jan 20, 2010 at 03:41:26PM -0800, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 20 Jan 2010 15:34:51 -0800 (PST)
> 
> > Whereas 1 patch in the 1 place in the DMA API implementation would fix
> > everything for everybody.
> 
> For the record I just checked DMAR and Intel-IOMMU and they
> don't even implement the SYNC ops, they are NULL and thus
> can't be effected by the changes being discussed here.
> 
> The generic DMA SYNC operations check for a NULL function
> pointer in the DMA operations, and do nothing if it is NULL.
> 
> Thus, so far it is proven that only the DMA debugging code 
> actually has this SYNC length requirement.

Thanks, I feel at least 75% safer ;-)

Good night!
Jarek P.

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[Stephen Hemminger]

On Thu, 21 Jan 2010 00:59:20 +0100
Jarek Poplawski <jarkao2@gmail.com> wrote:

> On Wed, Jan 20, 2010 at 03:41:26PM -0800, David Miller wrote:
> > From: David Miller <davem@davemloft.net>
> > Date: Wed, 20 Jan 2010 15:34:51 -0800 (PST)
> > 
> > > Whereas 1 patch in the 1 place in the DMA API implementation would fix
> > > everything for everybody.
> > 
> > For the record I just checked DMAR and Intel-IOMMU and they
> > don't even implement the SYNC ops, they are NULL and thus
> > can't be effected by the changes being discussed here.
> > 
> > The generic DMA SYNC operations check for a NULL function
> > pointer in the DMA operations, and do nothing if it is NULL.
> > 
> > Thus, so far it is proven that only the DMA debugging code 
> > actually has this SYNC length requirement.
> 
> Thanks, I feel at least 75% safer ;-)
> 

The usage of jumbo frames on the Marvell Yukon-2 chip version that
Michael has is suspect. In order to do Jumbo frames, the driver has
turns off the store-forward buffer; this reduces the window for transmit
underrun, and triggers a problem.  I suspect the unwind logic for
transmit underrun is broken and may not be fixable without more hardware
information.  Turning off jumbo frame support for that chip type
is probably the safest solution.

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 20 Jan 2010 16:04:07 -0800

> The usage of jumbo frames on the Marvell Yukon-2 chip version that
> Michael has is suspect. In order to do Jumbo frames, the driver has
> turns off the store-forward buffer; this reduces the window for transmit
> underrun, and triggers a problem.  I suspect the unwind logic for
> transmit underrun is broken and may not be fixable without more hardware
> information.  Turning off jumbo frame support for that chip type
> is probably the safest solution.

So let's do that.

Meanwhile I think the DMA debugging patch you posted earlier today
need a little work, will reply there.

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[Jarek Poplawski]

On Wed, Jan 20, 2010 at 03:34:51PM -0800, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Thu, 21 Jan 2010 00:21:02 +0100
> 
> > "dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
> >                       unsigned long offset, size_t size,
> >                       enum dma_data_direction direction)
> > 
> > Does a partial sync, starting at offset and continuing for size.  You
> > must be careful to observe the cache alignment and width when doing
> > anything like this.  You must also be extra careful about accessing
> > memory you intend to sync partially." [Documentation/DMA-API.txt]
> > 
> > looks like even if implemented might be very tricky, especially if
> > untested.
> 
> This is an absurd set of requirements.  What driver author is
> going to get this right?
> 
> Any alignment issues are of the domain of the implementation and as
> Alan said should be handled there.
> 
> Back to the main issue, these localized fixes in the driver leave all
> of the other cases in the tree unhandled.  There are likely to be
> SCSI, USB host controller, SOUND, and other drivers with the same
> issue.
> 
> At what point do we understand that peppering workarounds in the
> drivers is the wrong way to go about this?

*Temporary* only workarounds. Mainly simple cases. (e.g. I'm not sure
of tg3)

> 
> Whereas 1 patch in the 1 place in the DMA API implementation would fix
> everything for everybody.

1) As I mentioned earlier there were similar dmar errors reported with
real oopses, but of course it might be unconnected. (But why risk...)

2) Why would the author of this doc above warn us so much if it were
so obviously simple and safe?

> 
> It's great that we had this report to track down the issue, now let's
> fix it correctly so nobody else (regardless of device they have) has
> to see it.

I hope you're right as usual! (But I disagree as usual ;-)

Jarek P.

Re: [PATCH 02/11] sky2: fix DMA sync_single length error

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 21 Jan 2010 00:52:45 +0100

> On Wed, Jan 20, 2010 at 03:34:51PM -0800, David Miller wrote:
>> Whereas 1 patch in the 1 place in the DMA API implementation would fix
>> everything for everybody.
> 
> 1) As I mentioned earlier there were similar dmar errors reported with
> real oopses, but of course it might be unconnected. (But why risk...)

Show me the DMAR code that handles DMA SYNC requests.

There isn't any.

So it's impossible for this to fix things for DMAR users.

[PATCH 03/11] skge: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: skge-dmar-error.patch --]
[-- Type: text/plain, Size: 878 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/skge.c	2010-01-20 10:10:29.538786102 -0800
+++ b/drivers/net/skge.c	2010-01-20 10:11:18.443974883 -0800
@@ -3078,11 +3078,13 @@ static struct sk_buff *skge_rx_get(struc
 
 		pci_dma_sync_single_for_cpu(skge->hw->pdev,
 					    pci_unmap_addr(e, mapaddr),
-					    len, PCI_DMA_FROMDEVICE);
+					    pci_unmap_len(e, maplen),
+					    PCI_DMA_FROMDEVICE);
 		skb_copy_from_linear_data(e->skb, skb->data, len);
 		pci_dma_sync_single_for_device(skge->hw->pdev,
 					       pci_unmap_addr(e, mapaddr),
-					       len, PCI_DMA_FROMDEVICE);
+					       pci_unmap_len(e, maplen),
+					       PCI_DMA_FROMDEVICE);
 		skge_rx_reuse(e, skge->rx_buf_size);
 	} else {
 		struct sk_buff *nskb;

-- 

[PATCH 04/11] r8169: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: r8169-dmar-error.patch --]
[-- Type: text/plain, Size: 1623 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Get rid of unnecessary flag variable and move sync for
device to logical place next to sync_for_cpu

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/r8169.c	2010-01-20 10:12:22.488138228 -0800
+++ b/drivers/net/r8169.c	2010-01-20 10:15:21.199555458 -0800
@@ -4437,22 +4437,21 @@ static inline bool rtl8169_try_rx_copy(s
 				       dma_addr_t addr)
 {
 	struct sk_buff *skb;
-	bool done = false;
 
 	if (pkt_size >= rx_copybreak)
-		goto out;
+		return false;
 
 	skb = netdev_alloc_skb_ip_align(tp->dev, pkt_size);
 	if (!skb)
-		goto out;
+		return false;
 
-	pci_dma_sync_single_for_cpu(tp->pci_dev, addr, pkt_size,
+	pci_dma_sync_single_for_cpu(tp->pci_dev, addr, tp->rx_buf_sz,
 				    PCI_DMA_FROMDEVICE);
 	skb_copy_from_linear_data(*sk_buff, skb->data, pkt_size);
 	*sk_buff = skb;
-	done = true;
-out:
-	return done;
+	pci_dma_sync_single_for_device(tp->pci_dev, addr, tp->rx_buf_sz,
+				       PCI_DMA_FROMDEVICE);
+	return true;
 }
 
 static int rtl8169_rx_interrupt(struct net_device *dev,
@@ -4512,11 +4511,9 @@ static int rtl8169_rx_interrupt(struct n
 
 			rtl8169_rx_csum(skb, desc);
 
-			if (rtl8169_try_rx_copy(&skb, tp, pkt_size, addr)) {
-				pci_dma_sync_single_for_device(pdev, addr,
-					pkt_size, PCI_DMA_FROMDEVICE);
+			if (rtl8169_try_rx_copy(&skb, tp, pkt_size, addr))
 				rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
-			} else {
+			else {
 				pci_unmap_single(pdev, addr, tp->rx_buf_sz,
 						 PCI_DMA_FROMDEVICE);
 				tp->Rx_skbuff[entry] = NULL;

-- 

[PATCH 05/11] rrunner: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: rrunner-dma.patch --]
[-- Type: text/plain, Size: 855 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/rrunner.c	2010-01-20 11:32:54.859390177 -0800
+++ b/drivers/net/rrunner.c	2010-01-20 11:37:40.431004646 -0800
@@ -971,7 +971,7 @@ static void rx_int(struct net_device *de
 				} else {
 					pci_dma_sync_single_for_cpu(rrpriv->pci_dev,
 								    desc->addr.addrlo,
-								    pkt_len,
+								    dev->mtu + HIPPI_HLEN,
 								    PCI_DMA_FROMDEVICE);
 
 					memcpy(skb_put(skb, pkt_len),
@@ -979,7 +979,7 @@ static void rx_int(struct net_device *de
 
 					pci_dma_sync_single_for_device(rrpriv->pci_dev,
 								       desc->addr.addrlo,
-								       pkt_len,
+								       dev->mtu + HIPPI_HLEN,
 								       PCI_DMA_FROMDEVICE);
 				}
 			}else{

-- 

[PATCH 06/11] de2104x: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: de2104x.patch --]
[-- Type: text/plain, Size: 992 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/tulip/de2104x.c	2010-01-20 11:41:20.878138314 -0800
+++ b/drivers/net/tulip/de2104x.c	2010-01-20 11:41:59.858847380 -0800
@@ -456,11 +456,13 @@ static void de_rx (struct de_private *de
 					       buflen, PCI_DMA_FROMDEVICE);
 			de->rx_skb[rx_tail].skb = copy_skb;
 		} else {
-			pci_dma_sync_single_for_cpu(de->pdev, mapping, len, PCI_DMA_FROMDEVICE);
+			pci_dma_sync_single_for_cpu(de->pdev, mapping,
+						    buflen, PCI_DMA_FROMDEVICE);
 			skb_reserve(copy_skb, RX_OFFSET);
 			skb_copy_from_linear_data(skb, skb_put(copy_skb, len),
 						  len);
-			pci_dma_sync_single_for_device(de->pdev, mapping, len, PCI_DMA_FROMDEVICE);
+			pci_dma_sync_single_for_device(de->pdev, mapping,
+						       buflen, PCI_DMA_FROMDEVICE);
 
 			/* We'll reuse the original ring buffer. */
 			skb = copy_skb;

-- 

Re: [PATCH 06/11] de2104x: fix DMA sync_single length error

[Grant Grundler]

On Wed, Jan 20, 2010 at 12:45:05PM -0800, Stephen Hemminger wrote:
> The DMA api requires that the full mapping be sync'd when
> copying frame. First found by Jarek on sky2.

Are you referring to the example code in Documentation/PCI/PCI-DMA-mapping.txt?

I don't see any "Use the full mapping" statement otherwise.

Is there a DMA HW implementation that requires the syncing full buffer?

I think it would be interesting to point that out in the commit comments
since other NIC drivers are likely to also have this issue as well.

I have no objection to this patch. Just want to be clear why
it's being pushed.

Please added Acked-by: Grant Grundler <grundler@parisc-linux.org>

thanks,
grant


> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/drivers/net/tulip/de2104x.c	2010-01-20 11:41:20.878138314 -0800
> +++ b/drivers/net/tulip/de2104x.c	2010-01-20 11:41:59.858847380 -0800
> @@ -456,11 +456,13 @@ static void de_rx (struct de_private *de
>  					       buflen, PCI_DMA_FROMDEVICE);
>  			de->rx_skb[rx_tail].skb = copy_skb;
>  		} else {
> -			pci_dma_sync_single_for_cpu(de->pdev, mapping, len, PCI_DMA_FROMDEVICE);
> +			pci_dma_sync_single_for_cpu(de->pdev, mapping,
> +						    buflen, PCI_DMA_FROMDEVICE);
>  			skb_reserve(copy_skb, RX_OFFSET);
>  			skb_copy_from_linear_data(skb, skb_put(copy_skb, len),
>  						  len);
> -			pci_dma_sync_single_for_device(de->pdev, mapping, len, PCI_DMA_FROMDEVICE);
> +			pci_dma_sync_single_for_device(de->pdev, mapping,
> +						       buflen, PCI_DMA_FROMDEVICE);
>  
>  			/* We'll reuse the original ring buffer. */
>  			skb = copy_skb;
> 
> -- 

Re: [PATCH 06/11] de2104x: fix DMA sync_single length error

[Stephen Hemminger]

On Thu, 21 Jan 2010 18:02:44 -0700
Grant Grundler <grundler@parisc-linux.org> wrote:

> On Wed, Jan 20, 2010 at 12:45:05PM -0800, Stephen Hemminger wrote:
> > The DMA api requires that the full mapping be sync'd when
> > copying frame. First found by Jarek on sky2.
> 
> Are you referring to the example code in Documentation/PCI/PCI-DMA-mapping.txt?
> 
> I don't see any "Use the full mapping" statement otherwise.
> 
> Is there a DMA HW implementation that requires the syncing full buffer?
> 
> I think it would be interesting to point that out in the commit comments
> since other NIC drivers are likely to also have this issue as well.
> 
> I have no objection to this patch. Just want to be clear why
> it's being pushed.
> 
> Please added Acked-by: Grant Grundler <grundler@parisc-linux.org>

The documentation was in DMA-API.txt and it was wrong.
The DMA debug library was enforcing a bogus restriction.
Dave correctly dropped the patch.

[PATCH 07/11] sungem: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: sungem.patch --]
[-- Type: text/plain, Size: 947 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sungem.c	2010-01-20 11:43:58.779387706 -0800
+++ b/drivers/net/sungem.c	2010-01-20 11:52:50.398529616 -0800
@@ -847,9 +847,13 @@ static int gem_rx(struct gem *gp, int wo
 
 			skb_reserve(copy_skb, 2);
 			skb_put(copy_skb, len);
-			pci_dma_sync_single_for_cpu(gp->pdev, dma_addr, len, PCI_DMA_FROMDEVICE);
+			pci_dma_sync_single_for_cpu(gp->pdev, dma_addr,
+						    RX_BUF_ALLOC_SIZE(gp),
+						    PCI_DMA_FROMDEVICE);
 			skb_copy_from_linear_data(skb, copy_skb->data, len);
-			pci_dma_sync_single_for_device(gp->pdev, dma_addr, len, PCI_DMA_FROMDEVICE);
+			pci_dma_sync_single_for_device(gp->pdev, dma_addr,
+						       RX_BUF_ALLOC_SIZE(gp),
+						       PCI_DMA_FROMDEVICE);
 
 			/* We'll reuse the original ring buffer. */
 			skb = copy_skb;

-- 

[PATCH 08/11] sunhme: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: sunhme.patch --]
[-- Type: text/plain, Size: 949 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/sunhme.c	2010-01-20 11:51:53.579700152 -0800
+++ b/drivers/net/sunhme.c	2010-01-20 11:53:10.408138348 -0800
@@ -2062,9 +2062,11 @@ static void happy_meal_rx(struct happy_m
 
 			skb_reserve(copy_skb, 2);
 			skb_put(copy_skb, len);
-			dma_sync_single_for_cpu(hp->dma_dev, dma_addr, len, DMA_FROM_DEVICE);
+			dma_sync_single_for_cpu(hp->dma_dev, dma_addr,
+						RX_BUF_ALLOC_SIZE, DMA_FROM_DEVICE);
 			skb_copy_from_linear_data(skb, copy_skb->data, len);
-			dma_sync_single_for_device(hp->dma_dev, dma_addr, len, DMA_FROM_DEVICE);
+			dma_sync_single_for_device(hp->dma_dev, dma_addr,
+						   RX_BUF_ALLOC_SIZE, DMA_FROM_DEVICE);
 			/* Reuse original ring buffer. */
 			hme_write_rxd(hp, this,
 				      (RXFLAG_OWN|((RX_BUF_ALLOC_SIZE-RX_OFFSET)<<16)),

-- 

[PATCH 09/11] cassini: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: cassini.patch --]
[-- Type: text/plain, Size: 970 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/cassini.c	2010-01-20 11:54:22.608138316 -0800
+++ b/drivers/net/cassini.c	2010-01-20 11:55:56.108761882 -0800
@@ -2027,12 +2027,12 @@ static int cas_rx_process_pkt(struct cas
 		i = hlen;
 		if (!dlen) /* attach FCS */
 			i += cp->crc_size;
-		pci_dma_sync_single_for_cpu(cp->pdev, page->dma_addr + off, i,
-				    PCI_DMA_FROMDEVICE);
+		pci_dma_sync_single_for_cpu(cp->pdev, page->dma_addr,
+					    cp->page_size, PCI_DMA_FROMDEVICE);
 		addr = cas_page_map(page->buffer);
 		memcpy(p, addr + off, i);
-		pci_dma_sync_single_for_device(cp->pdev, page->dma_addr + off, i,
-				    PCI_DMA_FROMDEVICE);
+		pci_dma_sync_single_for_device(cp->pdev, page->dma_addr,
+					       cp->page_size, PCI_DMA_FROMDEVICE);
 		cas_page_unmap(addr);
 		RX_USED_ADD(page, 0x100);
 		p += hlen;

-- 

[PATCH 10/11] cxgb3: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: sge.patch --]
[-- Type: text/plain, Size: 975 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/cxgb3/sge.c	2010-01-20 11:57:01.479387584 -0800
+++ b/drivers/net/cxgb3/sge.c	2010-01-20 11:59:02.740012122 -0800
@@ -788,12 +788,15 @@ static struct sk_buff *get_packet(struct
 		if (likely(skb != NULL)) {
 			__skb_put(skb, len);
 			pci_dma_sync_single_for_cpu(adap->pdev,
-					    pci_unmap_addr(sd, dma_addr), len,
-					    PCI_DMA_FROMDEVICE);
+						    pci_unmap_addr(sd, dma_addr),
+						    fl->alloc_size,
+
+						    PCI_DMA_FROMDEVICE);
 			memcpy(skb->data, sd->skb->data, len);
 			pci_dma_sync_single_for_device(adap->pdev,
-					    pci_unmap_addr(sd, dma_addr), len,
-					    PCI_DMA_FROMDEVICE);
+						       pci_unmap_addr(sd, dma_addr),
+						       fl->alloc_size,
+						       PCI_DMA_FROMDEVICE);
 		} else if (!drop_thres)
 			goto use_orig_buf;
 recycle:

-- 

[PATCH 11/11] ipoib: fix DMA sync_single length error

[Stephen Hemminger]

[-- Attachment #1: ipoib.patch --]
[-- Type: text/plain, Size: 1356 bytes --]

The DMA api requires that the full mapping be sync'd when
copying frame. First found by Jarek on sky2.

Signed-off-by: Stephen Hemminger <shemminger-ZtmgI6mnKB3QT0dZR+AlfA@public.gmane.org>

--- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c	2010-01-20 12:02:29.269388203 -0800
+++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c	2010-01-20 12:24:37.828137536 -0800
@@ -624,11 +624,15 @@ void ipoib_cm_handle_rx_wc(struct net_de
 		small_skb = dev_alloc_skb(dlen + 12);
 		if (small_skb) {
 			skb_reserve(small_skb, 12);
-			ib_dma_sync_single_for_cpu(priv->ca, rx_ring[wr_id].mapping[0],
-						   dlen, DMA_FROM_DEVICE);
+			ib_dma_sync_single_for_cpu(priv->ca,
+						   rx_ring[wr_id].mapping[0],
+						   IPOIB_CM_HEAD_SIZE,
+						   DMA_FROM_DEVICE);
 			skb_copy_from_linear_data(skb, small_skb->data, dlen);
-			ib_dma_sync_single_for_device(priv->ca, rx_ring[wr_id].mapping[0],
-						      dlen, DMA_FROM_DEVICE);
+			ib_dma_sync_single_for_device(priv->ca,
+						      rx_ring[wr_id].mapping[0],
+						      IPOIB_CM_HEAD_SIZE,
+						      DMA_FROM_DEVICE);
 			skb_put(small_skb, dlen);
 			skb = small_skb;
 			goto copied;

-- 

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

Re: [PATCH 00/11] DMA sync errors

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 20 Jan 2010 12:44:59 -0800

> According to the DMA-API.txt
>   Synchronise a single contiguous or scatter/gather mapping.  All the
>   parameters must be the same as those passed into the single mapping
>   API.

I think this is unreasonable.

And it's going to kill performance for reception of small packets.

We might as well not do RX copybreak in any of these drivers any more
if we have to DMA sync the whole thing.

I think better to look at the various implementations of DMA APIs,
I bet they all handle this case just fine.  And those that don't
are easily fixed.  Then the documentation can be updated to match
and we can forget these crazy patches.

Re: [PATCH 00/11] DMA sync errors

[Stephen Hemminger]

This should fix the dma-debug API code (and documentation), to
avoid false positives when sync is done on a partial map.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/Documentation/DMA-API.txt	2010-01-20 15:17:01.390143729 -0800
+++ b/Documentation/DMA-API.txt	2010-01-20 15:18:48.967875255 -0800
@@ -377,9 +377,10 @@ void
 pci_dma_sync_sg(struct pci_dev *hwdev, struct scatterlist *sg,
 		       int nelems, int direction)
 
-Synchronise a single contiguous or scatter/gather mapping.  All the
-parameters must be the same as those passed into the single mapping
-API.
+Synchronise a single contiguous or scatter/gather mapping.  The
+device and handle must be the same as those passed into the single mapping
+API. The size can be less than the original mapping if only part
+of the mapping needs to be accessed.
 
 Notes:  You must do this:
 
--- a/lib/dma-debug.c	2010-01-20 15:22:55.919519883 -0800
+++ b/lib/dma-debug.c	2010-01-20 15:26:31.648895638 -0800
@@ -285,11 +285,9 @@ static struct dma_debug_entry *hash_buck
 	}
 
 	/*
-	 * If we have multiple matches but no perfect-fit, just return
-	 * NULL.
+	 * If we have multiple matches but no perfect-fit
+	 * return best value and let caller deal with it.
 	 */
-	ret = (matches == 1) ? ret : NULL;
-
 	return ret;
 }
 

Re: [PATCH 00/11] DMA sync errors

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 20 Jan 2010 15:28:32 -0800

> This should fix the dma-debug API code (and documentation), to
> avoid false positives when sync is done on a partial map.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
 ...
> --- a/lib/dma-debug.c	2010-01-20 15:22:55.919519883 -0800
> +++ b/lib/dma-debug.c	2010-01-20 15:26:31.648895638 -0800
> @@ -285,11 +285,9 @@ static struct dma_debug_entry *hash_buck
>  	}
>  
>  	/*
> -	 * If we have multiple matches but no perfect-fit, just return
> -	 * NULL.
> +	 * If we have multiple matches but no perfect-fit
> +	 * return best value and let caller deal with it.
>  	 */
> -	ret = (matches == 1) ? ret : NULL;
> -
>  	return ret;
>  }
>  

I think you have to enforce a perfect-fit match for the
non-SYNC cases.

check_sync() can pass in 'true' for a new bool argument "partial_ok"
added to hash_bucket_find() whereas check_unmap() can pass 'false'.

get_nr_mapped_entries() should pass 'false' as well.

Re: [PATCH 00/11] DMA sync errors

[Stephen Hemminger]

On Wed, 20 Jan 2010 16:18:05 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 20 Jan 2010 15:28:32 -0800
> 
> > This should fix the dma-debug API code (and documentation), to
> > avoid false positives when sync is done on a partial map.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>  ...
> > --- a/lib/dma-debug.c	2010-01-20 15:22:55.919519883 -0800
> > +++ b/lib/dma-debug.c	2010-01-20 15:26:31.648895638 -0800
> > @@ -285,11 +285,9 @@ static struct dma_debug_entry *hash_buck
> >  	}
> >  
> >  	/*
> > -	 * If we have multiple matches but no perfect-fit, just return
> > -	 * NULL.
> > +	 * If we have multiple matches but no perfect-fit
> > +	 * return best value and let caller deal with it.
> >  	 */
> > -	ret = (matches == 1) ? ret : NULL;
> > -
> >  	return ret;
> >  }
> >  
> 
> I think you have to enforce a perfect-fit match for the
> non-SYNC cases.
> 
> check_sync() can pass in 'true' for a new bool argument "partial_ok"
> added to hash_bucket_find() whereas check_unmap() can pass 'false'.
> 
> get_nr_mapped_entries() should pass 'false' as well.

check_unmap has checks for different size, different type,
call_ents, direction etc.

The code for get_nr_mapped_entries() is the only question;
it is used by unmap_sg() and probably needs perfect match only.

Re: [PATCH 00/11] DMA sync errors

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 20 Jan 2010 16:34:10 -0800

> check_unmap has checks for different size, different type,
> call_ents, direction etc.
> 
> The code for get_nr_mapped_entries() is the only question;
> it is used by unmap_sg() and probably needs perfect match only.

Ok.

Re: [PATCH 00/11] DMA sync errors

[Jarek Poplawski]

On Wed, Jan 20, 2010 at 04:34:10PM -0800, Stephen Hemminger wrote:
> On Wed, 20 Jan 2010 16:18:05 -0800 (PST)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Stephen Hemminger <shemminger@vyatta.com>
> > Date: Wed, 20 Jan 2010 15:28:32 -0800
> > 
> > > This should fix the dma-debug API code (and documentation), to
> > > avoid false positives when sync is done on a partial map.
> > > 
> > > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> >  ...
> > > --- a/lib/dma-debug.c	2010-01-20 15:22:55.919519883 -0800
> > > +++ b/lib/dma-debug.c	2010-01-20 15:26:31.648895638 -0800
> > > @@ -285,11 +285,9 @@ static struct dma_debug_entry *hash_buck
> > >  	}
> > >  
> > >  	/*
> > > -	 * If we have multiple matches but no perfect-fit, just return
> > > -	 * NULL.
> > > +	 * If we have multiple matches but no perfect-fit
> > > +	 * return best value and let caller deal with it.
> > >  	 */
> > > -	ret = (matches == 1) ? ret : NULL;
> > > -
> > >  	return ret;
> > >  }
> > >  
> > 
> > I think you have to enforce a perfect-fit match for the
> > non-SYNC cases.
> > 
> > check_sync() can pass in 'true' for a new bool argument "partial_ok"
> > added to hash_bucket_find() whereas check_unmap() can pass 'false'.
> > 
> > get_nr_mapped_entries() should pass 'false' as well.
> 
> check_unmap has checks for different size, different type,
> call_ents, direction etc.
> 
> The code for get_nr_mapped_entries() is the only question;
> it is used by unmap_sg() and probably needs perfect match only.
> 

Hmm... After looking at it more, mainly dma_sync_single_range()
implementations, I didn't find any reasons for these comments in
DMA-API.txt, but even if I missed something, this patch definitely
shows these lib/dma-debug warnings are buggy if they depend on the
number of mapped entries (while otherwise ref->size < entry->size
isn't reported). Btw, it seems code should try to return minimum
entry->size for such a check.

Thanks Stephen, now I feel 99.9% safe ;-)

Sorry for mistrust, David!
Jarek P.

Re: [PATCH 00/11] DMA sync errors

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 21 Jan 2010 12:44:23 +0000

> Sorry for mistrust, David!

That's twice in the past week Jarek.

One more and you will be in real troubles :-)