[PATCH] pci_alloc_consistent in an interrupt context

Hi!

This is the patch which allows pci_alloc_consistent (and consistent_alloc)
to be called from an interrupt context.
It's the required behavior according to Documentation/DMA-mapping.txt.

Thanks,

  Eugene Surovegin <mailto:ebs@innocent.com>

On Thu, Jun 13, 2002 at 12:28:29PM -0700, Eugene Surovegin wrote:

This is the patch which allows pci_alloc_consistent (and consistent_alloc)
to be called from an interrupt context.
It's the required behavior according to Documentation/DMA-mapping.txt.

It looks okay.  But I don't see (immediatly) why the change to
pci_alloc_consistent was needed as well.  Can you (or Dan Malek, if
you're reading this..) explain?

But aside from that, it looks good and the next step would be to try and
get the generic changes into 2.5 (and the 2.4.20-pre1, 2.4.19 is more or
less frozen, esp for a change like this I suspect).

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Tom Rini wrote:

It looks okay.

It's not quite right.......

The consistent_alloc() should not always call get_vm_area() with GFP_ATOMIC
set.  It should use the 'gfp' that is passed into consistent_alloc().
Normally, you will call consistent_alloc() with GFP_KERNEL, and
interrupt functions must know to call the consistent allocators with GFP_ATOMIC.

..... But I don't see (immediatly) why the change to
pci_alloc_consistent was needed as well.

It was a mistake on my part......when CONFIG_NOT_COHERENT_CACHE is used,
the consisten_alloc() returns the dma_handle, and we have to ensure we
don't do the virt_to_bus later to get it (because it will be wrong once
iopa() is discarded :-)

But aside from that, it looks good and the next step would be to try and
get the generic changes into 2.5 (and the 2.4.20-pre1, 2.4.19 is more or
less frozen, esp for a change like this I suspect).

Good Luck ;-)


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, Jun 13, 2002 at 05:47:32PM -0400, Dan Malek wrote:

Tom Rini wrote:

quoted

..... But I don't see (immediatly) why the change to
pci_alloc_consistent was needed as well.

It was a mistake on my part......when CONFIG_NOT_COHERENT_CACHE is used,
the consisten_alloc() returns the dma_handle, and we have to ensure we
don't do the virt_to_bus later to get it (because it will be wrong once
iopa() is discarded :-)

Ah..  So this part is a correct and necessary fix, separate from the
rest of the patch?

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Dan,

At 02:47 PM 6/13/2002, you wrote:

It's not quite right.......

The consistent_alloc() should not always call get_vm_area() with GFP_ATOMIC
set.  It should use the 'gfp' that is passed into consistent_alloc().
Normally, you will call consistent_alloc() with GFP_KERNEL, and
interrupt functions must know to call the consistent allocators with
GFP_ATOMIC.

I don't think that we should use gfp passed to consisten_alloc as a
parameter for get_vm_area().

This is gfp for _memory_ itself and may contain GFP_DMA for example. I
didn't want to allocate
struct vm_area from DMA pool in that case.

May be the better solution is to mask out GFP_KERNEL or GFP_ATOMIC from gfp
and use it.

What do you think?

Eugene


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Hi!

Here is an addition to my first patch.

Instead of using gfp which was passed to consistent_alloc,
I use a modified version with masked out zone modifiers (like __GFP_DMA).

Thanks,

Eugene

Eugene Surovegin writes:

I don't think that we should use gfp passed to consisten_alloc as a
parameter for get_vm_area().

This is gfp for _memory_ itself and may contain GFP_DMA for example. I
didn't want to allocate
struct vm_area from DMA pool in that case.

May be the better solution is to mask out GFP_KERNEL or GFP_ATOMIC from gfp
and use it.

What do you think?

Maybe the best answer is to change get_vm_area to use

	(in_interrupt()? GFP_ATOMIC: GFP_KERNEL)

instead of GFP_KERNEL in the kmalloc call, and eliminate the need to
add the extra parameter.  I'll run that past davem and see what he
thinks.

Paul.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

If we are going to be calling get_vm_area from interrupt context we
need to change the write_lock/write_unlock calls to write_lock_irqsave
and write_unlock_irqrestore.

Paul.

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, Jun 13, 2002 at 02:56:35PM -0700, Tom Rini wrote:

