Skip to contentSkip to conversation

p54pci: update receive dma buffers before and after processing

PATCH In review Christian Lamparter 3 participants 4 messages Feb 11, 2011 Networking drivers (wireless)

lore
Share & tools
lore

Christian Lamparter

author
#0 lore raw

PATCH 1/1 p54pci: update receive dma buffers before and after processing

Documentation/DMA-API-HOWTO.txt states:

"DMA transfers need to be synced properly in order for the cpu and device to see the most uptodate and correct copy of the DMA buffer."

Cc <[email protected]>
Signed-off-by Christian Lamparter <[email protected]>

---
Embarrassingly, this bug is unique to p54 and has been around since 2.6.25. This tells us something interesting about our userbase.
---

drivers/net/wireless/p54/p54pci.c +9 -5
index 1eacba4..0494d7b 100644
@@ -199,6 +199,7 @@ static void p54p_check_rx_ring(struct ieee80211_hw *dev, u32 *index,
 	while (i != idx) {
 		u16 len;
 		struct sk_buff *skb;
+		dma_addr_t dma_addr;
 		desc = &ring[i];
 		len = le16_to_cpu(desc->len);
 		skb = rx_buf[i];

@@ -216,17 +217,20 @@ static void p54p_check_rx_ring(struct ieee80211_hw *dev, u32 *index,
 
 			len = priv->common.rx_mtu;
 		}
+		dma_addr = le32_to_cpu(desc->host_addr);
+		pci_dma_sync_single_for_cpu(priv->pdev, dma_addr,
+			priv->common.rx_mtu + 32, PCI_DMA_FROMDEVICE);
 		skb_put(skb, len);
 
 		if (p54_rx(dev, skb)) {
-			pci_unmap_single(priv->pdev,
-					 le32_to_cpu(desc->host_addr),
-					 priv->common.rx_mtu + 32,
-					 PCI_DMA_FROMDEVICE);
+			pci_unmap_single(priv->pdev, dma_addr,
+				priv->common.rx_mtu + 32, PCI_DMA_FROMDEVICE);
 			rx_buf[i] = NULL;
-			desc->host_addr = 0;
+			desc->host_addr = cpu_to_le32(0);
 		} else {
 			skb_trim(skb, 0);
+			pci_dma_sync_single_for_device(priv->pdev, dma_addr,
+				priv->common.rx_mtu + 32, PCI_DMA_FROMDEVICE);
 			desc->len = cpu_to_le16(priv->common.rx_mtu + 32);
 		}
#1 lore raw
Quoted 6 lines · Christian Lamparter

Documentation/DMA-API-HOWTO.txt states:

"DMA transfers need to be synced properly in order for the cpu and device to see the most uptodate and correct copy of the DMA buffer."

Quoted 6 lines
Cc <[email protected]>
Signed-off-by Christian Lamparter <[email protected]>

---
Embarrassingly, this bug is unique to p54 and has been around since 2.6.25. This tells us something interesting about our userbase.

Most likely it tells us that most people using p54 are using little endian boxes with coherent DMA... :-)

John

Signature · 2 lines
John W. Linville                Someday the world will need a hero, and you
[email protected]                  might be all we have.  Be ready.
#2 lore raw
In reply to Christian Lamparter
Quoted 6 lines · Christian Lamparter

Documentation/DMA-API-HOWTO.txt states:

"DMA transfers need to be synced properly in order for the cpu and device to see the most uptodate and correct copy of the DMA buffer."

Quoted 6 lines
Cc <[email protected]>
Signed-off-by Christian Lamparter <[email protected]>

---
Embarrassingly, this bug is unique to p54 and has been around since 2.6.25. This tells us something interesting about our userbase.

I do use it on a LE system that probably has coherent DMA. Did someone run into this bug?

Quoted 22 lines

---

drivers/net/wireless/p54/p54pci.c +4 -0
index 1eacba4..0494d7b 100644
@@ -199,6 +199,7 @@ static void p54p_check_rx_ring(struct ieee80211_hw *dev, u32 *index,
 	while (i != idx) {
 		u16 len;
 		struct sk_buff *skb;
+		dma_addr_t dma_addr;
 		desc = &ring[i];
 		len = le16_to_cpu(desc->len);
 		skb = rx_buf[i];

@@ -216,17 +217,20 @@ static void p54p_check_rx_ring(struct ieee80211_hw *dev, u32 *index,
 
 			len = priv->common.rx_mtu;
 		}
+		dma_addr = le32_to_cpu(desc->host_addr);
+		pci_dma_sync_single_for_cpu(priv->pdev, dma_addr,
+			priv->common.rx_mtu + 32, PCI_DMA_FROMDEVICE);
 		skb_put(skb, len);

drivers/net/wireless/p54/p54pci.c static void p54p_check_rx_ring(struct ieee80211_hw *dev, u32 *index, · line 229

Quoted 10 lines
drivers/net/wireless/p54/p54pci.c +3 -5
 		if (p54_rx(dev, skb)) {
-			pci_unmap_single(priv->pdev,
-					 le32_to_cpu(desc->host_addr),
-					 priv->common.rx_mtu + 32,
-					 PCI_DMA_FROMDEVICE);
+			pci_unmap_single(priv->pdev, dma_addr,
+				priv->common.rx_mtu + 32, PCI_DMA_FROMDEVICE);
 			rx_buf[i] = NULL;
-			desc->host_addr = 0;
+			desc->host_addr = cpu_to_le32(0);

How does a BE zero differ from a LE zero? Using it this way emphasizes that it needs to be translated - I'm not objecting, just curious.

Larry

author
#3 lore raw
Quoted 12 lines · On Friday 11 February 2011 02:37:00 Larry Finger wrote:

drivers/net/wireless/p54/p54pci.c static void p54p_check_rx_ring(struct ieee80211_hw *dev, u32 *index, · line 218

Quoted 9 lines · Christian Lamparter

---

drivers/net/wireless/p54/p54pci.c +1 -1
index 1eacba4..0494d7b 100644
@@ -216,17 +217,20 @@ static void p54p_check_rx_ring(struct ieee80211_hw *dev, u32 *index,
 			rx_buf[i] = NULL;
-			desc->host_addr = 0;
+			desc->host_addr = cpu_to_le32(0);

How does a BE zero differ from a LE zero?

As far as I can know "0" is something like an endian palindrome.

Otherwise [non-NULL] we would have fixed this long ago, because
then a warning is generated by "sparse":
drivers/net/wireless/p54/p54pci.c:229:20: warning: incorrect type in assignment (different base types)
drivers/net/wireless/p54/p54pci.c:229:20: expected restricted __le32 [usertype] host_addr
drivers/net/wireless/p54/p54pci.c:229:20: got int

Quoted 2 lines

Using it this way emphasizes that it needs to be translated - I'm not objecting, just curious.

Furthermore the compiler optimizes *constant* cpu_to_le [and vice versa] conversions, so there's no hidden cost from using them.

Regards,
Chr