Skip to contentSkip to conversation

e100: Add missing dma sync for proper operation with non-coherent caches.

PATCH In review David Daney 4 participants 6 messages Aug 26, 2008

Subsystems (3)
Networking Network drivers MIPS
lore
Share & tools
lore

David Daney

author
#0 lore raw

PATCH 1/1 e100: Add missing dma sync for proper operation with non-coherent caches.

I am running the e100 driver on a MIPS 4KEc system (32 bit mips with non-coherent DMA). There was a problem where received packets would get 'stuck' for several seconds at a time and then be released all at once.

The cause was that if an interrupt were received when no RX packets were available, the status for the receive buffer would be stuck in the cache, so when the next interrupt arrived the old status value was read (indicating no packets available) instead of the new value.

The fix is to call pci_dma_sync_single_for_device on the RX if the packet is not available to invalidate the cache so that at the next interrupt valid status is returned.

The driver currently calls pci_dma_sync_single_for_cpu before reading the status, and this is indeed needed for cases like the R10000 CPU where the cache can be polluted by speculative execution, but for most machines it is a nop.

The patch was tested on 2.6.17-rc4 on a MIPS 4KEc.

Signed-off-by David Daney <[email protected]>
---
 drivers/net/e100.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/net/e100.c +5 -0
index 19d32a2..fb8d551 100644
@@ -1840,6 +1840,11 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 
 			if (readb(&nic->csr->scb.status) & rus_no_res)
 				nic->ru_running = RU_SUSPENDED;
+		/* We are done looking at the buffer.  Prepare it for
+		 * more DMA.  */
+		pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr,
+					       sizeof(struct rfd),
+					       PCI_DMA_FROMDEVICE);
 		return -ENODATA;
 	}
Signature · 8 lines
1.5.5.1


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
#1 lore raw
Quoted 14 lines · David Daney

I am running the e100 driver on a MIPS 4KEc system (32 bit mips with non-coherent DMA). There was a problem where received packets would get 'stuck' for several seconds at a time and then be released all at once.

The cause was that if an interrupt were received when no RX packets were available, the status for the receive buffer would be stuck in the cache, so when the next interrupt arrived the old status value was read (indicating no packets available) instead of the new value.

The fix is to call pci_dma_sync_single_for_device on the RX if the packet is not available to invalidate the cache so that at the next interrupt valid status is returned.

Quoted 6 lines

The driver currently calls pci_dma_sync_single_for_cpu before reading the status, and this is indeed needed for cases like the R10000 CPU where the cache can be polluted by speculative execution, but for most machines it is a nop.

The patch was tested on 2.6.17-rc4 on a MIPS 4KEc.

lol, that's a bit old :)

A LOT of work has gone into 2.6.26+'s version of e100 that addresses specifically non-coherent DMA machines, including a patch that looks very close to what you write below here.

can you test the latest git tree and see if you still need (a modified or updated version) of your patch?

I know for sure that the guys maintaining e100 do not have anything close to your system, so they can't test that.

Cheers,

Auke

Quoted 22 lines
Signed-off-by David Daney <[email protected]>
---
 drivers/net/e100.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/net/e100.c +5 -0
index 19d32a2..fb8d551 100644
@@ -1840,6 +1840,11 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 
 			if (readb(&nic->csr->scb.status) & rus_no_res)
 				nic->ru_running = RU_SUSPENDED;
+		/* We are done looking at the buffer.  Prepare it for
+		 * more DMA.  */
+		pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr,
+					       sizeof(struct rfd),
+					       PCI_DMA_FROMDEVICE);
 		return -ENODATA;
 	}
author
#2 lore raw
Quoted 20 lines · Auke
Quoted 19 lines · David Daney

I am running the e100 driver on a MIPS 4KEc system (32 bit mips with non-coherent DMA). There was a problem where received packets would get 'stuck' for several seconds at a time and then be released all at once.

The cause was that if an interrupt were received when no RX packets were available, the status for the receive buffer would be stuck in the cache, so when the next interrupt arrived the old status value was read (indicating no packets available) instead of the new value.

The fix is to call pci_dma_sync_single_for_device on the RX if the packet is not available to invalidate the cache so that at the next interrupt valid status is returned.

The driver currently calls pci_dma_sync_single_for_cpu before reading the status, and this is indeed needed for cases like the R10000 CPU where the cache can be polluted by speculative execution, but for most machines it is a nop.

Quoted 5 lines
Quoted 1 line

The patch was tested on 2.6.17-rc4 on a MIPS 4KEc.

lol, that's a bit old :)

It was a typo. The real version is 2.6.27-rc4 (aka the HEAD).

The bug is present on the HEAD. Sorry for the confusion.

David Daney.

Quoted 36 lines