On Thu, Jun 13, 2002 at 05:47:32PM -0400, Dan Malek wrote:

quoted

Tom Rini wrote:

quoted

..... But I don't see (immediatly) why the change to
pci_alloc_consistent was needed as well.

It was a mistake on my part......when CONFIG_NOT_COHERENT_CACHE is used,
the consisten_alloc() returns the dma_handle, and we have to ensure we
don't do the virt_to_bus later to get it (because it will be wrong once
iopa() is discarded :-)

Ah..  So this part is a correct and necessary fix, separate from the
rest of the patch?

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

diff -urN /home/dgibson/kernel/linuxppc_2_4_devel/arch/ppc/kernel/pci-dma.c linux-grinch/arch/ppc/kernel/pci-dma.c

--- /home/dgibson/kernel/linuxppc_2_4_devel/arch/ppc/kernel/pci-dma.c	Sat Jan 19 19:07:14 2002
+++ linux-grinch/arch/ppc/kernel/pci-dma.c	Fri Jun 14 10:23:01 2002

@@ -25,25 +25,16 @@

 	if (hwdev == NULL || hwdev->dma_mask != 0xffffffff)
 		gfp |= GFP_DMA;
-#ifdef CONFIG_NOT_COHERENT_CACHE
 	ret = consistent_alloc(gfp, size, dma_handle);
-#else
-	ret = (void *)__get_free_pages(gfp, get_order(size));
-#endif

-	if (ret != NULL) {
+	if (ret != NULL)
 		memset(ret, 0, size);
-		*dma_handle = virt_to_bus(ret);
-	}
+
 	return ret;
 }

 void pci_free_consistent(struct pci_dev *hwdev, size_t size,
 			 void *vaddr, dma_addr_t dma_handle)
 {
-#ifdef CONFIG_NOT_COHERENT_CACHE
 	consistent_free(vaddr);
-#else
-	free_pages((unsigned long)vaddr, get_order(size));
-#endif
 }

diff -urN /home/dgibson/kernel/linuxppc_2_4_devel/include/asm-ppc/io.h linux-grinch/include/asm-ppc/io.h

--- /home/dgibson/kernel/linuxppc_2_4_devel/include/asm-ppc/io.h	Sat May 11 02:02:08 2002
+++ linux-grinch/include/asm-ppc/io.h	Fri Jun 14 10:21:11 2002

@@ -458,8 +458,17 @@
 #define dma_cache_wback(_start,_size)		do { } while (0)
 #define dma_cache_wback_inv(_start,_size)	do { } while (0)

-#define consistent_alloc(gfp, size, handle)	NULL
-#define consistent_free(addr, size)		do { } while (0)
+static inline void *consistent_alloc(int gfp, size_t size, dma_addr_t *dma_handle)
+{
+	void *vaddr;
+
+	vaddr = kmalloc(size, gfp);
+	if (vaddr)
+		*dma_handle = virt_to_bus((unsigned long) vaddr);
+	return vaddr;
+}
+
+#define consistent_free(addr, size)		kfree(addr)
 #define consistent_sync(addr, size, rw)		do { } while (0)
 #define consistent_sync_page(pg, off, sz, rw)	do { } while (0)




--

David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Fri, Jun 14, 2002 at 10:24:19AM +1000, David Gibson wrote:

On Thu, Jun 13, 2002 at 02:56:35PM -0700, Tom Rini wrote:

quoted

On Thu, Jun 13, 2002 at 05:47:32PM -0400, Dan Malek wrote:

quoted

Tom Rini wrote:

quoted

..... But I don't see (immediatly) why the change to
pci_alloc_consistent was needed as well.

It was a mistake on my part......when CONFIG_NOT_COHERENT_CACHE is used,
the consisten_alloc() returns the dma_handle, and we have to ensure we
don't do the virt_to_bus later to get it (because it will be wrong once
iopa() is discarded :-)

Ah..  So this part is a correct and necessary fix, separate from the
rest of the patch?

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

Er, the problem of setting dma_handle twice?

My only concern is that are things still consistent on non consistent
procs?

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, Jun 13, 2002 at 05:38:07PM -0700, Tom Rini wrote:

On Fri, Jun 14, 2002 at 10:24:19AM +1000, David Gibson wrote:

quoted

On Thu, Jun 13, 2002 at 02:56:35PM -0700, Tom Rini wrote:

