[PATCH] pci_alloc_consistent in an interrupt context

[PATCH] pci_alloc_consistent in an interrupt context

[Eugene Surovegin]

[-- Attachment #1: Type: text/plain, Size: 245 bytes --]

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>

[-- Attachment #2: consistent_alloc.patch --]
[-- Type: application/octet-stream, Size: 3292 bytes --]

diff -ur a/arch/ppc/kernel/pci-dma.c b/arch/ppc/kernel/pci-dma.c
--- a/arch/ppc/kernel/pci-dma.c	Sat Jan 19 00:07:14 2002
+++ b/arch/ppc/kernel/pci-dma.c	Wed Jun 12 19:06:12 2002
@@ -33,7 +33,9 @@
 
 	if (ret != NULL) {
 		memset(ret, 0, size);
+#ifndef CONFIG_NOT_COHERENT_CACHE
 		*dma_handle = virt_to_bus(ret);
+#endif
 	}
 	return ret;
 }
diff -ur a/arch/ppc/mm/cachemap.c b/arch/ppc/mm/cachemap.c
--- a/arch/ppc/mm/cachemap.c	Wed Jun 12 20:40:03 2002
+++ b/arch/ppc/mm/cachemap.c	Thu Jun 13 11:47:41 2002
@@ -69,10 +69,7 @@
 	phys_addr_t pa;
 	struct vm_struct *area;
 	void	 *ret;
-
-	if (in_interrupt())
-		BUG();
-
+	
 	/* Only allocate page size areas.
 	*/
 	size = PAGE_ALIGN(size);
@@ -92,7 +89,7 @@
 
 	/* Allocate some common virtual space to map the new pages.
 	*/
-	area = get_vm_area(size, VM_ALLOC);
+	area = get_vm_area(size, VM_ALLOC, GFP_ATOMIC);
 	if (area == 0) {
 		free_pages(page, order);
 		return NULL;
@@ -123,8 +119,6 @@
  */
 void consistent_free(void *vaddr)
 {
-	if (in_interrupt())
-		BUG();
 	vfree(vaddr);
 }
 
diff -ur a/arch/ppc/mm/pgtable.c b/arch/ppc/mm/pgtable.c
--- a/arch/ppc/mm/pgtable.c	Sat Apr  6 13:06:49 2002
+++ b/arch/ppc/mm/pgtable.c	Thu Jun 13 11:41:02 2002
@@ -172,7 +172,7 @@
 	
 	if (mem_init_done) {
 		struct vm_struct *area;
-		area = get_vm_area(size, VM_IOREMAP);
+		area = get_vm_area(size, VM_IOREMAP, GFP_KERNEL);
 		if (area == 0)
 			return NULL;
 		v = VMALLOC_VMADDR(area->addr);
diff -ur a/drivers/pcmcia/hd64465_ss.c b/drivers/pcmcia/hd64465_ss.c
--- a/drivers/pcmcia/hd64465_ss.c	Wed Jul 11 07:27:34 2001
+++ b/drivers/pcmcia/hd64465_ss.c	Thu Jun 13 11:46:00 2002
@@ -918,7 +918,7 @@
 	for (i=0 ; i<MAX_WIN ; i++)
 	    sp->mem_maps[i].map = i;
 	
-	if ((sp->io_vma = get_vm_area(HS_IO_MAP_SIZE, VM_IOREMAP)) == 0)
+	if ((sp->io_vma = get_vm_area(HS_IO_MAP_SIZE, VM_IOREMAP, GFP_KERNEL)) == 0)
 	    return -ENOMEM;
 
 	hd64465_register_irq_demux(sp->irq, hs_irq_demux, sp);
diff -ur a/include/linux/vmalloc.h b/include/linux/vmalloc.h
--- a/include/linux/vmalloc.h	Fri Jan  5 23:26:19 2001
+++ b/include/linux/vmalloc.h	Thu Jun 13 11:41:02 2002
@@ -18,7 +18,8 @@
 	struct vm_struct * next;
 };
 
-extern struct vm_struct * get_vm_area (unsigned long size, unsigned long flags);
+extern struct vm_struct * get_vm_area (unsigned long size, unsigned long flags,
+				       int gfp_mask);
 extern void vfree(void * addr);
 extern void * __vmalloc (unsigned long size, int gfp_mask, pgprot_t prot);
 extern long vread(char *buf, char *addr, unsigned long count);
diff -ur a/mm/vmalloc.c b/mm/vmalloc.c
--- a/mm/vmalloc.c	Thu Jan 10 05:28:35 2002
+++ b/mm/vmalloc.c	Thu Jun 13 11:41:03 2002
@@ -168,12 +168,12 @@
 	return ret;
 }
 
-struct vm_struct * get_vm_area(unsigned long size, unsigned long flags)
+struct vm_struct * get_vm_area(unsigned long size, unsigned long flags, int gfp_mask)
 {
 	unsigned long addr;
 	struct vm_struct **p, *tmp, *area;
 
-	area = (struct vm_struct *) kmalloc(sizeof(*area), GFP_KERNEL);
+	area = (struct vm_struct *) kmalloc(sizeof(*area), gfp_mask);
 	if (!area)
 		return NULL;
 	size += PAGE_SIZE;
@@ -236,7 +236,7 @@
 		BUG();
 		return NULL;
 	}
-	area = get_vm_area(size, VM_ALLOC);
+	area = get_vm_area(size, VM_ALLOC, GFP_KERNEL);
 	if (!area)
 		return NULL;
 	addr = area->addr;

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Tom Rini]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Dan Malek]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Tom Rini]

