Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Friday, October 01, 2010, Alan Stern wrote:
> On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:
> 
> > > >  If RPM_IRQ is not set, but
> > > > power.callbacks_in_irq is set, we should fall back to the normal behavior
> > > > (ie. do not avoid sleeping).
> > > 
> > > By "do not avoid sleeping", do you mean that 
> > > 
> > > 	(1) the helper functions should be allowed to sleep, or
> > > 
> > > 	(2) the callbacks should be invoked with interrupts enabled?
> > > 
> > > The patch already does (1).  By contrast, (2) isn't safe.  It means
> > > there is a risk of having one thread do a busy-wait with interrupts
> > > disabled, waiting for another thread that can sleep.
> > 
> > In my opinion the _irq operations should really be one-shot, without
> > any looping, waking up parents etc.  I mean, if the parent is not RPM_ACTIVE,
> > the _irq resume should immediately fail and analogously for the _irq
> > suspend.  And so on.  As simple as reasonably possible.
> 
> But what if the device's own status is currently SUSPENDING or
> RESUMING?  Do you want to fail when that happens, instead of waiting
> for the concurrent operation to finish?

Yes.  IMO an _irq() suspend should only be allowed if the status is RPM_ACTIVE
and an _irq() resume should fail if the status is not RPM_SUSPENDED.  The
error code returned should depend on the situation, though.

> There's no way to prevent this
> on SMP systems, because a wakeup request can arrive while a
> software-initiated resume is in progress.

I know that. :-)

> > > Going further, I'm still a little dubious about the whole point of
> > > having idle notifications at all.  I don't see why the PM core can't
> > > notify a subsystem that a device is idle just by calling the
> > > runtime_suspend routine.
> > 
> > Well, calling _idle is like saying "I think this device may be idle, please
> > consider suspending it", while calling _suspend is like saying "suspend
> > this device unless you can't".  I think that's enough of a difference to
> > have separate _idle.
> 
> > A subsystem or a driver may want to schedule the suspend with a timeout
> > if _idle is called instead of just suspending synchronously.  The r8169 driver
> > actually does something similar (although a bit more complicated).
> 
> What you are describing is basically what autosuspend was intended to
> accomplish (that is, formalizing the difference between "the device
> just became idle, you should consider when to suspend it" and "the
> device has been idle for a sufficiently long time, please suspend it
> now").  Would the r8169 driver be simplified if it used autosuspend?

At the moment it suspends when the network cable is removed from the device
and the hack I mentioned is used during the resume after the cable has been
reinserted (it checks if the cable is still there and schedules suspend if not).

If we removed the immediate idle notification after a successful resume, it
might need to be reworked slightly.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:

> > > In my opinion the _irq operations should really be one-shot, without
> > > any looping, waking up parents etc.  I mean, if the parent is not RPM_ACTIVE,
> > > the _irq resume should immediately fail and analogously for the _irq
> > > suspend.  And so on.  As simple as reasonably possible.
> > 
> > But what if the device's own status is currently SUSPENDING or
> > RESUMING?  Do you want to fail when that happens, instead of waiting
> > for the concurrent operation to finish?
> 
> Yes.  IMO an _irq() suspend should only be allowed if the status is RPM_ACTIVE
> and an _irq() resume should fail if the status is not RPM_SUSPENDED.  The
> error code returned should depend on the situation, though.
> 
> > There's no way to prevent this
> > on SMP systems, because a wakeup request can arrive while a
> > software-initiated resume is in progress.
> 
> I know that. :-)

I suspect Kevin will not want to live with this restriction, but I'd
like to hear from him.  Kevin?

> > What you are describing is basically what autosuspend was intended to
> > accomplish (that is, formalizing the difference between "the device
> > just became idle, you should consider when to suspend it" and "the
> > device has been idle for a sufficiently long time, please suspend it
> > now").  Would the r8169 driver be simplified if it used autosuspend?
> 
> At the moment it suspends when the network cable is removed from the device
> and the hack I mentioned is used during the resume after the cable has been
> reinserted (it checks if the cable is still there and schedules suspend if not).

That does seem like a strange hack.  Instead of scheduling a suspend,
why not simply do a suspend as soon as you learn that the cable has
been removed again?  Is there a problem about the connection status
bouncing?

> If we removed the immediate idle notification after a successful resume, it
> might need to be reworked slightly.

My suggestion was that we remove the idle notification after a failed 
suspend, not the notification after a successful resume.

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Monday, October 11, 2010, Alan Stern wrote:
> On Sat, 9 Oct 2010, Rafael J. Wysocki wrote:
> 
> > I wonder if we can do the "fast suspend" and "fast resume" under the
> > power.lock spinlock.  That would allow us to avoid some complications
> > related to RPM_RESUMING and RPM_SUSPENDING.  Namely,
> > if the device is flagged as "power.irq_safe", it will always suspend and
> > resume "atomically" under its power.lock spinlock.  Then, the status will
> > always be either RPM_ACTIVE or RPM_SUSPENDED (or RPM_ERROR,
> > which is uninteresting).
> 
> We could do this.  It has some disadvantages but they aren't terrible.
> 
> >  Also, if "fast suspend" doesn't change the device
> > parent's power.child_count, we won't need to worry about parents any more.
> 
> I don't know about that.  If the parent's child_count doesn't change 
> then the parent can never suspend.  Of course, if there is no parent or 
> the parent is disabled for runtime PM then this doesn't matter.

I think the recursive resuming of the parents is inherently nonatomic and
it shouldn't be done in the "fast suspend/resume" case.  So, I think it might
make sense to prevent the parent from ever suspending if children are supposed
to use the "fast" ops.

> > I'm still not sure what to do with _idle() in that case.
> 
> Clearly we should not hold the spinlock while running the runtime_idle
> callback.  That would make it impossible for the callback to ask for a
> suspend.

That's correct.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Sat, 9 Oct 2010, Rafael J. Wysocki wrote:

> I wonder if we can do the "fast suspend" and "fast resume" under the
> power.lock spinlock.  That would allow us to avoid some complications
> related to RPM_RESUMING and RPM_SUSPENDING.  Namely,
> if the device is flagged as "power.irq_safe", it will always suspend and
> resume "atomically" under its power.lock spinlock.  Then, the status will
> always be either RPM_ACTIVE or RPM_SUSPENDED (or RPM_ERROR,
> which is uninteresting).

We could do this.  It has some disadvantages but they aren't terrible.

>  Also, if "fast suspend" doesn't change the device
> parent's power.child_count, we won't need to worry about parents any more.

I don't know about that.  If the parent's child_count doesn't change 
then the parent can never suspend.  Of course, if there is no parent or 
the parent is disabled for runtime PM then this doesn't matter.

> I'm still not sure what to do with _idle() in that case.

Clearly we should not hold the spinlock while running the runtime_idle
callback.  That would make it impossible for the callback to ask for a
suspend.

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Thu, 7 Oct 2010, Kevin Hilman wrote:
>
>> > Do you need "normal" resume to work after "atomic" suspend, or is it
>> > sufficient that "atomic" suspend will require "atomic" resume?
>> 
>> hmm... while I'm definitely needing an "atomic" resume after a "normal"
>> suspend, for now I can't think of a case where a "normal" resume would
>> be needed after an "atomic" suspend.  All the cases where I'm currently
>> using an atomic suspend also have a corresponding atomic resume.
>> 
>> As I write this, it wouldn't surprise me down the road to find some HW
>> errata that requires the device in a specific state only before idle,
>> but not caring about the state after idle.  That would be a case where
>> an atomic suspend would be needed, but the resume would be "normal"
>> sometime later when the device is next needed.
>
> Put it this way: Are you okay with just the following two 
> possibilities?
>
> 	(1) Both suspends and resumes always have interrupts enabled.
>
> 	(2) Both suspends and resumes always have interrupts disabled.
>
> In other words, is it okay to rule out the ability of mixing "atomic" 
> and "normal" runtime PM operations?

Yes, I think that's a reasonable limitation.

If a driver/subsystem needs to handle an occasional "atomic" runtime PM
operation, it's callbacks will have to be atomic as well, so I don't see
any reason it can't be made to use only atomic operations.

Kevin

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Friday, October 08, 2010, Alan Stern wrote:
> On Thu, 7 Oct 2010, Rafael J. Wysocki wrote:
> 
> > > 	If the PM core simply avoids releasing dev->power.lock before
> > > 	invoking the runtime_suspend or runtime_resume callback, the
> > > 	end result is almost the same as with busy-waiting.
> > 
> > This is slightly more complicated, because suspend is a bit different from
> > resume.  I think it's generally acceptable to refuse to do a "fast path" suspend
> > if the device state is not RPM_ACTIVE at the moment, but refusing to do
> > a "fast path" resume may be more problematic (more on that later).
> 
> That's what I thought too, but Kevin has suggested this might not be 
> so.
> 
> > The particular case we need to handle (I think) is that some devices need to be
> > resumed "atomically" as a part of bringing a CPU package out of a C-like-state
> > (Kevin, is that correct?).  In that case not only we can't defer the resume (by
> > using a workqueue), but also we have to finish the resume within strict time
> > constraints (the time to leave the given C-like-state is one of the parameters
> > used by cpuidle governors to decide which state to choose).
> 
> To what extent can this really happen?  If the CPU was previously in a 
> C-like state then it couldn't be in the middle of changing the device's 
> power state.  Maybe a different CPU was suspending or resuming the 
> device, but if that's so then why would this CPU need to resume it as 
> part of bringing the package out of the C-like state?

That's the case when user space writes to /sys/devices/.../power/control on
another CPU.

> > On the other hand, busy waiting (by looping) in the case the state is
> > RPM_RESUMING is as though the callbacks were executed under a spinlock and we
> > happened to wait on it.  I'd prefer to avoid that.  Moreover, if the state is
> > RPM_SUSPENDING at the time a "fast path" resume is started, we are almost
> > guaranteed to violate the time constraints (by busy waiting for the suspend to
> > complete and carrying out the resume).
> 
> I'm not convinced that the time constraints are all that hard.  At 
> least, I can't think of a situation where we do face a hard time limit 
> and also the device might be in the middle of a transition when we need 
> to resume it.

Now that I think of it, you seem to be right.

> > So, here's an idea:
> > 
> > First, let's introduce two flags in struct dev_pm_info, irq_safe and
> > fast_resume.  The former has to be set so that the things above work.
> > 
> > Second, let's add a new flag to pass to __pm_runtime_{suspend|resume}(),
> > RPM_FAST_RESUME such that if it is passed to __pm_runtime_suspend(), it
> > causes fast_resume to be set for the given device (if the status is already
> > RPM_SUSPENDED, it just sets the flag, otherwise it suspends the device
> > normally and then sets the flag).  Now, if fast_resume is set,
> > __pm_runtime_resume() has to be passed RPM_FAST_RESUME, or it will
> > fail (it also will fail if passed RPM_FAST_RESUME and fast_resume isn't
> > set).  In that case the status has to be RPM_SUSPENDED or RPM_RESUMING
> > (otherwise fast_resume won't be set)
> 
> Why not?  Does fast_resume ever get cleared?

Yes, when the "fast resume" has completed.

> >  and if it is RPM_RESUMING, this means
> > that the other resume has been called with RPM_FAST_RESUME too (it would
> > fail otherwise), so it's fine to busy loop until the status is RPM_SUSPENDED
> > (it's the caller's problem to avoid that).  RPM_FAST_RESUME causes
> > __pm_runtime_resume() to avod turning interrupts on.
> > 
> > If necessary, there may be a flag for __pm_runtime_suspend() that will
> > carry out "atomic" suspend, but I'm not sure if that's necessary.  Kevin?
> 
> This seems much more complicated than we need.  Why not require for any
> particular device that all resumes and suspends are either always fast
> or always normal (i.e., always done with interrupts disabled or always
> done with interrupts enabled)?  That's what the original patch does.
>
> Sure, maybe some devices don't need to have fast suspend all the time.  
> But we don't lose anything by requiring them to always use it in order
> to gain the benefit of fast resume.

I think we should avoid turning interrupts off for the whole suspend/resume
time if not strictly necessary.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Friday, October 08, 2010, Rafael J. Wysocki wrote:
> On Friday, October 08, 2010, Alan Stern wrote:
> > On Thu, 7 Oct 2010, Rafael J. Wysocki wrote:
> > 
> > > > 	If the PM core simply avoids releasing dev->power.lock before
> > > > 	invoking the runtime_suspend or runtime_resume callback, the
> > > > 	end result is almost the same as with busy-waiting.
> > > 
> > > This is slightly more complicated, because suspend is a bit different from
> > > resume.  I think it's generally acceptable to refuse to do a "fast path" suspend
> > > if the device state is not RPM_ACTIVE at the moment, but refusing to do
> > > a "fast path" resume may be more problematic (more on that later).
> > 
> > That's what I thought too, but Kevin has suggested this might not be 
> > so.
> > 
> > > The particular case we need to handle (I think) is that some devices need to be
> > > resumed "atomically" as a part of bringing a CPU package out of a C-like-state
> > > (Kevin, is that correct?).  In that case not only we can't defer the resume (by
> > > using a workqueue), but also we have to finish the resume within strict time
> > > constraints (the time to leave the given C-like-state is one of the parameters
> > > used by cpuidle governors to decide which state to choose).
> > 
> > To what extent can this really happen?  If the CPU was previously in a 
> > C-like state then it couldn't be in the middle of changing the device's 
> > power state.  Maybe a different CPU was suspending or resuming the 
> > device, but if that's so then why would this CPU need to resume it as 
> > part of bringing the package out of the C-like state?
> 
> That's the case when user space writes to /sys/devices/.../power/control on
> another CPU.
> 
> > > On the other hand, busy waiting (by looping) in the case the state is
> > > RPM_RESUMING is as though the callbacks were executed under a spinlock and we
> > > happened to wait on it.  I'd prefer to avoid that.  Moreover, if the state is
> > > RPM_SUSPENDING at the time a "fast path" resume is started, we are almost
> > > guaranteed to violate the time constraints (by busy waiting for the suspend to
> > > complete and carrying out the resume).
> > 
> > I'm not convinced that the time constraints are all that hard.  At 
> > least, I can't think of a situation where we do face a hard time limit 
> > and also the device might be in the middle of a transition when we need 
> > to resume it.
> 
> Now that I think of it, you seem to be right.

However, there's the following scenario (I don't know how realistic it is, though).
It is not related to the cpuidle case, it is related to the case when the device
is woken up by its own IRQ.  Namely, in that case it is perfectly possible that
the IRQ appears exactly when the device is suspending, so if there's a "fast
resume" in the interrupt handler, it will have to wait for the suspend to
complete (or fail).  That in theory may lead to a violation of time constraints
(if there are any).

...
> > This seems much more complicated than we need.  Why not require for any
> > particular device that all resumes and suspends are either always fast
> > or always normal (i.e., always done with interrupts disabled or always
> > done with interrupts enabled)?  That's what the original patch does.
> >
> > Sure, maybe some devices don't need to have fast suspend all the time.  
> > But we don't lose anything by requiring them to always use it in order
> > to gain the benefit of fast resume.
> 
> I think we should avoid turning interrupts off for the whole suspend/resume
> time if not strictly necessary.

Well, perhaps that's not a problem.

I wonder if we can do the "fast suspend" and "fast resume" under the
power.lock spinlock.  That would allow us to avoid some complications
related to RPM_RESUMING and RPM_SUSPENDING.  Namely,
if the device is flagged as "power.irq_safe", it will always suspend and
resume "atomically" under its power.lock spinlock.  Then, the status will
always be either RPM_ACTIVE or RPM_SUSPENDED (or RPM_ERROR,
which is uninteresting).  Also, if "fast suspend" doesn't change the device
parent's power.child_count, we won't need to worry about parents any more.

I'm still not sure what to do with _idle() in that case.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Thursday, October 07, 2010, Alan Stern wrote:
> On Thu, 7 Oct 2010, Kevin Hilman wrote:
> 
> > My confusion is not about the use of spinlocks, it's a question of what
> > is being busy-waited for, and the thread that is being waited for is
> > going to complete when interrupts are disabled.
> > 
> > Sorry to be dense, but can you (re)summarize what you're proposing as I
> > think I'm getting mixed up with all the various options we've been
> > tossing around.
> > 
> > If it can work, I'm certainly in favor of a busy-wait approach as it 
> > really ensures that sync requests are handled quickly.
> 
> Okay, here's the story in a nutshell.  Allowing a subsystem's or
> driver's runtime-PM callbacks to run with interrupts disabled faces two
> obstacles:
> 
>    (1): We don't want two different CPUs to run callbacks for the
> 	same device at the same time.  So if a callback is already
> 	running on one CPU (i.e., if the device's runtime status is
> 	either SUSPENDING or RESUMING) then another CPU can't be
> 	allowed to invoke a callback.
> 
> 	Thus, you can't do a synchronous pm_runtime_resume_irq()
> 	if the device is in the middle of a suspend or resume
> 	operation.  We're left with two choices: Fail the synchronous
> 	call and force the driver to defer matters to a workqueue
> 	(possibly masking an IRQ line in the meantime), or busy-wait
> 	until the concurrent operation finishes.
> 
> 	If the PM core simply avoids releasing dev->power.lock before
> 	invoking the runtime_suspend or runtime_resume callback, the
> 	end result is almost the same as with busy-waiting.

