Streaming DMA mapping question

Streaming DMA mapping question

[Adam Kropelin]

I'm working on revamping the DMA mapping of a driver and have been reading
Documentation/DMA-Mapping.txt and becoming one with it. On i386 I notice the
following discrepency:

According to the docs, you should either unmap or sync your DMA buffer before
touching it from the host. The i386 implementation of pci_unmap is empty --no
problem; there must not be any unmap work to do on this arch. But the 
implementation of pci_dma_sync does contain a flush_write_buffers() call. This
makes me think that perhaps if I'm going to modify the buffer before I submit it
back to the controller I need to do:

/* so I can read it; not necessary on i386 */
pci_dma_sync_single(...);

fiddle_with_buffer(...);

/* so adapter sees my changes; this is necessary on i386 */
pci_dma_sync_single(...);

Otherwise I don't see why it is safe to touch the buffer after pci_unmap.

Someone lend me a clue?

--Adam
 

Re: Streaming DMA mapping question

[David S. Miller]

   From: Adam Kropelin <akropel1@rochester.rr.com>
   Date: Fri, 13 Sep 2002 15:39:16 -0400
   
   According to the docs, you should either unmap or sync your DMA buffer before
   touching it from the host. The i386 implementation of pci_unmap is empty --no
   problem; there must not be any unmap work to do on this arch. But the 
   implementation of pci_dma_sync does contain a flush_write_buffers() call. This
   makes me think that perhaps if I'm going to modify the buffer before I submit it
   back to the controller I need to do:

Actually, rather it appears that the i386 pci_unmap_*() routines need
the write buffer flush as well.

The x86 implementation is a bad example to read if you're trying to
see what the worst case scenerio is.

Just follow the document and your driver will work properly on all
platforms.  DMA-mapping.txt was meant to be written in a way such
that you should not ever need to look at an implementation of the
interfaces to figure out how to use them.

Re: Streaming DMA mapping question

[Adam Kropelin]

On Fri, Sep 13, 2002 at 12:36:41PM -0700, David S. Miller wrote:
>    From: Adam Kropelin <akropel1@rochester.rr.com>
>    Date: Fri, 13 Sep 2002 15:39:16 -0400
>    
>    According to the docs, you should either unmap or sync your DMA buffer before
>    touching it from the host. The i386 implementation of pci_unmap is empty --no
>    problem; there must not be any unmap work to do on this arch. But the 
>    implementation of pci_dma_sync does contain a flush_write_buffers() call. This
>    makes me think that perhaps if I'm going to modify the buffer before I submit it
>    back to the controller I need to do:
> 
> Actually, rather it appears that the i386 pci_unmap_*() routines need
> the write buffer flush as well.

Ah, a bug then. 

> The x86 implementation is a bad example to read if you're trying to
> see what the worst case scenerio is.

I was looking at the x86 implementation to help me narrow down the possible
source of a bug I'm seeing in the driver. I noticed the driver was examining a
DMA buffer without unmapping or syncing. Before I went to the work to fix the
code I wanted to see if those operations were no-ops on the platform I'm testing
on. I'll definitely fix the driver regardless, but I didn't want to blame the
lockup on it if unmapping won't actually change anything on this platform. Thus
my confusion upon seeing pci_unmap_*() and pci_dma_sync_*() differ.

> Just follow the document and your driver will work properly on all
> platforms.  DMA-mapping.txt was meant to be written in a way such
> that you should not ever need to look at an implementation of the
> interfaces to figure out how to use them.

Absolutely. It does that job very well and I am greatly appreciative for the
code groveling time that document has saved me. Kudos to you and others who
spent time writing it.

--Adam

Re: Streaming DMA mapping question

[David S. Miller]

   From: Adam Kropelin <akropel1@rochester.rr.com>
   Date: Fri, 13 Sep 2002 16:21:50 -0400

   On Fri, Sep 13, 2002 at 12:36:41PM -0700, David S. Miller wrote:
   > Actually, rather it appears that the i386 pci_unmap_*() routines need
   > the write buffer flush as well.
   
   Ah, a bug then. 
   
On further discussion with Alan Cox, the bug is actually that
pci_map_*() needs the write buffer flush added.  pci_map_*()
and pci_dma_sync_*() transfer ownership from CPU to PCI controller
as abstracted in DMA-mapping.txt   Therefore these are the cases
where the CPU write buffers need to be flushed.

pci_unmap_*() is ok as-is.

   I was looking at the x86 implementation to help me narrow down the possible
   source of a bug I'm seeing in the driver. I noticed the driver was examining a
   DMA buffer without unmapping or syncing.

Really, the cases handled by the x86 write buffer fluses are very
marginal and unlikely to happen.  In fact the write buffer flush on
x86 is done on winchip and ppro chips only.

I think you're problems are elsewhere :-)
   
   Kudos to you and others who spent time writing it.

Thank you.

   

Re: Streaming DMA mapping question

[Adam Kropelin]

On Fri, Sep 13, 2002 at 01:28:42PM -0700, David S. Miller wrote:
>    From: Adam Kropelin <akropel1@rochester.rr.com>
>    Date: Fri, 13 Sep 2002 16:21:50 -0400
> 
>    On Fri, Sep 13, 2002 at 12:36:41PM -0700, David S. Miller wrote:
>    > Actually, rather it appears that the i386 pci_unmap_*() routines need
>    > the write buffer flush as well.
>    
>    Ah, a bug then. 
>    
> On further discussion with Alan Cox, the bug is actually that
> pci_map_*() needs the write buffer flush added.  pci_map_*()
> and pci_dma_sync_*() transfer ownership from CPU to PCI controller
> as abstracted in DMA-mapping.txt   Therefore these are the cases
> where the CPU write buffers need to be flushed.