quoted

On Thu, Jun 13, 2002 at 05:47:32PM -0400, Dan Malek wrote:

quoted

Tom Rini wrote:

quoted

..... But I don't see (immediatly) why the change to
pci_alloc_consistent was needed as well.

It was a mistake on my part......when CONFIG_NOT_COHERENT_CACHE is used,
the consisten_alloc() returns the dma_handle, and we have to ensure we
don't do the virt_to_bus later to get it (because it will be wrong once
iopa() is discarded :-)

Ah..  So this part is a correct and necessary fix, separate from the
rest of the patch?

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

Er, the problem of setting dma_handle twice?

Well, it fixes that and as a bonus gets rid of the ifdefs in
pci_{alloc,free}_consistent() and means that if we ever port a driver
using consistent_{alloc,free}() to a processor that *is* cache
coherent it will Just Work.

My only concern is that are things still consistent on non consistent
procs?

Absolutely - no change to the code path at all on non cache coherent
processors.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Fri, Jun 14, 2002 at 10:45:39AM +1000, David Gibson wrote:

On Thu, Jun 13, 2002 at 05:38:07PM -0700, Tom Rini wrote:

[snip]

quoted

Er, the problem of setting dma_handle twice?

Well, it fixes that and as a bonus gets rid of the ifdefs in
pci_{alloc,free}_consistent() and means that if we ever port a driver
using consistent_{alloc,free}() to a processor that *is* cache
coherent it will Just Work.

In theory at that point OCP would either have its API fully flushed out
or be applied nicely on top of the generic driver work in 2.5 and it
would just work anyhow :)

quoted

My only concern is that are things still consistent on non consistent
procs?

Absolutely - no change to the code path at all on non cache coherent
processors.

So kmalloc/kfree are equivilent to __get_free_pages/free_pages ?

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

At 05:24 PM 6/13/2002, David Gibson wrote:

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

So, kmalloc will allocate non-cacheable memory?

