Re: Debounce code vs. dma_sync_single_range_for_cpu() and e100 driver.

Re: Debounce code vs. dma_sync_single_range_for_cpu() and e100 driver.

[Krzysztof Halasa]

A copy of my mail to linux-arm list.

Arch = ARM, CPU = IXP425, net device = 82551 using e100 driver.

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> Cache handling is performed when ownership of buffers transfer from the
> CPU to the device.  At this point in time, the CPU must not read or write
> any cache lines associated with the buffer.

Well, e100 driver uses streaming mapping for it's RX/TX buffers as well
as for the descriptors. Now it gets tricky - RX ring descriptors can be
written by the device at any time (when completing RX) and at the same
time the CPU has to be able to check the descriptor's status. It seems
it can't be formally done with the streaming DMA API, since it doesn't
provide invalidate and flush operations, it's rather about transfering
control.

I guess the coherent/consistent allocations should be used for this
purpose (= uncached RAM pages on IXP4xx if I understand it correctly).

Now that I know the inner working of DMA API the attached patch "fixes"
the e100 problems, but it still doesn't seem to be a valid use of the
API.

Comments?

--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -1762,6 +1762,9 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 
 			if (ioread8(&nic->csr->scb.status) & rus_no_res)
 				nic->ru_running = RU_SUSPENDED;
+		pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr,
+					       sizeof(struct rfd),
+					       PCI_DMA_BIDIRECTIONAL);
 		return -ENODATA;
 	}
 

Without the patch:

$ ping 10.150 -i 0.2
PING 10.150 (10.0.0.150) 56(84) bytes of data.
64 bytes from 10.0.0.150: icmp_seq=1 ttl=64 time=619 ms
64 bytes from 10.0.0.150: icmp_seq=2 ttl=64 time=410 ms
64 bytes from 10.0.0.150: icmp_seq=3 ttl=64 time=200 ms
64 bytes from 10.0.0.150: icmp_seq=4 ttl=64 time=0.498 ms
64 bytes from 10.0.0.150: icmp_seq=5 ttl=64 time=2880 ms

With the patch:

$ ping 10.150 -i 0.2
PING 10.150 (10.0.0.150) 56(84) bytes of data.
64 bytes from 10.0.0.150: icmp_seq=1 ttl=64 time=7.05 ms
64 bytes from 10.0.0.150: icmp_seq=2 ttl=64 time=0.269 ms
64 bytes from 10.0.0.150: icmp_seq=3 ttl=64 time=0.887 ms
64 bytes from 10.0.0.150: icmp_seq=4 ttl=64 time=0.311 ms
64 bytes from 10.0.0.150: icmp_seq=5 ttl=64 time=0.878 ms

IXP425 533 MHz, i82551, 2.6.30.

OTOH, nobody uses e100 on non-coherent ARM? Perhaps a slower CPU (with
maybe higher IRQ latency) would hit the problem less frequently?

Thanks for your help.
-- 
Krzysztof Halasa

Re: Debounce code vs. dma_sync_single_range_for_cpu() and e100 driver.

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Tue, 14 Jul 2009 21:34:01 +0200

> Well, e100 driver uses streaming mapping for it's RX/TX buffers as well
> as for the descriptors. Now it gets tricky - RX ring descriptors can be
> written by the device at any time (when completing RX) and at the same
> time the CPU has to be able to check the descriptor's status. It seems
> it can't be formally done with the streaming DMA API, since it doesn't
> provide invalidate and flush operations, it's rather about transfering
> control.
> 
> I guess the coherent/consistent allocations should be used for this
> purpose (= uncached RAM pages on IXP4xx if I understand it correctly).
> 
> Now that I know the inner working of DMA API the attached patch "fixes"
> the e100 problems, but it still doesn't seem to be a valid use of the
> API.

E100's use of streaming mappings for RX descriptors is a bug, it
should be using consistent mappings for sure.

And it's especially buggy if it isn't doing DMA API sync calls before
looking at descriptor fields, as your patch seems to cure.

