Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

[Andrew Morton]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> -------- Forwarded Message --------
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> To: Johannes Berg <johannes@sipsolutions.net>
> Cc: linuxppc-dev list <linuxppc-dev@ozlabs.org>, Michael Hanselmann
> <linux-kernel@hansmi.ch>, cpufreq@lists.linux.org.uk
> Subject: Re: via-pmu runs device_power_down in atomic context
> Date: Thu, 25 May 2006 12:28:15 +1000
> 
> On Wed, 2006-05-24 at 10:01 +0200, Johannes Berg wrote:
> > Hey,
> > 
> > Everytime I suspend my powerbook, I see the following trace:
> > 
> > [10655.887546] BUG: sleeping function called from invalid context at include/linux/rwsem.h:43
> > [10655.887558] in_atomic():0, irqs_disabled():1
> > [10655.887562] Call Trace:
> > [10655.887565] [C581BD20] [C00081E8] show_stack+0x50/0x190 (unreliable)
> > [10655.887582] [C581BD50] [C0023BB0] __might_sleep+0xcc/0xe8
> > [10655.887592] [C581BD60] [C0038290] blocking_notifier_call_chain+0x2c/0xc0
> > [10655.887606] [C581BD80] [C01E90C0] cpufreq_suspend+0x130/0x148
> > [10655.887616] [C581BDB0] [C019D9E8] sysdev_suspend+0x10c/0x300
> > [10655.887627] [C581BDF0] [C01A3888] device_power_down+0x74/0xac
> > [10655.887636] [C581BE10] [C01B1264] pmac_suspend_devices+0x98/0x188
> > [10655.887643] [C581BE30] [C01B18F0] pmu_ioctl+0x59c/0xbc0
> > [10655.887649] [C581BED0] [C008E898] do_ioctl+0x80/0x84
> > [10655.887660] [C581BEE0] [C008E928] vfs_ioctl+0x8c/0x48c
> > [10655.887666] [C581BF10] [C008ED68] sys_ioctl+0x40/0x74
> > [10655.887673] [C581BF40] [C000F3A4] ret_from_syscall+0x0/0x38
> > 
> > The might_sleep() comes from down_read() and this happens because
> > blocking_notifier_call_chain calls it, it is also commented to run in
> > process context so this is all proper.
> 
> device_power_down should be called with interrupts off, thus the PMU
> driver is fine. It's a misnamed function, it calls the sysdev's suspend
> and those should be called with irq off. I think the problem is more due
> to some cpufreq or notifier change that somebody done to recent kernels
> and that added some might_sleep.... I wonder why. 
> 
> Andrew, what's up there ? What is this new
> "blocking_notifier_call_chain" thing ? notifiers use to not use
> semaphores and not be blocking... at least powermac implementation of
> cpufreq relies on that.

notifiers used to be racy too - we just waddled across them without any
locking.

Alan made a best-effort conversion of callers, and there have been a few
problems.

Here, pmac has gone and unilaterally decided that device_power_down() is
atomic, even though device_power_down() _already_ calls suspend_device(),
which does down().  So I'd say you've gone and found a via-pmu bug here.

A way of shutting up the warning would be to use an atomic notifier, but
it'll still be buggy.  Better would be to teach pmac_suspend_devices() not
to assume things which aren't true ;)

Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

[Benjamin Herrenschmidt]

> Here, pmac has gone and unilaterally decided that device_power_down() is
> atomic, even though device_power_down() _already_ calls suspend_device(),
> which does down().  So I'd say you've gone and found a via-pmu bug here.