This is slightly more complicated, because suspend is a bit different from
resume.  I think it's generally acceptable to refuse to do a "fast path" suspend
if the device state is not RPM_ACTIVE at the moment, but refusing to do
a "fast path" resume may be more problematic (more on that later).

>    (2): In general we can't resume a device if its parent is suspended.
> 	If the parent's runtime_resume routine needs to run with
> 	interrupts enabled then there's no way to resume the device
> 	while keeping interrupts disabled.
> 
> 	Possible solutions involve, again, deferring matters to a
> 	workqueue, or else simply not allowing the situation to arise
> 	in the first place (forbid a device to have interrupt-disabled 
> 	callbacks unless its parent does too or the parent doesn't use 
> 	runtime PM at all).
> 
> In general I'm against the solutions that require a workqueue.

OK

> Raphael appears to favor workqueues for (1) and be against them for (2).

The particular case we need to handle (I think) is that some devices need to be
resumed "atomically" as a part of bringing a CPU package out of a C-like-state
(Kevin, is that correct?).  In that case not only we can't defer the resume (by
using a workqueue), but also we have to finish the resume within strict time
constraints (the time to leave the given C-like-state is one of the parameters
used by cpuidle governors to decide which state to choose).

On the other hand, busy waiting (by looping) in the case the state is
RPM_RESUMING is as though the callbacks were executed under a spinlock and we
happened to wait on it.  I'd prefer to avoid that.  Moreover, if the state is
RPM_SUSPENDING at the time a "fast path" resume is started, we are almost
guaranteed to violate the time constraints (by busy waiting for the suspend to
complete and carrying out the resume).

So, here's an idea:

First, let's introduce two flags in struct dev_pm_info, irq_safe and
fast_resume.  The former has to be set so that the things above work.

Second, let's add a new flag to pass to __pm_runtime_{suspend|resume}(),
RPM_FAST_RESUME such that if it is passed to __pm_runtime_suspend(), it
causes fast_resume to be set for the given device (if the status is already
RPM_SUSPENDED, it just sets the flag, otherwise it suspends the device
normally and then sets the flag).  Now, if fast_resume is set,
__pm_runtime_resume() has to be passed RPM_FAST_RESUME, or it will
fail (it also will fail if passed RPM_FAST_RESUME and fast_resume isn't
set).  In that case the status has to be RPM_SUSPENDED or RPM_RESUMING
(otherwise fast_resume won't be set) and if it is RPM_RESUMING, this means
that the other resume has been called with RPM_FAST_RESUME too (it would
fail otherwise), so it's fine to busy loop until the status is RPM_SUSPENDED
(it's the caller's problem to avoid that).  RPM_FAST_RESUME causes
__pm_runtime_resume() to avod turning interrupts on.

If necessary, there may be a flag for __pm_runtime_suspend() that will
carry out "atomic" suspend, but I'm not sure if that's necessary.  Kevin?

Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Thursday, October 07, 2010, Alan Stern wrote:
>> On Thu, 7 Oct 2010, Kevin Hilman wrote:
>> 
>> > My confusion is not about the use of spinlocks, it's a question of what
>> > is being busy-waited for, and the thread that is being waited for is
>> > going to complete when interrupts are disabled.
>> > 
>> > Sorry to be dense, but can you (re)summarize what you're proposing as I
>> > think I'm getting mixed up with all the various options we've been
>> > tossing around.
>> > 
>> > If it can work, I'm certainly in favor of a busy-wait approach as it 
>> > really ensures that sync requests are handled quickly.
>> 
>> Okay, here's the story in a nutshell.  Allowing a subsystem's or
>> driver's runtime-PM callbacks to run with interrupts disabled faces two
>> obstacles:
>> 
>>    (1): We don't want two different CPUs to run callbacks for the
>> 	same device at the same time.  So if a callback is already
>> 	running on one CPU (i.e., if the device's runtime status is
>> 	either SUSPENDING or RESUMING) then another CPU can't be
>> 	allowed to invoke a callback.
>> 
>> 	Thus, you can't do a synchronous pm_runtime_resume_irq()
>> 	if the device is in the middle of a suspend or resume
>> 	operation.  We're left with two choices: Fail the synchronous
>> 	call and force the driver to defer matters to a workqueue
>> 	(possibly masking an IRQ line in the meantime), or busy-wait
>> 	until the concurrent operation finishes.
>> 
>> 	If the PM core simply avoids releasing dev->power.lock before
>> 	invoking the runtime_suspend or runtime_resume callback, the
>> 	end result is almost the same as with busy-waiting.
>
> This is slightly more complicated, because suspend is a bit different from
> resume.  I think it's generally acceptable to refuse to do a "fast path" suspend
> if the device state is not RPM_ACTIVE at the moment, but refusing to do
> a "fast path" resume may be more problematic (more on that later).
>
>>    (2): In general we can't resume a device if its parent is suspended.
>> 	If the parent's runtime_resume routine needs to run with
>> 	interrupts enabled then there's no way to resume the device
>> 	while keeping interrupts disabled.
>> 
>> 	Possible solutions involve, again, deferring matters to a
>> 	workqueue, or else simply not allowing the situation to arise
>> 	in the first place (forbid a device to have interrupt-disabled 
>> 	callbacks unless its parent does too or the parent doesn't use 
>> 	runtime PM at all).
>> 
>> In general I'm against the solutions that require a workqueue.
>
> OK
>
>> Raphael appears to favor workqueues for (1) and be against them for (2).
>
> The particular case we need to handle (I think) is that some devices need to be
> resumed "atomically" as a part of bringing a CPU package out of a C-like-state
> (Kevin, is that correct?).  

Correct, but there is more than one problematic case.

Another problem is in devices that are not atomic with C-states, but
receive wakeup IRQs while they're suspended and thus need to
pm_runtime_get() from their ISRs, without having an essentially
unbounded interrupt latency before the ISR can be handled.

Consider the following sequence for a given device

- device: ->runtime_suspend() /* registers not accessible */
- pm_idle
- IRQs disabled
- idle
- wakeup event
- idle loop finishes
- IRQs enabled
- device: ->runtime_resume()

Now, consider the 'wakeup event' is an IRQ for that device.  That means
as soon as IRQs are (re)enabled (and before some other activity has
triggered a ->runtime_resume), the device's ISR is called.  Since it is
RPM_SUSPENDED, it's registers are not accessible so it needs to do a
pm_runtime_get().

The case that raised this initially was a GPIO IRQ demux, where the
initial ISR is just a chained handler whichfigures out which GPIO
fired and then call genirq for that IRQ.

All that being said, while I've currently described these as two
different problems, the second one could be solved by converting it to
the first one.  IOW, make GPIO be one of those devices that are
suspended/resumed atomically with the CPU so that it is guaranteed to be
awake when IRQs are re-enabled.

While that would work, I've been trying to move in the opposite
direction by trying to dis-connect them from CPU idle.

> In that case not only we can't defer the resume (by
> using a workqueue), but also we have to finish the resume within strict time
> constraints (the time to leave the given C-like-state is one of the parameters
> used by cpuidle governors to decide which state to choose).
>
> On the other hand, busy waiting (by looping) in the case the state is
> RPM_RESUMING is as though the callbacks were executed under a spinlock and we
> happened to wait on it.  I'd prefer to avoid that.  Moreover, if the state is
> RPM_SUSPENDING at the time a "fast path" resume is started, we are almost
> guaranteed to violate the time constraints (by busy waiting for the suspend to
> complete and carrying out the resume).
>
> So, here's an idea:
>
> First, let's introduce two flags in struct dev_pm_info, irq_safe and
> fast_resume.  The former has to be set so that the things above work.
>
> Second, let's add a new flag to pass to __pm_runtime_{suspend|resume}(),
> RPM_FAST_RESUME such that if it is passed to __pm_runtime_suspend(), it
> causes fast_resume to be set for the given device (if the status is already
> RPM_SUSPENDED, it just sets the flag, otherwise it suspends the device
> normally and then sets the flag).  Now, if fast_resume is set,
> __pm_runtime_resume() has to be passed RPM_FAST_RESUME, or it will
> fail (it also will fail if passed RPM_FAST_RESUME and fast_resume isn't
> set).  In that case the status has to be RPM_SUSPENDED or RPM_RESUMING
> (otherwise fast_resume won't be set) and if it is RPM_RESUMING, this means
> that the other resume has been called with RPM_FAST_RESUME too (it would
> fail otherwise), so it's fine to busy loop until the status is RPM_SUSPENDED
> (it's the caller's problem to avoid that).  RPM_FAST_RESUME causes
> __pm_runtime_resume() to avod turning interrupts on.
>
> If necessary, there may be a flag for __pm_runtime_suspend() that will
> carry out "atomic" suspend, but I'm not sure if that's necessary.  Kevin?

I think we'll need both "atomic" suspend & resume.  

One of the devices I'm currently managing in this atomic fashion (inside
pm_idle) is the UARTs (of course, I'm not currently using runtime PM for
this, I'm calling platform-specific hooks directly since IRQs are
disabled.)  For the UARTs I'm using both atomic suspend and resume,
primarily for debug so I am sure UARTs are awake to see any panics
during the last parts of the suspend/idle process and the early parts of
the resume path.   This UART case is a bit of a hack until the driver is
converted to runtime PM, but illustrates some debug related reasons we
might also need atomic suspend and resume.

We also have some workarounds for hardware errata that require
putting certain devices in a specific state just before (and after)
idle.  The drivers for these devices will need both suspend and resume.

Kevin

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Friday, October 08, 2010, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Thursday, October 07, 2010, Alan Stern wrote:
> >> On Thu, 7 Oct 2010, Kevin Hilman wrote:
> >> 
> >> > My confusion is not about the use of spinlocks, it's a question of what
> >> > is being busy-waited for, and the thread that is being waited for is
> >> > going to complete when interrupts are disabled.
> >> > 
> >> > Sorry to be dense, but can you (re)summarize what you're proposing as I
> >> > think I'm getting mixed up with all the various options we've been
> >> > tossing around.
> >> > 
> >> > If it can work, I'm certainly in favor of a busy-wait approach as it 
> >> > really ensures that sync requests are handled quickly.
> >> 
> >> Okay, here's the story in a nutshell.  Allowing a subsystem's or
> >> driver's runtime-PM callbacks to run with interrupts disabled faces two
> >> obstacles:
> >> 
> >>    (1): We don't want two different CPUs to run callbacks for the
> >> 	same device at the same time.  So if a callback is already
> >> 	running on one CPU (i.e., if the device's runtime status is
> >> 	either SUSPENDING or RESUMING) then another CPU can't be
> >> 	allowed to invoke a callback.
> >> 
> >> 	Thus, you can't do a synchronous pm_runtime_resume_irq()
> >> 	if the device is in the middle of a suspend or resume
> >> 	operation.  We're left with two choices: Fail the synchronous
> >> 	call and force the driver to defer matters to a workqueue
> >> 	(possibly masking an IRQ line in the meantime), or busy-wait
> >> 	until the concurrent operation finishes.
> >> 
> >> 	If the PM core simply avoids releasing dev->power.lock before
> >> 	invoking the runtime_suspend or runtime_resume callback, the
> >> 	end result is almost the same as with busy-waiting.
> >
> > This is slightly more complicated, because suspend is a bit different from
> > resume.  I think it's generally acceptable to refuse to do a "fast path" suspend
> > if the device state is not RPM_ACTIVE at the moment, but refusing to do
> > a "fast path" resume may be more problematic (more on that later).
> >
> >>    (2): In general we can't resume a device if its parent is suspended.
> >> 	If the parent's runtime_resume routine needs to run with
> >> 	interrupts enabled then there's no way to resume the device
> >> 	while keeping interrupts disabled.
> >> 
> >> 	Possible solutions involve, again, deferring matters to a
> >> 	workqueue, or else simply not allowing the situation to arise
> >> 	in the first place (forbid a device to have interrupt-disabled 
> >> 	callbacks unless its parent does too or the parent doesn't use 
> >> 	runtime PM at all).
> >> 
> >> In general I'm against the solutions that require a workqueue.
> >
> > OK
> >
> >> Raphael appears to favor workqueues for (1) and be against them for (2).
> >
> > The particular case we need to handle (I think) is that some devices need to be
> > resumed "atomically" as a part of bringing a CPU package out of a C-like-state
> > (Kevin, is that correct?).  
> 
> Correct, but there is more than one problematic case.
> 
> Another problem is in devices that are not atomic with C-states, but
> receive wakeup IRQs while they're suspended and thus need to
> pm_runtime_get() from their ISRs, without having an essentially
> unbounded interrupt latency before the ISR can be handled.
> 
> Consider the following sequence for a given device
> 
> - device: ->runtime_suspend() /* registers not accessible */
> - pm_idle
> - IRQs disabled
> - idle
> - wakeup event
> - idle loop finishes
> - IRQs enabled
> - device: ->runtime_resume()
> 
> Now, consider the 'wakeup event' is an IRQ for that device.  That means
> as soon as IRQs are (re)enabled (and before some other activity has
> triggered a ->runtime_resume), the device's ISR is called.  Since it is
> RPM_SUSPENDED, it's registers are not accessible so it needs to do a
> pm_runtime_get().
> 
> The case that raised this initially was a GPIO IRQ demux, where the
> initial ISR is just a chained handler whichfigures out which GPIO
> fired and then call genirq for that IRQ.
> 
> All that being said, while I've currently described these as two
> different problems, the second one could be solved by converting it to
> the first one.  IOW, make GPIO be one of those devices that are
> suspended/resumed atomically with the CPU so that it is guaranteed to be
> awake when IRQs are re-enabled.
> 
> While that would work, I've been trying to move in the opposite
> direction by trying to dis-connect them from CPU idle.
> 
> > In that case not only we can't defer the resume (by
> > using a workqueue), but also we have to finish the resume within strict time
> > constraints (the time to leave the given C-like-state is one of the parameters
> > used by cpuidle governors to decide which state to choose).
> >
> > On the other hand, busy waiting (by looping) in the case the state is
> > RPM_RESUMING is as though the callbacks were executed under a spinlock and we
> > happened to wait on it.  I'd prefer to avoid that.  Moreover, if the state is
> > RPM_SUSPENDING at the time a "fast path" resume is started, we are almost
> > guaranteed to violate the time constraints (by busy waiting for the suspend to
> > complete and carrying out the resume).
> >
> > So, here's an idea:
> >
> > First, let's introduce two flags in struct dev_pm_info, irq_safe and
> > fast_resume.  The former has to be set so that the things above work.
> >
> > Second, let's add a new flag to pass to __pm_runtime_{suspend|resume}(),
> > RPM_FAST_RESUME such that if it is passed to __pm_runtime_suspend(), it
> > causes fast_resume to be set for the given device (if the status is already
> > RPM_SUSPENDED, it just sets the flag, otherwise it suspends the device
> > normally and then sets the flag).  Now, if fast_resume is set,
> > __pm_runtime_resume() has to be passed RPM_FAST_RESUME, or it will
> > fail (it also will fail if passed RPM_FAST_RESUME and fast_resume isn't
> > set).  In that case the status has to be RPM_SUSPENDED or RPM_RESUMING
> > (otherwise fast_resume won't be set) and if it is RPM_RESUMING, this means
> > that the other resume has been called with RPM_FAST_RESUME too (it would
> > fail otherwise), so it's fine to busy loop until the status is RPM_SUSPENDED
> > (it's the caller's problem to avoid that).  RPM_FAST_RESUME causes
> > __pm_runtime_resume() to avod turning interrupts on.
> >
> > If necessary, there may be a flag for __pm_runtime_suspend() that will
> > carry out "atomic" suspend, but I'm not sure if that's necessary.  Kevin?
> 
> I think we'll need both "atomic" suspend & resume.  
> 
> One of the devices I'm currently managing in this atomic fashion (inside
> pm_idle) is the UARTs (of course, I'm not currently using runtime PM for
> this, I'm calling platform-specific hooks directly since IRQs are
> disabled.)  For the UARTs I'm using both atomic suspend and resume,
> primarily for debug so I am sure UARTs are awake to see any panics
> during the last parts of the suspend/idle process and the early parts of
> the resume path.   This UART case is a bit of a hack until the driver is
> converted to runtime PM, but illustrates some debug related reasons we
> might also need atomic suspend and resume.
> 
> We also have some workarounds for hardware errata that require
> putting certain devices in a specific state just before (and after)
> idle.  The drivers for these devices will need both suspend and resume.