Re: Debounce code vs. dma_sync_single_range_for_cpu() and e100 driver.

[Krzysztof Halasa]

(Added Cc:)

David Miller <davem@davemloft.net> writes:

> And it's especially buggy if it isn't doing DMA API sync calls before
> looking at descriptor fields, as your patch seems to cure.

Well, it does that but doesn't "sync for device" _after_ looking at the
RX descriptor (when the device is to own the desc again). On IXP4xx the
ownership transfer (cache flush/invalidate) happens on "sync for device"
only (which IMHO seems a bit fragile, though).

So what do we do?

Maybe you apply the workaround for 2.6.31 and I (or someone) will
convert e100 to coherent allocs for packet descriptors, post-31?

This isn't a terrible problem (x86 isn't affected), perhaps we should
leave it as is for 2.6.31 making sure it doesn't get out of sight with
the workaround in place?

Guess it's too risky for me to mess with the coherent allocs conversion
for 31, I don't know e100 code at all (and I have to leave for few weeks
on Thursday).



E100: work around the driver using streaming DMA mapping for RX descriptors.

E100 places it's RX packet descriptors inside skb->data and uses them
with bidirectional streaming DMA mapping. Unfortunately it fails to
transfer skb->data ownership to the device after it reads the
descriptor's status, breaking on non-coherent (e.g., ARM) platforms.

This have to be converted to use coherent memory for the descriptors.

Signed-off-by: Krzysztof Halasa <khc@pm.waw.pl>

--- a/drivers/net/e100.c
+++ b/drivers/net/e100.c
@@ -1762,6 +1762,9 @@ static int e100_rx_indicate(struct nic *nic, struct rx *rx,
 
 			if (ioread8(&nic->csr->scb.status) & rus_no_res)
 				nic->ru_running = RU_SUSPENDED;
+		pci_dma_sync_single_for_device(nic->pdev, rx->dma_addr,
+					       sizeof(struct rfd),
+					       PCI_DMA_BIDIRECTIONAL);
 		return -ENODATA;
 	}
 

Re: Debounce code vs. dma_sync_single_range_for_cpu() and e100 driver.

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Tue, 14 Jul 2009 23:01:54 +0200

> Maybe you apply the workaround for 2.6.31 and I (or someone) will
> convert e100 to coherent allocs for packet descriptors, post-31?

I think that's a good plan.

Will give a few days for Intel folks to chime in before applying
your patch for 2.6.31, but otherwise that's my plan.

------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time, 
vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize  
details at: http://p.sf.net/sfu/Challenge

Re: Debounce code vs. dma_sync_single_range_for_cpu() and e100 driver.

[Jeff Kirsher]

On Tue, Jul 14, 2009 at 2:05 PM, David Miller<davem@davemloft.net> wrote:
> From: Krzysztof Halasa <khc@pm.waw.pl>
> Date: Tue, 14 Jul 2009 23:01:54 +0200
>
>> Maybe you apply the workaround for 2.6.31 and I (or someone) will
>> convert e100 to coherent allocs for packet descriptors, post-31?
>
> I think that's a good plan.
>
> Will give a few days for Intel folks to chime in before applying
> your patch for 2.6.31, but otherwise that's my plan.

Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

-- 
Cheers,
Jeff

Re: Debounce code vs. dma_sync_single_range_for_cpu() and e100 driver.

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 16 Jul 2009 15:27:01 -0700

> On Tue, Jul 14, 2009 at 2:05 PM, David Miller<davem@davemloft.net> wrote:
>> From: Krzysztof Halasa <khc@pm.waw.pl>
>> Date: Tue, 14 Jul 2009 23:01:54 +0200
>>
>>> Maybe you apply the workaround for 2.6.31 and I (or someone) will
>>> convert e100 to coherent allocs for packet descriptors, post-31?
>>
>> I think that's a good plan.
>>
>> Will give a few days for Intel folks to chime in before applying
>> your patch for 2.6.31, but otherwise that's my plan.
> 
> Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied, and queued for -stable, thanks everyone.