I seriously doubt it (actually I've just tested it:).

Eugene.


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:

At 05:24 PM 6/13/2002, David Gibson wrote:

quoted

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

So, kmalloc will allocate non-cacheable memory?

I seriously doubt it (actually I've just tested it:).

No, but on cache coherent processors we don't need non-cacheable
memory.  That's the whole point.  Likewise with __get_free_pages()
which is what pci_alloc_consistent() uses on cache-coherent processors
now.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Tom Rini wrote:

Ah..  So this part is a correct and necessary fix, separate from the
rest of the patch?

Yes, thank you.


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

At 06:33 PM 6/13/2002, David Gibson wrote:

On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:

quoted

At 05:24 PM 6/13/2002, David Gibson wrote:

quoted

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

So, kmalloc will allocate non-cacheable memory?

I seriously doubt it (actually I've just tested it:).

No, but on cache coherent processors we don't need non-cacheable
memory.  That's the whole point.  Likewise with __get_free_pages()
which is what pci_alloc_consistent() uses on cache-coherent processors
now.

OK, I got it :)

One problem though, kmalloc allocated memory will not be necessarily
properly aligned
as required by DMA-mappings.txt

Some PCI drivers may break because of this.

Eugene.


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, Jun 13, 2002 at 06:57:45PM -0700, Eugene Surovegin wrote:

At 06:33 PM 6/13/2002, David Gibson wrote:

quoted

On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:

quoted

At 05:24 PM 6/13/2002, David Gibson wrote:

quoted

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

So, kmalloc will allocate non-cacheable memory?

I seriously doubt it (actually I've just tested it:).

No, but on cache coherent processors we don't need non-cacheable
memory.  That's the whole point.  Likewise with __get_free_pages()
which is what pci_alloc_consistent() uses on cache-coherent processors
now.

OK, I got it :)

One problem though, kmalloc allocated memory will not be necessarily
properly aligned
as required by DMA-mappings.txt

Some PCI drivers may break because of this.

Ah, yes, indeed.  How about this:

diff -urN /home/dgibson/kernel/linuxppc_2_4_devel/arch/ppc/kernel/pci-dma.c linux-grinch/arch/ppc/kernel/pci-dma.c

--- /home/dgibson/kernel/linuxppc_2_4_devel/arch/ppc/kernel/pci-dma.c	Fri Jun 14 07:57:36 2002
+++ linux-grinch/arch/ppc/kernel/pci-dma.c	Fri Jun 14 10:23:01 2002

@@ -25,28 +25,16 @@

 	if (hwdev == NULL || hwdev->dma_mask != 0xffffffff)
 		gfp |= GFP_DMA;
-
-#ifdef CONFIG_NOT_COHERENT_CACHE
 	ret = consistent_alloc(gfp, size, dma_handle);
-#else
-	ret = (void *)__get_free_pages(gfp, get_order(size));
-#endif

-	if (ret != NULL) {
+	if (ret != NULL)
 		memset(ret, 0, size);
-#ifndef CONFIG_NOT_COHERENT_CACHE
-		*dma_handle = virt_to_bus(ret);
-#endif
-	}
+
 	return ret;
 }

 void pci_free_consistent(struct pci_dev *hwdev, size_t size,
 			 void *vaddr, dma_addr_t dma_handle)
 {
-#ifdef CONFIG_NOT_COHERENT_CACHE
 	consistent_free(vaddr);
-#else
-	free_pages((unsigned long)vaddr, get_order(size));
-#endif
 }

diff -urN /home/dgibson/kernel/linuxppc_2_4_devel/include/asm-ppc/io.h linux-grinch/include/asm-ppc/io.h

--- /home/dgibson/kernel/linuxppc_2_4_devel/include/asm-ppc/io.h	Sat May 11 02:02:08 2002
+++ linux-grinch/include/asm-ppc/io.h	Fri Jun 14 12:05:35 2002

@@ -458,8 +458,17 @@
 #define dma_cache_wback(_start,_size)		do { } while (0)
 #define dma_cache_wback_inv(_start,_size)	do { } while (0)

-#define consistent_alloc(gfp, size, handle)	NULL
-#define consistent_free(addr, size)		do { } while (0)
+static inline void *consistent_alloc(int gfp, size_t size, dma_addr_t *dma_handle)
+{
+	unsigned long vaddr;
+
+	vaddr = __get_free_pages(gfp, get_order(size));
+	if (vaddr)
+		*dma_handle = virt_to_bus(vaddr);
+	return (void *)vaddr;
+}
+
+#define consistent_free(addr, size)		kfree(addr)
 #define consistent_sync(addr, size, rw)		do { } while (0)
 #define consistent_sync_page(pg, off, sz, rw)	do { } while (0)



--

David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

David Gibson wrote:

 > ......  Likewise with __get_free_pages()

which is what pci_alloc_consistent() uses on cache-coherent processors
now.

....and my concern is you are replacing a call to __get_free_pages() with
a call to kmalloc().  Are you sure you want to be doing this?  There may
be users of pci_alloc_consistent that assume page alignment which may not
happen when you call kmalloc().


	-- Dan

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Fri, Jun 14, 2002 at 12:06:40PM +1000, David Gibson wrote:

On Thu, Jun 13, 2002 at 06:57:45PM -0700, Eugene Surovegin wrote:

quoted

At 06:33 PM 6/13/2002, David Gibson wrote:

quoted

On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:

quoted

At 05:24 PM 6/13/2002, David Gibson wrote:

quoted

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

So, kmalloc will allocate non-cacheable memory?

I seriously doubt it (actually I've just tested it:).

No, but on cache coherent processors we don't need non-cacheable
memory.  That's the whole point.  Likewise with __get_free_pages()
which is what pci_alloc_consistent() uses on cache-coherent processors
now.

OK, I got it :)

One problem though, kmalloc allocated memory will not be necessarily
properly aligned
as required by DMA-mappings.txt

Some PCI drivers may break because of this.

Ah, yes, indeed.  How about this:

One last thing, did you mean to keep consistent_free as kfree() still
and not free_pages() ?

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Eugene Surovegin wrote:

This is gfp for _memory_ itself and may contain GFP_DMA for example. I
didn't want to allocate
struct vm_area from DMA pool in that case.

Well, you are using it for DMA, aren't you? :-)

May be the better solution is to mask out GFP_KERNEL or GFP_ATOMIC from
gfp and use it.

What do you think?

I don't recall if deep in the VM code it already does this.  I guess we
can just to make sure.  I think anyone that would test for these flags
should do the masking to ensure they get the proper indicator.

My only concern is we should honor the flag passed to us rather than
always force GFP_ATOMIC.  Mask them if you wish :-)


	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Paul Mackerras wrote:

 > .... eliminate the need to

