2005-12-02 20:33:06

by Carlos Martín Nieto

[permalink] [raw]
Subject: Debug: sleeping function called in atomic context

Hello,

I've been having this problem since -rc2, but today the output stopped long
enough for me to copy it.

This happens on shutdown, I think the USB driver is deregistering itself or
something or other.

This is part of the output with -rc4

in_atomic():1 irqs_disabled():0
Debug: sleeping function called in atomic context at linux/rwsem.h:66
change_page_attr_addr+46
ioremap_change_attr+75
iounmap+14
:usbcore:usb_hcd_pci_remove+62

I couldn't copy more before the computer went off. My .config is attached.
--
Carlos Martín http://www.cmartin.tk


Attachments:
(No filename) (566.00 B)
config (36.89 kB)
Download all attachments

2005-12-03 12:51:36

by Arnd Bergmann

[permalink] [raw]
Subject: Re: Debug: sleeping function called in atomic context

Am Freitag 02 Dezember 2005 21:32 schrieb Carlos Martín:
>  I've been having this problem since -rc2, but today the output stopped
> long enough for me to copy it.

It seems you copied enough of it.

>  This happens on shutdown, I think the USB driver is deregistering itself
> or something or other.
>
>  This is part of the output with -rc4
>
> in_atomic():1 irqs_disabled():0
> Debug: sleeping function called in atomic context at linux/rwsem.h:66
> change_page_attr_addr+46
> ioremap_change_attr+75
> iounmap+14
>
> :usbcore:usb_hcd_pci_remove+62

This is what happens:
ioumap calls ioremap_change_attr with vmlist_lock held. change_page_attr_addr
then tries to acquire init_mm.mmap_sem, which must not be taken with
spinlock held.

It looks like the incorrect code has been in there for quite some time, but
that path was not taken for some reason.

Arnd <><

2005-12-03 22:18:04

by Andi Kleen

[permalink] [raw]
Subject: Re: Debug: sleeping function called in atomic context

On Sat, Dec 03, 2005 at 01:51:14PM +0100, Arnd Bergmann wrote:
> It looks like the incorrect code has been in there for quite some time, but
> that path was not taken for some reason.

Yes, or nobody tried it with preempt debugging. Anyways this patch
should fix it. Can the OP test?

----


i386/x86-64: Don't call change_page_attr with a spinlock hold.

It's illegal because it can sleep.

Use a two step lookup scheme instead. First look up the vm_struct,
then change the direct mapping, then finally unmap it. That's
ok because nobody can change the particular virtual address range
as long as the vm_struct is still in the global list.

Also added some LinuxDoc documentation to iounmap.

Signed-off-by: Andi Kleen <[email protected]>

Index: linux/arch/i386/mm/ioremap.c
===================================================================
--- linux.orig/arch/i386/mm/ioremap.c
+++ linux/arch/i386/mm/ioremap.c
@@ -223,9 +223,15 @@ void __iomem *ioremap_nocache (unsigned
}
EXPORT_SYMBOL(ioremap_nocache);