OK, thanks for the info.

Do you need "normal" resume to work after "atomic" suspend, or is it
sufficient that "atomic" suspend will require "atomic" resume?

Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Friday, October 08, 2010, Kevin Hilman wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> 
>> > On Thursday, October 07, 2010, Alan Stern wrote:
>> >> On Thu, 7 Oct 2010, Kevin Hilman wrote:
>> >> 
>> >> > My confusion is not about the use of spinlocks, it's a question of what
>> >> > is being busy-waited for, and the thread that is being waited for is
>> >> > going to complete when interrupts are disabled.
>> >> > 
>> >> > Sorry to be dense, but can you (re)summarize what you're proposing as I
>> >> > think I'm getting mixed up with all the various options we've been
>> >> > tossing around.
>> >> > 
>> >> > If it can work, I'm certainly in favor of a busy-wait approach as it 
>> >> > really ensures that sync requests are handled quickly.
>> >> 
>> >> Okay, here's the story in a nutshell.  Allowing a subsystem's or
>> >> driver's runtime-PM callbacks to run with interrupts disabled faces two
>> >> obstacles:
>> >> 
>> >>    (1): We don't want two different CPUs to run callbacks for the
>> >> 	same device at the same time.  So if a callback is already
>> >> 	running on one CPU (i.e., if the device's runtime status is
>> >> 	either SUSPENDING or RESUMING) then another CPU can't be
>> >> 	allowed to invoke a callback.
>> >> 
>> >> 	Thus, you can't do a synchronous pm_runtime_resume_irq()
>> >> 	if the device is in the middle of a suspend or resume
>> >> 	operation.  We're left with two choices: Fail the synchronous
>> >> 	call and force the driver to defer matters to a workqueue
>> >> 	(possibly masking an IRQ line in the meantime), or busy-wait
>> >> 	until the concurrent operation finishes.
>> >> 
>> >> 	If the PM core simply avoids releasing dev->power.lock before
>> >> 	invoking the runtime_suspend or runtime_resume callback, the
>> >> 	end result is almost the same as with busy-waiting.
>> >
>> > This is slightly more complicated, because suspend is a bit different from
>> > resume.  I think it's generally acceptable to refuse to do a "fast path" suspend
>> > if the device state is not RPM_ACTIVE at the moment, but refusing to do
>> > a "fast path" resume may be more problematic (more on that later).
>> >
>> >>    (2): In general we can't resume a device if its parent is suspended.
>> >> 	If the parent's runtime_resume routine needs to run with
>> >> 	interrupts enabled then there's no way to resume the device
>> >> 	while keeping interrupts disabled.
>> >> 
>> >> 	Possible solutions involve, again, deferring matters to a
>> >> 	workqueue, or else simply not allowing the situation to arise
>> >> 	in the first place (forbid a device to have interrupt-disabled 
>> >> 	callbacks unless its parent does too or the parent doesn't use 
>> >> 	runtime PM at all).
>> >> 
>> >> In general I'm against the solutions that require a workqueue.
>> >
>> > OK
>> >
>> >> Raphael appears to favor workqueues for (1) and be against them for (2).
>> >
>> > The particular case we need to handle (I think) is that some devices need to be
>> > resumed "atomically" as a part of bringing a CPU package out of a C-like-state
>> > (Kevin, is that correct?).  
>> 
>> Correct, but there is more than one problematic case.
>> 
>> Another problem is in devices that are not atomic with C-states, but
>> receive wakeup IRQs while they're suspended and thus need to
>> pm_runtime_get() from their ISRs, without having an essentially
>> unbounded interrupt latency before the ISR can be handled.
>> 
>> Consider the following sequence for a given device
>> 
>> - device: ->runtime_suspend() /* registers not accessible */
>> - pm_idle
>> - IRQs disabled
>> - idle
>> - wakeup event
>> - idle loop finishes
>> - IRQs enabled
>> - device: ->runtime_resume()
>> 
>> Now, consider the 'wakeup event' is an IRQ for that device.  That means
>> as soon as IRQs are (re)enabled (and before some other activity has
>> triggered a ->runtime_resume), the device's ISR is called.  Since it is
>> RPM_SUSPENDED, it's registers are not accessible so it needs to do a
>> pm_runtime_get().
>> 
>> The case that raised this initially was a GPIO IRQ demux, where the
>> initial ISR is just a chained handler whichfigures out which GPIO
>> fired and then call genirq for that IRQ.
>> 
>> All that being said, while I've currently described these as two
>> different problems, the second one could be solved by converting it to
>> the first one.  IOW, make GPIO be one of those devices that are
>> suspended/resumed atomically with the CPU so that it is guaranteed to be
>> awake when IRQs are re-enabled.
>> 
>> While that would work, I've been trying to move in the opposite
>> direction by trying to dis-connect them from CPU idle.
>> 
>> > In that case not only we can't defer the resume (by
>> > using a workqueue), but also we have to finish the resume within strict time
>> > constraints (the time to leave the given C-like-state is one of the parameters
>> > used by cpuidle governors to decide which state to choose).
>> >
>> > On the other hand, busy waiting (by looping) in the case the state is
>> > RPM_RESUMING is as though the callbacks were executed under a spinlock and we
>> > happened to wait on it.  I'd prefer to avoid that.  Moreover, if the state is
>> > RPM_SUSPENDING at the time a "fast path" resume is started, we are almost
>> > guaranteed to violate the time constraints (by busy waiting for the suspend to
>> > complete and carrying out the resume).
>> >
>> > So, here's an idea:
>> >
>> > First, let's introduce two flags in struct dev_pm_info, irq_safe and
>> > fast_resume.  The former has to be set so that the things above work.
>> >
>> > Second, let's add a new flag to pass to __pm_runtime_{suspend|resume}(),
>> > RPM_FAST_RESUME such that if it is passed to __pm_runtime_suspend(), it
>> > causes fast_resume to be set for the given device (if the status is already
>> > RPM_SUSPENDED, it just sets the flag, otherwise it suspends the device
>> > normally and then sets the flag).  Now, if fast_resume is set,
>> > __pm_runtime_resume() has to be passed RPM_FAST_RESUME, or it will
>> > fail (it also will fail if passed RPM_FAST_RESUME and fast_resume isn't
>> > set).  In that case the status has to be RPM_SUSPENDED or RPM_RESUMING
>> > (otherwise fast_resume won't be set) and if it is RPM_RESUMING, this means
>> > that the other resume has been called with RPM_FAST_RESUME too (it would
>> > fail otherwise), so it's fine to busy loop until the status is RPM_SUSPENDED
>> > (it's the caller's problem to avoid that).  RPM_FAST_RESUME causes
>> > __pm_runtime_resume() to avod turning interrupts on.
>> >
>> > If necessary, there may be a flag for __pm_runtime_suspend() that will
>> > carry out "atomic" suspend, but I'm not sure if that's necessary.  Kevin?
>> 
>> I think we'll need both "atomic" suspend & resume.  
>> 
>> One of the devices I'm currently managing in this atomic fashion (inside
>> pm_idle) is the UARTs (of course, I'm not currently using runtime PM for
>> this, I'm calling platform-specific hooks directly since IRQs are
>> disabled.)  For the UARTs I'm using both atomic suspend and resume,
>> primarily for debug so I am sure UARTs are awake to see any panics
>> during the last parts of the suspend/idle process and the early parts of
>> the resume path.   This UART case is a bit of a hack until the driver is
>> converted to runtime PM, but illustrates some debug related reasons we
>> might also need atomic suspend and resume.
>> 
>> We also have some workarounds for hardware errata that require
>> putting certain devices in a specific state just before (and after)
>> idle.  The drivers for these devices will need both suspend and resume.
>
> OK, thanks for the info.
>
> Do you need "normal" resume to work after "atomic" suspend, or is it
> sufficient that "atomic" suspend will require "atomic" resume?

hmm... while I'm definitely needing an "atomic" resume after a "normal"
suspend, for now I can't think of a case where a "normal" resume would
be needed after an "atomic" suspend.  All the cases where I'm currently
using an atomic suspend also have a corresponding atomic resume.

As I write this, it wouldn't surprise me down the road to find some HW
errata that requires the device in a specific state only before idle,
but not caring about the state after idle.  That would be a case where
an atomic suspend would be needed, but the resume would be "normal"
sometime later when the device is next needed.

Kevin

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Thu, 7 Oct 2010, Kevin Hilman wrote:

> > Do you need "normal" resume to work after "atomic" suspend, or is it
> > sufficient that "atomic" suspend will require "atomic" resume?
> 
> hmm... while I'm definitely needing an "atomic" resume after a "normal"
> suspend, for now I can't think of a case where a "normal" resume would
> be needed after an "atomic" suspend.  All the cases where I'm currently
> using an atomic suspend also have a corresponding atomic resume.
> 
> As I write this, it wouldn't surprise me down the road to find some HW
> errata that requires the device in a specific state only before idle,
> but not caring about the state after idle.  That would be a case where
> an atomic suspend would be needed, but the resume would be "normal"
> sometime later when the device is next needed.

Put it this way: Are you okay with just the following two 
possibilities?

	(1) Both suspends and resumes always have interrupts enabled.

	(2) Both suspends and resumes always have interrupts disabled.

In other words, is it okay to rule out the ability of mixing "atomic" 
and "normal" runtime PM operations?

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Friday, October 08, 2010, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Friday, October 08, 2010, Kevin Hilman wrote:
> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> >> 
> >> > On Thursday, October 07, 2010, Alan Stern wrote:
> >> >> On Thu, 7 Oct 2010, Kevin Hilman wrote:
> >> >> 
> >> >> > My confusion is not about the use of spinlocks, it's a question of what
> >> >> > is being busy-waited for, and the thread that is being waited for is
> >> >> > going to complete when interrupts are disabled.
> >> >> > 
> >> >> > Sorry to be dense, but can you (re)summarize what you're proposing as I
> >> >> > think I'm getting mixed up with all the various options we've been
> >> >> > tossing around.
> >> >> > 
> >> >> > If it can work, I'm certainly in favor of a busy-wait approach as it 
> >> >> > really ensures that sync requests are handled quickly.
> >> >> 
> >> >> Okay, here's the story in a nutshell.  Allowing a subsystem's or
> >> >> driver's runtime-PM callbacks to run with interrupts disabled faces two
> >> >> obstacles:
> >> >> 
> >> >>    (1): We don't want two different CPUs to run callbacks for the
> >> >> 	same device at the same time.  So if a callback is already
> >> >> 	running on one CPU (i.e., if the device's runtime status is
> >> >> 	either SUSPENDING or RESUMING) then another CPU can't be
> >> >> 	allowed to invoke a callback.
> >> >> 
> >> >> 	Thus, you can't do a synchronous pm_runtime_resume_irq()
> >> >> 	if the device is in the middle of a suspend or resume
> >> >> 	operation.  We're left with two choices: Fail the synchronous
> >> >> 	call and force the driver to defer matters to a workqueue
> >> >> 	(possibly masking an IRQ line in the meantime), or busy-wait
> >> >> 	until the concurrent operation finishes.
> >> >> 
> >> >> 	If the PM core simply avoids releasing dev->power.lock before
> >> >> 	invoking the runtime_suspend or runtime_resume callback, the
> >> >> 	end result is almost the same as with busy-waiting.
> >> >
> >> > This is slightly more complicated, because suspend is a bit different from
> >> > resume.  I think it's generally acceptable to refuse to do a "fast path" suspend
> >> > if the device state is not RPM_ACTIVE at the moment, but refusing to do
> >> > a "fast path" resume may be more problematic (more on that later).
> >> >
> >> >>    (2): In general we can't resume a device if its parent is suspended.
> >> >> 	If the parent's runtime_resume routine needs to run with
> >> >> 	interrupts enabled then there's no way to resume the device
> >> >> 	while keeping interrupts disabled.
> >> >> 
> >> >> 	Possible solutions involve, again, deferring matters to a
> >> >> 	workqueue, or else simply not allowing the situation to arise
> >> >> 	in the first place (forbid a device to have interrupt-disabled 
> >> >> 	callbacks unless its parent does too or the parent doesn't use 
> >> >> 	runtime PM at all).
> >> >> 
> >> >> In general I'm against the solutions that require a workqueue.
> >> >
> >> > OK
> >> >
> >> >> Raphael appears to favor workqueues for (1) and be against them for (2).
> >> >
> >> > The particular case we need to handle (I think) is that some devices need to be
> >> > resumed "atomically" as a part of bringing a CPU package out of a C-like-state
> >> > (Kevin, is that correct?).  
> >> 
> >> Correct, but there is more than one problematic case.
> >> 
> >> Another problem is in devices that are not atomic with C-states, but
> >> receive wakeup IRQs while they're suspended and thus need to
> >> pm_runtime_get() from their ISRs, without having an essentially
> >> unbounded interrupt latency before the ISR can be handled.
> >> 
> >> Consider the following sequence for a given device
> >> 
> >> - device: ->runtime_suspend() /* registers not accessible */
> >> - pm_idle
> >> - IRQs disabled
> >> - idle
> >> - wakeup event
> >> - idle loop finishes
> >> - IRQs enabled
> >> - device: ->runtime_resume()
> >> 
> >> Now, consider the 'wakeup event' is an IRQ for that device.  That means
> >> as soon as IRQs are (re)enabled (and before some other activity has
> >> triggered a ->runtime_resume), the device's ISR is called.  Since it is
> >> RPM_SUSPENDED, it's registers are not accessible so it needs to do a
> >> pm_runtime_get().
> >> 
> >> The case that raised this initially was a GPIO IRQ demux, where the
> >> initial ISR is just a chained handler whichfigures out which GPIO
> >> fired and then call genirq for that IRQ.
> >> 
> >> All that being said, while I've currently described these as two
> >> different problems, the second one could be solved by converting it to
> >> the first one.  IOW, make GPIO be one of those devices that are
> >> suspended/resumed atomically with the CPU so that it is guaranteed to be
> >> awake when IRQs are re-enabled.
> >> 
> >> While that would work, I've been trying to move in the opposite
> >> direction by trying to dis-connect them from CPU idle.
> >> 
> >> > In that case not only we can't defer the resume (by
> >> > using a workqueue), but also we have to finish the resume within strict time
> >> > constraints (the time to leave the given C-like-state is one of the parameters
> >> > used by cpuidle governors to decide which state to choose).
> >> >
> >> > On the other hand, busy waiting (by looping) in the case the state is
> >> > RPM_RESUMING is as though the callbacks were executed under a spinlock and we
> >> > happened to wait on it.  I'd prefer to avoid that.  Moreover, if the state is
> >> > RPM_SUSPENDING at the time a "fast path" resume is started, we are almost
> >> > guaranteed to violate the time constraints (by busy waiting for the suspend to
> >> > complete and carrying out the resume).
> >> >
> >> > So, here's an idea:
> >> >
> >> > First, let's introduce two flags in struct dev_pm_info, irq_safe and
> >> > fast_resume.  The former has to be set so that the things above work.
> >> >
> >> > Second, let's add a new flag to pass to __pm_runtime_{suspend|resume}(),
> >> > RPM_FAST_RESUME such that if it is passed to __pm_runtime_suspend(), it
> >> > causes fast_resume to be set for the given device (if the status is already
> >> > RPM_SUSPENDED, it just sets the flag, otherwise it suspends the device
> >> > normally and then sets the flag).  Now, if fast_resume is set,
> >> > __pm_runtime_resume() has to be passed RPM_FAST_RESUME, or it will
> >> > fail (it also will fail if passed RPM_FAST_RESUME and fast_resume isn't
> >> > set).  In that case the status has to be RPM_SUSPENDED or RPM_RESUMING
> >> > (otherwise fast_resume won't be set) and if it is RPM_RESUMING, this means
> >> > that the other resume has been called with RPM_FAST_RESUME too (it would
> >> > fail otherwise), so it's fine to busy loop until the status is RPM_SUSPENDED
> >> > (it's the caller's problem to avoid that).  RPM_FAST_RESUME causes
> >> > __pm_runtime_resume() to avod turning interrupts on.
> >> >
> >> > If necessary, there may be a flag for __pm_runtime_suspend() that will
> >> > carry out "atomic" suspend, but I'm not sure if that's necessary.  Kevin?
> >> 
> >> I think we'll need both "atomic" suspend & resume.  
> >> 
> >> One of the devices I'm currently managing in this atomic fashion (inside
> >> pm_idle) is the UARTs (of course, I'm not currently using runtime PM for
> >> this, I'm calling platform-specific hooks directly since IRQs are
> >> disabled.)  For the UARTs I'm using both atomic suspend and resume,
> >> primarily for debug so I am sure UARTs are awake to see any panics
> >> during the last parts of the suspend/idle process and the early parts of
> >> the resume path.   This UART case is a bit of a hack until the driver is
> >> converted to runtime PM, but illustrates some debug related reasons we
> >> might also need atomic suspend and resume.
> >> 
> >> We also have some workarounds for hardware errata that require
> >> putting certain devices in a specific state just before (and after)
> >> idle.  The drivers for these devices will need both suspend and resume.
> >
> > OK, thanks for the info.
> >
> > Do you need "normal" resume to work after "atomic" suspend, or is it
> > sufficient that "atomic" suspend will require "atomic" resume?
> 
> hmm... while I'm definitely needing an "atomic" resume after a "normal"
> suspend, for now I can't think of a case where a "normal" resume would
> be needed after an "atomic" suspend.  All the cases where I'm currently
> using an atomic suspend also have a corresponding atomic resume.
> 
> As I write this, it wouldn't surprise me down the road to find some HW
> errata that requires the device in a specific state only before idle,
> but not caring about the state after idle.  That would be a case where
> an atomic suspend would be needed, but the resume would be "normal"
> sometime later when the device is next needed.

