Subject: pci_alloc_consistent usage

From: Wayne Gowcher <[email-masked]>
Date: Fri, 23 May 2003 14:59:35 -0700 (PDT)

I am working on a driver that uses PCI DMA transfers
from system memory to the PCI device. On the 2.4.18
kernel it worked OK, but now when it is recompiled for
2.4.20 it doesn't. Digging into things I have found
that the function virt_to_phys() has been changed from
:
return PHYSADDR(address)

to
return (unsigned long)address - PAGE_OFFSET

Where PAGE_OFFSET is 0x8000 0000, and where PHYSADDR
would AND the address against 0x1FFF FFFF. As far as I
can tell the problem comes from pci_alloc_consistent
doing :

ret = UNCAC_ADDR(ret) 

which converts a 0x8xxx address to 0xAxxx, and then
when you pass this 0xAxxx_xxxx address through
virt_to_phys() you get an address of the form
0x2xxx_xxxx. This 0x2xxx_xxxx is passed to the dma
controller as the physical address to where it must
read / write data, and because it is 0x2xxx_xxxx and
not 0x0xxx_xxxx an exception occurs.

At first I just tried AND'ing out the 0xA.. like
PHYSADDR used to do it, but with that change i no
longer get the exception, but the driver does not dma
the data across - it just sits there.

I read DMA-mapping.txt and it says virt_to_phys() will
be phased out, and should be used, but doesn't
elaborate any further (like how you should do it now
).

So after that long intro, my question is :

Anybody know where I'm going wrong and how to fix
things ?

Also any tips on what drivers to look at for good
examples would also be appreciated.

TIA

__________________________________
Do you Yahoo!?
The New Yahoo! Search - Faster. Easier. Bingo.
http://search.yahoo.com

---

From: Ralf Baechle <[email-masked]>
Date: Tue, 27 May 2003 11:17:40 +0200

> not 0x0xxx_xxxx an exception occurs.

The change was partly done to catch broken code, partly because a
subtraction is potencially faster on some processors.

> elaborate any further (like how you should do it now).

Use the value returned by pci_alloc_consistent in *dma_handle instead
of trying to do any conversions with of pci_alloc_consistent's return
value.

  Ralf

---

From: Atsushi Nemoto <[email-masked]>
Date: Tue, 27 May 2003 19:07:49 +0900 (JST)

>>>>> On Tue, 27 May 2003 11:17:40 +0200, Ralf Baechle <[email-masked]> said:
ralf> Use the value returned by pci_alloc_consistent in *dma_handle
ralf> instead of trying to do any conversions with of
ralf> pci_alloc_consistent's return value.

How about virt_to_page()?

Currently, many sound drivers (including ALSA) pass a
pci_alloc_consistent's return value to virt_to_page.  While
virt_to_page is defined as below, we can not pass KSEG1 address
(returned by pci_alloc_consistent) to virt_to_page.

#define __pa(x)		((unsigned long) (x) - PAGE_OFFSET)
#define virt_to_page(kaddr)	(mem_map + (__pa(kaddr) >> PAGE_SHIFT))

Following definition can handle this case properly,

#define virt_to_page(kaddr)	(mem_map + (PHYSADDR(kaddr) >> PAGE_SHIFT))

But as ralf said, PHYSADDR() may be is a bit slower then __pa().

Is it worth to change?  Or should we fix the driver using virt_to_page
with pci_alloc_consistent?

---
Atsushi Nemoto

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 27 May 2003 11:53:22 +0100

On Tue, May 27, 2003 at 07:07:49PM +0900, Atsushi Nemoto wrote:
> pci_alloc_consistent's return value to virt_to_page.

You are not allow to do so.  Any driver doing this is broken.

---

From: Ralf Baechle <[email-masked]>
Date: Tue, 27 May 2003 13:22:37 +0200

> You are not allow to do so.  Any driver doing this is broken.

That however is exactly what's documented in DMA-mapping.txt:

[...]
portably refer to any piece of memory.  If you have a cpu pointer
(which may be validly DMA'd too) you may easily obtain the page
and offset using something like this:
                                                                                
        struct page *page = virt_to_page(ptr);
        unsigned long offset = ((unsigned long)ptr & ~PAGE_MASK);
[...]

While it's officially documented I still don't like it.  A double
conversion such as page_address(virt_to_page(ptr)) would accidently
turn a pointer of an uncached mapping into one to a cached area for the
same object - that will almost certainly not work as expected on a
non-coherent machine.

  Ralf

---

From: Christoph Hellwig <[email-masked]>
Date: Tue, 27 May 2003 12:33:29 +0100

On Tue, May 27, 2003 at 01:22:37PM +0200, Ralf Baechle wrote:
> While it's officially documented I still don't like it.

Hmm, I remembered that some ports used vmalloc-like allocators for
this and virt_to_page doesn't work for those..

---

From: Ralf Baechle <[email-masked]>
Date: Tue, 27 May 2003 13:45:51 +0200

> this and virt_to_page doesn't work for those..

There's at least one MIPS system which we don't support anymore but which
would have to be supported by something like this.

Whatever - virt_to_page should then be considered a a legacy API which we
have to try to support as well as possible in the hope it's going to fade
away ...

  Ralf

---

From: Atsushi Nemoto <[email-masked]>
Date: Wed, 28 May 2003 10:30:46 +0900 (JST)

>>>>> On Tue, 27 May 2003 13:22:37 +0200, Ralf Baechle <[email-masked]> said:
ralf> While it's officially documented I still don't like it.  A
ralf> double conversion such as page_address(virt_to_page(ptr)) would
ralf> accidently turn a pointer of an uncached mapping into one to a
ralf> cached area for the same object - that will almost certainly not
ralf> work as expected on a non-coherent machine.

Yes, it's dangerous, I agree.

ralf> Whatever - virt_to_page should then be considered a a legacy API
ralf> which we have to try to support as well as possible in the hope
ralf> it's going to fade away ...

I hope so.  For now we can do something like:

     buf = pci_alloc_consistent(dev, size, &dmabuf);
#if defined(__mips__) && defined(CONFIG_NONCOHERENT_IO)
     page = virt_to_page(CAC_ADDR(buf));
#else
     page = virt_to_page(buf);
#endif

Ugly but I do not want to add extra overhead to virt_to_page itself...

---
Atsushi Nemoto