* pmf_register_irq_client gives sleep with locks held warning @ 2006-05-30 20:19 Johannes Berg 2006-06-01 0:59 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 9+ messages in thread From: Johannes Berg @ 2006-05-30 20:19 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: linuxppc-dev list [-- Attachment #1: Type: text/plain, Size: 1167 bytes --] Hi, When testing headphone detection stuff I got this: [ 634.218762] BUG: sleeping function called from invalid context at mm/slab.c:2794 [ 634.218774] in_atomic():0, irqs_disabled():1 [ 634.218777] Call Trace: [ 634.218779] [D67ADC00] [C00085F8] show_stack+0x50/0x190 (unreliable) [ 634.218794] [D67ADC30] [C0024030] __might_sleep+0xcc/0xe8 [ 634.218804] [D67ADC40] [C0076C04] __kmalloc+0xf4/0xf8 [ 634.218815] [D67ADC60] [C00B37D4] proc_create+0x9c/0xe8 [ 634.218827] [D67ADC90] [C00B3930] proc_mkdir_mode+0x2c/0x88 [ 634.218833] [D67ADCB0] [C005227C] register_handler_proc+0xe4/0xfc [ 634.218844] [D67ADD50] [C0051D70] setup_irq+0x118/0x16c [ 634.218850] [D67ADD70] [C0051E68] request_irq+0xa4/0xb8 [ 634.218855] [D67ADDA0] [C0020BA0] macio_do_gpio_irq_enable+0x40/0x50 [ 634.218860] [D67ADDB0] [C00208BC] pmf_register_irq_client+0x88/0x9c [..., not important] The problem obviously is that request_irq via setup_irq and ... may sleep via kmalloc, while pmf_register_irq_client spin_lock_irqsave()s around the call. The solution is probably to use spin_lock() instead of spin_lock_irqsave() here. Comments? johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 793 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: pmf_register_irq_client gives sleep with locks held warning 2006-05-30 20:19 pmf_register_irq_client gives sleep with locks held warning Johannes Berg @ 2006-06-01 0:59 ` Benjamin Herrenschmidt 2006-06-08 18:58 ` Johannes Berg 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Herrenschmidt @ 2006-06-01 0:59 UTC (permalink / raw) To: Johannes Berg; +Cc: Andrew Morton, linuxppc-dev list On Tue, 2006-05-30 at 22:19 +0200, Johannes Berg wrote: > Hi, > > When testing headphone detection stuff I got this: > > [ 634.218762] BUG: sleeping function called from invalid context at mm/slab.c:2794 > [ 634.218774] in_atomic():0, irqs_disabled():1 > [ 634.218777] Call Trace: > [ 634.218779] [D67ADC00] [C00085F8] show_stack+0x50/0x190 (unreliable) > [ 634.218794] [D67ADC30] [C0024030] __might_sleep+0xcc/0xe8 > [ 634.218804] [D67ADC40] [C0076C04] __kmalloc+0xf4/0xf8 > [ 634.218815] [D67ADC60] [C00B37D4] proc_create+0x9c/0xe8 > [ 634.218827] [D67ADC90] [C00B3930] proc_mkdir_mode+0x2c/0x88 > [ 634.218833] [D67ADCB0] [C005227C] register_handler_proc+0xe4/0xfc > [ 634.218844] [D67ADD50] [C0051D70] setup_irq+0x118/0x16c > [ 634.218850] [D67ADD70] [C0051E68] request_irq+0xa4/0xb8 > [ 634.218855] [D67ADDA0] [C0020BA0] macio_do_gpio_irq_enable+0x40/0x50 > [ 634.218860] [D67ADDB0] [C00208BC] pmf_register_irq_client+0x88/0x9c > [..., not important] That's a bug in the PowerMac PMF code. What about this patch ? This fixes request_irq() potentially called from atomic context. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Index: linux-work/arch/powerpc/platforms/powermac/pfunc_core.c =================================================================== --- linux-work.orig/arch/powerpc/platforms/powermac/pfunc_core.c 2006-04-19 15:04:38.000000000 +1000 +++ linux-work/arch/powerpc/platforms/powermac/pfunc_core.c 2006-06-01 10:58:57.000000000 +1000 @@ -11,6 +11,7 @@ #include <linux/kernel.h> #include <linux/spinlock.h> #include <linux/module.h> +#include <linux/mutex.h> #include <asm/semaphore.h> #include <asm/prom.h> @@ -546,6 +547,7 @@ static LIST_HEAD(pmf_devices); static spinlock_t pmf_lock = SPIN_LOCK_UNLOCKED; +static DEFINE_MUTEX(pmf_irq_mutex); static void pmf_release_device(struct kref *kref) { @@ -864,15 +866,17 @@ spin_lock_irqsave(&pmf_lock, flags); func = __pmf_find_function(target, name, PMF_FLAGS_INT_GEN); - if (func == NULL) { - spin_unlock_irqrestore(&pmf_lock, flags); + if (func) + func = pmf_get_function(func); + spin_unlock_irqrestore(&pmf_lock, flags); + if (func == NULL) return -ENODEV; - } + mutex_lock(&pmf_irq_mutex); if (list_empty(&func->irq_clients)) func->dev->handlers->irq_enable(func); list_add(&client->link, &func->irq_clients); client->func = func; - spin_unlock_irqrestore(&pmf_lock, flags); + mutex_unlock(&pmf_irq_mutex); return 0; } @@ -881,16 +885,16 @@ void pmf_unregister_irq_client(struct pmf_irq_client *client) { struct pmf_function *func = client->func; - unsigned long flags; BUG_ON(func == NULL); - spin_lock_irqsave(&pmf_lock, flags); + mutex_lock(&pmf_irq_mutex); client->func = NULL; list_del(&client->link); if (list_empty(&func->irq_clients)) func->dev->handlers->irq_disable(func); - spin_unlock_irqrestore(&pmf_lock, flags); + mutex_unlock(&pmf_irq_mutex); + pmf_put_function(func); } EXPORT_SYMBOL_GPL(pmf_unregister_irq_client); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: pmf_register_irq_client gives sleep with locks held warning 2006-06-01 0:59 ` Benjamin Herrenschmidt @ 2006-06-08 18:58 ` Johannes Berg 2006-06-10 0:03 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 9+ messages in thread From: Johannes Berg @ 2006-06-08 18:58 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Andrew Morton, linuxppc-dev list On Thu, 2006-06-01 at 10:59 +1000, Benjamin Herrenschmidt wrote: > This fixes request_irq() potentially called from atomic context. > [...] Sorry, almost forgot about this. I don't think your patch is right, it seems to me that now pmf_unregister_irq_client races against pmf_do_irq: what happens when an interrupt comes in right in the middle of the list_del()? It might actually not do any harm due to the way list_del() and pmf_do_irq() work and the fact that client->handler() must still be prepared to be called... But I'd rather make it explicit. By the way, what do I do when like this I change a patch? Do I rely on your Signed-off-by and simply put mine instead (like I did now)? Anyway, here's a changed patch that illustrates my point. I've tested it and it works and as expected fixes the problem :) --- This fixes request_irq() being called with interrupts disabled in the powermac platform function code when registering the first irq client for a given platform function. Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> Signed-off-by: Johannes Berg <johannes@sipsolutions.net> --- a/arch/powerpc/platforms/powermac/pfunc_core.c +++ b/arch/powerpc/platforms/powermac/pfunc_core.c @@ -11,6 +11,7 @@ #include <linux/delay.h> #include <linux/kernel.h> #include <linux/spinlock.h> #include <linux/module.h> +#include <linux/mutex.h> #include <asm/semaphore.h> #include <asm/prom.h> @@ -546,6 +547,7 @@ struct pmf_device { static LIST_HEAD(pmf_devices); static spinlock_t pmf_lock = SPIN_LOCK_UNLOCKED; +static DEFINE_MUTEX(pmf_irq_mutex); static void pmf_release_device(struct kref *kref) { @@ -864,16 +866,25 @@ int pmf_register_irq_client(struct devic spin_lock_irqsave(&pmf_lock, flags); func = __pmf_find_function(target, name, PMF_FLAGS_INT_GEN); - if (func == NULL) { - spin_unlock_irqrestore(&pmf_lock, flags); + if (func) + func = pmf_get_function(func); + spin_unlock_irqrestore(&pmf_lock, flags); + if (func == NULL) return -ENODEV; - } + + /* guard against manipulations of list */ + mutex_lock(&pmf_irq_mutex); if (list_empty(&func->irq_clients)) func->dev->handlers->irq_enable(func); + + /* guard against pmf_do_irq while changing list */ + spin_lock_irqsave(&pmf_lock, flags); list_add(&client->link, &func->irq_clients); - client->func = func; spin_unlock_irqrestore(&pmf_lock, flags); + client->func = func; + mutex_unlock(&pmf_irq_mutex); + return 0; } EXPORT_SYMBOL_GPL(pmf_register_irq_client); @@ -885,12 +896,19 @@ void pmf_unregister_irq_client(struct pm BUG_ON(func == NULL); - spin_lock_irqsave(&pmf_lock, flags); + /* guard against manipulations of list */ + mutex_lock(&pmf_irq_mutex); client->func = NULL; + + /* guard against pmf_do_irq while changing list */ + spin_lock_irqsave(&pmf_lock, flags); list_del(&client->link); + spin_unlock_irqrestore(&pmf_lock, flags); + if (list_empty(&func->irq_clients)) func->dev->handlers->irq_disable(func); - spin_unlock_irqrestore(&pmf_lock, flags); + mutex_unlock(&pmf_irq_mutex); + pmf_put_function(func); } EXPORT_SYMBOL_GPL(pmf_unregister_irq_client); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: pmf_register_irq_client gives sleep with locks held warning 2006-06-08 18:58 ` Johannes Berg @ 2006-06-10 0:03 ` Benjamin Herrenschmidt 2006-06-10 9:27 ` Johannes Berg 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Herrenschmidt @ 2006-06-10 0:03 UTC (permalink / raw) To: Johannes Berg; +Cc: Andrew Morton, linuxppc-dev list On Thu, 2006-06-08 at 20:58 +0200, Johannes Berg wrote: > On Thu, 2006-06-01 at 10:59 +1000, Benjamin Herrenschmidt wrote: > > > This fixes request_irq() potentially called from atomic context. > > [...] > > Sorry, almost forgot about this. > > I don't think your patch is right, it seems to me that now > pmf_unregister_irq_client races against pmf_do_irq: what happens when an > interrupt comes in right in the middle of the list_del()? Yeah, possibly... too late for 2.6.17 tho. > By the way, what do I do when like this I change a patch? Do I rely on > your Signed-off-by and simply put mine instead (like I did now)? You add yours. > Anyway, here's a changed patch that illustrates my point. I've tested it > and it works and as expected fixes the problem :) Too late :) Cheers, Ben. > --- > This fixes request_irq() being called with interrupts disabled in the > powermac platform function code when registering the first irq client > for a given platform function. > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net> > > --- a/arch/powerpc/platforms/powermac/pfunc_core.c > +++ b/arch/powerpc/platforms/powermac/pfunc_core.c > @@ -11,6 +11,7 @@ #include <linux/delay.h> > #include <linux/kernel.h> > #include <linux/spinlock.h> > #include <linux/module.h> > +#include <linux/mutex.h> > > #include <asm/semaphore.h> > #include <asm/prom.h> > @@ -546,6 +547,7 @@ struct pmf_device { > > static LIST_HEAD(pmf_devices); > static spinlock_t pmf_lock = SPIN_LOCK_UNLOCKED; > +static DEFINE_MUTEX(pmf_irq_mutex); > > static void pmf_release_device(struct kref *kref) > { > @@ -864,16 +866,25 @@ int pmf_register_irq_client(struct devic > > spin_lock_irqsave(&pmf_lock, flags); > func = __pmf_find_function(target, name, PMF_FLAGS_INT_GEN); > - if (func == NULL) { > - spin_unlock_irqrestore(&pmf_lock, flags); > + if (func) > + func = pmf_get_function(func); > + spin_unlock_irqrestore(&pmf_lock, flags); > + if (func == NULL) > return -ENODEV; > - } > + > + /* guard against manipulations of list */ > + mutex_lock(&pmf_irq_mutex); > if (list_empty(&func->irq_clients)) > func->dev->handlers->irq_enable(func); > + > + /* guard against pmf_do_irq while changing list */ > + spin_lock_irqsave(&pmf_lock, flags); > list_add(&client->link, &func->irq_clients); > - client->func = func; > spin_unlock_irqrestore(&pmf_lock, flags); > > + client->func = func; > + mutex_unlock(&pmf_irq_mutex); > + > return 0; > } > EXPORT_SYMBOL_GPL(pmf_register_irq_client); > @@ -885,12 +896,19 @@ void pmf_unregister_irq_client(struct pm > > BUG_ON(func == NULL); > > - spin_lock_irqsave(&pmf_lock, flags); > + /* guard against manipulations of list */ > + mutex_lock(&pmf_irq_mutex); > client->func = NULL; > + > + /* guard against pmf_do_irq while changing list */ > + spin_lock_irqsave(&pmf_lock, flags); > list_del(&client->link); > + spin_unlock_irqrestore(&pmf_lock, flags); > + > if (list_empty(&func->irq_clients)) > func->dev->handlers->irq_disable(func); > - spin_unlock_irqrestore(&pmf_lock, flags); > + mutex_unlock(&pmf_irq_mutex); > + pmf_put_function(func); > } > EXPORT_SYMBOL_GPL(pmf_unregister_irq_client); > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: pmf_register_irq_client gives sleep with locks held warning 2006-06-10 0:03 ` Benjamin Herrenschmidt @ 2006-06-10 9:27 ` Johannes Berg 2006-06-10 22:04 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 9+ messages in thread From: Johannes Berg @ 2006-06-10 9:27 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Andrew Morton, linuxppc-dev list [-- Attachment #1: Type: text/plain, Size: 526 bytes --] On Sat, 2006-06-10 at 10:03 +1000, Benjamin Herrenschmidt wrote: > > I don't think your patch is right, it seems to me that now > > pmf_unregister_irq_client races against pmf_do_irq: what happens when an > > interrupt comes in right in the middle of the list_del()? > > Yeah, possibly... too late for 2.6.17 tho. That's ok, we don't have any in-kernel users anyway. Alas the alsa people will be dissatisfied because they like to ship new drivers for old kernels or something. Oh well, I don't care. johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 793 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: pmf_register_irq_client gives sleep with locks held warning 2006-06-10 9:27 ` Johannes Berg @ 2006-06-10 22:04 ` Benjamin Herrenschmidt 2006-06-13 8:43 ` Johannes Berg 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Herrenschmidt @ 2006-06-10 22:04 UTC (permalink / raw) To: Johannes Berg; +Cc: Andrew Morton, linuxppc-dev list On Sat, 2006-06-10 at 11:27 +0200, Johannes Berg wrote: > On Sat, 2006-06-10 at 10:03 +1000, Benjamin Herrenschmidt wrote: > > > > I don't think your patch is right, it seems to me that now > > > pmf_unregister_irq_client races against pmf_do_irq: what happens when an > > > interrupt comes in right in the middle of the list_del()? > > > > Yeah, possibly... too late for 2.6.17 tho. > > That's ok, we don't have any in-kernel users anyway. Alas the alsa > people will be dissatisfied because they like to ship new drivers for > old kernels or something. Oh well, I don't care. We still want people to build out-of-tree for 2.6.17 (please keep the git there for that), as it will take a while before 2.6.18 is here. The remaining possible bug in the pmf irq code is probably harmless in 99.99% of the cases in practice :) Ben. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: pmf_register_irq_client gives sleep with locks held warning 2006-06-10 22:04 ` Benjamin Herrenschmidt @ 2006-06-13 8:43 ` Johannes Berg 2006-06-13 9:58 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 9+ messages in thread From: Johannes Berg @ 2006-06-13 8:43 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Andrew Morton, linuxppc-dev list [-- Attachment #1: Type: text/plain, Size: 705 bytes --] On Sun, 2006-06-11 at 08:04 +1000, Benjamin Herrenschmidt wrote: > > That's ok, we don't have any in-kernel users anyway. Alas the alsa > > people will be dissatisfied because they like to ship new drivers for > > old kernels or something. Oh well, I don't care. > > We still want people to build out-of-tree for 2.6.17 (please keep the > git there for that), as it will take a while before 2.6.18 is here. Sure, I'll continue maintaining snd-aoa in that repository. > The remaining possible bug in the pmf irq code is probably harmless in > 99.99% of the cases in practice :) I see your patch is in, I'll make a patch that fixes it then instead of replacing it, I guess. johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 793 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: pmf_register_irq_client gives sleep with locks held warning 2006-06-13 8:43 ` Johannes Berg @ 2006-06-13 9:58 ` Benjamin Herrenschmidt 2006-06-13 15:43 ` [PATCH] make pmf irq_client functions safe against pmf interrupts coming in Johannes Berg 0 siblings, 1 reply; 9+ messages in thread From: Benjamin Herrenschmidt @ 2006-06-13 9:58 UTC (permalink / raw) To: Johannes Berg; +Cc: Andrew Morton, linuxppc-dev list > I see your patch is in, I'll make a patch that fixes it then instead of > replacing it, I guess. Please do, thanks. Ben. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] make pmf irq_client functions safe against pmf interrupts coming in 2006-06-13 9:58 ` Benjamin Herrenschmidt @ 2006-06-13 15:43 ` Johannes Berg 0 siblings, 0 replies; 9+ messages in thread From: Johannes Berg @ 2006-06-13 15:43 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Andrew Morton, linuxppc-dev list This fixes the pmf irq_client functions to be safe against pmf interrupts coming in while a client is registered/unregistered. Signed-off-by: Johannes Berg <johannes@sipsolutions.net> --- linux-2.6-git.orig/arch/powerpc/platforms/powermac/pfunc_core.c 2006-06-13 17:37:35.791076087 +0200 +++ linux-2.6-git/arch/powerpc/platforms/powermac/pfunc_core.c 2006-06-13 17:40:16.213632271 +0200 @@ -871,10 +871,17 @@ int pmf_register_irq_client(struct devic spin_unlock_irqrestore(&pmf_lock, flags); if (func == NULL) return -ENODEV; + + /* guard against manipulations of list */ mutex_lock(&pmf_irq_mutex); if (list_empty(&func->irq_clients)) func->dev->handlers->irq_enable(func); + + /* guard against pmf_do_irq while changing list */ + spin_lock_irqsave(&pmf_lock, flags); list_add(&client->link, &func->irq_clients); + spin_unlock_irqrestore(&pmf_lock, flags); + client->func = func; mutex_unlock(&pmf_irq_mutex); @@ -885,12 +892,19 @@ EXPORT_SYMBOL_GPL(pmf_register_irq_clien void pmf_unregister_irq_client(struct pmf_irq_client *client) { struct pmf_function *func = client->func; + unsigned long flags; BUG_ON(func == NULL); + /* guard against manipulations of list */ mutex_lock(&pmf_irq_mutex); client->func = NULL; + + /* guard against pmf_do_irq while changing list */ + spin_lock_irqsave(&pmf_lock, flags); list_del(&client->link); + spin_unlock_irqrestore(&pmf_lock, flags); + if (list_empty(&func->irq_clients)) func->dev->handlers->irq_disable(func); mutex_unlock(&pmf_irq_mutex); ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2006-06-13 16:06 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-05-30 20:19 pmf_register_irq_client gives sleep with locks held warning Johannes Berg 2006-06-01 0:59 ` Benjamin Herrenschmidt 2006-06-08 18:58 ` Johannes Berg 2006-06-10 0:03 ` Benjamin Herrenschmidt 2006-06-10 9:27 ` Johannes Berg 2006-06-10 22:04 ` Benjamin Herrenschmidt 2006-06-13 8:43 ` Johannes Berg 2006-06-13 9:58 ` Benjamin Herrenschmidt 2006-06-13 15:43 ` [PATCH] make pmf irq_client functions safe against pmf interrupts coming in Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).