Still, I think it wouldn't hurt if "fast resume" is used in that case.

I'm trying to figure out if atomic suspend should always imply atomic resume.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Thu, 7 Oct 2010, Rafael J. Wysocki wrote:

> > 	If the PM core simply avoids releasing dev->power.lock before
> > 	invoking the runtime_suspend or runtime_resume callback, the
> > 	end result is almost the same as with busy-waiting.
> 
> This is slightly more complicated, because suspend is a bit different from
> resume.  I think it's generally acceptable to refuse to do a "fast path" suspend
> if the device state is not RPM_ACTIVE at the moment, but refusing to do
> a "fast path" resume may be more problematic (more on that later).

That's what I thought too, but Kevin has suggested this might not be 
so.

> The particular case we need to handle (I think) is that some devices need to be
> resumed "atomically" as a part of bringing a CPU package out of a C-like-state
> (Kevin, is that correct?).  In that case not only we can't defer the resume (by
> using a workqueue), but also we have to finish the resume within strict time
> constraints (the time to leave the given C-like-state is one of the parameters
> used by cpuidle governors to decide which state to choose).

To what extent can this really happen?  If the CPU was previously in a 
C-like state then it couldn't be in the middle of changing the device's 
power state.  Maybe a different CPU was suspending or resuming the 
device, but if that's so then why would this CPU need to resume it as 
part of bringing the package out of the C-like state?

> On the other hand, busy waiting (by looping) in the case the state is
> RPM_RESUMING is as though the callbacks were executed under a spinlock and we
> happened to wait on it.  I'd prefer to avoid that.  Moreover, if the state is
> RPM_SUSPENDING at the time a "fast path" resume is started, we are almost
> guaranteed to violate the time constraints (by busy waiting for the suspend to
> complete and carrying out the resume).

I'm not convinced that the time constraints are all that hard.  At 
least, I can't think of a situation where we do face a hard time limit 
and also the device might be in the middle of a transition when we need 
to resume it.

> So, here's an idea:
> 
> First, let's introduce two flags in struct dev_pm_info, irq_safe and
> fast_resume.  The former has to be set so that the things above work.
> 
> Second, let's add a new flag to pass to __pm_runtime_{suspend|resume}(),
> RPM_FAST_RESUME such that if it is passed to __pm_runtime_suspend(), it
> causes fast_resume to be set for the given device (if the status is already
> RPM_SUSPENDED, it just sets the flag, otherwise it suspends the device
> normally and then sets the flag).  Now, if fast_resume is set,
> __pm_runtime_resume() has to be passed RPM_FAST_RESUME, or it will
> fail (it also will fail if passed RPM_FAST_RESUME and fast_resume isn't
> set).  In that case the status has to be RPM_SUSPENDED or RPM_RESUMING
> (otherwise fast_resume won't be set)

Why not?  Does fast_resume ever get cleared?

>  and if it is RPM_RESUMING, this means
> that the other resume has been called with RPM_FAST_RESUME too (it would
> fail otherwise), so it's fine to busy loop until the status is RPM_SUSPENDED
> (it's the caller's problem to avoid that).  RPM_FAST_RESUME causes
> __pm_runtime_resume() to avod turning interrupts on.
> 
> If necessary, there may be a flag for __pm_runtime_suspend() that will
> carry out "atomic" suspend, but I'm not sure if that's necessary.  Kevin?

This seems much more complicated than we need.  Why not require for any
particular device that all resumes and suspends are either always fast
or always normal (i.e., always done with interrupts disabled or always
done with interrupts enabled)?  That's what the original patch does.

Sure, maybe some devices don't need to have fast suspend all the time.  
But we don't lose anything by requiring them to always use it in order
to gain the benefit of fast resume.

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Thu, 7 Oct 2010, Kevin Hilman wrote:

> My confusion is not about the use of spinlocks, it's a question of what
> is being busy-waited for, and the thread that is being waited for is
> going to complete when interrupts are disabled.
> 
> Sorry to be dense, but can you (re)summarize what you're proposing as I
> think I'm getting mixed up with all the various options we've been
> tossing around.
> 
> If it can work, I'm certainly in favor of a busy-wait approach as it 
> really ensures that sync requests are handled quickly.

Okay, here's the story in a nutshell.  Allowing a subsystem's or
driver's runtime-PM callbacks to run with interrupts disabled faces two
obstacles:

   (1): We don't want two different CPUs to run callbacks for the
	same device at the same time.  So if a callback is already
	running on one CPU (i.e., if the device's runtime status is
	either SUSPENDING or RESUMING) then another CPU can't be
	allowed to invoke a callback.

	Thus, you can't do a synchronous pm_runtime_resume_irq()
	if the device is in the middle of a suspend or resume
	operation.  We're left with two choices: Fail the synchronous
	call and force the driver to defer matters to a workqueue
	(possibly masking an IRQ line in the meantime), or busy-wait
	until the concurrent operation finishes.

	If the PM core simply avoids releasing dev->power.lock before
	invoking the runtime_suspend or runtime_resume callback, the
	end result is almost the same as with busy-waiting.

   (2): In general we can't resume a device if its parent is suspended.
	If the parent's runtime_resume routine needs to run with
	interrupts enabled then there's no way to resume the device
	while keeping interrupts disabled.

	Possible solutions involve, again, deferring matters to a
	workqueue, or else simply not allowing the situation to arise
	in the first place (forbid a device to have interrupt-disabled 
	callbacks unless its parent does too or the parent doesn't use 
	runtime PM at all).

In general I'm against the solutions that require a workqueue.  Raphael 
appears to favor workqueues for (1) and be against them for (2).

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Wed, 6 Oct 2010, Rafael J. Wysocki wrote:
>
>> Defer the resume.  That's the only thing you can do in any case, whether you're
>> going to busy loop or just use a workqueue.
>
> They are not the same.  With a busy-wait you handle the device as soon
> as possible, before the interrupt routine returns.  With a workqueue
> you have to mask the entire IRQ line, possibly losing interrupt
> requests from other devices, until the workqueue routine can run.
>
>> > On the whole, I don't see any striking reason for the PM core not to 
>> > busy-wait during a concurrent state change.
>> 
>> I do.  That shouldn't happen in a fast path and we're talking about one,
>> aren't we?  Besides, I don't like busy waiting as a rule.
>
> On Wed, 6 Oct 2010, Kevin Hilman wrote:
>
>> Not sure I follow where you're going with this last paragraph.  Of
>> course, calls from ISR context cannot busy wait.
>
> What do you guys think spin_lock() does?  It busy-waits until the lock 
> is free!  If you don't like busy-waiting then you don't like spinlocks, 
> and if you believe ISR's can't busy-wait then you believe they can't 
> acquire spinlocks.  :-)

My confusion is not about the use of spinlocks, it's a question of what
is being busy-waited for, and the thread that is being waited for is
going to complete when interrupts are disabled.

Sorry to be dense, but can you (re)summarize what you're proposing as I
think I'm getting mixed up with all the various options we've been
tossing around.

If it can work, I'm certainly in favor of a busy-wait approach as it 
really ensures that sync requests are handled quickly.

Kevin

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Wed, 6 Oct 2010, Rafael J. Wysocki wrote:

> Defer the resume.  That's the only thing you can do in any case, whether you're
> going to busy loop or just use a workqueue.

They are not the same.  With a busy-wait you handle the device as soon
as possible, before the interrupt routine returns.  With a workqueue
you have to mask the entire IRQ line, possibly losing interrupt
requests from other devices, until the workqueue routine can run.

> > On the whole, I don't see any striking reason for the PM core not to 
> > busy-wait during a concurrent state change.
> 
> I do.  That shouldn't happen in a fast path and we're talking about one,
> aren't we?  Besides, I don't like busy waiting as a rule.

On Wed, 6 Oct 2010, Kevin Hilman wrote:

> Not sure I follow where you're going with this last paragraph.  Of
> course, calls from ISR context cannot busy wait.

What do you guys think spin_lock() does?  It busy-waits until the lock 
is free!  If you don't like busy-waiting then you don't like spinlocks, 
and if you believe ISR's can't busy-wait then you believe they can't 
acquire spinlocks.  :-)

Really, what I'm talking about is not much different from continuing to
hold dev->power.lock across the runtime_resume and runtime_suspend
callbacks.  Would you prefer to do that instead of explicitly
busy-waiting?  There would be two disadvantages.  First, the
runtime_suspend routine would not be able to call pm_request_idle() or
pm_schedule_suspend() and then return -EBUSY.  Second, if the driver
uses a private lock then the callback routines might not be able to
acquire it.

On Wed, 6 Oct 2010, Rafael J. Wysocki wrote:

> Overall, we seem to have a corner case to cover and we're considering doing
> intrusive changes to the framework because of that, even though that changes
> may not be really necessary in practice.  I think we should focus more on the
> corner case instead.

I'm not sure what you mean.

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Wednesday, October 06, 2010, Alan Stern wrote:
> On Wed, 6 Oct 2010, Kevin Hilman wrote:
> 
> > >> I think I can live with the above restrictions (the _irq methods failing
> > >> unless they can immediately run.)  For the rare corner cases I've
> > >> currently run into, this will work fine as they happen because of a
> > >> wakeup IRQ, where we know the device is RPM_SUSPENDED.
> > >
> > > Then what will the driver do if it gets a wakeup IRQ but it can't
> > > resume the device because some other thread is in the middle of a
> > > suspend or resume?
> > 
> > In those cases, I guess it will have to return IRQ_NONE, and print some
> > sort of error.
> 
> Without handling the IRQ?  I guess if the transient state (SUSPENDING
> or RESUMING) ends quickly enough then the kernel won't permanently
> disable the IRQ line (although getting hundreds of repeated error
> messages in the system log might prove daunting).  Would you want to 
> rely on that?
> 
> >  Alternatively, it could fall back to the high-latency
> > case of masking the IRQ and doing an asynchronous call then call the ISR
> > in the runtime_resume callback.
> 
> Yes, this probably would be acceptable since it wouldn't happen very 
> often.  Still, having two different pathways (one of which is very low 
> probability) usually isn't a good idea.
> 
> > For the corner cases that I've run into, other transitions will not be
> > in progress because system has just woken up (so no threads are in the
> > middle of suspends or resumes) and the IRQ fires as soon as pm_idle
> > enables interrupts, and before there's a chance to schedule anything
> > else.
> 
> Ah, but once you integrate the runtime PM framework into your driver, 
> you run the risk of resumes occurring for reasons that aren't under 
> your control.  For example, the user can force a runtime-suspended 
> device to resume by writing "on" to the device's power/control sysfs 
> attribute.  What would happen if a wakeup IRQ arrived just as the 
> resume was starting?

Defer the resume.  That's the only thing you can do in any case, whether you're
going to busy loop or just use a workqueue.

> (Actually, this particular failure mode shouldn't concern you very much
> since it applies only to SMP systems.  But it's important to think of
> the future -- I can imagine SMP OMAPs coming out before too long.)
> 
> On the whole, I don't see any striking reason for the PM core not to 
> busy-wait during a concurrent state change.

I do.  That shouldn't happen in a fast path and we're talking about one,
aren't we?  Besides, I don't like busy waiting as a rule.

> Refusing to wake up a 
> suspended parent ... okay, maybe that's a little more defensible.  
> Especially since in many or most cases the parent (if there is one) 
> will probably be runtime-PM-disabled anyway.

Either we do that, or we _must_ require that the parent's resume be irq-safe
(and the same for all the parents up in the device chain).

Overall, we seem to have a corner case to cover and we're considering doing
intrusive changes to the framework because of that, even though that changes
may not be really necessary in practice.  I think we should focus more on the
corner case instead.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Wed, 6 Oct 2010, Kevin Hilman wrote:
>
>> >> I think I can live with the above restrictions (the _irq methods failing
>> >> unless they can immediately run.)  For the rare corner cases I've
>> >> currently run into, this will work fine as they happen because of a
>> >> wakeup IRQ, where we know the device is RPM_SUSPENDED.
>> >
>> > Then what will the driver do if it gets a wakeup IRQ but it can't
>> > resume the device because some other thread is in the middle of a
>> > suspend or resume?
>> 
>> In those cases, I guess it will have to return IRQ_NONE, and print some
>> sort of error.
>
> Without handling the IRQ?  I guess if the transient state (SUSPENDING
> or RESUMING) ends quickly enough then the kernel won't permanently
> disable the IRQ line (although getting hundreds of repeated error
> messages in the system log might prove daunting).  Would you want to 
> rely on that?

Probably not, I think the delayed approach is better.

>>  Alternatively, it could fall back to the high-latency
>> case of masking the IRQ and doing an asynchronous call then call the ISR
>> in the runtime_resume callback.
>
> Yes, this probably would be acceptable since it wouldn't happen very 
> often.  Still, having two different pathways (one of which is very low 
> probability) usually isn't a good idea.

>> For the corner cases that I've run into, other transitions will not be
>> in progress because system has just woken up (so no threads are in the
>> middle of suspends or resumes) and the IRQ fires as soon as pm_idle
>> enables interrupts, and before there's a chance to schedule anything
>> else.
>
> Ah, but once you integrate the runtime PM framework into your driver, 
> you run the risk of resumes occurring for reasons that aren't under 
> your control.  For example, the user can force a runtime-suspended 
> device to resume by writing "on" to the device's power/control sysfs 
> attribute.  What would happen if a wakeup IRQ arrived just as the 
> resume was starting?

I guess the "mask-IRQ, pm_runtime_get(), return" approach would work
here too.  But I agree with you about the drivers having to handle both
of these cases is not the greatest.

> (Actually, this particular failure mode shouldn't concern you very much
> since it applies only to SMP systems.  But it's important to think of
> the future -- I can imagine SMP OMAPs coming out before too long.)

It already concerns me since I'm working on SMP OMAPs too.  The OMAP4 is
a dual ARM Cortex A9[1].

> On the whole, I don't see any striking reason for the PM core not to
> busy-wait during a concurrent state change.  Refusing to wake up a
> suspended parent ... okay, maybe that's a little more defensible.
> Especially since in many or most cases the parent (if there is one)
> will probably be runtime-PM-disabled anyway.

Not sure I follow where you're going with this last paragraph.  Of
course, calls from ISR context cannot busy wait.

Kevin


[1] http://focus.ti.com/general/docs/wtbu/wtbuproductcontent.tsp?templateId=6123&navigationId=12842&contentId=53247

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Wed, 6 Oct 2010, Kevin Hilman wrote:

> >> I think I can live with the above restrictions (the _irq methods failing
> >> unless they can immediately run.)  For the rare corner cases I've
> >> currently run into, this will work fine as they happen because of a
> >> wakeup IRQ, where we know the device is RPM_SUSPENDED.
> >
> > Then what will the driver do if it gets a wakeup IRQ but it can't
> > resume the device because some other thread is in the middle of a
> > suspend or resume?
> 
> In those cases, I guess it will have to return IRQ_NONE, and print some
> sort of error.