+/**
+ * iounmap - Free a IO remapping
+ * @addr: virtual address from ioremap_*
+ *
+ * Caller must ensure there is only one unmapping for the same pointer.
+ */
void iounmap(volatile void __iomem *addr)
{
- struct vm_struct *p;
+ struct vm_struct *p, *o;

if ((void __force *)addr <= high_memory)
return;
@@ -239,22 +245,35 @@ void iounmap(volatile void __iomem *addr
addr < phys_to_virt(ISA_END_ADDRESS))
return;

- write_lock(&vmlist_lock);
- p = __remove_vm_area((void *)(PAGE_MASK & (unsigned long __force)addr));
- if (!p) {
- printk(KERN_WARNING "iounmap: bad address %p\n", addr);
+ /* Use the vm area unlocked, assuming the caller
+ ensures there isn't another iounmap for the same address
+ in parallel. Reuse of the virtual address is prevented by
+ leaving it in the global lists until we're done with it.
+ cpa takes care of the direct mappings. */
+ read_lock(&vmlist_lock);
+ for (p = vmlist ; p ;p = p->next) {
+ if (p->addr == addr)
+ break;
+ }
+ read_unlock(&vmlist_lock);
+
+ if (!p) {
+ printk("iounmap: bad address %p\n", addr);
dump_stack();
- goto out_unlock;
+ return;
}

+ /* Reset the direct mapping. Can block */
if ((p->flags >> 20) && p->phys_addr < virt_to_phys(high_memory) - 1) {
change_page_attr(virt_to_page(__va(p->phys_addr)),
p->size >> PAGE_SHIFT,
PAGE_KERNEL);
global_flush_tlb();
}
-out_unlock:
- write_unlock(&vmlist_lock);
+
+ /* Finally remove it */
+ o = remove_vm_area((void *)((unsigned long)addr & PAGE_MASK));
+ BUG_ON(p != o || o == NULL);
kfree(p);
}
EXPORT_SYMBOL(iounmap);
Index: linux/arch/x86_64/mm/ioremap.c
===================================================================
--- linux.orig/arch/x86_64/mm/ioremap.c
+++ linux/arch/x86_64/mm/ioremap.c
@@ -247,9 +247,15 @@ void __iomem *ioremap_nocache (unsigned
return __ioremap(phys_addr, size, _PAGE_PCD);
}

+/**
+ * iounmap - Free a IO remapping
+ * @addr: virtual address from ioremap_*
+ *
+ * Caller must ensure there is only one unmapping for the same pointer.
+ */
void iounmap(volatile void __iomem *addr)
{
- struct vm_struct *p;
+ struct vm_struct *p, *o;

if (addr <= high_memory)
return;
@@ -257,12 +263,30 @@ void iounmap(volatile void __iomem *addr
addr < phys_to_virt(ISA_END_ADDRESS))
return;

- write_lock(&vmlist_lock);
- p = __remove_vm_area((void *)((unsigned long)addr & PAGE_MASK));
- if (!p)
+ /* Use the vm area unlocked, assuming the caller
+ ensures there isn't another iounmap for the same address
+ in parallel. Reuse of the virtual address is prevented by
+ leaving it in the global lists until we're done with it.
+ cpa takes care of the direct mappings. */
+ read_lock(&vmlist_lock);
+ for (p = vmlist ; p ;p = p->next) {
+ if (p->addr == addr)
+ break;
+ }
+ read_unlock(&vmlist_lock);
+
+ if (!p) {
printk("iounmap: bad address %p\n", addr);
- else if (p->flags >> 20)
+ dump_stack();
+ return;
+ }
+
+ /* Reset the direct mapping. Can block */
+ if (p->flags >> 20)
ioremap_change_attr(p->phys_addr, p->size, 0);
- write_unlock(&vmlist_lock);
+
+ /* Finally remove it */
+ o = remove_vm_area((void *)((unsigned long)addr & PAGE_MASK));
+ BUG_ON(p != o || o == NULL);
kfree(p);
}

2005-12-03 23:21:41

by Andi Kleen

[permalink] [raw]
Subject: Re: Debug: sleeping function called in atomic context II


Please use this patch instead. The previous one had a stupid bug.


i386/x86-64: Don't call change_page_attr with a spinlock hold.

It's illegal because it can sleep.

Use a two step lookup scheme instead. First look up the vm_struct,
then change the direct mapping, then finally unmap it. That's
ok because nobody can change the particular virtual address range
as long as the vm_struct is still in the global list.

Also added some LinuxDoc documentation to iounmap.

Signed-off-by: Andi Kleen <[email protected]>

Index: linux/arch/i386/mm/ioremap.c
===================================================================
--- linux.orig/arch/i386/mm/ioremap.c
+++ linux/arch/i386/mm/ioremap.c
@@ -223,9 +223,15 @@ void __iomem *ioremap_nocache (unsigned
}
EXPORT_SYMBOL(ioremap_nocache);

+/**
+ * iounmap - Free a IO remapping
+ * @addr: virtual address from ioremap_*
+ *
+ * Caller must ensure there is only one unmapping for the same pointer.
+ */
void iounmap(volatile void __iomem *addr)
{
- struct vm_struct *p;
+ struct vm_struct *p, *o;

if ((void __force *)addr <= high_memory)
return;
@@ -239,22 +245,37 @@ void iounmap(volatile void __iomem *addr
addr < phys_to_virt(ISA_END_ADDRESS))
return;

- write_lock(&vmlist_lock);
- p = __remove_vm_area((void *)(PAGE_MASK & (unsigned long __force)addr));
- if (!p) {
- printk(KERN_WARNING "iounmap: bad address %p\n", addr);
+ addr = (volatile void *)(PAGE_MASK & (unsigned long __force)addr);
+
+ /* Use the vm area unlocked, assuming the caller
+ ensures there isn't another iounmap for the same address
+ in parallel. Reuse of the virtual address is prevented by
+ leaving it in the global lists until we're done with it.
+ cpa takes care of the direct mappings. */
+ read_lock(&vmlist_lock);
+ for (p = vmlist; p; p = p->next) {
+ if (p->addr == addr)
+ break;
+ }
+ read_unlock(&vmlist_lock);
+
+ if (!p) {
+ printk("iounmap: bad address %p\n", addr);
dump_stack();
- goto out_unlock;
+ return;
}

+ /* Reset the direct mapping. Can block */
if ((p->flags >> 20) && p->phys_addr < virt_to_phys(high_memory) - 1) {
change_page_attr(virt_to_page(__va(p->phys_addr)),
p->size >> PAGE_SHIFT,
PAGE_KERNEL);
global_flush_tlb();
}
-out_unlock:
- write_unlock(&vmlist_lock);
+
+ /* Finally remove it */
+ o = remove_vm_area((void *)p);
+ BUG_ON(p != o || o == NULL);
kfree(p);
}
EXPORT_SYMBOL(iounmap);
Index: linux/arch/x86_64/mm/ioremap.c
===================================================================
--- linux.orig/arch/x86_64/mm/ioremap.c
+++ linux/arch/x86_64/mm/ioremap.c
@@ -247,9 +247,15 @@ void __iomem *ioremap_nocache (unsigned
return __ioremap(phys_addr, size, _PAGE_PCD);
}

+/**
+ * iounmap - Free a IO remapping
+ * @addr: virtual address from ioremap_*
+ *
+ * Caller must ensure there is only one unmapping for the same pointer.
+ */
void iounmap(volatile void __iomem *addr)
{
- struct vm_struct *p;
+ struct vm_struct *p, *o;

if (addr <= high_memory)
return;
@@ -257,12 +263,31 @@ void iounmap(volatile void __iomem *addr
addr < phys_to_virt(ISA_END_ADDRESS))
return;

- write_lock(&vmlist_lock);
- p = __remove_vm_area((void *)((unsigned long)addr & PAGE_MASK));
- if (!p)
+ addr = (volatile void *)(PAGE_MASK & (unsigned long __force)addr);
+ /* Use the vm area unlocked, assuming the caller
+ ensures there isn't another iounmap for the same address
+ in parallel. Reuse of the virtual address is prevented by
+ leaving it in the global lists until we're done with it.
+ cpa takes care of the direct mappings. */
+ read_lock(&vmlist_lock);
+ for (p = vmlist; p; p = p->next) {
+ if (p->addr == addr)
+ break;
+ }
+ read_unlock(&vmlist_lock);
+
+ if (!p) {
printk("iounmap: bad address %p\n", addr);
- else if (p->flags >> 20)
+ dump_stack();
+ return;
+ }
+
+ /* Reset the direct mapping. Can block */
+ if (p->flags >> 20)
ioremap_change_attr(p->phys_addr, p->size, 0);
- write_unlock(&vmlist_lock);
+
+ /* Finally remove it */
+ o = remove_vm_area((void *)addr);
+ BUG_ON(p != o || o == NULL);
kfree(p);
}

2005-12-04 13:11:52

by Carlos Martín Nieto

[permalink] [raw]
Subject: Re: Debug: sleeping function called in atomic context II

On Sunday 04 December 2005 00:21, Andi Kleen wrote:
>
> Please use this patch instead. The previous one had a stupid bug.
OK, tested x86_64 and it doesn't give any errors.

What is this diff against? patch kept failing, so I had to patch manually.

--
Carlos Mart?n http://www.cmartin.tk