No. Look at the implementation (and the comment) in device_power_down().
It's designed to be called with irqs off...  Of course, somebody changed
the locking in there and it's indeed ending up calling suspend_device()
for devices on the irq_off list which calls down ... bad bad... that's
another bug in the drivers/power/* to add to an already long list.
Fortunately, very few (if any) devics rely on this irq_off list. But
sysdev's do. 

> A way of shutting up the warning would be to use an atomic notifier, but
> it'll still be buggy.  Better would be to teach pmac_suspend_devices() not
> to assume things which aren't true ;)

No. If we call device_power_down with interrupts enabled, very bad
things will happen. This powermac code is very carefully crafted to do
things in a strict order and it's along those lines that the callbacks
in the device model were initially defined. Now, people who don't
understand shit about how to make power management reliable may have
broken things around, but the powermac implementation is right there.

Ben.

Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

[Andrew Morton]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> 
> > Here, pmac has gone and unilaterally decided that device_power_down() is
> > atomic, even though device_power_down() _already_ calls suspend_device(),
> > which does down().  So I'd say you've gone and found a via-pmu bug here.
> 
> No. Look at the implementation (and the comment) in device_power_down().
> It's designed to be called with irqs off...  Of course, somebody changed
> the locking in there and it's indeed ending up calling suspend_device()
> for devices on the irq_off list which calls down ... bad bad... that's
> another bug in the drivers/power/* to add to an already long list.
> Fortunately, very few (if any) devics rely on this irq_off list. But
> sysdev's do. 

uh-huh.

> > A way of shutting up the warning would be to use an atomic notifier, but
> > it'll still be buggy.  Better would be to teach pmac_suspend_devices() not
> > to assume things which aren't true ;)
> 
> No. If we call device_power_down with interrupts enabled, very bad
> things will happen. This powermac code is very carefully crafted to do
> things in a strict order and it's along those lines that the callbacks
> in the device model were initially defined. Now, people who don't
> understand shit about how to make power management reliable may have
> broken things around, but the powermac implementation is right there.
> 

This requirement to keep interrupts off in there breaks things again and
again and again and again.  And this time: again.

It looks like we (again) have to live with it in which case conversion to
an atomic notifier is probably needed.  That may break other things though.
Alan would know.

Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

[Benjamin Herrenschmidt]

> No. If we call device_power_down with interrupts enabled, very bad
> things will happen. This powermac code is very carefully crafted to do
> things in a strict order and it's along those lines that the callbacks
> in the device model were initially defined. Now, people who don't
> understand shit about how to make power management reliable may have
> broken things around, but the powermac implementation is right there.

To be more precise, sysdev suspend is supposed to happen with irq offs
(it's specifically designed to handle legacy things, interrupt
controllers, ec...). Thus cpufreq suspend/resume need to assume that
it's being called in that context or be made something else than a
sysdev...

Regarding the possible down() if we walk through the irq_off device
list, well, that's indeed annoying, we probably need to pass a "no_lock"
argument. Never hit that one since as I told you, there are really few
if not no drivers using this facility of deferring suspend to after
interrupts are disabled. Also, the down there is harmless as in no
normal circumstances should it ever turn into a schedule() (there should
be no contention possible that late in the suspend process).

Ben.

Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

[Benjamin Herrenschmidt]

> This requirement to keep interrupts off in there breaks things again and
> again and again and again.  And this time: again.

Where else ?

If you look at this function it's _designed_ for interrupts off ! the
driver suspend have the option of deferring their suspend() callback
until after irqs have been turned off (needed for legacy stuff afaik but
rarely used) and the sysdev's are very low level things whose suspend
and resume callbacks should be called after that point as well. 

Ben.

Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

[Benjamin Herrenschmidt]

On Thu, 2006-05-25 at 15:46 +1000, Benjamin Herrenschmidt wrote:
> > This requirement to keep interrupts off in there breaks things again and
> > again and again and again.  And this time: again.
> 
> Where else ?
> 
> If you look at this function it's _designed_ for interrupts off ! the
> driver suspend have the option of deferring their suspend() callback
> until after irqs have been turned off (needed for legacy stuff afaik but
> rarely used) and the sysdev's are very low level things whose suspend
> and resume callbacks should be called after that point as well. 

BTW. The root of the problem with cpufreq is it's upside down design :)
Well, not all of it, but the fact that it registers a sysdev. It's not
the "midlayer" (ugh) business to register devices and hook things like
PM events to pass them down to actual drivers. It should be the cpufreq
drivers themselves that attach to the device model in a way or another
and "instanciate" the ability to control the cpu frequency, passing
along their struct device.

The driver itself should pick the right type of "device" to attach to
(sysdev's are just weird beast and were a bad idea in the first place
since they aren't even struct device).

But then, iirc, that's because we also did the cpu stuff in sysfs upside
down too ... :)

Now, wether the cpufreq notifier might end up calling things that will
sleep or not ... hrm... that's an interesting issue. Part of the problem
is that because cpufreq is a sysdev, it will be called so late in the
suspend process, pretty much everything else is asleep. So I'm quite
confident that things attaching to the cpufreq notifiers other than bits
of cpufreq itself are likely to break anyway. Is it documented anywhere
that registering a cpufreq notifier might cause it to be called in
atomic context or very later in the suspend process (possibly after the
interrupt controller hs been put down) ?

Yes it's messy, no I don't have a miracle solution, but I think here the
proper way to fix it in the long run is for cpufreq not to be a sysdev
or anything like that and to stop trying to do the suspend/resume thing
for the drivers. Drivers are in charge, they get to create the device of
whatever type it is and get the suspend/resume events whenever they are
sent. cpufreq can then provide maybe "helpers" to help work out what to
do at suspend and/or resume time but with the knowledge that for some
drivers maybe, this will happen in atomic context.

Ben.

Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

[Alan Stern]

On Wed, 24 May 2006, Andrew Morton wrote:

> > device_power_down should be called with interrupts off, thus the PMU
> > driver is fine. It's a misnamed function, it calls the sysdev's suspend
> > and those should be called with irq off. I think the problem is more due
> > to some cpufreq or notifier change that somebody done to recent kernels
> > and that added some might_sleep.... I wonder why. 
> > 
> > Andrew, what's up there ? What is this new
> > "blocking_notifier_call_chain" thing ? notifiers use to not use
> > semaphores and not be blocking... at least powermac implementation of
> > cpufreq relies on that.
> 
> notifiers used to be racy too - we just waddled across them without any
> locking.
> 
> Alan made a best-effort conversion of callers, and there have been a few
> problems.
> 
> Here, pmac has gone and unilaterally decided that device_power_down() is
> atomic, even though device_power_down() _already_ calls suspend_device(),
> which does down().  So I'd say you've gone and found a via-pmu bug here.
> 
> A way of shutting up the warning would be to use an atomic notifier, but
> it'll still be buggy.  Better would be to teach pmac_suspend_devices() not
> to assume things which aren't true ;)

Someone else had a problem with the cpufreq conversion earlier.  I posted 
a message on the cpufreq mailing list but nobody ever responded to it.  
This may or may not be related to that earlier problem.

In essence, the problem seemed to be that the cpufreq notifier chain is
sometimes expected to be blocking and sometimes expected to be atomic,
based on the "val" code passed to notifier_call_chain.  The cleanest
solution would be to split the single notifier chain into two chains,
one always blocking and the other always atomic.

Somebody who knows more about cpufreq than I do will have to make that
change.

Alan Stern

Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

[Andrew Morton]

Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, 24 May 2006, Andrew Morton wrote:
> 
> > > device_power_down should be called with interrupts off, thus the PMU
> > > driver is fine. It's a misnamed function, it calls the sysdev's suspend
> > > and those should be called with irq off. I think the problem is more due
> > > to some cpufreq or notifier change that somebody done to recent kernels
> > > and that added some might_sleep.... I wonder why. 
> > > 
> > > Andrew, what's up there ? What is this new
> > > "blocking_notifier_call_chain" thing ? notifiers use to not use
> > > semaphores and not be blocking... at least powermac implementation of
> > > cpufreq relies on that.
> > 
> > notifiers used to be racy too - we just waddled across them without any
> > locking.
> > 
> > Alan made a best-effort conversion of callers, and there have been a few
> > problems.
> > 
> > Here, pmac has gone and unilaterally decided that device_power_down() is
> > atomic, even though device_power_down() _already_ calls suspend_device(),
> > which does down().  So I'd say you've gone and found a via-pmu bug here.
> > 
> > A way of shutting up the warning would be to use an atomic notifier, but
> > it'll still be buggy.  Better would be to teach pmac_suspend_devices() not
> > to assume things which aren't true ;)
> 
> Someone else had a problem with the cpufreq conversion earlier.  I posted 
> a message on the cpufreq mailing list but nobody ever responded to it.  
> This may or may not be related to that earlier problem.
> 
> In essence, the problem seemed to be that the cpufreq notifier chain is
> sometimes expected to be blocking and sometimes expected to be atomic,
> based on the "val" code passed to notifier_call_chain.  The cleanest
> solution would be to split the single notifier chain into two chains,
> one always blocking and the other always atomic.
> 
> Somebody who knows more about cpufreq than I do will have to make that
> change.
> 

I wouldn't describe the cpufreq project as a teeming hive of frenetic
activity, and we need something pronto.

We could go back to a raw_notifier and be as buggy as we used to be.

Re: [Fwd: Re: via-pmu runs device_power_down in atomic context]

[Jon Loeliger]

On Thu, 2006-05-25 at 09:44, Andrew Morton wrote:

> > In essence, the problem seemed to be that the cpufreq notifier chain is
> > sometimes expected to be blocking and sometimes expected to be atomic,
> > based on the "val" code passed to notifier_call_chain.  The cleanest
> > solution would be to split the single notifier chain into two chains,
> > one always blocking and the other always atomic.
> > 
> > Somebody who knows more about cpufreq than I do will have to make that
> > change.
> > 
> 
> I wouldn't describe the cpufreq project as a teeming hive of frenetic
> activity, and we need something pronto.
> 
> We could go back to a raw_notifier and be as buggy as we used to be.

It _is_ actively being pursued today for a cleaned up
implementation.  If there are issues or requirements
here, we should really pass them on to the linux-pm list.

Thanks,
jdl

[PATCH] Make cpufreq_transition_notifier a raw notifier

[Alan Stern]

The cpufreq code has problems with atomic vs. blocking notifier calls and
enabling vs. disabling interrupts (show up on pmac).  As a temporary
band-aid, this patch (as697) makes the cpufreq_transition_notifier_list
into a raw notifier.


Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

On Thu, 25 May 2006, Jon Loeliger wrote:

> On Thu, 2006-05-25 at 09:44, Andrew Morton wrote:
> 
> > > In essence, the problem seemed to be that the cpufreq notifier chain is
> > > sometimes expected to be blocking and sometimes expected to be atomic,
> > > based on the "val" code passed to notifier_call_chain.  The cleanest
> > > solution would be to split the single notifier chain into two chains,
> > > one always blocking and the other always atomic.
> > > 
> > > Somebody who knows more about cpufreq than I do will have to make that
> > > change.
> > > 
> > 
> > I wouldn't describe the cpufreq project as a teeming hive of frenetic
> > activity, and we need something pronto.
> > 
> > We could go back to a raw_notifier and be as buggy as we used to be.
> 
> It _is_ actively being pursued today for a cleaned up
> implementation.  If there are issues or requirements
> here, we should really pass them on to the linux-pm list.
> 
> Thanks,
> jdl

Here's the patch Andrew asked for.  I have no idea whether it will solve 
any of these problems, and I'm certain that in the long run we shouldn't 
keep it.  Perhaps it will at least help stabilize 2.6.17.

It's up to you guys to see if the patch helps at all and to decide whether 
or not it should be applied.

Alan Stern


Index: usb-2.6/drivers/cpufreq/cpufreq.c
===================================================================
--- usb-2.6.orig/drivers/cpufreq/cpufreq.c
+++ usb-2.6/drivers/cpufreq/cpufreq.c
@@ -50,10 +50,9 @@ static void handle_update(void *data);
  * validation process for a new CPU frequency policy; the
  * "transition" list for kernel code that needs to handle
  * changes to devices when the CPU clock speed changes.
- * The mutex locks both lists.
  */
 static BLOCKING_NOTIFIER_HEAD(cpufreq_policy_notifier_list);