Without handling the IRQ?  I guess if the transient state (SUSPENDING
or RESUMING) ends quickly enough then the kernel won't permanently
disable the IRQ line (although getting hundreds of repeated error
messages in the system log might prove daunting).  Would you want to 
rely on that?

>  Alternatively, it could fall back to the high-latency
> case of masking the IRQ and doing an asynchronous call then call the ISR
> in the runtime_resume callback.

Yes, this probably would be acceptable since it wouldn't happen very 
often.  Still, having two different pathways (one of which is very low 
probability) usually isn't a good idea.

> For the corner cases that I've run into, other transitions will not be
> in progress because system has just woken up (so no threads are in the
> middle of suspends or resumes) and the IRQ fires as soon as pm_idle
> enables interrupts, and before there's a chance to schedule anything
> else.

Ah, but once you integrate the runtime PM framework into your driver, 
you run the risk of resumes occurring for reasons that aren't under 
your control.  For example, the user can force a runtime-suspended 
device to resume by writing "on" to the device's power/control sysfs 
attribute.  What would happen if a wakeup IRQ arrived just as the 
resume was starting?

(Actually, this particular failure mode shouldn't concern you very much
since it applies only to SMP systems.  But it's important to think of
the future -- I can imagine SMP OMAPs coming out before too long.)

On the whole, I don't see any striking reason for the PM core not to 
busy-wait during a concurrent state change.  Refusing to wake up a 
suspended parent ... okay, maybe that's a little more defensible.  
Especially since in many or most cases the parent (if there is one) 
will probably be runtime-PM-disabled anyway.

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Wednesday, October 06, 2010, Alan Stern wrote:
> On Tue, 5 Oct 2010, Kevin Hilman wrote:
> 
> > Alan Stern <stern@rowland.harvard.edu> writes:
> > 
> > > On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:
> > >
> > >> > > In my opinion the _irq operations should really be one-shot, without
> > >> > > any looping, waking up parents etc.  I mean, if the parent is not RPM_ACTIVE,
> > >> > > the _irq resume should immediately fail and analogously for the _irq
> > >> > > suspend.  And so on.  As simple as reasonably possible.
> > >> > 
> > >> > But what if the device's own status is currently SUSPENDING or
> > >> > RESUMING?  Do you want to fail when that happens, instead of waiting
> > >> > for the concurrent operation to finish?
> > >> 
> > >> Yes.  IMO an _irq() suspend should only be allowed if the status is RPM_ACTIVE
> > >> and an _irq() resume should fail if the status is not RPM_SUSPENDED.  The
> > >> error code returned should depend on the situation, though.
> > >> 
> > >> > There's no way to prevent this
> > >> > on SMP systems, because a wakeup request can arrive while a
> > >> > software-initiated resume is in progress.
> > >> 
> > >> I know that. :-)
> > >
> > > I suspect Kevin will not want to live with this restriction, but I'd
> > > like to hear from him.  Kevin?
> > 
> > [ Apologies for the delays... I've been running around preparing OMAP PM
> >   stuff for the upcoming merge window. ]
> > 
> > I think I can live with the above restrictions (the _irq methods failing
> > unless they can immediately run.)  For the rare corner cases I've
> > currently run into, this will work fine as they happen because of a
> > wakeup IRQ, where we know the device is RPM_SUSPENDED.
> 
> Then what will the driver do if it gets a wakeup IRQ but it can't
> resume the device because some other thread is in the middle of a
> suspend or resume?

Queue up a resume request and return.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Tue, 5 Oct 2010, Kevin Hilman wrote:
>
>> Alan Stern <stern@rowland.harvard.edu> writes:
>> 
>> > On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:
>> >
>> >> > > In my opinion the _irq operations should really be one-shot, without
>> >> > > any looping, waking up parents etc.  I mean, if the parent is not RPM_ACTIVE,
>> >> > > the _irq resume should immediately fail and analogously for the _irq
>> >> > > suspend.  And so on.  As simple as reasonably possible.
>> >> > 
>> >> > But what if the device's own status is currently SUSPENDING or
>> >> > RESUMING?  Do you want to fail when that happens, instead of waiting
>> >> > for the concurrent operation to finish?
>> >> 
>> >> Yes.  IMO an _irq() suspend should only be allowed if the status is RPM_ACTIVE
>> >> and an _irq() resume should fail if the status is not RPM_SUSPENDED.  The
>> >> error code returned should depend on the situation, though.
>> >> 
>> >> > There's no way to prevent this
>> >> > on SMP systems, because a wakeup request can arrive while a
>> >> > software-initiated resume is in progress.
>> >> 
>> >> I know that. :-)
>> >
>> > I suspect Kevin will not want to live with this restriction, but I'd
>> > like to hear from him.  Kevin?
>> 
>> [ Apologies for the delays... I've been running around preparing OMAP PM
>>   stuff for the upcoming merge window. ]
>> 
>> I think I can live with the above restrictions (the _irq methods failing
>> unless they can immediately run.)  For the rare corner cases I've
>> currently run into, this will work fine as they happen because of a
>> wakeup IRQ, where we know the device is RPM_SUSPENDED.
>
> Then what will the driver do if it gets a wakeup IRQ but it can't
> resume the device because some other thread is in the middle of a
> suspend or resume?

In those cases, I guess it will have to return IRQ_NONE, and print some
sort of error.  Alternatively, it could fall back to the high-latency
case of masking the IRQ and doing an asynchronous call then call the ISR
in the runtime_resume callback.

For the corner cases that I've run into, other transitions will not be
in progress because system has just woken up (so no threads are in the
middle of suspends or resumes) and the IRQ fires as soon as pm_idle
enables interrupts, and before there's a chance to schedule anything
else.

Kevin

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Tue, 5 Oct 2010, Kevin Hilman wrote:

> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:
> >
> >> > > In my opinion the _irq operations should really be one-shot, without
> >> > > any looping, waking up parents etc.  I mean, if the parent is not RPM_ACTIVE,
> >> > > the _irq resume should immediately fail and analogously for the _irq
> >> > > suspend.  And so on.  As simple as reasonably possible.
> >> > 
> >> > But what if the device's own status is currently SUSPENDING or
> >> > RESUMING?  Do you want to fail when that happens, instead of waiting
> >> > for the concurrent operation to finish?
> >> 
> >> Yes.  IMO an _irq() suspend should only be allowed if the status is RPM_ACTIVE
> >> and an _irq() resume should fail if the status is not RPM_SUSPENDED.  The
> >> error code returned should depend on the situation, though.
> >> 
> >> > There's no way to prevent this
> >> > on SMP systems, because a wakeup request can arrive while a
> >> > software-initiated resume is in progress.
> >> 
> >> I know that. :-)
> >
> > I suspect Kevin will not want to live with this restriction, but I'd
> > like to hear from him.  Kevin?
> 
> [ Apologies for the delays... I've been running around preparing OMAP PM
>   stuff for the upcoming merge window. ]
> 
> I think I can live with the above restrictions (the _irq methods failing
> unless they can immediately run.)  For the rare corner cases I've
> currently run into, this will work fine as they happen because of a
> wakeup IRQ, where we know the device is RPM_SUSPENDED.

Then what will the driver do if it gets a wakeup IRQ but it can't
resume the device because some other thread is in the middle of a
suspend or resume?

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Sunday, October 03, 2010, Alan Stern wrote:
> On Sun, 3 Oct 2010, Rafael J. Wysocki wrote:
> 
> > On Saturday, October 02, 2010, Alan Stern wrote:
> > > On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:
> > ...
> > > > At the moment it suspends when the network cable is removed from the device
> > > > and the hack I mentioned is used during the resume after the cable has been
> > > > reinserted (it checks if the cable is still there and schedules suspend if not).
> > > 
> > > That does seem like a strange hack.  Instead of scheduling a suspend,
> > > why not simply do a suspend as soon as you learn that the cable has
> > > been removed again?  Is there a problem about the connection status
> > > bouncing?
> > 
> > I don't remember 100%, but ISTR there was a problem with that.
> 
> Regardless, it seems like the sort of thing autosuspend would handle
> easily with no need for idle notifications.  Just call
> pm_runtime_mark_last_busy when the cable is plugged in, then do the
> runtime resume, and then call pm_request_autosuspend.  The check for 
> the cable being disconnected would have to move from the runtime_idle 
> callback to the runtime_suspend callback -- but then the runtime_idle 
> callback wouldn't have to be present at all.

It would be necessary because of the PCI bus type's implementation of it.

Anyway, I'll have a look, but I don't use the hardware any more, so testing
would be rather hard. :-)

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Sun, 3 Oct 2010, Rafael J. Wysocki wrote:

> On Saturday, October 02, 2010, Alan Stern wrote:
> > On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:
> ...
> > > At the moment it suspends when the network cable is removed from the device
> > > and the hack I mentioned is used during the resume after the cable has been
> > > reinserted (it checks if the cable is still there and schedules suspend if not).
> > 
> > That does seem like a strange hack.  Instead of scheduling a suspend,
> > why not simply do a suspend as soon as you learn that the cable has
> > been removed again?  Is there a problem about the connection status
> > bouncing?
> 
> I don't remember 100%, but ISTR there was a problem with that.

Regardless, it seems like the sort of thing autosuspend would handle
easily with no need for idle notifications.  Just call
pm_runtime_mark_last_busy when the cable is plugged in, then do the
runtime resume, and then call pm_request_autosuspend.  The check for 
the cable being disconnected would have to move from the runtime_idle 
callback to the runtime_suspend callback -- but then the runtime_idle 
callback wouldn't have to be present at all.

Alan Stern

P.S.: Kevin, it looks like your name somehow got removed from the CC 
list on my last message.  This was odd because the message contained a 
question directed at you.  In case you didn't receive it, here's a 
link:

https://lists.linux-foundation.org/pipermail/linux-pm/2010-October/028908.html

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Saturday, October 02, 2010, Alan Stern wrote:
> On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:
...
> > At the moment it suspends when the network cable is removed from the device
> > and the hack I mentioned is used during the resume after the cable has been
> > reinserted (it checks if the cable is still there and schedules suspend if not).
> 
> That does seem like a strange hack.  Instead of scheduling a suspend,
> why not simply do a suspend as soon as you learn that the cable has
> been removed again?  Is there a problem about the connection status
> bouncing?

I don't remember 100%, but ISTR there was a problem with that.

> > If we removed the immediate idle notification after a successful resume, it
> > might need to be reworked slightly.
> 
> My suggestion was that we remove the idle notification after a failed 
> suspend, not the notification after a successful resume.

And I said I was fine with that.

Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:
>
>> > > In my opinion the _irq operations should really be one-shot, without
>> > > any looping, waking up parents etc.  I mean, if the parent is not RPM_ACTIVE,
>> > > the _irq resume should immediately fail and analogously for the _irq
>> > > suspend.  And so on.  As simple as reasonably possible.
>> > 
>> > But what if the device's own status is currently SUSPENDING or
>> > RESUMING?  Do you want to fail when that happens, instead of waiting
>> > for the concurrent operation to finish?
>> 
>> Yes.  IMO an _irq() suspend should only be allowed if the status is RPM_ACTIVE
>> and an _irq() resume should fail if the status is not RPM_SUSPENDED.  The
>> error code returned should depend on the situation, though.
>> 
>> > There's no way to prevent this
>> > on SMP systems, because a wakeup request can arrive while a
>> > software-initiated resume is in progress.
>> 
>> I know that. :-)
>
> I suspect Kevin will not want to live with this restriction, but I'd
> like to hear from him.  Kevin?

[ Apologies for the delays... I've been running around preparing OMAP PM
  stuff for the upcoming merge window. ]

I think I can live with the above restrictions (the _irq methods failing
unless they can immediately run.)  For the rare corner cases I've
currently run into, this will work fine as they happen because of a
wakeup IRQ, where we know the device is RPM_SUSPENDED.

Kevin

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Friday, October 01, 2010, Alan Stern wrote:
> On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:
> 
> > On Thursday, September 30, 2010, Alan Stern wrote:
> > > This patch (as1431) adds a synchronous runtime-PM interface suitable
> > > for use in interrupt handlers.  Four new helper functions are defined:
> > > 
> > > 	pm_runtime_suspend_irq(), pm_runtime_resume_irq(),
> > > 	pm_runtime_get_sync_irq(), pm_runtime_put_sync_irq(),
> > > 
> > > together with pm_runtime_callbacks_in_irq(), which subsystems use to
> > > tell the PM core that the runtime callbacks should be invoked with
> > > interrupts disabled.
> > 
> > BTW, I like some changes made by your patch that aren't really related to
> > the issue at hand, so I think the patch below can be applied regardless of
> > the other changes, unless I made a mistake I can see now.
> 
> It looks like a good change, but you forgot to preserve the assignments 
> to dev->power.runtime_error.

That's correct and there's a difference betwee _idle and the other cases
because of that.  I'll send updated patch with a changelog shortly.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:

> > >  If RPM_IRQ is not set, but
> > > power.callbacks_in_irq is set, we should fall back to the normal behavior
> > > (ie. do not avoid sleeping).
> > 
> > By "do not avoid sleeping", do you mean that 
> > 
> > 	(1) the helper functions should be allowed to sleep, or
> > 
> > 	(2) the callbacks should be invoked with interrupts enabled?
> > 
> > The patch already does (1).  By contrast, (2) isn't safe.  It means
> > there is a risk of having one thread do a busy-wait with interrupts
> > disabled, waiting for another thread that can sleep.
> 
> In my opinion the _irq operations should really be one-shot, without
> any looping, waking up parents etc.  I mean, if the parent is not RPM_ACTIVE,
> the _irq resume should immediately fail and analogously for the _irq
> suspend.  And so on.  As simple as reasonably possible.

But what if the device's own status is currently SUSPENDING or
RESUMING?  Do you want to fail when that happens, instead of waiting
for the concurrent operation to finish?  There's no way to prevent this
on SMP systems, because a wakeup request can arrive while a
software-initiated resume is in progress.

> > Going further, I'm still a little dubious about the whole point of
> > having idle notifications at all.  I don't see why the PM core can't
> > notify a subsystem that a device is idle just by calling the
> > runtime_suspend routine.
> 
> Well, calling _idle is like saying "I think this device may be idle, please
> consider suspending it", while calling _suspend is like saying "suspend
> this device unless you can't".  I think that's enough of a difference to
> have separate _idle.

> A subsystem or a driver may want to schedule the suspend with a timeout
> if _idle is called instead of just suspending synchronously.  The r8169 driver
> actually does something similar (although a bit more complicated).