On Thu, Jun 13, 2002 at 05:47:32PM -0400, Dan Malek wrote:
> Tom Rini wrote:
>
> >..... 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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[David Gibson]

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:
> > Tom Rini wrote:
> >
> > >..... 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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Tom Rini]

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:
> >
> > On Thu, Jun 13, 2002 at 05:47:32PM -0400, Dan Malek wrote:
> > > Tom Rini wrote:
> > >
> > > >..... 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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[David Gibson]

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:
> >
> > 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:
> > > > Tom Rini wrote:
> > > >
> > > > >..... 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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Tom Rini]

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]
> > 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 :)

> > 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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[David Gibson]

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:
> > On Thu, Jun 13, 2002 at 05:38:07PM -0700, Tom Rini wrote:
> [snip]
> > > 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.

> > > 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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Tom Rini]

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:
> >
> > 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]
> > > > 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().

> > > > 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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[David Gibson]

On Fri, Jun 14, 2002 at 07:59:30AM -0700, Tom Rini wrote:
> > > > > 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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Eugene Surovegin]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[David Gibson]

On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:
>
> 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:).

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Eugene Surovegin]

At 06:33 PM 6/13/2002, David Gibson wrote:
>On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:
> >
> > 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:).
>
>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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[David Gibson]

On Thu, Jun 13, 2002 at 06:57:45PM -0700, Eugene Surovegin wrote:
>
> At 06:33 PM 6/13/2002, David Gibson wrote:
> >On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:
> >>
> >> 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:).
> >
> >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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Tom Rini]

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:
> >
> > At 06:33 PM 6/13/2002, David Gibson wrote:
> > >On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:
> > >>
> > >> 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:).
> > >
> > >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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[David Gibson]

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:
> > On Thu, Jun 13, 2002 at 06:57:45PM -0700, Eugene Surovegin wrote:
> > >
> > > At 06:33 PM 6/13/2002, David Gibson wrote:
> > > >On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:
> > > >>
> > > >> 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:).
> > > >
> > > >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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Tom Rini]

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:
> >
> > 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:
> > > >
> > > > At 06:33 PM 6/13/2002, David Gibson wrote:
> > > > >On Thu, Jun 13, 2002 at 06:25:11PM -0700, Eugene Surovegin wrote:
> > > > >>
> > > > >> 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:).
> > > > >
> > > > >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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Dan Malek]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Dan Malek]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Eugene Surovegin]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Paul Mackerras]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Dan Malek]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Eugene Surovegin]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Paul Mackerras]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Dan Malek]

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/

Re: [PATCH] pci_alloc_consistent in an interrupt context

[Eugene Surovegin]

Dan,

At 07:15 PM 6/13/2002, Dan Malek wrote:
>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? :-)

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/

[PATCH] pci_alloc_consistent in an interrupt context, part 2

[Eugene Surovegin]

[-- Attachment #1: Type: text/plain, Size: 194 bytes --]

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

[-- Attachment #2: consistent_alloc_2.patch --]
[-- Type: application/octet-stream, Size: 595 bytes --]

===== cachemap.c 1.14 vs 1.15 =====
--- 1.14/arch/ppc/mm/cachemap.c	Thu Jun 13 11:47:41 2002
+++ 1.15/arch/ppc/mm/cachemap.c	Thu Jun 13 16:00:15 2002
@@ -88,8 +88,10 @@
 	invalidate_dcache_range(page, page + size);
 
 	/* Allocate some common virtual space to map the new pages.
+	   Mask out zone modifiers from gfp; we don't want 
+	   to restrict vm_struct allocation to DMA or high memory pool only
 	*/
-	area = get_vm_area(size, VM_ALLOC, GFP_ATOMIC);
+	area = get_vm_area(size, VM_ALLOC, gfp & ~(__GFP_DMA | __GFP_HIGHMEM));
 	if (area == 0) {
 		free_pages(page, order);
 		return NULL;