-static BLOCKING_NOTIFIER_HEAD(cpufreq_transition_notifier_list);
+static RAW_NOTIFIER_HEAD(cpufreq_transition_notifier_list);
 
 
 static LIST_HEAD(cpufreq_governor_list);
@@ -263,14 +262,14 @@ void cpufreq_notify_transition(struct cp
 				freqs->old = policy->cur;
 			}
 		}
-		blocking_notifier_call_chain(&cpufreq_transition_notifier_list,
+		raw_notifier_call_chain(&cpufreq_transition_notifier_list,
 				CPUFREQ_PRECHANGE, freqs);
 		adjust_jiffies(CPUFREQ_PRECHANGE, freqs);
 		break;
 
 	case CPUFREQ_POSTCHANGE:
 		adjust_jiffies(CPUFREQ_POSTCHANGE, freqs);
-		blocking_notifier_call_chain(&cpufreq_transition_notifier_list,
+		raw_notifier_call_chain(&cpufreq_transition_notifier_list,
 				CPUFREQ_POSTCHANGE, freqs);
 		if (likely(policy) && likely(policy->cpu == freqs->cpu))
 			policy->cur = freqs->new;
@@ -1014,7 +1013,7 @@ static int cpufreq_suspend(struct sys_de
 		freqs.old = cpu_policy->cur;
 		freqs.new = cur_freq;
 
-		blocking_notifier_call_chain(&cpufreq_transition_notifier_list,
+		raw_notifier_call_chain(&cpufreq_transition_notifier_list,
 				    CPUFREQ_SUSPENDCHANGE, &freqs);
 		adjust_jiffies(CPUFREQ_SUSPENDCHANGE, &freqs);
 
@@ -1095,7 +1094,7 @@ static int cpufreq_resume(struct sys_dev
 			freqs.old = cpu_policy->cur;
 			freqs.new = cur_freq;
 
-			blocking_notifier_call_chain(
+			raw_notifier_call_chain(
 					&cpufreq_transition_notifier_list,
 					CPUFREQ_RESUMECHANGE, &freqs);
 			adjust_jiffies(CPUFREQ_RESUMECHANGE, &freqs);
@@ -1141,7 +1140,7 @@ int cpufreq_register_notifier(struct not
 
 	switch (list) {
 	case CPUFREQ_TRANSITION_NOTIFIER:
-		ret = blocking_notifier_chain_register(
+		ret = raw_notifier_chain_register(
 				&cpufreq_transition_notifier_list, nb);
 		break;
 	case CPUFREQ_POLICY_NOTIFIER:
@@ -1173,7 +1172,7 @@ int cpufreq_unregister_notifier(struct n
 
 	switch (list) {
 	case CPUFREQ_TRANSITION_NOTIFIER:
-		ret = blocking_notifier_chain_unregister(
+		ret = raw_notifier_chain_unregister(
 				&cpufreq_transition_notifier_list, nb);
 		break;
 	case CPUFREQ_POLICY_NOTIFIER:

Re: [PATCH] Make cpufreq_transition_notifier a raw notifier

[Johannes Berg]

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

On Thu, 2006-05-25 at 13:53 -0400, Alan Stern wrote:

> Here's the patch Andrew asked for.  I have no idea whether it will solve 
> any of these problems, and I'm certain that in the long run we shouldn't 
> keep it.  Perhaps it will at least help stabilize 2.6.17.
> 
> It's up to you guys to see if the patch helps at all and to decide whether 
> or not it should be applied.

Maybe I should note that I don't actually have seen any problems because
of this, the kernel just warns about it. I guess that's because the
semaphore won't ever actually be contended at that point.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: [PATCH] Make cpufreq_transition_notifier a raw notifier

[Alan Stern]

On Thu, 25 May 2006, Johannes Berg wrote:

> On Thu, 2006-05-25 at 13:53 -0400, Alan Stern wrote:
> 
> > Here's the patch Andrew asked for.  I have no idea whether it will solve 
> > any of these problems, and I'm certain that in the long run we shouldn't 
> > keep it.  Perhaps it will at least help stabilize 2.6.17.
> > 
> > It's up to you guys to see if the patch helps at all and to decide whether 
> > or not it should be applied.
> 
> Maybe I should note that I don't actually have seen any problems because
> of this, the kernel just warns about it. I guess that's because the
> semaphore won't ever actually be contended at that point.

The problem isn't contention for the semaphore.  The problem is that 
down_read() will enable interrupts even when the semaphore is not in 
contention, but parts of the cpufreq notifier chain need to execute with 
interrupts disabled.

Alan Stern

Re: [PATCH] Make cpufreq_transition_notifier a raw notifier

[Benjamin Herrenschmidt]

On Thu, 2006-05-25 at 13:53 -0400, Alan Stern wrote:
> The cpufreq code has problems with atomic vs. blocking notifier calls and
> enabling vs. disabling interrupts (show up on pmac).  As a temporary
> band-aid, this patch (as697) makes the cpufreq_transition_notifier_list
> into a raw notifier.

Thanks Alan. That should fix our warning for now, though we'll have to
sync with brodo to get cpufreq right. As I explained separately, I think
part of the problem is bcs cpufreq is a sysdev, it shouldn't have to be,
at least not necessarily. I'm worried that even if they work in atomic
context, some drivers who hook on the cpufreq events might still be
buggy when notified so late in the sleep process.