What you are describing is basically what autosuspend was intended to
accomplish (that is, formalizing the difference between "the device
just became idle, you should consider when to suspend it" and "the
device has been idle for a sufficiently long time, please suspend it
now").  Would the r8169 driver be simplified if it used autosuspend?

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Thursday, September 30, 2010, Alan Stern wrote:
> On Thu, 30 Sep 2010, Rafael J. Wysocki wrote:
> 
> > > --- usb-2.6.orig/include/linux/pm.h
> > > +++ usb-2.6/include/linux/pm.h
> > > @@ -485,6 +485,7 @@ struct dev_pm_info {
> > >  	unsigned int		run_wake:1;
> > >  	unsigned int		runtime_auto:1;
> > >  	unsigned int		no_callbacks:1;
> > > +	unsigned int		callbacks_in_irq:1;
> > 
> > I kind of don't like this name.  What about irq_safe ?
> 
> Sure, that's a better name.
> 
> > > --- usb-2.6.orig/include/linux/pm_runtime.h
> > > +++ usb-2.6/include/linux/pm_runtime.h
> > > @@ -21,6 +21,7 @@
> > >  #define RPM_GET_PUT		0x04	/* Increment/decrement the
> > >  					    usage_count */
> > >  #define RPM_AUTO		0x08	/* Use autosuspend_delay */
> > > +#define RPM_IRQ			0x10	/* Don't enable interrupts */
> > >  
> > >  #ifdef CONFIG_PM_RUNTIME
> > >  
> > > @@ -40,6 +41,7 @@ extern int pm_generic_runtime_idle(struc
> > >  extern int pm_generic_runtime_suspend(struct device *dev);
> > >  extern int pm_generic_runtime_resume(struct device *dev);
> > >  extern void pm_runtime_no_callbacks(struct device *dev);
> > > +extern void pm_runtime_callbacks_in_irq(struct device *dev);
> > 
> > Perhaps this one can be called pm_runtime_irq_safe() in analogy to the struct
> > member?
> 
> Yes.
> 
> > Hmm.  This looks rather complicated.  I don't really feel good about this
> > change.  It seems to me that it might be better to actually implement
> > the _irq() helpers from scratch.  I think I'll try to do that.
> 
> That might work out better, but there will be a substantial amount of
> code duplication.  Try it and see how it turns out.

I think you're right, but I'm not sure yet.

> > Overall, it looks like there's some overlap between RPM_IRQ and
> > power.callbacks_in_irq, where the latter may influence the behavior of the
> > helpers even if RPM_IRQ is unset.
> 
> Exactly.  That was intentional.
> 
> > I think we should return error code if RPM_IRQ is used, but 
> > power.callbacks_in_irq is not set.
> 
> The patch does that.
> 
> >  If RPM_IRQ is not set, but
> > power.callbacks_in_irq is set, we should fall back to the normal behavior
> > (ie. do not avoid sleeping).
> 
> By "do not avoid sleeping", do you mean that 
> 
> 	(1) the helper functions should be allowed to sleep, or
> 
> 	(2) the callbacks should be invoked with interrupts enabled?
> 
> The patch already does (1).  By contrast, (2) isn't safe.  It means
> there is a risk of having one thread do a busy-wait with interrupts
> disabled, waiting for another thread that can sleep.

In my opinion the _irq operations should really be one-shot, without
any looping, waking up parents etc.  I mean, if the parent is not RPM_ACTIVE,
the _irq resume should immediately fail and analogously for the _irq
suspend.  And so on.  As simple as reasonably possible.

> > That's why I think it's better to change the name of power.callbacks_in_irq
> > to power.irq_safe.
> > 
> > Also I think we should refuse to set power.callbacks_in_irq for a device
> > whose partent (1) doesn't have power.callbacks_in_irq and (2) doesn't
> > have power.ignore_children set , and (3) doesn't have power.disable_depth > 0.
> > Then, once we've set power.callbacks_in_irq for a device, we should take
> > this into consideration when trying to change the parent's settings.
> 
> That's not a bad idea, but I don't know if it's necessary in practice.  
> With the current patch, if you violate this condition you will simply
> encounter an error when the PM core refuses to resume a sleeping
> parent.  Maybe we should allow violations and the resulting refusals
> but print a warning message in the system log.

Yes, see above.  I think printing a message in case a suspended parent
prevented an _irq resume from happening, for example, is a good idea.

> > There probably is more subtelties like this, but I can't see them at the moment.
> 
> There are some subtle aspects related to the interplay between idle and
> the other callbacks.  In principle, the idle and suspend callbacks
> doesn't need to have interrupts disabled -- we don't care if interrupt
> handlers can't do synchronous suspends; they only need synchronous
> resumes.  But I wanted to keep things simple, so I treated all the
> callbacks the same.

OK

> Going further, I'm still a little dubious about the whole point of
> having idle notifications at all.  I don't see why the PM core can't
> notify a subsystem that a device is idle just by calling the
> runtime_suspend routine.

Well, calling _idle is like saying "I think this device may be idle, please
consider suspending it", while calling _suspend is like saying "suspend
this device unless you can't".  I think that's enough of a difference to
have separate _idle.

> The routine doesn't have to suspend if it
> doesn't want to, and it can always schedule a delayed suspend or an
> autosuspend.  Maybe you know of a use case where having explicit idle
> notifications really does help, but I can't think of any.

A subsystem or a driver may want to schedule the suspend with a timeout
if _idle is called instead of just suspending synchronously.  The r8169 driver
actually does something similar (although a bit more complicated).

> One more thing: I'm not sure we really need the "notify" variable in
> rpm_suspend.  When a suspend callback fails, it means the device is in
> use.  Whatever is using the device should then be responsible for
> sending its own idle notification when it is finished; we shouldn't
> need to send a notification as soon as the suspend callback fails.

OK, I'm fine with removing it or calling rpm_idle(dev, RPM_ASYNC)
instead of rpm_idle(dev, 0) in there.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Thu, 30 Sep 2010, Rafael J. Wysocki wrote:

> > --- usb-2.6.orig/include/linux/pm.h
> > +++ usb-2.6/include/linux/pm.h
> > @@ -485,6 +485,7 @@ struct dev_pm_info {
> >  	unsigned int		run_wake:1;
> >  	unsigned int		runtime_auto:1;
> >  	unsigned int		no_callbacks:1;
> > +	unsigned int		callbacks_in_irq:1;
> 
> I kind of don't like this name.  What about irq_safe ?

Sure, that's a better name.

> > --- usb-2.6.orig/include/linux/pm_runtime.h
> > +++ usb-2.6/include/linux/pm_runtime.h
> > @@ -21,6 +21,7 @@
> >  #define RPM_GET_PUT		0x04	/* Increment/decrement the
> >  					    usage_count */
> >  #define RPM_AUTO		0x08	/* Use autosuspend_delay */
> > +#define RPM_IRQ			0x10	/* Don't enable interrupts */
> >  
> >  #ifdef CONFIG_PM_RUNTIME
> >  
> > @@ -40,6 +41,7 @@ extern int pm_generic_runtime_idle(struc
> >  extern int pm_generic_runtime_suspend(struct device *dev);
> >  extern int pm_generic_runtime_resume(struct device *dev);
> >  extern void pm_runtime_no_callbacks(struct device *dev);
> > +extern void pm_runtime_callbacks_in_irq(struct device *dev);
> 
> Perhaps this one can be called pm_runtime_irq_safe() in analogy to the struct
> member?

Yes.

> Hmm.  This looks rather complicated.  I don't really feel good about this
> change.  It seems to me that it might be better to actually implement
> the _irq() helpers from scratch.  I think I'll try to do that.

That might work out better, but there will be a substantial amount of
code duplication.  Try it and see how it turns out.

> Overall, it looks like there's some overlap between RPM_IRQ and
> power.callbacks_in_irq, where the latter may influence the behavior of the
> helpers even if RPM_IRQ is unset.

Exactly.  That was intentional.

> I think we should return error code if RPM_IRQ is used, but 
> power.callbacks_in_irq is not set.

The patch does that.

>  If RPM_IRQ is not set, but
> power.callbacks_in_irq is set, we should fall back to the normal behavior
> (ie. do not avoid sleeping).

By "do not avoid sleeping", do you mean that 

	(1) the helper functions should be allowed to sleep, or

	(2) the callbacks should be invoked with interrupts enabled?

The patch already does (1).  By contrast, (2) isn't safe.  It means
there is a risk of having one thread do a busy-wait with interrupts
disabled, waiting for another thread that can sleep.

> That's why I think it's better to change the name of power.callbacks_in_irq
> to power.irq_safe.
> 
> Also I think we should refuse to set power.callbacks_in_irq for a device
> whose partent (1) doesn't have power.callbacks_in_irq and (2) doesn't
> have power.ignore_children set , and (3) doesn't have power.disable_depth > 0.
> Then, once we've set power.callbacks_in_irq for a device, we should take
> this into consideration when trying to change the parent's settings.

That's not a bad idea, but I don't know if it's necessary in practice.  
With the current patch, if you violate this condition you will simply
encounter an error when the PM core refuses to resume a sleeping
parent.  Maybe we should allow violations and the resulting refusals
but print a warning message in the system log.

> There probably is more subtelties like this, but I can't see them at the moment.

There are some subtle aspects related to the interplay between idle and
the other callbacks.  In principle, the idle and suspend callbacks
doesn't need to have interrupts disabled -- we don't care if interrupt
handlers can't do synchronous suspends; they only need synchronous
resumes.  But I wanted to keep things simple, so I treated all the
callbacks the same.

Going further, I'm still a little dubious about the whole point of
having idle notifications at all.  I don't see why the PM core can't
notify a subsystem that a device is idle just by calling the
runtime_suspend routine.  The routine doesn't have to suspend if it
doesn't want to, and it can always schedule a delayed suspend or an
autosuspend.  Maybe you know of a use case where having explicit idle
notifications really does help, but I can't think of any.


One more thing: I'm not sure we really need the "notify" variable in
rpm_suspend.  When a suspend callback fails, it means the device is in
use.  Whatever is using the device should then be responsible for
sending its own idle notification when it is finished; we shouldn't
need to send a notification as soon as the suspend callback fails.

Also, this can theoretically lead to nasty loops.  Suspend callback
fails, idle notification gets sent, the runtime_idle routine tries to
do another pm_runtime_suspend, rinse and repeat...  This is made even
worse by the fact that the rpm_idle call after a failed suspend isn't
asynchronous -- you could overflow the kernel's stack.  Relying on the
driver to be properly written doesn't help: The device could become
busy when each suspend is about to begin, and it could become inactive
again just before the idle notification.  The driver could do the right 
thing at each step and still overflow the stack.

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Thu, 30 Sep 2010, Rafael J. Wysocki wrote:

> > --- usb-2.6.orig/include/linux/pm.h
> > +++ usb-2.6/include/linux/pm.h
> > @@ -485,6 +485,7 @@ struct dev_pm_info {
> >  	unsigned int		run_wake:1;
> >  	unsigned int		runtime_auto:1;
> >  	unsigned int		no_callbacks:1;
> > +	unsigned int		callbacks_in_irq:1;
> 
> I kind of don't like this name.  What about irq_safe ?

Sure, that's a better name.

> > --- usb-2.6.orig/include/linux/pm_runtime.h
> > +++ usb-2.6/include/linux/pm_runtime.h
> > @@ -21,6 +21,7 @@
> >  #define RPM_GET_PUT		0x04	/* Increment/decrement the
> >  					    usage_count */
> >  #define RPM_AUTO		0x08	/* Use autosuspend_delay */
> > +#define RPM_IRQ			0x10	/* Don't enable interrupts */
> >  
> >  #ifdef CONFIG_PM_RUNTIME
> >  
> > @@ -40,6 +41,7 @@ extern int pm_generic_runtime_idle(struc
> >  extern int pm_generic_runtime_suspend(struct device *dev);
> >  extern int pm_generic_runtime_resume(struct device *dev);
> >  extern void pm_runtime_no_callbacks(struct device *dev);
> > +extern void pm_runtime_callbacks_in_irq(struct device *dev);
> 
> Perhaps this one can be called pm_runtime_irq_safe() in analogy to the struct
> member?

Yes.

> Hmm.  This looks rather complicated.  I don't really feel good about this
> change.  It seems to me that it might be better to actually implement
> the _irq() helpers from scratch.  I think I'll try to do that.

That might work out better, but there will be a substantial amount of
code duplication.  Try it and see how it turns out.

> Overall, it looks like there's some overlap between RPM_IRQ and
> power.callbacks_in_irq, where the latter may influence the behavior of the
> helpers even if RPM_IRQ is unset.

Exactly.  That was intentional.

> I think we should return error code if RPM_IRQ is used, but 
> power.callbacks_in_irq is not set.

The patch does that.

>  If RPM_IRQ is not set, but
> power.callbacks_in_irq is set, we should fall back to the normal behavior
> (ie. do not avoid sleeping).

By "do not avoid sleeping", do you mean that 

	(1) the helper functions should be allowed to sleep, or

	(2) the callbacks should be invoked with interrupts enabled?

The patch already does (1).  By contrast, (2) isn't safe.  It means
there is a risk of having one thread do a busy-wait with interrupts
disabled, waiting for another thread that can sleep.

> That's why I think it's better to change the name of power.callbacks_in_irq
> to power.irq_safe.
> 
> Also I think we should refuse to set power.callbacks_in_irq for a device
> whose partent (1) doesn't have power.callbacks_in_irq and (2) doesn't
> have power.ignore_children set , and (3) doesn't have power.disable_depth > 0.
> Then, once we've set power.callbacks_in_irq for a device, we should take
> this into consideration when trying to change the parent's settings.

That's not a bad idea, but I don't know if it's necessary in practice.  
With the current patch, if you violate this condition you will simply
encounter an error when the PM core refuses to resume a sleeping
parent.  Maybe we should allow violations and the resulting refusals
but print a warning message in the system log.

> There probably is more subtelties like this, but I can't see them at the moment.

There are some subtle aspects related to the interplay between idle and
the other callbacks.  In principle, the idle and suspend callbacks
doesn't need to have interrupts disabled -- we don't care if interrupt
handlers can't do synchronous suspends; they only need synchronous
resumes.  But I wanted to keep things simple, so I treated all the
callbacks the same.

Going further, I'm still a little dubious about the whole point of
having idle notifications at all.  I don't see why the PM core can't
notify a subsystem that a device is idle just by calling the
runtime_suspend routine.  The routine doesn't have to suspend if it
doesn't want to, and it can always schedule a delayed suspend or an
autosuspend.  Maybe you know of a use case where having explicit idle
notifications really does help, but I can't think of any.


One more thing: I'm not sure we really need the "notify" variable in
rpm_suspend.  When a suspend callback fails, it means the device is in
use.  Whatever is using the device should then be responsible for
sending its own idle notification when it is finished; we shouldn't
need to send a notification as soon as the suspend callback fails.

Also, this can theoretically lead to nasty loops.  Suspend callback
fails, idle notification gets sent, the runtime_idle routine tries to
do another pm_runtime_suspend, rinse and repeat...  This is made even
worse by the fact that the rpm_idle call after a failed suspend isn't
asynchronous -- you could overflow the kernel's stack.  Relying on the
driver to be properly written doesn't help: The device could become
busy when each suspend is about to begin, and it could become inactive
again just before the idle notification.  The driver could do the right 
thing at each step and still overflow the stack.

Alan Stern

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

Hi,

On Thursday, September 30, 2010, Alan Stern wrote:
> This patch (as1431) adds a synchronous runtime-PM interface suitable
> for use in interrupt handlers.  Four new helper functions are defined:
> 
> 	pm_runtime_suspend_irq(), pm_runtime_resume_irq(),
> 	pm_runtime_get_sync_irq(), pm_runtime_put_sync_irq(),
> 
> together with pm_runtime_callbacks_in_irq(), which subsystems use to
> tell the PM core that the runtime callbacks should be invoked with
> interrupts disabled.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> In the end it turned out that a new RPM_IRQ call flag was needed along
> with the callbacks_in_irq flag in dev_pm_info.  The latter is required
> for the reasons I explained before, and RPM_IRQ tells the core whether
> or not it must leave interrupts disabled while waiting for a concurrent
> state change.
> 
> Kevin, this should be good enough to satisfy all your needs.  How does 
> it look?
> 
> Alan Stern
> 
> 
> Index: usb-2.6/include/linux/pm.h
> ===================================================================
> --- usb-2.6.orig/include/linux/pm.h
> +++ usb-2.6/include/linux/pm.h
> @@ -485,6 +485,7 @@ struct dev_pm_info {
>  	unsigned int		run_wake:1;
>  	unsigned int		runtime_auto:1;
>  	unsigned int		no_callbacks:1;
> +	unsigned int		callbacks_in_irq:1;

I kind of don't like this name.  What about irq_safe ?

>  	unsigned int		use_autosuspend:1;
>  	unsigned int		timer_autosuspends:1;
>  	enum rpm_request	request;
> Index: usb-2.6/include/linux/pm_runtime.h
> ===================================================================
> --- usb-2.6.orig/include/linux/pm_runtime.h
> +++ usb-2.6/include/linux/pm_runtime.h
> @@ -21,6 +21,7 @@
>  #define RPM_GET_PUT		0x04	/* Increment/decrement the
>  					    usage_count */
>  #define RPM_AUTO		0x08	/* Use autosuspend_delay */
> +#define RPM_IRQ			0x10	/* Don't enable interrupts */
>  
>  #ifdef CONFIG_PM_RUNTIME
>  
> @@ -40,6 +41,7 @@ extern int pm_generic_runtime_idle(struc
>  extern int pm_generic_runtime_suspend(struct device *dev);
>  extern int pm_generic_runtime_resume(struct device *dev);
>  extern void pm_runtime_no_callbacks(struct device *dev);
> +extern void pm_runtime_callbacks_in_irq(struct device *dev);

Perhaps this one can be called pm_runtime_irq_safe() in analogy to the struct
member?

>  extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
>  extern void pm_runtime_set_autosuspend_delay(struct device *dev, int delay);
>  extern unsigned long pm_runtime_autosuspend_expiration(struct device *dev);
> @@ -123,6 +125,7 @@ static inline int pm_generic_runtime_idl
>  static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
>  static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
>  static inline void pm_runtime_no_callbacks(struct device *dev) {}
> +static inline void pm_runtime_callbacks_in_irq(struct device *dev) {}
>  
>  static inline void pm_runtime_mark_last_busy(struct device *dev) {}
>  static inline void __pm_runtime_use_autosuspend(struct device *dev,
> @@ -144,6 +147,11 @@ static inline int pm_runtime_suspend(str
>  	return __pm_runtime_suspend(dev, 0);
>  }
>  
> +static inline int pm_runtime_suspend_irq(struct device *dev)
> +{
> +	return __pm_runtime_suspend(dev, RPM_IRQ);
> +}
> +
>  static inline int pm_runtime_autosuspend(struct device *dev)
>  {
>  	return __pm_runtime_suspend(dev, RPM_AUTO);
> @@ -154,6 +162,11 @@ static inline int pm_runtime_resume(stru
>  	return __pm_runtime_resume(dev, 0);
>  }
>  
> +static inline int pm_runtime_resume_irq(struct device *dev)
> +{
> +	return __pm_runtime_resume(dev, RPM_IRQ);
> +}
> +
>  static inline int pm_request_idle(struct device *dev)
>  {
>  	return __pm_runtime_idle(dev, RPM_ASYNC);
> @@ -179,6 +192,11 @@ static inline int pm_runtime_get_sync(st
>  	return __pm_runtime_resume(dev, RPM_GET_PUT);
>  }
>  
> +static inline int pm_runtime_get_sync_irq(struct device *dev)
> +{
> +	return __pm_runtime_resume(dev, RPM_GET_PUT | RPM_IRQ);
> +}
> +
>  static inline int pm_runtime_put(struct device *dev)
>  {
>  	return __pm_runtime_idle(dev, RPM_GET_PUT | RPM_ASYNC);
> @@ -195,6 +213,11 @@ static inline int pm_runtime_put_sync(st
>  	return __pm_runtime_idle(dev, RPM_GET_PUT);
>  }
>  
> +static inline int pm_runtime_put_sync_irq(struct device *dev)
> +{
> +	return __pm_runtime_idle(dev, RPM_GET_PUT | RPM_IRQ);
> +}
> +
>  static inline int pm_runtime_put_sync_autosuspend(struct device *dev)
>  {
>  	return __pm_runtime_suspend(dev, RPM_GET_PUT | RPM_AUTO);
> Index: usb-2.6/drivers/base/power/runtime.c
> ===================================================================
> --- usb-2.6.orig/drivers/base/power/runtime.c
> +++ usb-2.6/drivers/base/power/runtime.c
> @@ -170,10 +170,13 @@ static int rpm_idle(struct device *dev, 
>  	__releases(&dev->power.lock) __acquires(&dev->power.lock)
>  {
>  	int retval;
> +	int (*func)(struct device *dev);
>  
>  	retval = rpm_check_suspend_allowed(dev);
>  	if (retval < 0)
>  		;	/* Conditions are wrong. */
> +	else if ((rpmflags & RPM_IRQ) && !dev->power.callbacks_in_irq)
> +		retval = -EWOULDBLOCK;
>  
>  	/* Idle notifications are allowed only in the RPM_ACTIVE state. */
>  	else if (dev->power.runtime_status != RPM_ACTIVE)
> @@ -214,25 +217,27 @@ static int rpm_idle(struct device *dev, 
>  
>  	dev->power.idle_notification = true;
>  
> -	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
> -		spin_unlock_irq(&dev->power.lock);
> -
> -		dev->bus->pm->runtime_idle(dev);
> -
> -		spin_lock_irq(&dev->power.lock);
> -	} else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle) {
> -		spin_unlock_irq(&dev->power.lock);
> +	func = NULL;
> +	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
> +		func = dev->bus->pm->runtime_idle;
> +	else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle)
> +		func = dev->type->pm->runtime_idle;
> +	else if (dev->class && dev->class->pm && dev->class->pm->runtime_idle)
> +		func = dev->class->pm->runtime_idle;
> +	if (func) {
> +		if (dev->power.callbacks_in_irq) {
> +			spin_unlock(&dev->power.lock);

Hmm.  This looks rather complicated.  I don't really feel good about this
change.  It seems to me that it might be better to actually implement
the _irq() helpers from scratch.  I think I'll try to do that.

Overall, it looks like there's some overlap between RPM_IRQ and
power.callbacks_in_irq, where the latter may influence the behavior of the
helpers even if RPM_IRQ is unset.

I think we should return error code if RPM_IRQ is used, but 
power.callbacks_in_irq is not set.  If RPM_IRQ is not set, but
power.callbacks_in_irq is set, we should fall back to the normal behavior
(ie. do not avoid sleeping).

That's why I think it's better to change the name of power.callbacks_in_irq
to power.irq_safe.

Also I think we should refuse to set power.callbacks_in_irq for a device
whose partent (1) doesn't have power.callbacks_in_irq and (2) doesn't
have power.ignore_children set , and (3) doesn't have power.disable_depth > 0.
Then, once we've set power.callbacks_in_irq for a device, we should take
this into consideration when trying to change the parent's settings.

There probably is more subtelties like this, but I can't see them at the moment.

Thanks,
Rafael

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Thursday, September 30, 2010, Alan Stern wrote:
> This patch (as1431) adds a synchronous runtime-PM interface suitable
> for use in interrupt handlers.  Four new helper functions are defined:
> 
> 	pm_runtime_suspend_irq(), pm_runtime_resume_irq(),
> 	pm_runtime_get_sync_irq(), pm_runtime_put_sync_irq(),
> 
> together with pm_runtime_callbacks_in_irq(), which subsystems use to
> tell the PM core that the runtime callbacks should be invoked with
> interrupts disabled.

BTW, I like some changes made by your patch that aren't really related to
the issue at hand, so I think the patch below can be applied regardless of
the other changes, unless I made a mistake I can see now.

Thanks,
Rafael


---
 drivers/base/power/runtime.c |  123 +++++++++++++++++--------------------------
 1 file changed, 51 insertions(+), 72 deletions(-)

Index: linux-2.6/drivers/base/power/runtime.c
===================================================================
--- linux-2.6.orig/drivers/base/power/runtime.c
+++ linux-2.6/drivers/base/power/runtime.c
@@ -153,6 +153,27 @@ static int rpm_check_suspend_allowed(str
 	return retval;
 }
 
+/**
+ * rpm_callback - Run a given runtime PM callback for a given device.
+ * @cb: Runtime PM callback to run.
+ * @dev: Device to run the callback for.
+ */
+static int rpm_callback(int (*cb)(struct device *), struct device *dev)
+	__releases(&dev->power.lock) __acquires(&dev->power.lock)
+{
+	int retval;
+
+	if (!cb)
+		return -ENOSYS;
+
+	spin_unlock_irq(&dev->power.lock);
+
+	retval = cb(dev);
+
+	spin_lock_irq(&dev->power.lock);
+
+	return retval;
+}
 
 /**
  * rpm_idle - Notify device bus type if the device can be suspended.
@@ -167,8 +188,8 @@ static int rpm_check_suspend_allowed(str
  * This function must be called under dev->power.lock with interrupts disabled.
  */
 static int rpm_idle(struct device *dev, int rpmflags)
-	__releases(&dev->power.lock) __acquires(&dev->power.lock)
 {
+	int (*callback)(struct device *);
 	int retval;
 
 	retval = rpm_check_suspend_allowed(dev);
@@ -214,26 +235,16 @@ static int rpm_idle(struct device *dev, 
 
 	dev->power.idle_notification = true;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
-
-		dev->bus->pm->runtime_idle(dev);
-
-		spin_lock_irq(&dev->power.lock);
-	} else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
-
-		dev->type->pm->runtime_idle(dev);
-
-		spin_lock_irq(&dev->power.lock);
-	} else if (dev->class && dev->class->pm
-	    && dev->class->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
+		callback = dev->bus->pm->runtime_idle;
+	else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle)
+		callback = dev->type->pm->runtime_idle;
+	else if (dev->class && dev->class->pm)
+		callback = dev->class->pm->runtime_idle;
+	else
+		callback = NULL;
 
-		dev->class->pm->runtime_idle(dev);
-
-		spin_lock_irq(&dev->power.lock);
-	}
+	rpm_callback(callback, dev);
 
 	dev->power.idle_notification = false;
 	wake_up_all(&dev->power.wait_queue);
@@ -261,6 +272,7 @@ static int rpm_idle(struct device *dev, 
 static int rpm_suspend(struct device *dev, int rpmflags)
 	__releases(&dev->power.lock) __acquires(&dev->power.lock)
 {
+	int (*callback)(struct device *);
 	struct device *parent = NULL;
 	bool notify = false;
 	int retval;
@@ -351,33 +363,16 @@ static int rpm_suspend(struct device *de
 
 	__update_runtime_status(dev, RPM_SUSPENDING);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->bus->pm->runtime_suspend(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->type && dev->type->pm
-	    && dev->type->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->type->pm->runtime_suspend(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->class && dev->class->pm
-	    && dev->class->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->class->pm->runtime_suspend(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else {
-		retval = -ENOSYS;
-	}
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend)
+		callback = dev->bus->pm->runtime_suspend;
+	else if (dev->type && dev->type->pm && dev->type->pm->runtime_suspend)
+		callback = dev->type->pm->runtime_suspend;
+	else if (dev->class && dev->class->pm)
+		callback = dev->class->pm->runtime_suspend;
+	else
+		callback = NULL;
 
+	retval = rpm_callback(callback, dev);
 	if (retval) {
 		__update_runtime_status(dev, RPM_ACTIVE);
 		dev->power.deferred_resume = 0;
@@ -443,6 +438,7 @@ static int rpm_suspend(struct device *de
 static int rpm_resume(struct device *dev, int rpmflags)
 	__releases(&dev->power.lock) __acquires(&dev->power.lock)
 {
+	int (*callback)(struct device *);
 	struct device *parent = NULL;
 	int retval = 0;
 
@@ -563,33 +559,16 @@ static int rpm_resume(struct device *dev
 
 	__update_runtime_status(dev, RPM_RESUMING);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->bus->pm->runtime_resume(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->type && dev->type->pm
-	    && dev->type->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->type->pm->runtime_resume(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->class && dev->class->pm
-	    && dev->class->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->class->pm->runtime_resume(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else {
-		retval = -ENOSYS;
-	}
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume)
+		callback = dev->bus->pm->runtime_resume;
+	else if (dev->type && dev->type->pm && dev->type->pm->runtime_resume)
+		callback = dev->type->pm->runtime_resume;
+	else if (dev->class && dev->class->pm)
+		callback = dev->class->pm->runtime_resume;
+	else
+		callback = NULL;
 
+	retval = rpm_callback(callback, dev);
 	if (retval) {
 		__update_runtime_status(dev, RPM_SUSPENDED);
 		pm_runtime_cancel_pending(dev);

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Fri, 1 Oct 2010, Rafael J. Wysocki wrote:

> On Thursday, September 30, 2010, Alan Stern wrote:
> > This patch (as1431) adds a synchronous runtime-PM interface suitable
> > for use in interrupt handlers.  Four new helper functions are defined:
> > 
> > 	pm_runtime_suspend_irq(), pm_runtime_resume_irq(),
> > 	pm_runtime_get_sync_irq(), pm_runtime_put_sync_irq(),
> > 
> > together with pm_runtime_callbacks_in_irq(), which subsystems use to
> > tell the PM core that the runtime callbacks should be invoked with
> > interrupts disabled.
> 
> BTW, I like some changes made by your patch that aren't really related to
> the issue at hand, so I think the patch below can be applied regardless of
> the other changes, unless I made a mistake I can see now.

It looks like a good change, but you forgot to preserve the assignments 
to dev->power.runtime_error.

Alan Stern

[PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

This patch (as1431) adds a synchronous runtime-PM interface suitable
for use in interrupt handlers.  Four new helper functions are defined:

	pm_runtime_suspend_irq(), pm_runtime_resume_irq(),
	pm_runtime_get_sync_irq(), pm_runtime_put_sync_irq(),

together with pm_runtime_callbacks_in_irq(), which subsystems use to
tell the PM core that the runtime callbacks should be invoked with
interrupts disabled.

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

---

In the end it turned out that a new RPM_IRQ call flag was needed along
with the callbacks_in_irq flag in dev_pm_info.  The latter is required
for the reasons I explained before, and RPM_IRQ tells the core whether
or not it must leave interrupts disabled while waiting for a concurrent
state change.

Kevin, this should be good enough to satisfy all your needs.  How does 
it look?

Alan Stern


Index: usb-2.6/include/linux/pm.h
===================================================================
--- usb-2.6.orig/include/linux/pm.h
+++ usb-2.6/include/linux/pm.h
@@ -485,6 +485,7 @@ struct dev_pm_info {
 	unsigned int		run_wake:1;
 	unsigned int		runtime_auto:1;
 	unsigned int		no_callbacks:1;
+	unsigned int		callbacks_in_irq:1;
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
 	enum rpm_request	request;
Index: usb-2.6/include/linux/pm_runtime.h
===================================================================
--- usb-2.6.orig/include/linux/pm_runtime.h
+++ usb-2.6/include/linux/pm_runtime.h
@@ -21,6 +21,7 @@
 #define RPM_GET_PUT		0x04	/* Increment/decrement the
 					    usage_count */
 #define RPM_AUTO		0x08	/* Use autosuspend_delay */
+#define RPM_IRQ			0x10	/* Don't enable interrupts */
 
 #ifdef CONFIG_PM_RUNTIME
 
@@ -40,6 +41,7 @@ extern int pm_generic_runtime_idle(struc
 extern int pm_generic_runtime_suspend(struct device *dev);
 extern int pm_generic_runtime_resume(struct device *dev);
 extern void pm_runtime_no_callbacks(struct device *dev);
+extern void pm_runtime_callbacks_in_irq(struct device *dev);
 extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
 extern void pm_runtime_set_autosuspend_delay(struct device *dev, int delay);
 extern unsigned long pm_runtime_autosuspend_expiration(struct device *dev);
@@ -123,6 +125,7 @@ static inline int pm_generic_runtime_idl
 static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
 static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
 static inline void pm_runtime_no_callbacks(struct device *dev) {}
+static inline void pm_runtime_callbacks_in_irq(struct device *dev) {}
 
 static inline void pm_runtime_mark_last_busy(struct device *dev) {}
 static inline void __pm_runtime_use_autosuspend(struct device *dev,
@@ -144,6 +147,11 @@ static inline int pm_runtime_suspend(str
 	return __pm_runtime_suspend(dev, 0);
 }
 
+static inline int pm_runtime_suspend_irq(struct device *dev)
+{
+	return __pm_runtime_suspend(dev, RPM_IRQ);
+}
+
 static inline int pm_runtime_autosuspend(struct device *dev)
 {
 	return __pm_runtime_suspend(dev, RPM_AUTO);
@@ -154,6 +162,11 @@ static inline int pm_runtime_resume(stru
 	return __pm_runtime_resume(dev, 0);
 }
 
+static inline int pm_runtime_resume_irq(struct device *dev)
+{
+	return __pm_runtime_resume(dev, RPM_IRQ);
+}
+
 static inline int pm_request_idle(struct device *dev)
 {
 	return __pm_runtime_idle(dev, RPM_ASYNC);
@@ -179,6 +192,11 @@ static inline int pm_runtime_get_sync(st
 	return __pm_runtime_resume(dev, RPM_GET_PUT);
 }
 
+static inline int pm_runtime_get_sync_irq(struct device *dev)
+{
+	return __pm_runtime_resume(dev, RPM_GET_PUT | RPM_IRQ);
+}
+
 static inline int pm_runtime_put(struct device *dev)
 {
 	return __pm_runtime_idle(dev, RPM_GET_PUT | RPM_ASYNC);
@@ -195,6 +213,11 @@ static inline int pm_runtime_put_sync(st
 	return __pm_runtime_idle(dev, RPM_GET_PUT);
 }
 
+static inline int pm_runtime_put_sync_irq(struct device *dev)
+{
+	return __pm_runtime_idle(dev, RPM_GET_PUT | RPM_IRQ);
+}
+
 static inline int pm_runtime_put_sync_autosuspend(struct device *dev)
 {
 	return __pm_runtime_suspend(dev, RPM_GET_PUT | RPM_AUTO);
Index: usb-2.6/drivers/base/power/runtime.c
===================================================================
--- usb-2.6.orig/drivers/base/power/runtime.c
+++ usb-2.6/drivers/base/power/runtime.c
@@ -170,10 +170,13 @@ static int rpm_idle(struct device *dev, 
 	__releases(&dev->power.lock) __acquires(&dev->power.lock)
 {
 	int retval;
+	int (*func)(struct device *dev);
 
 	retval = rpm_check_suspend_allowed(dev);
 	if (retval < 0)
 		;	/* Conditions are wrong. */
+	else if ((rpmflags & RPM_IRQ) && !dev->power.callbacks_in_irq)
+		retval = -EWOULDBLOCK;
 
 	/* Idle notifications are allowed only in the RPM_ACTIVE state. */
 	else if (dev->power.runtime_status != RPM_ACTIVE)
@@ -214,25 +217,27 @@ static int rpm_idle(struct device *dev, 
 
 	dev->power.idle_notification = true;
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
-
-		dev->bus->pm->runtime_idle(dev);
-
-		spin_lock_irq(&dev->power.lock);
-	} else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
+	func = NULL;
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_idle)
+		func = dev->bus->pm->runtime_idle;
+	else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle)
+		func = dev->type->pm->runtime_idle;
+	else if (dev->class && dev->class->pm && dev->class->pm->runtime_idle)
+		func = dev->class->pm->runtime_idle;
+	if (func) {
+		if (dev->power.callbacks_in_irq) {
+			spin_unlock(&dev->power.lock);
 
-		dev->type->pm->runtime_idle(dev);
+			func(dev);
 
-		spin_lock_irq(&dev->power.lock);
-	} else if (dev->class && dev->class->pm
-	    && dev->class->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
+			spin_lock(&dev->power.lock);
+		} else {
+			spin_unlock_irq(&dev->power.lock);
 
-		dev->class->pm->runtime_idle(dev);
+			func(dev);
 
-		spin_lock_irq(&dev->power.lock);
+			spin_lock_irq(&dev->power.lock);
+		}
 	}
 
 	dev->power.idle_notification = false;
@@ -264,6 +269,7 @@ static int rpm_suspend(struct device *de
 	struct device *parent = NULL;
 	bool notify = false;
 	int retval;
+	int (*func)(struct device *dev);
 
 	dev_dbg(dev, "%s flags 0x%x\n", __func__, rpmflags);
 
@@ -272,6 +278,8 @@ static int rpm_suspend(struct device *de
 
 	if (retval < 0)
 		;	/* Conditions are wrong. */
+	else if ((rpmflags & RPM_IRQ) && !dev->power.callbacks_in_irq)
+		retval = -EWOULDBLOCK;
 
 	/* Synchronous suspends are not allowed in the RPM_RESUMING state. */
 	else if (dev->power.runtime_status == RPM_RESUMING &&
@@ -310,27 +318,35 @@ static int rpm_suspend(struct device *de
 	pm_runtime_cancel_pending(dev);
 
 	if (dev->power.runtime_status == RPM_SUSPENDING) {
-		DEFINE_WAIT(wait);
-
 		if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
 			retval = -EINPROGRESS;
 			goto out;
 		}
 
 		/* Wait for the other suspend running in parallel with us. */
-		for (;;) {
-			prepare_to_wait(&dev->power.wait_queue, &wait,
-					TASK_UNINTERRUPTIBLE);
-			if (dev->power.runtime_status != RPM_SUSPENDING)
-				break;
+		if (rpmflags & RPM_IRQ) {
+			spin_unlock(&dev->power.lock);
+
+			while (dev->power.runtime_status == RPM_SUSPENDING)
+				cpu_relax();
+
+			spin_lock(&dev->power.lock);
+		} else {
+			DEFINE_WAIT(wait);
 
 			spin_unlock_irq(&dev->power.lock);
 
-			schedule();
+			for (;;) {
+				prepare_to_wait(&dev->power.wait_queue, &wait,
+				    TASK_UNINTERRUPTIBLE);
+				if (dev->power.runtime_status != RPM_SUSPENDING)
+					break;
+				schedule();
+			}
+			finish_wait(&dev->power.wait_queue, &wait);
 
 			spin_lock_irq(&dev->power.lock);
 		}
-		finish_wait(&dev->power.wait_queue, &wait);
 		goto repeat;
 	}
 
@@ -351,28 +367,28 @@ static int rpm_suspend(struct device *de
 
 	__update_runtime_status(dev, RPM_SUSPENDING);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->bus->pm->runtime_suspend(dev);
+	func = NULL;
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend)
+		func = dev->bus->pm->runtime_suspend;
+	else if (dev->type && dev->type->pm && dev->type->pm->runtime_suspend)
+		func = dev->type->pm->runtime_suspend;
+	else if (dev->class && dev->class->pm &&
+	    dev->class->pm->runtime_suspend)
+		func = dev->class->pm->runtime_suspend;
+	if (func) {
+		if (dev->power.callbacks_in_irq) {
+			spin_unlock(&dev->power.lock);
 
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->type && dev->type->pm
-	    && dev->type->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
+			retval = func(dev);
 
-		retval = dev->type->pm->runtime_suspend(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->class && dev->class->pm
-	    && dev->class->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
+			spin_lock(&dev->power.lock);
+		} else {
+			spin_unlock_irq(&dev->power.lock);
 
-		retval = dev->class->pm->runtime_suspend(dev);
+			retval = func(dev);
 
-		spin_lock_irq(&dev->power.lock);
+			spin_lock_irq(&dev->power.lock);
+		}
 		dev->power.runtime_error = retval;
 	} else {
 		retval = -ENOSYS;
@@ -401,20 +417,20 @@ static int rpm_suspend(struct device *de
 	wake_up_all(&dev->power.wait_queue);
 
 	if (dev->power.deferred_resume) {
-		rpm_resume(dev, 0);
+		rpm_resume(dev, rpmflags);
 		retval = -EAGAIN;
 		goto out;
 	}
 
 	if (notify)
-		rpm_idle(dev, 0);
+		rpm_idle(dev, rpmflags);
 
 	if (parent && !parent->power.ignore_children) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock(&dev->power.lock);
 
 		pm_request_idle(parent);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock(&dev->power.lock);
 	}
 
  out:
@@ -445,6 +461,7 @@ static int rpm_resume(struct device *dev
 {
 	struct device *parent = NULL;
 	int retval = 0;
+	int (*func)(struct device *dev);
 
 	dev_dbg(dev, "%s flags 0x%x\n", __func__, rpmflags);
 
@@ -453,6 +470,8 @@ static int rpm_resume(struct device *dev
 		retval = -EINVAL;
 	else if (dev->power.disable_depth > 0)
 		retval = -EAGAIN;
+	else if ((rpmflags & RPM_IRQ) && !dev->power.callbacks_in_irq)
+		retval = -EWOULDBLOCK;
 	if (retval)
 		goto out;
 
@@ -473,8 +492,6 @@ static int rpm_resume(struct device *dev
 
 	if (dev->power.runtime_status == RPM_RESUMING
 	    || dev->power.runtime_status == RPM_SUSPENDING) {
-		DEFINE_WAIT(wait);
-
 		if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
 			if (dev->power.runtime_status == RPM_SUSPENDING)
 				dev->power.deferred_resume = true;
@@ -484,20 +501,31 @@ static int rpm_resume(struct device *dev
 		}
 
 		/* Wait for the operation carried out in parallel with us. */
-		for (;;) {
-			prepare_to_wait(&dev->power.wait_queue, &wait,
-					TASK_UNINTERRUPTIBLE);
-			if (dev->power.runtime_status != RPM_RESUMING
-			    && dev->power.runtime_status != RPM_SUSPENDING)
-				break;
+		if (rpmflags & RPM_IRQ) {
+			spin_unlock(&dev->power.lock);
+
+			while (dev->power.runtime_status == RPM_SUSPENDING
+			    || dev->power.runtime_status == RPM_RESUMING)
+				cpu_relax();
+
+			spin_lock(&dev->power.lock);
+		} else {
+			DEFINE_WAIT(wait);
 
 			spin_unlock_irq(&dev->power.lock);
 
-			schedule();
+			for (;;) {
+				prepare_to_wait(&dev->power.wait_queue, &wait,
+				    TASK_UNINTERRUPTIBLE);
+				if (dev->power.runtime_status != RPM_SUSPENDING
+				    && dev->power.runtime_status != RPM_RESUMING)
+					break;
+				schedule();
+			}
+			finish_wait(&dev->power.wait_queue, &wait);
 
 			spin_lock_irq(&dev->power.lock);
 		}
-		finish_wait(&dev->power.wait_queue, &wait);
 		goto repeat;
 	}
 
@@ -546,7 +574,7 @@ static int rpm_resume(struct device *dev
 		 */
 		if (!parent->power.disable_depth
 		    && !parent->power.ignore_children) {
-			rpm_resume(parent, 0);
+			rpm_resume(parent, rpmflags & ~RPM_NOWAIT);
 			if (parent->power.runtime_status != RPM_ACTIVE)
 				retval = -EBUSY;
 		}
@@ -563,28 +591,28 @@ static int rpm_resume(struct device *dev
 
 	__update_runtime_status(dev, RPM_RESUMING);
 
-	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
-
-		retval = dev->bus->pm->runtime_resume(dev);
+	func = NULL;
+	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume)
+		func = dev->bus->pm->runtime_resume;
+	else if (dev->type && dev->type->pm && dev->type->pm->runtime_resume)
+		func = dev->type->pm->runtime_resume;
+	else if (dev->class && dev->class->pm &&
+	    dev->class->pm->runtime_resume)
+		func = dev->class->pm->runtime_resume;
+	if (func) {
+		if (dev->power.callbacks_in_irq) {
+			spin_unlock(&dev->power.lock);
 
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->type && dev->type->pm
-	    && dev->type->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
+			retval = func(dev);
 
-		retval = dev->type->pm->runtime_resume(dev);
-
-		spin_lock_irq(&dev->power.lock);
-		dev->power.runtime_error = retval;
-	} else if (dev->class && dev->class->pm
-	    && dev->class->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
+			spin_lock(&dev->power.lock);
+		} else {
+			spin_unlock_irq(&dev->power.lock);
 
-		retval = dev->class->pm->runtime_resume(dev);
+			retval = func(dev);
 
-		spin_lock_irq(&dev->power.lock);
+			spin_lock_irq(&dev->power.lock);
+		}
 		dev->power.runtime_error = retval;
 	} else {
 		retval = -ENOSYS;
@@ -602,15 +630,15 @@ static int rpm_resume(struct device *dev
 	wake_up_all(&dev->power.wait_queue);
 
 	if (!retval)
-		rpm_idle(dev, RPM_ASYNC);
+		rpm_idle(dev, rpmflags | RPM_ASYNC);
 
  out:
 	if (parent) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock(&dev->power.lock);
 
 		pm_runtime_put(parent);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock(&dev->power.lock);
 	}
 
 	dev_dbg(dev, "%s returns %d\n", __func__, retval);
@@ -1086,7 +1114,6 @@ EXPORT_SYMBOL_GPL(pm_runtime_allow);
  * Set the power.no_callbacks flag, which tells the PM core that this
  * device is power-managed through its parent and has no run-time PM
  * callbacks of its own.  The run-time sysfs attributes will be removed.
- *
  */
 void pm_runtime_no_callbacks(struct device *dev)
 {
@@ -1099,6 +1126,22 @@ void pm_runtime_no_callbacks(struct devi
 EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks);
 
 /**
+ * pm_runtime_callbacks_in_irq - Leave interrupts disabled during callbacks.
+ * @dev: Device to handle
+ *
+ * Set the power.callbacks_in_irq flag, which tells the PM core that the
+ * run-time PM callbacks for this device should always be invoked with
+ * interrupts disabled.
+ */
+void pm_runtime_callbacks_in_irq(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.callbacks_in_irq = 1;
+	spin_unlock_irq(&dev->power.lock);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_callbacks_in_irq);
+
+/**
  * update_autosuspend - Handle a change to a device's autosuspend settings.
  * @dev: Device to handle.
  * @old_delay: The former autosuspend_delay value.
Index: usb-2.6/Documentation/power/runtime_pm.txt
===================================================================
--- usb-2.6.orig/Documentation/power/runtime_pm.txt
+++ usb-2.6/Documentation/power/runtime_pm.txt
@@ -50,6 +50,18 @@ type's callbacks are not defined) of giv
 and device class callbacks are referred to as subsystem-level callbacks in what
 follows.
 
+By default, the callbacks are always invoked in process context with interrupts
+enabled.  However subsystems can tell the PM core that the callbacks for a
+device should be invoked with interrupts disabled, by calling
+pm_runtime_callbacks_in_irq().  This implies that the callback routines must
+not block or sleep, but it also means that the following synchronous helper
+functions can be used from within an interrupt handler:
+
+		pm_runtime_resume_irq(),
+		pm_runtime_suspend_irq(),
+		pm_runtime_get_sync_irq(),
+		pm_runtime_put_sync_irq().
+
 The subsystem-level suspend callback is _entirely_ _responsible_ for handling
 the suspend of the device as appropriate, which may, but need not include
 executing the device driver's own ->runtime_suspend() callback (from the
@@ -237,6 +249,10 @@ defined in include/linux/pm.h:
       Section 8); it may be modified only by the pm_runtime_no_callbacks()
       helper function
 
+  unsigned int callbacks_in_irq;
+    - indicates that the ->runtime_idle(), ->runtime_suspend(), and
+      ->runtime_resume() callbacks should be invoked with interrupts disabled.
+
   unsigned int use_autosuspend;
     - indicates that the device's driver supports delayed autosuspend (see
       Section 9); it may be modified only by the
@@ -285,6 +301,11 @@ drivers/base/power/runtime.c and include
       not yet expired then an autosuspend is scheduled for the appropriate time
       and 0 is returned
 
+  int pm_runtime_suspend_irq(struct device *dev);
+    - same as pm_runtime_suspend() except that this function may be called
+      in interrupt context; returns an error unless
+      pm_runtime_callbacks_in_irq(dev) was called previously
+
   int pm_runtime_resume(struct device *dev);
     - execute the subsystem-level resume callback for the device; returns 0 on
       success, 1 if the device's run-time PM status was already 'active' or
@@ -292,6 +313,11 @@ drivers/base/power/runtime.c and include
       resume the device again in future, but 'power.runtime_error' should be
       checked additionally
 
+  int pm_runtime_resume_irq(struct device *dev);
+    - same as pm_runtime_resume() except that this function may be called
+      in interrupt context; returns an error unless
+      pm_runtime_callbacks_in_irq(dev) was called previously
+
   int pm_request_idle(struct device *dev);
     - submit a request to execute the subsystem-level idle callback for the
       device (the request is represented by a work item in pm_wq); returns 0 on
@@ -329,6 +355,10 @@ drivers/base/power/runtime.c and include
     - increment the device's usage counter, run pm_runtime_resume(dev) and
       return its result
 
+  int pm_runtime_get_sync_irq(struct device *dev);
+    - increment the device's usage counter, run pm_runtime_resume_irq(dev) and
+      return its result
+
   void pm_runtime_put_noidle(struct device *dev);
     - decrement the device's usage counter
 
@@ -344,6 +374,10 @@ drivers/base/power/runtime.c and include
     - decrement the device's usage counter; if the result is 0 then run
       pm_runtime_idle(dev) and return its result
 
+  int pm_runtime_put_sync_irq(struct device *dev);
+    - decrement the device's usage counter; if the result is 0 then run
+      pm_runtime_idle(dev) and return its result
+
   int pm_runtime_put_sync_autosuspend(struct device *dev);
     - decrement the device's usage counter; if the result is 0 then run
       pm_runtime_autosuspend(dev) and return its result
@@ -397,6 +431,10 @@ drivers/base/power/runtime.c and include
       PM attributes from /sys/devices/.../power (or prevent them from being
       added when the device is registered)
 
+  void pm_runtime_callbacks_in_irq(struct device *dev);
+    - set the power.callbacks_in_irq flag for the device, causing all callbacks
+      to be invoked with interrupts disabled
+
   void pm_runtime_mark_last_busy(struct device *dev);
     - set the power.last_busy field to the current time
 
@@ -422,14 +460,18 @@ drivers/base/power/runtime.c and include
 It is safe to execute the following helper functions from interrupt context:
 
 pm_request_idle()
+pm_runtime_suspend_irq()
 pm_request_autosuspend()
 pm_schedule_suspend()
+pm_runtime_resume_irq()
 pm_request_resume()
 pm_runtime_get_noresume()
 pm_runtime_get()
+pm_runtime_get_sync_irq()
 pm_runtime_put_noidle()
 pm_runtime_put()
 pm_runtime_put_autosuspend()
+pm_runtime_put_sync_irq()
 pm_runtime_enable()
 pm_suspend_ignore_children()
 pm_runtime_set_active()