add the extra parameter.  I'll run that past davem and see what he
thinks.

Thanks.

	-- Dan


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

Dan,

At 07:15 PM 6/13/2002, Dan Malek wrote:

Eugene Surovegin wrote:

quoted

This is gfp for _memory_ itself and may contain GFP_DMA for example. I
didn't want to allocate
struct vm_area from DMA pool in that case.

Well, you are using it for DMA, aren't you? :-)

Yeah, I use _memory_ for DMA, but I don't want waste DMA pool for struct
vm_struct itself.

Eugene


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

At 04:34 PM 6/13/2002, Paul Mackerras wrote:

Maybe the best answer is to change get_vm_area to use

        (in_interrupt()? GFP_ATOMIC: GFP_KERNEL)

instead of GFP_KERNEL in the kmalloc call, and eliminate the need to
add the extra parameter.  I'll run that past davem and see what he
thinks.

Well, your solution is much better :).

Do I understand correctly, we have to modify _all_ vmlist_lock locking to
use _irqsave counterparts?
Not just in get_vm_area, right?

Eugene.


** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, Jun 13, 2002 at 07:15:23PM -0700, Tom Rini wrote:

On Fri, Jun 14, 2002 at 12:06:40PM +1000, David Gibson wrote:

quoted

On Thu, Jun 13, 2002 at 06:57:45PM -0700, Eugene Surovegin wrote:

quoted

At 06:33 PM 6/13/2002, David Gibson wrote:

quoted

On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:

quoted

At 05:24 PM 6/13/2002, David Gibson wrote:

quoted

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

So, kmalloc will allocate non-cacheable memory?

I seriously doubt it (actually I've just tested it:).

No, but on cache coherent processors we don't need non-cacheable
memory.  That's the whole point.  Likewise with __get_free_pages()
which is what pci_alloc_consistent() uses on cache-coherent processors
now.

OK, I got it :)

One problem though, kmalloc allocated memory will not be necessarily
properly aligned
as required by DMA-mappings.txt

Some PCI drivers may break because of this.

Ah, yes, indeed.  How about this:

One last thing, did you mean to keep consistent_free as kfree() still
and not free_pages() ?

No I didn't.  Working on a fixed version - just running into some
other technical hitches.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Fri, Jun 14, 2002 at 01:58:03PM +1000, David Gibson wrote:

On Thu, Jun 13, 2002 at 07:15:23PM -0700, Tom Rini wrote:

quoted

On Fri, Jun 14, 2002 at 12:06:40PM +1000, David Gibson wrote:

quoted

On Thu, Jun 13, 2002 at 06:57:45PM -0700, Eugene Surovegin wrote:

quoted

At 06:33 PM 6/13/2002, David Gibson wrote:

quoted

On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:

quoted

At 05:24 PM 6/13/2002, David Gibson wrote:

quoted

That's right.  But I think the patch below is a better fix for the
problem.  It makes consistent_alloc()/consistent_free() just do the
right thing for both cache coherent and cache non-coherent processors,
so we can get rid of the ifdef in pci_alloc_consistent() and
pci_free_consistent().

So, kmalloc will allocate non-cacheable memory?

I seriously doubt it (actually I've just tested it:).

No, but on cache coherent processors we don't need non-cacheable
memory.  That's the whole point.  Likewise with __get_free_pages()
which is what pci_alloc_consistent() uses on cache-coherent processors
now.

OK, I got it :)

One problem though, kmalloc allocated memory will not be necessarily
properly aligned
as required by DMA-mappings.txt

Some PCI drivers may break because of this.

Ah, yes, indeed.  How about this:

One last thing, did you mean to keep consistent_free as kfree() still
and not free_pages() ?

No I didn't.  Working on a fixed version - just running into some
other technical hitches.

So are we sure it's worth it to remove now 2 #ifdefs ?

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Thu, Jun 13, 2002 at 05:51:56PM -0700, Tom Rini wrote:

On Fri, Jun 14, 2002 at 10:45:39AM +1000, David Gibson wrote:

quoted

On Thu, Jun 13, 2002 at 05:38:07PM -0700, Tom Rini wrote:

[snip]

quoted

quoted

Er, the problem of setting dma_handle twice?