A LOT of work has gone into 2.6.26+'s version of e100 that addresses specifically non-coherent DMA machines, including a patch that looks very close to what you write below here.

can you test the latest git tree and see if you still need (a modified or updated version) of your patch?

I know for sure that the guys maintaining e100 do not have anything close to your system, so they can't test that.

Cheers,

Auke

Quoted 21 lines
Signed-off-by David Daney <[email protected]>
---
 drivers/net/e100.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/net/e100.c +5 -0
index 19d32a2..fb8d551 100644
@@ -1840,6 +1840,11 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 
 			if (readb(&nic->csr->scb.status) & rus_no_res)
 				nic->ru_running = RU_SUSPENDED;
+		/* We are done looking at the buffer.  Prepare it for
+		 * more DMA.  */
+		pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr,
+					       sizeof(struct rfd),
+					       PCI_DMA_FROMDEVICE);
 		return -ENODATA;
 	}
#3 lore raw
In reply to David Daney
Quoted 32 lines · David Daney

I am running the e100 driver on a MIPS 4KEc system (32 bit mips with non-coherent DMA). There was a problem where received packets would get 'stuck' for several seconds at a time and then be released all at once.

The cause was that if an interrupt were received when no RX packets were available, the status for the receive buffer would be stuck in the cache, so when the next interrupt arrived the old status value was read (indicating no packets available) instead of the new value.

The fix is to call pci_dma_sync_single_for_device on the RX if the packet is not available to invalidate the cache so that at the next interrupt valid status is returned.

The driver currently calls pci_dma_sync_single_for_cpu before reading the status, and this is indeed needed for cases like the R10000 CPU where the cache can be polluted by speculative execution, but for most machines it is a nop.

The patch was tested on 2.6.17-rc4 on a MIPS 4KEc.

Signed-off-by David Daney <[email protected]>
---
 drivers/net/e100.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
drivers/net/e100.c +0 -0
index 19d32a2..fb8d551 100644
@@ -1840,6 +1840,11 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,

drivers/net/e100.c static int e100_rx_indicate(struct nic *nic, struct rx *rx, · line 1850

Quoted 10 lines
drivers/net/e100.c +5 -0
 			if (readb(&nic->csr->scb.status) & rus_no_res)
 				nic->ru_running = RU_SUSPENDED;
+		/* We are done looking at the buffer.  Prepare it for
+		 * more DMA.  */
+		pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr,
+					       sizeof(struct rfd),
+					       PCI_DMA_FROMDEVICE);
 		return -ENODATA;
 	}

Should the call to pci_dma_sync_single_for_device be DMA_TO_DEVICE since we are giving the memory back to the device? -ack

author
#4 lore raw
Quoted 9 lines · David Acker
Quoted 7 lines · David Daney
drivers/net/e100.c +0 -0
index 19d32a2..fb8d551 100644
@@ -1840,6 +1840,11 @@ static int e100_rx_indicate(struct nic *nic, 
 struct rx *rx,
Quoted 12 lines

drivers/net/e100.c static int e100_rx_indicate(struct nic *nic, struct rx *rx, · line 1849

Quoted 10 lines
drivers/net/e100.c +5 -0
             if (readb(&nic->csr->scb.status) & rus_no_res)
                 nic->ru_running = RU_SUSPENDED;
+        /* We are done looking at the buffer.  Prepare it for
+         * more DMA.  */
+        pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr,
+                           sizeof(struct rfd),
+                           PCI_DMA_FROMDEVICE);
         return -ENODATA;
     }

Should the call to pci_dma_sync_single_for_device be DMA_TO_DEVICE since we are giving the memory back to the device?

No. We are giving the memory back to the device, but the direction of the data transfer is from the device to memory.

David Daney

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Reviewed-by on v1 1/1
#5 lore raw
In reply to David Daney
Quoted 14 lines · David Daney

I am running the e100 driver on a MIPS 4KEc system (32 bit mips with non-coherent DMA). There was a problem where received packets would get 'stuck' for several seconds at a time and then be released all at once.

The cause was that if an interrupt were received when no RX packets were available, the status for the receive buffer would be stuck in the cache, so when the next interrupt arrived the old status value was read (indicating no packets available) instead of the new value.

The fix is to call pci_dma_sync_single_for_device on the RX if the packet is not available to invalidate the cache so that at the next interrupt valid status is returned.

Quoted 8 lines

The driver currently calls pci_dma_sync_single_for_cpu before reading the status, and this is indeed needed for cases like the R10000 CPU where the cache can be polluted by speculative execution, but for most machines it is a nop.

The patch was tested on 2.6.17-rc4 on a MIPS 4KEc.

Signed-off-by David Daney <[email protected]>

Makes sense to me.

Reviewed-by Ralf Baechle <[email protected]>

Ralf

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/