pci_pool_free from IRQ

pci_pool_free from IRQ

[Pete Zaitcev]

David,

Russel King complained that you might be calling pci_consistent_free
from an interrupt, which is unsafe on ARM. Why don't you remove this
part from pci_pool_free():

+       else if (!is_page_busy (pool->blocks_per_page, page->bitmap))
+               pool_free_page (pool, page);

In that case, fully free pages will stick about until the whole
pool is destroyed, which I think is not a big deal.

-- Pete

Re: pci_pool_free from IRQ

[Albert D. Cahalan]

Pete Zaitcev writes:

> Russel King complained that you might be calling pci_consistent_free
> from an interrupt, which is unsafe on ARM.

This sure makes life difficult. Device removal events can be called
from interrupt context according to Documentation/pci.txt. This is
certainly a place where one might want to call pci_consistent_free.

Re: pci_pool_free from IRQ

[Alan Cox]

> This sure makes life difficult. Device removal events can be called
> from interrupt context according to Documentation/pci.txt. This is
> certainly a place where one might want to call pci_consistent_free.

None of our device code supports interrupt based device removal. In fact
many drivers use vmalloc directly so will hit the same problem the
pci_consistent_free hits on the ARM.

I suspect we should fix the documentation (and if need be the code) to reflect
the fact that you have to be completely out of your tree to handle device 
removal in the irq handler

Re: pci_pool_free from IRQ

[David S. Miller]

Alan Cox writes:
 > I suspect we should fix the documentation (and if need be the code) to reflect
 > the fact that you have to be completely out of your tree to handle device 
 > removal in the irq handler

Agreed.

Later,
David S. Miller
davem@redhat.com

Re: pci_pool_free from IRQ

[David Brownell]

Pete's patch to pci_pool_free() is fine with me, and I'd be glad
to see that bit of pci interface cleaned up.  Any changes needed
other than the pci.txt doc update?

- Dave

----- Original Message -----
From: "David S. Miller" <davem@redhat.com>
To: "Alan Cox" <alan@lxorguk.ukuu.org.uk>
Cc: "Albert D. Cahalan" <acahalan@cs.uml.edu>; "Pete Zaitcev" <zaitcev@redhat.com>;
<david-b@pacbell.net>; <johannes@erdfelt.com>; <rmk@arm.linux.org.uk>;
<linux-kernel@vger.kernel.org>
Sent: Tuesday, May 08, 2001 2:55 PM
Subject: Re: pci_pool_free from IRQ


>
> Alan Cox writes:
>  > I suspect we should fix the documentation (and if need be the code) to reflect
>  > the fact that you have to be completely out of your tree to handle device
>  > removal in the irq handler
>
> Agreed.
>
> Later,
> David S. Miller
> davem@redhat.com

Re: pci_pool_free from IRQ

[David S. Miller]

David Brownell writes:
 > Pete's patch to pci_pool_free() is fine with me, and I'd be glad
 > to see that bit of pci interface cleaned up.  Any changes needed
 > other than the pci.txt doc update?

Ummm... What Alan's saying is:

1) Whatever driver is trying to shut down from IRQ context
   is broken must be fixed.  pci_pool is fine.

2) The Documentation/ files which suggest that such device
   removal from IRQs is "OK" must be fixed because it is not
   "OK" to handle device removal from IRQ context.

So Pete's change is not needed.  A fix for the documentation and
broken drivers is needed instead.

Later,
David S. Miller
davem@redhat.com

Re: pci_pool_free from IRQ

[David Brownell]

>  > Pete's patch to pci_pool_free() is fine with me, and I'd be glad
>  > to see that bit of pci interface cleaned up.  Any changes needed
>  > other than the pci.txt doc update?
> 
> Ummm... What Alan's saying is:

(consistent with what I said -- those are two separate issues!)

> 1) Whatever driver is trying to shut down from IRQ context
>    is broken must be fixed.  pci_pool is fine.

