DMA sync errors

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]

From: Jarek Poplawski <[email protected]>

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):

Quoted 6 lines

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 <[email protected]>

Tested-by Michael Breuer <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

Acked-by Stephen Hemminger <[email protected]>

drivers/net/sky2.c +4 -2

@@ -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);
 	}

Signature · 1 line

-- 

Quoted 12 lines

From: Jarek Poplawski <[email protected]>

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):

Quoted 6 lines

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]

Quoted 4 lines

Reported-by Michael Breuer <[email protected]>

Tested-by Michael Breuer <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

Acked-by Stephen Hemminger <[email protected]>

Thanks for acking and completing this, Stephen!

From: Jarek Poplawski <[email protected]>
Date: Wed, 20 Jan 2010 22:32:59 +0100

Quoted 18 lines · Stephen Hemminger

Quoted 16 lines

From: Jarek Poplawski <[email protected]>

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):

Quoted 6 lines

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 <[email protected]>

Tested-by Michael Breuer <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

Acked-by Stephen Hemminger <[email protected]>

Quoted 1 line

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.

"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.

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

From: David Miller <[email protected]>
Date: Wed, 20 Jan 2010 15:34:51 -0800 (PST)

Quoted 2 lines

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.

Quoted 10 lines · David Miller

From: David Miller <[email protected]>
Date: Wed, 20 Jan 2010 15:34:51 -0800 (PST)

Quoted 2 lines

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.

Quoted 5 lines

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.

From: Jarek Poplawski <[email protected]>
Date: Thu, 21 Jan 2010 00:21:02 +0100

Quoted 11 lines

"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.

Quoted 2 lines

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...)

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 <[email protected]>

drivers/net/tulip/de2104x.c +4 -2

@@ -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;

Signature · 1 line

-- 

Quoted 2 lines · Stephen Hemminger

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 <[email protected]>

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.

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 <[email protected]>

lib/dma-debug.c +0 -0

@@ -285,11 +285,9 @@ static struct dma_debug_entry *hash_buck
 	}
 
 	/*

From: Stephen Hemminger <[email protected]>
Date: Wed, 20 Jan 2010 15:28:32 -0800

Quoted 4 lines

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 <[email protected]>

...

Quoted 6 lines

lib/dma-debug.c +0 -0

@@ -285,11 +285,9 @@ static struct dma_debug_entry *hash_buck
 	}
 
 	/*

lib/dma-debug.c static struct dma_debug_entry *hash_buck · line 293

Quoted 10 lines

lib/dma-debug.c +2 -4

-	 * 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.

Quoted 31 lines · On Wed, 20 Jan 2010 16:18:05 -0800 (PST) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Wed, 20 Jan 2010 15:28:32 -0800

Quoted 4 lines

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 <[email protected]>

...

Quoted 6 lines

lib/dma-debug.c +0 -0

@@ -285,11 +285,9 @@ static struct dma_debug_entry *hash_buck
 	}
 
 	/*

lib/dma-debug.c static struct dma_debug_entry *hash_buck · line 293

Quoted 10 lines

lib/dma-debug.c +2 -4

-	 * 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'.

Quoted 1 line

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.

Sorry for mistrust, David!