Well, it fixes that and as a bonus gets rid of the ifdefs in
pci_{alloc,free}_consistent() and means that if we ever port a driver
using consistent_{alloc,free}() to a processor that *is* cache
coherent it will Just Work.

In theory at that point OCP would either have its API fully flushed out
or be applied nicely on top of the generic driver work in 2.5 and it
would just work anyhow :)

Well at the moment OCP drivers call consistent_alloc() directly which
would always fail on cache-coherent processors.

quoted

quoted

My only concern is that are things still consistent on non consistent
procs?

Absolutely - no change to the code path at all on non cache coherent
processors.

So kmalloc/kfree are equivilent to __get_free_pages/free_pages ?

Read that again: on *non* cache coherent processors the code path is
the same.  kmalloc() vs. __get_free_pages() is a problem for
processors which *are* cache coherent.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Fri, Jun 14, 2002 at 03:14:50PM +1000, David Gibson wrote:

On Thu, Jun 13, 2002 at 05:51:56PM -0700, Tom Rini wrote:

quoted

On Fri, Jun 14, 2002 at 10:45:39AM +1000, David Gibson wrote:

quoted

On Thu, Jun 13, 2002 at 05:38:07PM -0700, Tom Rini wrote:

[snip]

quoted

quoted

Er, the problem of setting dma_handle twice?

Well, it fixes that and as a bonus gets rid of the ifdefs in
pci_{alloc,free}_consistent() and means that if we ever port a driver
using consistent_{alloc,free}() to a processor that *is* cache
coherent it will Just Work.

In theory at that point OCP would either have its API fully flushed out
or be applied nicely on top of the generic driver work in 2.5 and it
would just work anyhow :)

Well at the moment OCP drivers call consistent_alloc() directly which
would always fail on cache-coherent processors.

Yes.  And at the moment OCP is still a work in progress too.  As I said
before, by the time that pops up, OCP will either have it's API finished
(and look alot like PCI so drivers shouldn't be calling
consistent_alloc() themselves) or be worked in nicely with the generic
driver work in 2.5, which again will be copying pci_alloc_consistent()
to driver_alloc_consistent().

quoted

quoted

quoted

My only concern is that are things still consistent on non consistent
procs?

Absolutely - no change to the code path at all on non cache coherent
processors.

So kmalloc/kfree are equivilent to __get_free_pages/free_pages ?

Read that again: on *non* cache coherent processors the code path is
the same.  kmalloc() vs. __get_free_pages() is a problem for
processors which *are* cache coherent.

Yes.  And there's 2 things here which I'm wondering about:
(a) Why do we call __get_fre_pages/free_pages now on coherent procs?
Can we really just call something else and have it work?
(b) Is it really a good idea to have a function called
'consistent_alloc()' which doesn't actually do that, for the sake of
removing 2 ifdefs?

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/

On Fri, Jun 14, 2002 at 07:59:30AM -0700, Tom Rini wrote:

quoted

quoted

quoted

quoted

My only concern is that are things still consistent on non consistent
procs?

Absolutely - no change to the code path at all on non cache coherent
processors.

So kmalloc/kfree are equivilent to __get_free_pages/free_pages ?

Read that again: on *non* cache coherent processors the code path is
the same.  kmalloc() vs. __get_free_pages() is a problem for
processors which *are* cache coherent.

Yes.  And there's 2 things here which I'm wondering about:
(a) Why do we call __get_fre_pages/free_pages now on coherent procs?
Can we really just call something else and have it work?

Possibly not (though I'm not sure kmalloc() wouldn't work), which is
why my last patch reverted to using __get_free_pages()/free_pages().
But the only problem with kmalloc() is the page-alignment requirements
specified in DMA-mapping.txt, not the actual consistency/coherence.

(b) Is it really a good idea to have a function called
'consistent_alloc()' which doesn't actually do that, for the sake of
removing 2 ifdefs?

As paulus already pointer out, it *does* actually do that.  Normal
memory is consistent on cache coherent processsors - that's what cache
coherent means.

--
David Gibson			| For every complex problem there is a
david@gibson.dropbear.id.au	| solution which is simple, neat and
				| wrong.  -- H.L. Mencken
http://www.ozlabs.org/people/dgibson

** Sent via the linuxppc-embedded mail list. See http://lists.linuxppc.org/