In _that_ respect, yes.  pci_pool_destroy() called in shutdown
context "should" be OK.  Getting rid of pages then is fine.

> 2) The Documentation/ files which suggest that such device
>    removal from IRQs is "OK" must be fixed because it is not
>    "OK" to handle device removal from IRQ context.

All agreed.

> So Pete's change is not needed.  A fix for the documentation and
> broken drivers is needed instead.

Two issues are mixed up there.  Doc should address both:

Documentation/pci.txt

    ... that remove() is never called in_interrupt
        point (2) above

Documentation/DMA-mapping.txt

    pci_free_consistent() -- do not call in_interrupt
        ... unless we bugfix the ARM behavior "soon"

    pci_pool_destroy() -- do not call in_interrupt
        ... normally called on device remove()
        
    pci_pool_free() -- may be called in_interrupt
        ... often called in device interrupt handling

Pete's patch deferred some pci_free_consistent calls from
pci_pool_free() where they're unsafe (on ARM) to where
they're safe (all architectures, as discussed above).

- Dave

Re: pci_pool_free from IRQ

[Pete Zaitcev]

> From: "David S. Miller" <davem@redhat.com>
> Date: Tue, 8 May 2001 17:52:45 -0700 (PDT)

> Ummm... What Alan's saying is:
> 
> 1) Whatever driver is trying to shut down from IRQ context
>    is broken must be fixed.  pci_pool is fine.
> 
> 2) The Documentation/ files which suggest that such device
>    removal from IRQs is "OK" must be fixed because it is not
>    "OK" to handle device removal from IRQ context.
> 
> So Pete's change is not needed.  A fix for the documentation and
> broken drivers is needed instead.

David, I do not follow your logic here, sorry.

I wrote that a path exists from a function that is legal in
interrupt context (pci_pool_free) into a function that is
not legal in interrupt context (pci_free_consistent).
The change breaks that connection. Note that pci_pool_free
is called when driver operates normally.

When you write "fix documentation and broken drivers", you talk
about a fix for a part that processes PCI remove. This is entirely
fine by me. But I was talking about a regular interrupt procession
in driver. A fix in pci remove does not fix regular processing.

-- Pete

Re: pci_pool_free from IRQ

[David S. Miller]

Pete Zaitcev writes:
 > A fix in pci remove does not fix regular processing.

I see.  Here is where I was confused.

Later,
David S. Miller
davem@redhat.com

Re: pci_pool_free from IRQ

[Pete Zaitcev]

How about this (with documentation fixes by David-B):

diff -ur -X dontdiff linux-2.4.4/Documentation/DMA-mapping.txt linux-2.4.4-niph/Documentation/DMA-mapping.txt
--- linux-2.4.4/Documentation/DMA-mapping.txt	Thu Apr 19 08:38:48 2001
+++ linux-2.4.4-niph/Documentation/DMA-mapping.txt	Thu May 10 12:29:22 2001
@@ -240,6 +240,7 @@
 
 where dev, size are the same as in the above call and cpu_addr and
 dma_handle are the values pci_alloc_consistent returned to you.
+This function may not be called in interrupt context.
 
 If your driver needs lots of smaller memory regions, you can write
 custom code to subdivide pages returned by pci_alloc_consistent,
@@ -262,7 +263,8 @@
 sleeping context (f.e. in_interrupt is true or while holding SMP
 locks), pass SLAB_ATOMIC.  If your device has no boundary crossing
 restrictions, pass 0 for alloc; passing 4096 says memory allocated
-from this pool must not cross 4KByte boundaries.
+from this pool must not cross 4KByte boundaries (but at that time it
+may be better to go for pci_alloc_consistent directly instead).
 
 Allocate memory from a pci pool like this:
 
@@ -270,21 +272,23 @@
 
 flags are SLAB_KERNEL if blocking is permitted (not in_interrupt nor
 holding SMP locks), SLAB_ATOMIC otherwise.  Like pci_alloc_consistent,
