* [PATCH 00/11] DMA sync errors
@ 2010-01-20 20:44 Stephen Hemminger
2010-01-20 20:45 ` [PATCH 01/11] tg3: fix DMA sync_single length error Stephen Hemminger
` (11 more replies)
0 siblings, 12 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:44 UTC (permalink / raw)
To: David Miller, Jarek Poplawski; +Cc: netdev
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 01/11] tg3: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 20:45 ` [PATCH 02/11] sky2: " Stephen Hemminger
` (10 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski, Michael Chan; +Cc: netdev
[-- 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;
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
2010-01-20 20:45 ` [PATCH 01/11] tg3: fix DMA sync_single length error Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 21:32 ` Jarek Poplawski
2010-01-20 20:45 ` [PATCH 03/11] skge: " Stephen Hemminger
` (9 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski; +Cc: netdev
[-- 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);
}
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 03/11] skge: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
2010-01-20 20:45 ` [PATCH 01/11] tg3: fix DMA sync_single length error Stephen Hemminger
2010-01-20 20:45 ` [PATCH 02/11] sky2: " Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 20:45 ` [PATCH 04/11] r8169: " Stephen Hemminger
` (8 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski; +Cc: netdev
[-- 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;
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 04/11] r8169: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
` (2 preceding siblings ...)
2010-01-20 20:45 ` [PATCH 03/11] skge: " Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 20:45 ` [PATCH 05/11] rrunner: " Stephen Hemminger
` (7 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski, Francois Romieu; +Cc: netdev
[-- 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;
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 05/11] rrunner: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
` (3 preceding siblings ...)
2010-01-20 20:45 ` [PATCH 04/11] r8169: " Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 20:45 ` [PATCH 06/11] de2104x: " Stephen Hemminger
` (6 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski; +Cc: netdev
[-- 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{
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 06/11] de2104x: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
` (4 preceding siblings ...)
2010-01-20 20:45 ` [PATCH 05/11] rrunner: " Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-22 1:02 ` Grant Grundler
2010-01-20 20:45 ` [PATCH 07/11] sungem: " Stephen Hemminger
` (5 subsequent siblings)
11 siblings, 1 reply; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski, Grant Grundler; +Cc: netdev
[-- 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;
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 07/11] sungem: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
` (5 preceding siblings ...)
2010-01-20 20:45 ` [PATCH 06/11] de2104x: " Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 20:45 ` [PATCH 08/11] sunhme: " Stephen Hemminger
` (4 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski; +Cc: netdev
[-- 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;
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 08/11] sunhme: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
` (6 preceding siblings ...)
2010-01-20 20:45 ` [PATCH 07/11] sungem: " Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 20:45 ` [PATCH 09/11] cassini: " Stephen Hemminger
` (3 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski; +Cc: netdev
[-- 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)),
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 09/11] cassini: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
` (7 preceding siblings ...)
2010-01-20 20:45 ` [PATCH 08/11] sunhme: " Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 20:45 ` [PATCH 10/11] cxgb3: " Stephen Hemminger
` (2 subsequent siblings)
11 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski; +Cc: netdev
[-- 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;
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 10/11] cxgb3: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
` (8 preceding siblings ...)
2010-01-20 20:45 ` [PATCH 09/11] cassini: " Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 20:45 ` [PATCH 11/11] ipoib: " Stephen Hemminger
2010-01-20 22:46 ` [PATCH 00/11] DMA sync errors David Miller
11 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski, Divy Le Ray; +Cc: netdev
[-- 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:
--
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 11/11] ipoib: fix DMA sync_single length error
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
` (9 preceding siblings ...)
2010-01-20 20:45 ` [PATCH 10/11] cxgb3: " Stephen Hemminger
@ 2010-01-20 20:45 ` Stephen Hemminger
2010-01-20 22:46 ` [PATCH 00/11] DMA sync errors David Miller
11 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 20:45 UTC (permalink / raw)
To: David Miller, Jarek Poplawski, Roland Dreier
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA
[-- 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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 20:45 ` [PATCH 02/11] sky2: " Stephen Hemminger
@ 2010-01-20 21:32 ` Jarek Poplawski
2010-01-20 22:52 ` David Miller
0 siblings, 1 reply; 31+ messages in thread
From: Jarek Poplawski @ 2010-01-20 21:32 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
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);
> }
>
> --
>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] DMA sync errors
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
` (10 preceding siblings ...)
2010-01-20 20:45 ` [PATCH 11/11] ipoib: " Stephen Hemminger
@ 2010-01-20 22:46 ` David Miller
2010-01-20 23:28 ` Stephen Hemminger
11 siblings, 1 reply; 31+ messages in thread
From: David Miller @ 2010-01-20 22:46 UTC (permalink / raw)
To: shemminger; +Cc: jarkao2, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 21:32 ` Jarek Poplawski
@ 2010-01-20 22:52 ` David Miller
2010-01-20 23:21 ` Jarek Poplawski
0 siblings, 1 reply; 31+ messages in thread
From: David Miller @ 2010-01-20 22:52 UTC (permalink / raw)
To: jarkao2; +Cc: shemminger, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 22:52 ` David Miller
@ 2010-01-20 23:21 ` Jarek Poplawski
2010-01-20 23:34 ` David Miller
0 siblings, 1 reply; 31+ messages in thread
From: Jarek Poplawski @ 2010-01-20 23:21 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] DMA sync errors
2010-01-20 22:46 ` [PATCH 00/11] DMA sync errors David Miller
@ 2010-01-20 23:28 ` Stephen Hemminger
2010-01-21 0:18 ` David Miller
0 siblings, 1 reply; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-20 23:28 UTC (permalink / raw)
To: David Miller; +Cc: jarkao2, netdev
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;
}
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 23:21 ` Jarek Poplawski
@ 2010-01-20 23:34 ` David Miller
2010-01-20 23:41 ` David Miller
2010-01-20 23:52 ` Jarek Poplawski
0 siblings, 2 replies; 31+ messages in thread
From: David Miller @ 2010-01-20 23:34 UTC (permalink / raw)
To: jarkao2; +Cc: shemminger, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 23:34 ` David Miller
@ 2010-01-20 23:41 ` David Miller
2010-01-20 23:59 ` Jarek Poplawski
2010-01-20 23:52 ` Jarek Poplawski
1 sibling, 1 reply; 31+ messages in thread
From: David Miller @ 2010-01-20 23:41 UTC (permalink / raw)
To: jarkao2; +Cc: shemminger, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 23:34 ` David Miller
2010-01-20 23:41 ` David Miller
@ 2010-01-20 23:52 ` Jarek Poplawski
2010-01-20 23:56 ` David Miller
1 sibling, 1 reply; 31+ messages in thread
From: Jarek Poplawski @ 2010-01-20 23:52 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 23:52 ` Jarek Poplawski
@ 2010-01-20 23:56 ` David Miller
0 siblings, 0 replies; 31+ messages in thread
From: David Miller @ 2010-01-20 23:56 UTC (permalink / raw)
To: jarkao2; +Cc: shemminger, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 23:41 ` David Miller
@ 2010-01-20 23:59 ` Jarek Poplawski
2010-01-21 0:04 ` Stephen Hemminger
0 siblings, 1 reply; 31+ messages in thread
From: Jarek Poplawski @ 2010-01-20 23:59 UTC (permalink / raw)
To: David Miller; +Cc: shemminger, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-20 23:59 ` Jarek Poplawski
@ 2010-01-21 0:04 ` Stephen Hemminger
2010-01-21 0:15 ` David Miller
0 siblings, 1 reply; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-21 0:04 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 02/11] sky2: fix DMA sync_single length error
2010-01-21 0:04 ` Stephen Hemminger
@ 2010-01-21 0:15 ` David Miller
0 siblings, 0 replies; 31+ messages in thread
From: David Miller @ 2010-01-21 0:15 UTC (permalink / raw)
To: shemminger; +Cc: jarkao2, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] DMA sync errors
2010-01-20 23:28 ` Stephen Hemminger
@ 2010-01-21 0:18 ` David Miller
2010-01-21 0:34 ` Stephen Hemminger
0 siblings, 1 reply; 31+ messages in thread
From: David Miller @ 2010-01-21 0:18 UTC (permalink / raw)
To: shemminger; +Cc: jarkao2, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] DMA sync errors
2010-01-21 0:18 ` David Miller
@ 2010-01-21 0:34 ` Stephen Hemminger
2010-01-21 0:36 ` David Miller
2010-01-21 12:44 ` Jarek Poplawski
0 siblings, 2 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-21 0:34 UTC (permalink / raw)
To: David Miller; +Cc: jarkao2, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] DMA sync errors
2010-01-21 0:34 ` Stephen Hemminger
@ 2010-01-21 0:36 ` David Miller
2010-01-21 12:44 ` Jarek Poplawski
1 sibling, 0 replies; 31+ messages in thread
From: David Miller @ 2010-01-21 0:36 UTC (permalink / raw)
To: shemminger; +Cc: jarkao2, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] DMA sync errors
2010-01-21 0:34 ` Stephen Hemminger
2010-01-21 0:36 ` David Miller
@ 2010-01-21 12:44 ` Jarek Poplawski
2010-01-21 13:12 ` David Miller
1 sibling, 1 reply; 31+ messages in thread
From: Jarek Poplawski @ 2010-01-21 12:44 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 00/11] DMA sync errors
2010-01-21 12:44 ` Jarek Poplawski
@ 2010-01-21 13:12 ` David Miller
0 siblings, 0 replies; 31+ messages in thread
From: David Miller @ 2010-01-21 13:12 UTC (permalink / raw)
To: jarkao2; +Cc: shemminger, netdev
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 :-)
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/11] de2104x: fix DMA sync_single length error
2010-01-20 20:45 ` [PATCH 06/11] de2104x: " Stephen Hemminger
@ 2010-01-22 1:02 ` Grant Grundler
2010-01-22 1:12 ` Stephen Hemminger
0 siblings, 1 reply; 31+ messages in thread
From: Grant Grundler @ 2010-01-22 1:02 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, Jarek Poplawski, Grant Grundler, netdev
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;
>
> --
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 06/11] de2104x: fix DMA sync_single length error
2010-01-22 1:02 ` Grant Grundler
@ 2010-01-22 1:12 ` Stephen Hemminger
0 siblings, 0 replies; 31+ messages in thread
From: Stephen Hemminger @ 2010-01-22 1:12 UTC (permalink / raw)
To: Grant Grundler; +Cc: David Miller, Jarek Poplawski, Grant Grundler, netdev
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.
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2010-01-22 1:12 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-20 20:44 [PATCH 00/11] DMA sync errors Stephen Hemminger
2010-01-20 20:45 ` [PATCH 01/11] tg3: fix DMA sync_single length error Stephen Hemminger
2010-01-20 20:45 ` [PATCH 02/11] sky2: " Stephen Hemminger
2010-01-20 21:32 ` Jarek Poplawski
2010-01-20 22:52 ` David Miller
2010-01-20 23:21 ` Jarek Poplawski
2010-01-20 23:34 ` David Miller
2010-01-20 23:41 ` David Miller
2010-01-20 23:59 ` Jarek Poplawski
2010-01-21 0:04 ` Stephen Hemminger
2010-01-21 0:15 ` David Miller
2010-01-20 23:52 ` Jarek Poplawski
2010-01-20 23:56 ` David Miller
2010-01-20 20:45 ` [PATCH 03/11] skge: " Stephen Hemminger
2010-01-20 20:45 ` [PATCH 04/11] r8169: " Stephen Hemminger
2010-01-20 20:45 ` [PATCH 05/11] rrunner: " Stephen Hemminger
2010-01-20 20:45 ` [PATCH 06/11] de2104x: " Stephen Hemminger
2010-01-22 1:02 ` Grant Grundler
2010-01-22 1:12 ` Stephen Hemminger
2010-01-20 20:45 ` [PATCH 07/11] sungem: " Stephen Hemminger
2010-01-20 20:45 ` [PATCH 08/11] sunhme: " Stephen Hemminger
2010-01-20 20:45 ` [PATCH 09/11] cassini: " Stephen Hemminger
2010-01-20 20:45 ` [PATCH 10/11] cxgb3: " Stephen Hemminger
2010-01-20 20:45 ` [PATCH 11/11] ipoib: " Stephen Hemminger
2010-01-20 22:46 ` [PATCH 00/11] DMA sync errors David Miller
2010-01-20 23:28 ` Stephen Hemminger
2010-01-21 0:18 ` David Miller
2010-01-21 0:34 ` Stephen Hemminger
2010-01-21 0:36 ` David Miller
2010-01-21 12:44 ` Jarek Poplawski
2010-01-21 13:12 ` 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).