[patch 2.6.23-rc1] dma_free_coherent() needs irqs enabled (sigh)

[patch 2.6.23-rc1] dma_free_coherent() needs irqs enabled (sigh)

[David Brownell]

On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
call context requirement:  unlike its dma_alloc_coherent() sibling, it
may not be called with IRQs disabled.  (This was new behavior on ARM as
of late 2006, caused by ARM SMP updates.)  This little surprise can be
annoyingly driver-visible.

Since it looks like that restriction won't be removed, this patch changes
the definition of the API to include that requirement.  Also, to help
catch nonportable drivers, it updates the x86 and swiotlb versions to
include the relevant warnings.  (I already observed that it trips on the
bus_reset_tasklet of the new firewire_ohci driver.)

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Cc: David Miller <davem@davemloft.net>
Cc: Russell King <rmk@arm.linux.org.uk>
---
 Documentation/DMA-API.txt    |    3 +++
 arch/i386/kernel/pci-dma.c   |    3 ++-
 arch/x86_64/kernel/pci-dma.c |    1 +
 lib/swiotlb.c                |    1 +
 4 files changed, 7 insertions(+), 1 deletion(-)

--- g26.orig/Documentation/DMA-API.txt	2007-07-22 05:06:57.000000000 -0700
+++ g26/Documentation/DMA-API.txt	2007-07-22 14:48:45.000000000 -0700
@@ -68,6 +68,9 @@ size and dma_handle must all be the same
 consistent allocate.  cpu_addr must be the virtual address returned by
 the consistent allocate
 
+Note that unlike their sibling allocation calls, these routines
+may only be called with IRQs enabled.
+
 
 Part Ib - Using small dma-coherent buffers
 ------------------------------------------