-this returns two values, cpu_addr and dma_handle,
+this returns two values, cpu_addr and dma_handle.
 
 Free memory that was allocated from a pci_pool like this:
 
 	pci_pool_free(pool, cpu_addr, dma_handle);
 
 where pool is what you passed to pci_pool_alloc, and cpu_addr and
-dma_handle are the values pci_pool_alloc returned.
+dma_handle are the values pci_pool_alloc returned. This function
+may be called in interrupt context.
 
 Destroy a pci_pool by calling:
 
 	pci_pool_destroy(pool);
 
 Make sure you've called pci_pool_free for all memory allocated
-from a pool before you destroy the pool.
+from a pool before you destroy the pool. This function may not
+be called in interrupt context.
 
 			DMA Direction
 
diff -ur -X dontdiff linux-2.4.4/Documentation/pci.txt linux-2.4.4-niph/Documentation/pci.txt
--- linux-2.4.4/Documentation/pci.txt	Sun Sep 17 09:45:06 2000
+++ linux-2.4.4-niph/Documentation/pci.txt	Thu May 10 12:33:03 2001
@@ -60,8 +60,8 @@
 	remove		Pointer to a function which gets called whenever a device
 			being handled by this driver is removed (either during
 			deregistration of the driver or when it's manually pulled
-			out of a hot-pluggable slot). This function can be called
-			from interrupt context.
+			out of a hot-pluggable slot). This function always gets
+			called from process context, so it can sleep.
 	suspend,	Power management hooks -- called when the device goes to
 	resume		sleep or is resumed.
 
--- linux-2.4.4/drivers/pci/pci.c	Thu Apr 19 08:38:48 2001
+++ linux-2.4.4-niph/drivers/pci/pci.c	Thu May 10 12:36:28 2001
@@ -1701,8 +1701,11 @@
 	set_bit (block, &page->bitmap [map]);
 	if (waitqueue_active (&pool->waitq))
 		wake_up (&pool->waitq);
-	else if (!is_page_busy (pool->blocks_per_page, page->bitmap))
-		pool_free_page (pool, page);
+	/*
+	 * Resist a temptation to do
+	 *    if (!is_page_busy(bpp, page->bitmap)) pool_free_page(pool, page);
+	 * it is not interrupt safe. Better have empty pages hang around.
+	 */
 	spin_unlock_irqrestore (&pool->lock, flags);
 }
 

Re: pci_pool_free from IRQ

[David Brownell]

> How about this (with documentation fixes by David-B):

Actually I'd be just as happy to call the ARM pci_free_consistent()
behavior (BUG in_interrupt) the problem.  Particularly if that ARM
patch works OK!  I've gotten success reports with pci_pool from
folk using about half the architectures in linux/arch, and only ARM
showed this particular problem.  It appears there's no real need
to update the interface spec to accomodate ARM.

That means the doc fixes are simpler:  in DMA-mapping.txt just clarify
that some routines may be called in_interrupt (currently unspecified),
and the pci.txt change about pci_device.remove() (agreed to by
both Alan and DaveM, appended).

- Dave


> diff -ur -X dontdiff linux-2.4.4/Documentation/pci.txt linux-2.4.4-niph/Documentation/pci.txt
> --- linux-2.4.4/Documentation/pci.txt Sun Sep 17 09:45:06 2000
> +++ linux-2.4.4-niph/Documentation/pci.txt Thu May 10 12:33:03 2001
> @@ -60,8 +60,8 @@
>   remove Pointer to a function which gets called whenever a device
>   being handled by this driver is removed (either during
>   deregistration of the driver or when it's manually pulled
> - out of a hot-pluggable slot). This function can be called
> - from interrupt context.
> + out of a hot-pluggable slot). This function always gets
> + called from process context, so it can sleep.
>   suspend, Power management hooks -- called when the device goes to
>   resume sleep or is resumed.
>