Makes sense to me.

> Really, the cases handled by the x86 write buffer fluses are very
> marginal and unlikely to happen.  In fact the write buffer flush on
> x86 is done on winchip and ppro chips only.
> 
> I think you're problems are elsewhere :-)

Probably true, but the test machine I'm running on *is* SMP ppro ;)

--Adam

Re: Streaming DMA mapping question

[David S. Miller]

   From: Adam Kropelin <akropel1@rochester.rr.com>
   Date: Fri, 13 Sep 2002 16:52:41 -0400

   > I think you're problems are elsewhere :-)
   
   Probably true, but the test machine I'm running on *is* SMP ppro ;)

If you add the write buffer flush to the pci_map_*() routines,
does it make your problem go away?

Re: Streaming DMA mapping question

[Adam Kropelin]

On Fri, Sep 13, 2002 at 01:28:42PM -0700, David S. Miller wrote:
>    From: Adam Kropelin <akropel1@rochester.rr.com>
>    Date: Fri, 13 Sep 2002 16:21:50 -0400
> 
>    On Fri, Sep 13, 2002 at 12:36:41PM -0700, David S. Miller wrote:
>    > Actually, rather it appears that the i386 pci_unmap_*() routines need
>    > the write buffer flush as well.
>    
>    Ah, a bug then. 
>    
> On further discussion with Alan Cox, the bug is actually that
> pci_map_*() needs the write buffer flush added.  pci_map_*()
> and pci_dma_sync_*() transfer ownership from CPU to PCI controller
> as abstracted in DMA-mapping.txt   Therefore these are the cases
> where the CPU write buffers need to be flushed.

It seems that pci_dma_sync_*() transfers ownership in either direction. In the
example from DMA-mapping.txt, it is used to transfer ownership from the PCI
device to the host CPU. Additionally, the presence of the host write buffer
flush indicates that it can also be used to transfer ownership from the host CPU
to the PCI device.

Am I missing something?

If I'm right, would you accept a patch to clarify the issue in DMA-mapping.txt?

--Adam

Re: Streaming DMA mapping question

[David S. Miller]

   From: Adam Kropelin <akropel1@rochester.rr.com>
   Date: Fri, 13 Sep 2002 23:51:13 -0400
   
   It seems that pci_dma_sync_*() transfers ownership in either direction.

That's a bug in the documentation, and no platform which has to care
about this area actually does what you imply.

A new interface needs to be added to transfer control in the other
direction.  pci_dma_sync_*() only handles transferring control from
device to CPU..

I know this makes drivers like eepro100 buggy, this was discussed
a month or two ago wrt. MIPS on linux-kernel, check the archives
for the thread.

Re: Streaming DMA mapping question

[Martin Diehl]

On Sun, 15 Sep 2002, David S. Miller wrote:

>    From: Adam Kropelin <akropel1@rochester.rr.com>
>    Date: Fri, 13 Sep 2002 23:51:13 -0400
>    
>    It seems that pci_dma_sync_*() transfers ownership in either direction.
> 
> That's a bug in the documentation, and no platform which has to care
> about this area actually does what you imply.
> 
> A new interface needs to be added to transfer control in the other
> direction.  pci_dma_sync_*() only handles transferring control from
> device to CPU..
> 
> I know this makes drivers like eepro100 buggy, this was discussed
> a month or two ago wrt. MIPS on linux-kernel, check the archives
> for the thread.

Wasn't there a patch submitted which suggested to add pci_dma_prep_*()
calls in order to sync the cpu-driven changes back to the bus? I'm asking
because I'm dealing with a driver that needs to reuse streaming pci maps.

IIRC my impression was you might be willing to accept this for inclusion
but maybe I'm wrong or it got lost during the long "dmabuf inside struct
may cross cacheline" thread shortly thereafter.

Do you think it would be a good idea to add some nooped pci_dma_prep_*()
calls at the right place and expect them to represent some future 
solution?

Martin

Re: Streaming DMA mapping question

[David S. Miller]

   From: Martin Diehl <lists@mdiehl.de>
   Date: Mon, 16 Sep 2002 23:06:35 +0200 (CEST)
   
   Wasn't there a patch submitted which suggested to add pci_dma_prep_*()
   calls in order to sync the cpu-driven changes back to the bus? I'm asking
   because I'm dealing with a driver that needs to reuse streaming pci maps.
   
Yes, basically this is what must happen.

If someone would code up a patch that includes the MIPS
implementation, adds NOP implementations to every other asm/pci.h file
and updates the Documentation/DMA-mapping.txt file appropriately,
I'll probably just apply the patch and merge it upstream immediately.

I don't have time to work on this patch right now and I don't know how
the MIPS part should be implemented so...

Re: Streaming DMA mapping question

[Martin Diehl]

On Mon, 16 Sep 2002, David S. Miller wrote:

>    From: Martin Diehl <lists@mdiehl.de>
>    Date: Mon, 16 Sep 2002 23:06:35 +0200 (CEST)
>    
>    Wasn't there a patch submitted which suggested to add pci_dma_prep_*()
>    calls in order to sync the cpu-driven changes back to the bus? I'm asking
>    because I'm dealing with a driver that needs to reuse streaming pci maps.
>    
> Yes, basically this is what must happen.
> 
> If someone would code up a patch that includes the MIPS
> implementation, adds NOP implementations to every other asm/pci.h file
> and updates the Documentation/DMA-mapping.txt file appropriately,
> I'll probably just apply the patch and merge it upstream immediately.

Ok, thanks. The MIPS thing is far beyond my coverage anyway. Seems I just 
gonna stay with the my dummy __pci_dma_prep_single() simply to remind me
of the missing api call. The driver is i86-only, so it's a non-issue...

Martin