--- g26.orig/arch/x86_64/kernel/pci-dma.c	2007-07-22 05:13:10.000000000 -0700
+++ g26/arch/x86_64/kernel/pci-dma.c	2007-07-22 14:48:45.000000000 -0700
@@ -163,6 +163,7 @@ EXPORT_SYMBOL(dma_alloc_coherent);
 void dma_free_coherent(struct device *dev, size_t size,
 			 void *vaddr, dma_addr_t bus)
 {
+	WARN_ON(irqs_disabled());	/* for portability */
 	if (dma_ops->unmap_single)
 		dma_ops->unmap_single(dev, bus, size, 0);
 	free_pages((unsigned long)vaddr, get_order(size));
--- g26.orig/arch/i386/kernel/pci-dma.c	2007-07-22 05:06:57.000000000 -0700
+++ g26/arch/i386/kernel/pci-dma.c	2007-07-22 14:48:46.000000000 -0700
@@ -63,7 +63,8 @@ void dma_free_coherent(struct device *de
 {
 	struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
 	int order = get_order(size);
-	
+
+	WARN_ON(irqs_disabled());	/* for portability */
 	if (mem && vaddr >= mem->virt_base && vaddr < (mem->virt_base + (mem->size << PAGE_SHIFT))) {
 		int page = (vaddr - mem->virt_base) >> PAGE_SHIFT;
 
--- g26.orig/lib/swiotlb.c	2007-07-22 05:13:15.000000000 -0700
+++ g26/lib/swiotlb.c	2007-07-22 14:48:46.000000000 -0700
@@ -497,6 +497,7 @@ void
 swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
 		      dma_addr_t dma_handle)
 {
+	WARN_ON(irqs_disabled());
 	if (!(vaddr >= (void *)io_tlb_start
                     && vaddr < (void *)io_tlb_end))
 		free_pages((unsigned long) vaddr, get_order(size));

Re: [patch 2.6.23-rc1] dma_free_coherent() needs irqs enabled (sigh)

[Russell King]

On Tue, Jul 24, 2007 at 02:29:05PM -0700, David Brownell wrote:
> On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
> call context requirement:  unlike its dma_alloc_coherent() sibling, it
> may not be called with IRQs disabled.  (This was new behavior on ARM as
> of late 2006, caused by ARM SMP updates.)

I think you got the year wrong:

5edf71ae (Russell King      2005-11-25 15:52:51 +0000 364)      WARN_ON(irqs_disabled());

which is due to this commit:

[ARM] Do not call flush_tlb_kernel_range() with IRQs disabled.

We must not call TLB maintainence operations with interrupts disabled,
otherwise we risk a lockup in the SMP IPI code.

This means that consistent_free() can not be called from a context with
IRQs disabled.  In addition, we must not hold the lock in consistent_free
when we call flush_tlb_kernel_range().  However, we must continue to
prevent consistent_alloc() from re-using the memory region until we've
finished tearing down the mapping and dealing with the TLB.

Therefore, leave the vm_region entry in the list, but mark it inactive
before dropping the lock and starting the tear-down process.  After the
mapping has been torn down, re-acquire the lock and remove the entry
from the list.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

> Since it looks like that restriction won't be removed, this patch changes
> the definition of the API to include that requirement.

The PCI DMA-mapping API had this restriction.  For some reason, this
restriction was not carried forward into the DMA-API.  Unfortunately
the restriction can not be removed without causing the problems
described in the commit which introduced it.

Or alternatively we scrap ARM SMP entirely, which isn't going to happen.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [patch 2.6.23-rc1] dma_free_coherent() needs irqs enabled (sigh)

[David Brownell]

On Tuesday 24 July 2007, Russell King wrote:
> On Tue, Jul 24, 2007 at 02:29:05PM -0700, David Brownell wrote:
> > On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
> > call context requirement:  unlike its dma_alloc_coherent() sibling, it
> > may not be called with IRQs disabled.  (This was new behavior on ARM as
> > of late 2006, caused by ARM SMP updates.)
> 
> I think you got the year wrong:
> 
> 5edf71ae (Russell King      2005-11-25 15:52:51 +0000 364)      WARN_ON(irqs_disabled());
> 
> which is due to this commit:
> 
> [ARM] Do not call flush_tlb_kernel_range() with IRQs disabled.

This little "to do" list item has been sitting in my mailbox way
too long then.  Certainly since it was fair to say "last year"!  ;)

Of course, 2.6.23-rc1 also merges all the gadget API updates I
included to cope with this annoyance.  So with any luck the issue
will now finally have been properly whupped.


> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

Thanks...

 
> > Since it looks like that restriction won't be removed, this patch changes
> > the definition of the API to include that requirement.
> 
> The PCI DMA-mapping API had this restriction.  For some reason, this
> restriction was not carried forward into the DMA-API.  Unfortunately
> the restriction can not be removed without causing the problems
> described in the commit which introduced it.

Right, I noticed that.

- Dave

Re: [patch 2.6.23-rc1] dma_free_coherent() needs irqs enabled (sigh)

[Russell King]

On Tue, Jul 24, 2007 at 04:08:11PM -0700, David Brownell wrote:
> On Tuesday 24 July 2007, Russell King wrote:
> > On Tue, Jul 24, 2007 at 02:29:05PM -0700, David Brownell wrote:
> > > On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
> > > call context requirement:  unlike its dma_alloc_coherent() sibling, it
> > > may not be called with IRQs disabled.  (This was new behavior on ARM as
> > > of late 2006, caused by ARM SMP updates.)
> > 
> > I think you got the year wrong:
> > 
> > 5edf71ae (Russell King      2005-11-25 15:52:51 +0000 364)      WARN_ON(irqs_disabled());
> > 
> > which is due to this commit:
> > 
> > [ARM] Do not call flush_tlb_kernel_range() with IRQs disabled.
> 
> This little "to do" list item has been sitting in my mailbox way
> too long then.  Certainly since it was fair to say "last year"!  ;)

Are you intentionally not reading what I said?

> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
> Thanks...

That was part of the commit I quoted, not an endorsement of your patch,
though I think it does deserve an:

Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [patch 2.6.23-rc1] dma_free_coherent() needs irqs enabled (sigh)

[David Brownell]

On Tuesday 24 July 2007, Russell King wrote:
> > > 
> > > I think you got the year wrong:
> > > 
> > > 5edf71ae (Russell King      2005-11-25 15:52:51 +0000 364)      WARN_ON(irqs_disabled());
> > > 
> > > which is due to this commit:
> > > 
> > > [ARM] Do not call flush_tlb_kernel_range() with IRQs disabled.
> > 
> > This little "to do" list item has been sitting in my mailbox way
> > too long then.  Certainly since it was fair to say "last year"!  ;)
> 
> Are you intentionally not reading what I said?

Hardly.  Go back and read what *I* wrote!

It just took a while to notice that behavioral change, since
I don't normally run the relevant regression tests using lockdep.
It was sometime in the first half of 2006, ergo "since it was
fair to say 'last year'".

A bunch of piecemeal workarounds followed; and recently they were
all replaced with a more fundamental fix.  This doc patch was the
tail end of the process of recovering from that change ... and the
warnings are there to help other folk from seeing the same issue
in other contexts.

- Dave

Re: [patch 2.6.23-rc1] dma_free_coherent() needs irqs enabled (sigh)

[Stefan Richter]

For linux1394-devel:

David Brownell wrote at LKML:
> On at least ARM (and I'm told MIPS too) dma_free_coherent() has a newish
> call context requirement:  unlike its dma_alloc_coherent() sibling, it
> may not be called with IRQs disabled.  (This was new behavior on ARM as
> of late 2006, caused by ARM SMP updates.)  This little surprise can be
> annoyingly driver-visible.
> 
> Since it looks like that restriction won't be removed, this patch changes
> the definition of the API to include that requirement.  Also, to help
> catch nonportable drivers, it updates the x86 and swiotlb versions to
> include the relevant warnings.  (I already observed that it trips on the
> bus_reset_tasklet of the new firewire_ohci driver.)

Also in ohci_set_config_rom().
-- 
Stefan Richter
-=====-=-=== -=== ==--=
http://arcgraph.de/sr/

[PATCH] firewire: fw-ohci: dma_free_coherent needs IRQs enabled

[Stefan Richter]

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-ohci.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -907,6 +907,8 @@ static void bus_reset_tasklet(unsigned l
 	int self_id_count, i, j, reg;
 	int generation, new_generation;
 	unsigned long flags;
+	void *free_rom = NULL;
+	dma_addr_t free_rom_bus = 0;
 
 	reg = reg_read(ohci, OHCI1394_NodeID);
 	if (!(reg & OHCI1394_NodeID_idValid)) {
@@ -970,10 +972,10 @@ static void bus_reset_tasklet(unsigned l
 	 */
 
 	if (ohci->next_config_rom != NULL) {
-		if (ohci->next_config_rom != ohci->config_rom)
-			dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
-					  ohci->config_rom,
-					  ohci->config_rom_bus);
+		if (ohci->next_config_rom != ohci->config_rom) {
+			free_rom      = ohci->config_rom;
+			free_rom_bus  = ohci->config_rom_bus;
+		}
 		ohci->config_rom      = ohci->next_config_rom;
 		ohci->config_rom_bus  = ohci->next_config_rom_bus;
 		ohci->next_config_rom = NULL;
@@ -992,6 +994,10 @@ static void bus_reset_tasklet(unsigned l
 
 	spin_unlock_irqrestore(&ohci->lock, flags);
 
+	if (free_rom)
+		dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
+				  free_rom, free_rom_bus);
+
 	fw_core_handle_bus_reset(&ohci->card, ohci->node_id, generation,
 				 self_id_count, ohci->self_id_buffer);
 }
@@ -1199,7 +1205,7 @@ ohci_set_config_rom(struct fw_card *card
 {
 	struct fw_ohci *ohci;
 	unsigned long flags;
-	int retval = 0;
+	int retval = -EBUSY;
 	__be32 *next_config_rom;
 	dma_addr_t next_config_rom_bus;
 
@@ -1253,10 +1259,7 @@ ohci_set_config_rom(struct fw_card *card
 
 		reg_write(ohci, OHCI1394_ConfigROMmap,
 			  ohci->next_config_rom_bus);
-	} else {
-		dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
-				  next_config_rom, next_config_rom_bus);
-		retval = -EBUSY;
+		retval = 0;
 	}
 
 	spin_unlock_irqrestore(&ohci->lock, flags);
@@ -1270,6 +1273,9 @@ ohci_set_config_rom(struct fw_card *card
 	 */
 	if (retval == 0)
 		fw_core_initiate_bus_reset(&ohci->card, 1);
+	else
+		dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
+				  next_config_rom, next_config_rom_bus);
 
 	return retval;
 }

-- 
Stefan Richter
-=====-=-=== -=== ==--=
http://arcgraph.de/sr/