runtime_pm_get_sync() from ISR with IRQs disabled?

runtime_pm_get_sync() from ISR with IRQs disabled?

[Kevin Hilman]

Hello,

Looking for advice for a little runtime PM dilemma...

After some inactivity, a driver decides to supend iteslf using
pm_runtime_put_sync().

The device is now suspended, it's ->runtime_suspend() method has
disabled its clock, so its registers cannot be accessed anymore.

Now, as interrupts are still enabled, an interrupt for this device might
still arrive.  For example, if this device is a wakeup source, its
->runtime_suspend() method may not have masked its interrupt.

So, the IRQ fires, and the drivers ISR is called.  The driver wants to
access the device registers, but since it has been runtime suspended,
it's registers are not available.

The first reflex would be to simply do a pm_runtime_get_sync() in the
ISR, however this is not safe if the ISR is an IRQs-disabled handler (as
is the case for me, where the problematic handler is chained handler
used for demuxing GPIO IRQs.)

So, what is the "right" thing to do here?

A quick hack would be to for the drivers ISR to do a
pm_runtime_get_noresume() and directly call the its ->runtime_resume()
method, then do its normal stuff, followed by a pm_runtime_put() at the
end of the ISR.

Is this an acceptable hack given that it's only needed for the
increasingly rare cases of ISRs with interrupts disabled?

Or should we think of making a version of _get_sync() that is safe for
IRQs disabled contexts like this where we know the device is already
RPM_SUSPENDED?

Any advice appreciated...

Kevin

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Alan Stern]

On Thu, 23 Sep 2010, Kevin Hilman wrote:

> Hello,
> 
> Looking for advice for a little runtime PM dilemma...
> 
> After some inactivity, a driver decides to supend iteslf using
> pm_runtime_put_sync().
> 
> The device is now suspended, it's ->runtime_suspend() method has
> disabled its clock, so its registers cannot be accessed anymore.
> 
> Now, as interrupts are still enabled, an interrupt for this device might
> still arrive.  For example, if this device is a wakeup source, its
> ->runtime_suspend() method may not have masked its interrupt.

Right.

> So, the IRQ fires, and the drivers ISR is called.  The driver wants to
> access the device registers, but since it has been runtime suspended,
> it's registers are not available.
> 
> The first reflex would be to simply do a pm_runtime_get_sync() in the
> ISR, however this is not safe if the ISR is an IRQs-disabled handler (as
> is the case for me, where the problematic handler is chained handler
> used for demuxing GPIO IRQs.)

An ISR shouldn't call pm_runtime_get_sync() in any case.

> So, what is the "right" thing to do here?

You should call pm_runtime_get(), turn off the interrupt source, and
return.  Then your resume routine should check for an outstanding
interrupt or wakeup request and handle it (the easiest way may be 
simply to call the ISR).

> A quick hack would be to for the drivers ISR to do a
> pm_runtime_get_noresume() and directly call the its ->runtime_resume()
> method, then do its normal stuff, followed by a pm_runtime_put() at the
> end of the ISR.
> 
> Is this an acceptable hack given that it's only needed for the
> increasingly rare cases of ISRs with interrupts disabled?
> 
> Or should we think of making a version of _get_sync() that is safe for
> IRQs disabled contexts like this where we know the device is already
> RPM_SUSPENDED?
> 
> Any advice appreciated...

You're trying to fight the runtime-PM design instead of using it as it 
was intended.  We already have an API for starting a resume from 
interrupt context, and that's what you should use.

Alan Stern

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Kevin Hilman]

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

> On Thu, 23 Sep 2010, Kevin Hilman wrote:
>
>> Hello,
>> 
>> Looking for advice for a little runtime PM dilemma...
>> 
>> After some inactivity, a driver decides to supend iteslf using
>> pm_runtime_put_sync().
>> 
>> The device is now suspended, it's ->runtime_suspend() method has
>> disabled its clock, so its registers cannot be accessed anymore.
>> 
>> Now, as interrupts are still enabled, an interrupt for this device might
>> still arrive.  For example, if this device is a wakeup source, its
>> ->runtime_suspend() method may not have masked its interrupt.
>
> Right.
>
>> So, the IRQ fires, and the drivers ISR is called.  The driver wants to
>> access the device registers, but since it has been runtime suspended,
>> it's registers are not available.
>> 
>> The first reflex would be to simply do a pm_runtime_get_sync() in the
>> ISR, however this is not safe if the ISR is an IRQs-disabled handler (as
>> is the case for me, where the problematic handler is chained handler
>> used for demuxing GPIO IRQs.)
>
> An ISR shouldn't call pm_runtime_get_sync() in any case.
>
>> So, what is the "right" thing to do here?
>
> You should call pm_runtime_get(), turn off the interrupt source, and
> return.  Then your resume routine should check for an outstanding
> interrupt or wakeup request and handle it (the easiest way may be 
> simply to call the ISR).

For a "normal" device driver, your solution makes complete sense.  The
only catch is that it introduces potentically significant latency in the
interrupt handling and it requires the interrupt source to be masked,
potentially loosing other interrupts, while waiting for the runtime PM
workqueue to schedule.  For chained handlers in particular, this means
that *all* interrupts managed by the chained handler would be masked for
this additional time.  Not good.

The problematic device for me as an on-chip GPIO controller, and the ISR
in question is a chained handler (run with interrupts disabled) which
does the GPIO demux and then dispatches to the actual ISR.  Following the
above approach means that all GPIO interrupts (in that bank) would be
masked until ->runtime_resume() is called.  For a GPIO bank with
multiple edge-triggered IRQs, masked IRQs for that amount of time could
mean several missed interrupts while waiting.

For example, Here's what would happen:

- IRQ arrives
- ISR: [chained handler, called with IRQs disabled ]
  if (pm_runtime_suspended()) {
      pm_runtime_get()
      <mask the GPIO bank interrupt>
      /* starting now, we miss edge-triggered IRQs in this bank */
      state->irq_pending = true;
      return;
  }

... some time later (scheduler latency, other hard IRQs, soft IRQs?, etc.) ...

- pm_runtime_work()
      /* IRQs disabled */
      __pm_runtime_resume()
          /* IRQs enabled */
          bus->runtime_resume()
              driver->runtime_resume()
          /* IRQs disabled */
     /* IRQs enabled */

And in the driver's runtime_resume method()

  if (state->irq_pending) {
      /* disable IRQs */
      <call GPIO demux ISR, which will finally unmask the bank IRQ>
      /* enable IRQs */
      /* Here, we can finally receive interrupts on that bank again. */ 
  }
      
Not only is the additional interrupt latency a problem, but any other
drivers hooked up to these IRQs may have requested IRQF_DISABLED
handlers, expecting to be called in interrupt context with interrupts
disabled, which clearly will not be the case.  Hoever, this isn't a
major concern as we don't (currently) have IRQF_DISABLED handlers hooked
up to GPIO IRQs (that I know of.)

>> A quick hack would be to for the drivers ISR to do a
>> pm_runtime_get_noresume() and directly call the its ->runtime_resume()
>> method, then do its normal stuff, followed by a pm_runtime_put() at the
>> end of the ISR.
>> 
>> Is this an acceptable hack given that it's only needed for the
>> increasingly rare cases of ISRs with interrupts disabled?
>> 
>> Or should we think of making a version of _get_sync() that is safe for
>> IRQs disabled contexts like this where we know the device is already
>> RPM_SUSPENDED?
>> 
>> Any advice appreciated...
>
> You're trying to fight the runtime-PM design instead of using it as it 
> was intended.  We already have an API for starting a resume from 
> interrupt context, and that's what you should use.

It may seem like I'm trying to fight the design, but I'm actually trying
to find ways to use it.  I want to use the API (and we're using it
successfully in most of our drivers now.)  The problem is only in a few
of these corner cases where using it introduces significant changes from
previous behavior like introducing long, unbounded windows for missed
interrupts.

Kevin

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Rafael J. Wysocki]

On Friday, September 24, 2010, Kevin Hilman wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > On Thu, 23 Sep 2010, Kevin Hilman wrote:
...
> >
> > You're trying to fight the runtime-PM design instead of using it as it 
> > was intended.  We already have an API for starting a resume from 
> > interrupt context, and that's what you should use.
> 
> It may seem like I'm trying to fight the design, but I'm actually trying
> to find ways to use it.  I want to use the API (and we're using it
> successfully in most of our drivers now.)  The problem is only in a few
> of these corner cases where using it introduces significant changes from
> previous behavior like introducing long, unbounded windows for missed
> interrupts.

This really sounds to me like you need _noirq() runtime PM callbacks
and some framework around them.

I'm not fundamentally against that, but it will require some time to introduce,
if we decide that it's really necessary.

I need to think a bit more about that, thanks for the example.

Rafael

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Alan Stern]

On Fri, 24 Sep 2010, Rafael J. Wysocki wrote:

> On Friday, September 24, 2010, Kevin Hilman wrote:
> > Alan Stern <stern@rowland.harvard.edu> writes:
> > 
> > > On Thu, 23 Sep 2010, Kevin Hilman wrote:
> ...
> > >
> > > You're trying to fight the runtime-PM design instead of using it as it 
> > > was intended.  We already have an API for starting a resume from 
> > > interrupt context, and that's what you should use.
> > 
> > It may seem like I'm trying to fight the design, but I'm actually trying
> > to find ways to use it.  I want to use the API (and we're using it
> > successfully in most of our drivers now.)  The problem is only in a few
> > of these corner cases where using it introduces significant changes from
> > previous behavior like introducing long, unbounded windows for missed
> > interrupts.
> 
> This really sounds to me like you need _noirq() runtime PM callbacks
> and some framework around them.
> 
> I'm not fundamentally against that, but it will require some time to introduce,
> if we decide that it's really necessary.
> 
> I need to think a bit more about that, thanks for the example.

How about adding another flag to the dev_pm_info structure, to indicate 
that the runtime callbacks may be called in interrupt context?

Maybe that will lead to problems I haven't thought of.  But if it seems 
okay to you, I can code it up easily enough.

Alan Stern

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Rafael J. Wysocki]

On Monday, September 27, 2010, Alan Stern wrote:
> On Fri, 24 Sep 2010, Rafael J. Wysocki wrote:
> 
> > On Friday, September 24, 2010, Kevin Hilman wrote:
> > > Alan Stern <stern@rowland.harvard.edu> writes:
> > > 
> > > > On Thu, 23 Sep 2010, Kevin Hilman wrote:
> > ...
> > > >
> > > > You're trying to fight the runtime-PM design instead of using it as it 
> > > > was intended.  We already have an API for starting a resume from 
> > > > interrupt context, and that's what you should use.
> > > 
> > > It may seem like I'm trying to fight the design, but I'm actually trying
> > > to find ways to use it.  I want to use the API (and we're using it
> > > successfully in most of our drivers now.)  The problem is only in a few
> > > of these corner cases where using it introduces significant changes from
> > > previous behavior like introducing long, unbounded windows for missed
> > > interrupts.
> > 
> > This really sounds to me like you need _noirq() runtime PM callbacks
> > and some framework around them.
> > 
> > I'm not fundamentally against that, but it will require some time to introduce,
> > if we decide that it's really necessary.
> > 
> > I need to think a bit more about that, thanks for the example.
> 
> How about adding another flag to the dev_pm_info structure, to indicate 
> that the runtime callbacks may be called in interrupt context?
> 
> Maybe that will lead to problems I haven't thought of.  But if it seems 
> okay to you, I can code it up easily enough.

Hmm.  I was thinking about adding a new RPM_ flag for that, like RPM_FASTPATH,
telling the PM core to assume the callbacks will not sleep and that the call
might be from interrupt handler.

Thanks,
Rafael

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Alan Stern]

On Mon, 27 Sep 2010, Rafael J. Wysocki wrote:

> > How about adding another flag to the dev_pm_info structure, to indicate 
> > that the runtime callbacks may be called in interrupt context?
> > 
> > Maybe that will lead to problems I haven't thought of.  But if it seems 
> > okay to you, I can code it up easily enough.
> 
> Hmm.  I was thinking about adding a new RPM_ flag for that, like RPM_FASTPATH,
> telling the PM core to assume the callbacks will not sleep and that the call
> might be from interrupt handler.

That might appear more flexible, but is there any advantage to it?  
That is, would there be a situation where the driver knows that the 
next callback won't sleep but in general the callback might sleep 
sometimes?  It doesn't seem very likely.

Kevin, do you have any feelings about this?

Alan Stern

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Rafael J. Wysocki]

On Monday, September 27, 2010, Alan Stern wrote:
> On Mon, 27 Sep 2010, Rafael J. Wysocki wrote:
> 
> > > How about adding another flag to the dev_pm_info structure, to indicate 
> > > that the runtime callbacks may be called in interrupt context?
> > > 
> > > Maybe that will lead to problems I haven't thought of.  But if it seems 
> > > okay to you, I can code it up easily enough.
> > 
> > Hmm.  I was thinking about adding a new RPM_ flag for that, like RPM_FASTPATH,
> > telling the PM core to assume the callbacks will not sleep and that the call
> > might be from interrupt handler.
> 
> That might appear more flexible, but is there any advantage to it?  
> That is, would there be a situation where the driver knows that the 
> next callback won't sleep but in general the callback might sleep 
> sometimes?  It doesn't seem very likely.

A flag in dev_pm_info, when set, would always make the PM core behave as
though the call was made from interrupt context, even if it really wasn't,
while the RPM_ flag would allow the driver to tell it to do that when it's
really necessary.  The driver always knows which calls are made from an
interrupt handler. :-)

Thanks,
Rafael

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Alan Stern]

On Mon, 27 Sep 2010, Rafael J. Wysocki wrote:

> On Monday, September 27, 2010, Alan Stern wrote:
> > On Mon, 27 Sep 2010, Rafael J. Wysocki wrote:
> > 
> > > > How about adding another flag to the dev_pm_info structure, to indicate 
> > > > that the runtime callbacks may be called in interrupt context?
> > > > 
> > > > Maybe that will lead to problems I haven't thought of.  But if it seems 
> > > > okay to you, I can code it up easily enough.
> > > 
> > > Hmm.  I was thinking about adding a new RPM_ flag for that, like RPM_FASTPATH,
> > > telling the PM core to assume the callbacks will not sleep and that the call
> > > might be from interrupt handler.
> > 
> > That might appear more flexible, but is there any advantage to it?  
> > That is, would there be a situation where the driver knows that the 
> > next callback won't sleep but in general the callback might sleep 
> > sometimes?  It doesn't seem very likely.
> 
> A flag in dev_pm_info, when set, would always make the PM core behave as
> though the call was made from interrupt context, even if it really wasn't,
> while the RPM_ flag would allow the driver to tell it to do that when it's
> really necessary.  The driver always knows which calls are made from an
> interrupt handler. :-)

On Mon, 27 Sep 2010, Kevin Hilman wrote:

> For flexibility, I like the RPM_FASTPATH approach.
> 
> At least in the drivers I'm currently tinkering with, most of the
> callers can sleep.  It's only the occasional callback triggered from the
> ISR that can't sleep.
> 
> During this "normal" (non-ISR) usage, there might very well be
> concurrent or pending requests, some of which might have to sleep.  So,
> I think it's best to have the fastpath settable on a per-caller basis.

The RPM_FASTPATH approach turns out not to be viable.  It clearly
implies that some calls will be made with the flag and some calls will
be made without.  This can lead to incorrect behavior.

Here's what can happen.  Suppose the device is at low power and CPU 1
calls pm_runtime_resume without using the new flag.  An instant later,
a remote wakeup interrupt arrives and the interrupt handler on CPU 2
calls pm_runtime_resume with the new flag set.

Since CPU 1 has changed the status to RPM_RESUMING, CPU 2 has to wait
until the resume has completed, and since CPU 2 is in_interrupt, it has
to busy-wait.  But the process it's waiting for is running on CPU 1
with interrupts enabled and so can block for an arbitrarily long time.

This example shows that if _any_ resume callbacks are made in_interrupt
then they _all_ have to be made with interrupts disabled.  The same
sort of reasoning applies to suspend callbacks (replace RPM_RESUMING 
above with RPM_SUSPENDING).

Given this, I see no point in adding a special per-call flag.

There's another advantage to using a per-device flag.  Not all resume
calls are initiated by the driver; the PM core has to start a resume on
its own when rpm_resume sees that the device's parent is suspended.  
When this happens, the nested resume can be carried out with interrupts
disabled if the parent's driver supports it -- and the PM core won't
know that unless there's a flag for it in dev_pm_info.

Alan Stern

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Rafael J. Wysocki]

On Tuesday, September 28, 2010, Alan Stern wrote:
> On Mon, 27 Sep 2010, Rafael J. Wysocki wrote:
> 
> > On Monday, September 27, 2010, Alan Stern wrote:
> > > On Mon, 27 Sep 2010, Rafael J. Wysocki wrote:
...
> Given this, I see no point in adding a special per-call flag.
> 
> There's another advantage to using a per-device flag.  Not all resume
> calls are initiated by the driver; the PM core has to start a resume on
> its own when rpm_resume sees that the device's parent is suspended.  
> When this happens, the nested resume can be carried out with interrupts
> disabled if the parent's driver supports it -- and the PM core won't
> know that unless there's a flag for it in dev_pm_info.

OK, go for it, then.

Thanks,
Rafael

[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()

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

[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]

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 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 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 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

[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 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

[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

[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 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 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 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

[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

[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 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

[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

[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, 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

[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: [linux-pm] [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: [linux-pm] [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: [linux-pm] [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

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

[Alan Stern]

This patch (as1431b) makes the synchronous runtime-PM interface
suitable for use in interrupt handlers.  Subsystems can call the new
pm_runtime_irq_safe() function to tell the PM core that a device's
runtime-PM callbacks should be invoked with interrupts disabled
(runtime_suspend and runtime_resume callbacks will be invoked with the
spinlock held as well).  This permits the pm_runtime_get_sync() and
pm_runtime_put_sync() routines to be called from within interrupt
handlers.

When a device is declared irq-safe in this way, the PM core increments
the parent's usage count, so the parent will never be runtime
suspended.  This prevents difficult situations in which an irq-safe
device can't resume because it is forced to wait for its non-irq-safe
parent.

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

---

Index: usb-2.6/include/linux/pm.h
===================================================================
--- usb-2.6.orig/include/linux/pm.h
+++ usb-2.6/include/linux/pm.h
@@ -486,6 +486,7 @@ struct dev_pm_info {
 	unsigned int		run_wake:1;
 	unsigned int		runtime_auto:1;
 	unsigned int		no_callbacks:1;
+	unsigned int		irq_safe: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
@@ -40,6 +40,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_irq_safe(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 +124,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_irq_safe(struct device *dev) {}
 
 static inline void pm_runtime_mark_last_busy(struct device *dev) {}
 static inline void __pm_runtime_use_autosuspend(struct device *dev,
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
@@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
 		callback = NULL;
 
 	if (callback) {
-		spin_unlock_irq(&dev->power.lock);
+		if (dev->power.irq_safe) {
+			spin_unlock(&dev->power.lock);
 
-		callback(dev);
+			callback(dev);
 
-		spin_lock_irq(&dev->power.lock);
+			spin_lock(&dev->power.lock);
+		} else {
+			spin_unlock_irq(&dev->power.lock);
+
+			callback(dev);
+
+			spin_lock_irq(&dev->power.lock);
+		}
 	}
 
 	dev->power.idle_notification = false;
@@ -250,13 +258,16 @@ static int rpm_callback(int (*cb)(struct
 	if (!cb)
 		return -ENOSYS;
 
-	spin_unlock_irq(&dev->power.lock);
+	if (dev->power.irq_safe) {
+		retval = cb(dev);
+	} else {
+		spin_unlock_irq(&dev->power.lock);
 
-	retval = cb(dev);
+		retval = cb(dev);
 
-	spin_lock_irq(&dev->power.lock);
+		spin_lock_irq(&dev->power.lock);
+	}
 	dev->power.runtime_error = retval;
-
 	return retval;
 }
 
@@ -404,7 +415,7 @@ static int rpm_suspend(struct device *de
 		goto out;
 	}
 
-	if (parent && !parent->power.ignore_children) {
+	if (parent && !parent->power.ignore_children && !dev->power.irq_safe) {
 		spin_unlock_irq(&dev->power.lock);
 
 		pm_request_idle(parent);
@@ -527,10 +538,13 @@ static int rpm_resume(struct device *dev
 
 	if (!parent && dev->parent) {
 		/*
-		 * Increment the parent's resume counter and resume it if
-		 * necessary.
+		 * Increment the parent's usage counter and resume it if
+		 * necessary.  Not needed if dev is irq-safe; then the
+		 * parent is permanently resumed.
 		 */
 		parent = dev->parent;
+		if (dev->power.irq_safe)
+			goto skip_parent;
 		spin_unlock(&dev->power.lock);
 
 		pm_runtime_get_noresume(parent);
@@ -553,6 +567,7 @@ static int rpm_resume(struct device *dev
 			goto out;
 		goto repeat;
 	}
+ skip_parent:
 
 	if (dev->power.no_callbacks)
 		goto no_callback;	/* Assume success. */
@@ -584,7 +599,7 @@ static int rpm_resume(struct device *dev
 		rpm_idle(dev, RPM_ASYNC);
 
  out:
-	if (parent) {
+	if (parent && !dev->power.irq_safe) {
 		spin_unlock_irq(&dev->power.lock);
 
 		pm_runtime_put(parent);
@@ -1065,7 +1080,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)
 {
@@ -1078,6 +1092,28 @@ void pm_runtime_no_callbacks(struct devi
 EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks);
 
 /**
+ * pm_runtime_irq_safe - Leave interrupts disabled during callbacks.
+ * @dev: Device to handle
+ *
+ * Set the power.irq_safe flag, which tells the PM core that the
+ * ->runtime_suspend() and ->runtime_resume() callbacks for this device should
+ * always be invoked with the spinlock held, and all three callbacks should
+ * always be invoked with interrupts disabled.  It also causes the parent's
+ * usage counter to be permanently incremented, preventing the parent from
+ * runtime suspending -- otherwise an irq-safe child might have to wait for a
+ * non-irq-safe parent.
+ */
+void pm_runtime_irq_safe(struct device *dev)
+{
+	if (dev->parent)
+		pm_runtime_get_sync(dev->parent);
+	spin_lock_irq(&dev->power.lock);
+	dev->power.irq_safe = 1;
+	spin_unlock_irq(&dev->power.lock);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_irq_safe);
+
+/**
  * update_autosuspend - Handle a change to a device's autosuspend settings.
  * @dev: Device to handle.
  * @old_delay: The former autosuspend_delay value.
@@ -1199,4 +1235,6 @@ void pm_runtime_remove(struct device *de
 	/* Change the status back to 'suspended' to match the initial status. */
 	if (dev->power.runtime_status == RPM_ACTIVE)
 		pm_runtime_set_suspended(dev);
+	if (dev->power.irq_safe && dev->parent)
+		pm_runtime_put_sync(dev->parent);
 }
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,13 @@ 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 use the pm_runtime_irq_safe() helper function
+to tell the PM core that a device's callbacks should always be invoked with
+interrupts disabled.  This implies that the callback routines must not block
+or sleep, but it also means that the synchronous helper functions listed at the
+end of Section 4 can be used from within an interrupt handler.
+
 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 +244,11 @@ defined in include/linux/pm.h:
       Section 8); it may be modified only by the pm_runtime_no_callbacks()
       helper function
 
+  unsigned int irq_safe;
+    - indicates that the runtime-PM callbacks should be invoked with interrupts
+      disabled; the ->runtime_suspend() and ->runtime_resume() callbacks will
+      be made with the spinlock held as well
+
   unsigned int use_autosuspend;
     - indicates that the device's driver supports delayed autosuspend (see
       Section 9); it may be modified only by the
@@ -397,6 +409,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_irq_safe(struct device *dev);
+    - set the power.irq_safe flag for the device, causing the runtime-PM
+      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
 
@@ -438,6 +454,16 @@ pm_runtime_suspended()
 pm_runtime_mark_last_busy()
 pm_runtime_autosuspend_expiration()
 
+If pm_runtime_irq_safe() has been called for a device then the following helper
+functions may also be called in interrupt context:
+
+pm_runtime_idle()
+pm_runtime_suspend()
+pm_runtime_autosuspend()
+pm_runtime_resume()
+pm_runtime_get_sync()
+pm_runtime_put_sync()
+
 5. Run-time PM Initialization, Device Probing and Removal
 
 Initially, the run-time PM is disabled for all devices, which means that the

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

[Rafael J. Wysocki]

On Friday, November 19, 2010, Alan Stern wrote:
> This patch (as1431b) makes the synchronous runtime-PM interface
> suitable for use in interrupt handlers.  Subsystems can call the new
> pm_runtime_irq_safe() function to tell the PM core that a device's
> runtime-PM callbacks should be invoked with interrupts disabled
> (runtime_suspend and runtime_resume callbacks will be invoked with the
> spinlock held as well).  This permits the pm_runtime_get_sync() and
> pm_runtime_put_sync() routines to be called from within interrupt
> handlers.
> 
> When a device is declared irq-safe in this way, the PM core increments
> the parent's usage count, so the parent will never be runtime
> suspended.  This prevents difficult situations in which an irq-safe
> device can't resume because it is forced to wait for its non-irq-safe
> parent.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> Index: usb-2.6/include/linux/pm.h
> ===================================================================
> --- usb-2.6.orig/include/linux/pm.h
> +++ usb-2.6/include/linux/pm.h
> @@ -486,6 +486,7 @@ struct dev_pm_info {
>  	unsigned int		run_wake:1;
>  	unsigned int		runtime_auto:1;
>  	unsigned int		no_callbacks:1;
> +	unsigned int		irq_safe: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
> @@ -40,6 +40,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_irq_safe(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 +124,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_irq_safe(struct device *dev) {}
>  
>  static inline void pm_runtime_mark_last_busy(struct device *dev) {}
>  static inline void __pm_runtime_use_autosuspend(struct device *dev,
> 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
> @@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
>  		callback = NULL;
>  
>  	if (callback) {
> -		spin_unlock_irq(&dev->power.lock);
> +		if (dev->power.irq_safe) {
> +			spin_unlock(&dev->power.lock);
>  
> -		callback(dev);
> +			callback(dev);
>  
> -		spin_lock_irq(&dev->power.lock);
> +			spin_lock(&dev->power.lock);
> +		} else {
> +			spin_unlock_irq(&dev->power.lock);
> +
> +			callback(dev);
> +
> +			spin_lock_irq(&dev->power.lock);
> +		}
>  	}

I didn't like this change before and I still don't like it.  Quite frankly, I'm
not sure I can convince Linus to pull it. :-)

Why don't we simply execute the callback under the spinlock in the
IRQ safe case?

Rafael

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

[Alan Stern]

On Sat, 20 Nov 2010, Rafael J. Wysocki wrote:

> On Friday, November 19, 2010, Alan Stern wrote:
> > This patch (as1431b) makes the synchronous runtime-PM interface
> > suitable for use in interrupt handlers.  Subsystems can call the new
> > pm_runtime_irq_safe() function to tell the PM core that a device's
> > runtime-PM callbacks should be invoked with interrupts disabled
> > (runtime_suspend and runtime_resume callbacks will be invoked with the
> > spinlock held as well).  This permits the pm_runtime_get_sync() and
> > pm_runtime_put_sync() routines to be called from within interrupt
> > handlers.
> > 
> > When a device is declared irq-safe in this way, the PM core increments
> > the parent's usage count, so the parent will never be runtime
> > suspended.  This prevents difficult situations in which an irq-safe
> > device can't resume because it is forced to wait for its non-irq-safe
> > parent.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

> > --- usb-2.6.orig/drivers/base/power/runtime.c
> > +++ usb-2.6/drivers/base/power/runtime.c
> > @@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
> >  		callback = NULL;
> >  
> >  	if (callback) {
> > -		spin_unlock_irq(&dev->power.lock);
> > +		if (dev->power.irq_safe) {
> > +			spin_unlock(&dev->power.lock);
> >  
> > -		callback(dev);
> > +			callback(dev);
> >  
> > -		spin_lock_irq(&dev->power.lock);
> > +			spin_lock(&dev->power.lock);
> > +		} else {
> > +			spin_unlock_irq(&dev->power.lock);
> > +
> > +			callback(dev);
> > +
> > +			spin_lock_irq(&dev->power.lock);
> > +		}
> >  	}
> 
> I didn't like this change before and I still don't like it.  Quite frankly, I'm
> not sure I can convince Linus to pull it. :-)
> 
> Why don't we simply execute the callback under the spinlock in the
> IRQ safe case?

Because it wouldn't work.  The job of the runtime_idle callback is to
call pm_runtime_suspend when needed.  But if the callback runs under
the spinlock then pm_runtime_suspend would hang when it tries to grab
the lock.

I don't think Linus will object to this.  What he doesn't like is when
some code drops a lock, reacquires it, and then behaves as though the
lock had been held all along.  That's not the case here; rpm_idle()  
does not depend on any state remaining unchanged across the callback.

Alan Stern

Re: [linux-pm] [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Sat, 20 Nov 2010, Alan Stern wrote:

> On Sat, 20 Nov 2010, Rafael J. Wysocki wrote:
> 
> > On Friday, November 19, 2010, Alan Stern wrote:
> > > This patch (as1431b) makes the synchronous runtime-PM interface
> > > suitable for use in interrupt handlers.  Subsystems can call the new
> > > pm_runtime_irq_safe() function to tell the PM core that a device's
> > > runtime-PM callbacks should be invoked with interrupts disabled
> > > (runtime_suspend and runtime_resume callbacks will be invoked with the
> > > spinlock held as well).  This permits the pm_runtime_get_sync() and
> > > pm_runtime_put_sync() routines to be called from within interrupt
> > > handlers.
> > > 
> > > When a device is declared irq-safe in this way, the PM core increments
> > > the parent's usage count, so the parent will never be runtime
> > > suspended.  This prevents difficult situations in which an irq-safe
> > > device can't resume because it is forced to wait for its non-irq-safe
> > > parent.
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> > > --- usb-2.6.orig/drivers/base/power/runtime.c
> > > +++ usb-2.6/drivers/base/power/runtime.c
> > > @@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
> > >  		callback = NULL;
> > >  
> > >  	if (callback) {
> > > -		spin_unlock_irq(&dev->power.lock);
> > > +		if (dev->power.irq_safe) {
> > > +			spin_unlock(&dev->power.lock);
> > >  
> > > -		callback(dev);
> > > +			callback(dev);
> > >  
> > > -		spin_lock_irq(&dev->power.lock);
> > > +			spin_lock(&dev->power.lock);
> > > +		} else {
> > > +			spin_unlock_irq(&dev->power.lock);
> > > +
> > > +			callback(dev);
> > > +
> > > +			spin_lock_irq(&dev->power.lock);
> > > +		}
> > >  	}
> > 
> > I didn't like this change before and I still don't like it.  Quite frankly, I'm
> > not sure I can convince Linus to pull it. :-)
> > 
> > Why don't we simply execute the callback under the spinlock in the
> > IRQ safe case?
> 
> Because it wouldn't work.  The job of the runtime_idle callback is to
> call pm_runtime_suspend when needed.  But if the callback runs under
> the spinlock then pm_runtime_suspend would hang when it tries to grab
> the lock.
> 
> I don't think Linus will object to this.  What he doesn't like is when
> some code drops a lock, reacquires it, and then behaves as though the
> lock had been held all along.  That's not the case here; rpm_idle()  
> does not depend on any state remaining unchanged across the callback.

One other thing I forgot to mention...  If Linus doesn't like the way
the new code drops the spinlock and then reacquires it, then he must
also not like the existing code, which does the same thing.  The only
difference lies in whether or not interrupts are re-enabled.

Alan Stern

Re: [linux-pm] [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Saturday, November 20, 2010, Alan Stern wrote:
> On Sat, 20 Nov 2010, Alan Stern wrote:
> 
> > On Sat, 20 Nov 2010, Rafael J. Wysocki wrote:
> > 
> > > On Friday, November 19, 2010, Alan Stern wrote:
...
> > 
> > I don't think Linus will object to this.  What he doesn't like is when
> > some code drops a lock, reacquires it, and then behaves as though the
> > lock had been held all along.  That's not the case here; rpm_idle()  
> > does not depend on any state remaining unchanged across the callback.
> 
> One other thing I forgot to mention...  If Linus doesn't like the way
> the new code drops the spinlock and then reacquires it, then he must
> also not like the existing code, which does the same thing.  The only
> difference lies in whether or not interrupts are re-enabled.

The problem I have with this change is that switching interrupts off really is
a part of the locking operation, so using spin_unlock() after spin_lock_irq...()
is kind of like releasing the lock partially, which I don't think is valid
(even if we're going to reacquire the lock immediately).

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Saturday, November 20, 2010, Alan Stern wrote:
> On Sat, 20 Nov 2010, Rafael J. Wysocki wrote:
> 
> > On Friday, November 19, 2010, Alan Stern wrote:
> > > This patch (as1431b) makes the synchronous runtime-PM interface
> > > suitable for use in interrupt handlers.  Subsystems can call the new
> > > pm_runtime_irq_safe() function to tell the PM core that a device's
> > > runtime-PM callbacks should be invoked with interrupts disabled
> > > (runtime_suspend and runtime_resume callbacks will be invoked with the
> > > spinlock held as well).  This permits the pm_runtime_get_sync() and
> > > pm_runtime_put_sync() routines to be called from within interrupt
> > > handlers.
> > > 
> > > When a device is declared irq-safe in this way, the PM core increments
> > > the parent's usage count, so the parent will never be runtime
> > > suspended.  This prevents difficult situations in which an irq-safe
> > > device can't resume because it is forced to wait for its non-irq-safe
> > > parent.
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> > > --- usb-2.6.orig/drivers/base/power/runtime.c
> > > +++ usb-2.6/drivers/base/power/runtime.c
> > > @@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
> > >  		callback = NULL;
> > >  
> > >  	if (callback) {
> > > -		spin_unlock_irq(&dev->power.lock);
> > > +		if (dev->power.irq_safe) {
> > > +			spin_unlock(&dev->power.lock);
> > >  
> > > -		callback(dev);
> > > +			callback(dev);
> > >  
> > > -		spin_lock_irq(&dev->power.lock);
> > > +			spin_lock(&dev->power.lock);
> > > +		} else {
> > > +			spin_unlock_irq(&dev->power.lock);
> > > +
> > > +			callback(dev);
> > > +
> > > +			spin_lock_irq(&dev->power.lock);
> > > +		}
> > >  	}
> > 
> > I didn't like this change before and I still don't like it.  Quite frankly, I'm
> > not sure I can convince Linus to pull it. :-)
> > 
> > Why don't we simply execute the callback under the spinlock in the
> > IRQ safe case?
> 
> Because it wouldn't work.  The job of the runtime_idle callback is to
> call pm_runtime_suspend when needed.  But if the callback runs under
> the spinlock then pm_runtime_suspend would hang when it tries to grab
> the lock.

Yes, in the _idle case.  I actually should have put my comment under
the change in rpm_callback(), which is what I really meant.

Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
we need it, exactly?

> I don't think Linus will object to this.

Well, I guess we'll see. :-)

Thanks,
Rafael

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

[Alan Stern]

On Mon, 22 Nov 2010, Rafael J. Wysocki wrote:

> > > I didn't like this change before and I still don't like it.  Quite frankly, I'm
> > > not sure I can convince Linus to pull it. :-)
> > > 
> > > Why don't we simply execute the callback under the spinlock in the
> > > IRQ safe case?
> > 
> > Because it wouldn't work.  The job of the runtime_idle callback is to
> > call pm_runtime_suspend when needed.  But if the callback runs under
> > the spinlock then pm_runtime_suspend would hang when it tries to grab
> > the lock.
> 
> Yes, in the _idle case.  I actually should have put my comment under
> the change in rpm_callback(), which is what I really meant.

But the new rpm_callback() _does_ simply execute the callback under the
spinlock in the irq-safe case.  So I don't understand what you mean
here.

> Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
> we need it, exactly?

Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
irq-safe version of rpm_idle() then drivers wouldn't be able to call 
pm_runtime_put_sync() from within an interrupt handler.


> The problem I have with this change is that switching interrupts off really is
> a part of the locking operation, so using spin_unlock() after spin_lock_irq...()
> is kind of like releasing the lock partially, which I don't think is valid
> (even if we're going to reacquire the lock immediately).

On the contrary; spin_unlock() after spin_lock_irq() doesn't partially
release the lock -- it releases the lock _entirely_!  :-)

Besides which, the existing code already releases the spinlock before 
making callbacks, so there should be no reason to worry about that 
issue.  The new code either:

	releases the spinlock but keeps interrupts disabled (in
	rpm_idle), or

	doesn't release the spinlock (in rpm_callback).

Either way, I should think you'd find the new code _less_ objectionable 
than the existing code, not _more_ objectionable.

Alan Stern

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

[Rafael J. Wysocki]

On Monday, November 22, 2010, Alan Stern wrote:
> On Mon, 22 Nov 2010, Rafael J. Wysocki wrote:
> 
> > > > I didn't like this change before and I still don't like it.  Quite frankly, I'm
> > > > not sure I can convince Linus to pull it. :-)
> > > > 
> > > > Why don't we simply execute the callback under the spinlock in the
> > > > IRQ safe case?
> > > 
> > > Because it wouldn't work.  The job of the runtime_idle callback is to
> > > call pm_runtime_suspend when needed.  But if the callback runs under
> > > the spinlock then pm_runtime_suspend would hang when it tries to grab
> > > the lock.
> > 
> > Yes, in the _idle case.  I actually should have put my comment under
> > the change in rpm_callback(), which is what I really meant.
> 
> But the new rpm_callback() _does_ simply execute the callback under the
> spinlock in the irq-safe case.  So I don't understand what you mean
> here.

OK, sorry, I confused things.  I have no objections to this part, then, let's
focus on the _idle case.

> > Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
> > we need it, exactly?
> 
> Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
> irq-safe version of rpm_idle() then drivers wouldn't be able to call 
> pm_runtime_put_sync() from within an interrupt handler.

Right.  Which they can't do now, can they?

Why do you think we should allow them to do that?

> > The problem I have with this change is that switching interrupts off really is
> > a part of the locking operation, so using spin_unlock() after spin_lock_irq...()
> > is kind of like releasing the lock partially, which I don't think is valid
> > (even if we're going to reacquire the lock immediately).
> 
> On the contrary; spin_unlock() after spin_lock_irq() doesn't partially
> release the lock -- it releases the lock _entirely_!  :-)

Well, not really, as far as I understand it.  The semantics of spin_lock_irq()
is "turn interrupts off to prevent interrupt handlers running on _this_ CPU
from racing with us and acquire the lock to prevent _other_ CPUs from racing
with us".  If you build the code for non-SMP it becomes the first part only.
So IMO the "turn interrupts on/off" thing is a part of the full synchronization
mechanism in this case.

Anyway, though, if the only reason of doing this is to allow interrupt handlers
to call pm_runtime_put_sync(), then I rather wouldn't do it at all.

Thanks,
Rafael

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

[Alan Stern]

On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:

> > > Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
> > > we need it, exactly?
> > 
> > Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
> > irq-safe version of rpm_idle() then drivers wouldn't be able to call 
> > pm_runtime_put_sync() from within an interrupt handler.
> 
> Right.  Which they can't do now, can they?

True.  That was the point of this patch -- to allow interrupt handlers
to do runtime PM, which they can't do now.

> Why do you think we should allow them to do that?

Are you suggesting that interrupt handlers stick to pm_runtime_suspend 
and pm_runtime_resume, and ignore pm_runtime_get_sync and 
pm_runtime_put_sync?

Recall that after probing is finished, the driver core does a
pm_runtime_put_sync.  That might happen while an interrupt handler is
running on another CPU.  If the interrupt handler didn't increment the
usage_count, the driver core could cause the device to suspend while
the interrupt handler was still using it.

Or are you suggesting that interrupt handlers use pm_runtime_get_sync 
and implement a poor-man's version of pm_runtime_put_sync by doing:

	pm_runtime_put_no_idle();
	pm_runtime_suspend();

Is there some particular reason for denying interrupt handlers the
ability to use pm_runtime_put_sync?  It seems odd to disallow that 
while allowing pm_runtime_get_sync.

Or maybe you think that when pm_runtime_put_sync detects the 
usage_count has decremented to 0 and the device is irq-safe, it should 
call rpm_suspend directly instead of calling rpm_idle?

In short, I don't see any reason not to present the same API to
interrupt handlers for irq-safe devices as we present to
process-context drivers for ordinary devices.

> Anyway, though, if the only reason of doing this is to allow interrupt handlers
> to call pm_runtime_put_sync(), then I rather wouldn't do it at all.

Why not?

Alan Stern

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

[Rafael J. Wysocki]

On Tuesday, November 23, 2010, Alan Stern wrote:
> On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:
> 
> > > > Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
> > > > we need it, exactly?
> > > 
> > > Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
> > > irq-safe version of rpm_idle() then drivers wouldn't be able to call 
> > > pm_runtime_put_sync() from within an interrupt handler.
> > 
> > Right.  Which they can't do now, can they?
> 
> True.  That was the point of this patch -- to allow interrupt handlers
> to do runtime PM, which they can't do now.

The original idea was to allow suspend and resume to be carried out
with interrupts off, not necessarily by interrupt handlers.  We've never
considered doing that with _idle before.

> > Why do you think we should allow them to do that?
> 
> Are you suggesting that interrupt handlers stick to pm_runtime_suspend 
> and pm_runtime_resume, and ignore pm_runtime_get_sync and 
> pm_runtime_put_sync?

Why do they need the _sync versions?  What exactly is wrong with
calling

pm_runtime_put_noidle()
pm_runtime_suspend()

from an interrupt handler if it really wants the synchronous suspend to be
carried out at this point?

I don't really see a reason for calling pm_runtime_put_sync() by an interrupt
handler, but perhaps I'm overlooking something important.

> Recall that after probing is finished, the driver core does a
> pm_runtime_put_sync.  That might happen while an interrupt handler is
> running on another CPU.  If the interrupt handler didn't increment the
> usage_count, the driver core could cause the device to suspend while
> the interrupt handler was still using it.
> 
> Or are you suggesting that interrupt handlers use pm_runtime_get_sync 
> and implement a poor-man's version of pm_runtime_put_sync by doing:
> 
> 	pm_runtime_put_no_idle();
> 	pm_runtime_suspend();

Yes, I am.  Although simply calling pm_runtime_put() should work as well (yes,
the suspend won't occur immediately, but what's wrong with that?).

> Is there some particular reason for denying interrupt handlers the
> ability to use pm_runtime_put_sync?  It seems odd to disallow that 
> while allowing pm_runtime_get_sync.
> 
> Or maybe you think that when pm_runtime_put_sync detects the 
> usage_count has decremented to 0 and the device is irq-safe, it should 
> call rpm_suspend directly instead of calling rpm_idle?

That also would work for me, actually.

> In short, I don't see any reason not to present the same API to
> interrupt handlers for irq-safe devices as we present to
> process-context drivers for ordinary devices.
> 
> > Anyway, though, if the only reason of doing this is to allow interrupt handlers
> > to call pm_runtime_put_sync(), then I rather wouldn't do it at all.
> 
> Why not?

Because it's a fragile design with unusual behavior.  I'm pretty sure we'd see
some interesting abuse of it sooner or later. :-)

Thanks,
Rafael

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

[Kevin Hilman]

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

> On Tuesday, November 23, 2010, Alan Stern wrote:
>> On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:
>> 
>> > > > Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
>> > > > we need it, exactly?
>> > > 
>> > > Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
>> > > irq-safe version of rpm_idle() then drivers wouldn't be able to call 
>> > > pm_runtime_put_sync() from within an interrupt handler.
>> > 
>> > Right.  Which they can't do now, can they?
>> 
>> True.  That was the point of this patch -- to allow interrupt handlers
>> to do runtime PM, which they can't do now.
>
> The original idea was to allow suspend and resume to be carried out
> with interrupts off, not necessarily by interrupt handlers.  We've never
> considered doing that with _idle before.
>
>> > Why do you think we should allow them to do that?
>> 
>> Are you suggesting that interrupt handlers stick to pm_runtime_suspend 
>> and pm_runtime_resume, and ignore pm_runtime_get_sync and 
>> pm_runtime_put_sync?
>
> Why do they need the _sync versions?  What exactly is wrong with
> calling
>
> pm_runtime_put_noidle()
> pm_runtime_suspend()
>
> from an interrupt handler if it really wants the synchronous suspend to be
> carried out at this point?
>
> I don't really see a reason for calling pm_runtime_put_sync() by an interrupt
> handler, but perhaps I'm overlooking something important.

While I like the idea of the symmetry of having both _get_sync() and
_put_sync() callable from an interrupt handler, I can't currently think
of a situation where we would need to _put_sync() in the ISR.  A
standard _put() should suffice for all cases I can imagine.

Kevin

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

[Alan Stern]

On Tue, 23 Nov 2010, Kevin Hilman wrote:

> While I like the idea of the symmetry of having both _get_sync() and
> _put_sync() callable from an interrupt handler, I can't currently think
> of a situation where we would need to _put_sync() in the ISR.  A
> standard _put() should suffice for all cases I can imagine.

It's wasteful to go through the context switch to the workqueue process
if you don't need to.  And it's time consuming; you want to power down 
the device as soon as possible once the interrupt handler is finished, 
right?

What do you think of the pm_runtime_put_sync_suspend() proposal?

Alan Stern

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

[Kevin Hilman]

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

> On Tue, 23 Nov 2010, Kevin Hilman wrote:
>
>> While I like the idea of the symmetry of having both _get_sync() and
>> _put_sync() callable from an interrupt handler, I can't currently think
>> of a situation where we would need to _put_sync() in the ISR.  A
>> standard _put() should suffice for all cases I can imagine.
>
> It's wasteful to go through the context switch to the workqueue process
> if you don't need to.  And it's time consuming; you want to power down 
> the device as soon as possible once the interrupt handler is finished, 
> right?
>
> What do you think of the pm_runtime_put_sync_suspend() proposal?

That should be fine.

Thinking of this some more, I don't have any use cases currently where
we would any sort of put in the ISR, since the ISR is usually an
indicator that something else will be accessing the device shortly after
the ISR is finished.

Kevin

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

[Alan Stern]

On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:

> > Or maybe you think that when pm_runtime_put_sync detects the 
> > usage_count has decremented to 0 and the device is irq-safe, it should 
> > call rpm_suspend directly instead of calling rpm_idle?
> 
> That also would work for me, actually.

Okay, then consider this proposal.  I'll introduce a new
pm_runtime_put_sync_suspend() function which decrements the usage_count
and calls rpm_suspend directly if the count drops to 0.  Then interrupt
handlers could use this function in place of pm_runtime_put_sync(),
provided the device was irq-safe.

Not only that, pm_runtime_put_sync_suspend() would be available for
anyone to use.  It must be reasonably common for runtime_idle routines
to do nothing but call pm_runtime_suspend().  The new API call would
save a little overhead.

Alan Stern

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

[Rafael J. Wysocki]

On Wednesday, November 24, 2010, Alan Stern wrote:
> On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:
> 
> > > Or maybe you think that when pm_runtime_put_sync detects the 
> > > usage_count has decremented to 0 and the device is irq-safe, it should 
> > > call rpm_suspend directly instead of calling rpm_idle?
> > 
> > That also would work for me, actually.
> 
> Okay, then consider this proposal.  I'll introduce a new
> pm_runtime_put_sync_suspend() function which decrements the usage_count
> and calls rpm_suspend directly if the count drops to 0.  Then interrupt
> handlers could use this function in place of pm_runtime_put_sync(),
> provided the device was irq-safe.
> 
> Not only that, pm_runtime_put_sync_suspend() would be available for
> anyone to use.  It must be reasonably common for runtime_idle routines
> to do nothing but call pm_runtime_suspend().  The new API call would
> save a little overhead.

Fine by me.

Thanks,
Rafael

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

[Alan Stern]

This patch (as1431c) makes the synchronous runtime-PM interface
suitable for use in interrupt handlers.  Subsystems can call the new
pm_runtime_irq_safe() function to tell the PM core that a device's
runtime_suspend and runtime_resume callbacks should be invoked with
interrupts disabled and the spinlock held.  This permits the
pm_runtime_get_sync() and the new pm_runtime_put_sync_suspend()
routines to be called from within interrupt handlers.

When a device is declared irq-safe in this way, the PM core increments
the parent's usage count, so the parent will never be runtime
suspended.  This prevents difficult situations in which an irq-safe
device can't resume because it is forced to wait for its non-irq-safe
parent.

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

---

Index: usb-2.6/include/linux/pm.h
===================================================================
--- usb-2.6.orig/include/linux/pm.h
+++ usb-2.6/include/linux/pm.h
@@ -486,6 +486,7 @@ struct dev_pm_info {
 	unsigned int		run_wake:1;
 	unsigned int		runtime_auto:1;
 	unsigned int		no_callbacks:1;
+	unsigned int		irq_safe: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
@@ -40,6 +40,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_irq_safe(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 +124,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_irq_safe(struct device *dev) {}
 
 static inline void pm_runtime_mark_last_busy(struct device *dev) {}
 static inline void __pm_runtime_use_autosuspend(struct device *dev,
@@ -195,6 +197,11 @@ static inline int pm_runtime_put_sync(st
 	return __pm_runtime_idle(dev, RPM_GET_PUT);
 }
 
+static inline int pm_runtime_put_sync_suspend(struct device *dev)
+{
+	return __pm_runtime_suspend(dev, RPM_GET_PUT);
+}
+
 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
@@ -250,13 +250,16 @@ static int rpm_callback(int (*cb)(struct
 	if (!cb)
 		return -ENOSYS;
 
-	spin_unlock_irq(&dev->power.lock);
+	if (dev->power.irq_safe) {
+		retval = cb(dev);
+	} else {
+		spin_unlock_irq(&dev->power.lock);
 
-	retval = cb(dev);
+		retval = cb(dev);
 
-	spin_lock_irq(&dev->power.lock);
+		spin_lock_irq(&dev->power.lock);
+	}
 	dev->power.runtime_error = retval;
-
 	return retval;
 }
 
@@ -404,7 +407,7 @@ static int rpm_suspend(struct device *de
 		goto out;
 	}
 
-	if (parent && !parent->power.ignore_children) {
+	if (parent && !parent->power.ignore_children && !dev->power.irq_safe) {
 		spin_unlock_irq(&dev->power.lock);
 
 		pm_request_idle(parent);
@@ -527,10 +530,13 @@ static int rpm_resume(struct device *dev
 
 	if (!parent && dev->parent) {
 		/*
-		 * Increment the parent's resume counter and resume it if
-		 * necessary.
+		 * Increment the parent's usage counter and resume it if
+		 * necessary.  Not needed if dev is irq-safe; then the
+		 * parent is permanently resumed.
 		 */
 		parent = dev->parent;
+		if (dev->power.irq_safe)
+			goto skip_parent;
 		spin_unlock(&dev->power.lock);
 
 		pm_runtime_get_noresume(parent);
@@ -553,6 +559,7 @@ static int rpm_resume(struct device *dev
 			goto out;
 		goto repeat;
 	}
+ skip_parent:
 
 	if (dev->power.no_callbacks)
 		goto no_callback;	/* Assume success. */
@@ -584,7 +591,7 @@ static int rpm_resume(struct device *dev
 		rpm_idle(dev, RPM_ASYNC);
 
  out:
-	if (parent) {
+	if (parent && !dev->power.irq_safe) {
 		spin_unlock_irq(&dev->power.lock);
 
 		pm_runtime_put(parent);
@@ -1065,7 +1072,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)
 {
@@ -1078,6 +1084,27 @@ void pm_runtime_no_callbacks(struct devi
 EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks);
 
 /**
+ * pm_runtime_irq_safe - Leave interrupts disabled during callbacks.
+ * @dev: Device to handle
+ *
+ * Set the power.irq_safe flag, which tells the PM core that the
+ * ->runtime_suspend() and ->runtime_resume() callbacks for this device should
+ * always be invoked with the spinlock held and interrupts disabled.  It also
+ * causes the parent's usage counter to be permanently incremented, preventing
+ * the parent from runtime suspending -- otherwise an irq-safe child might have
+ * to wait for a non-irq-safe parent.
+ */
+void pm_runtime_irq_safe(struct device *dev)
+{
+	if (dev->parent)
+		pm_runtime_get_sync(dev->parent);
+	spin_lock_irq(&dev->power.lock);
+	dev->power.irq_safe = 1;
+	spin_unlock_irq(&dev->power.lock);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_irq_safe);
+
+/**
  * update_autosuspend - Handle a change to a device's autosuspend settings.
  * @dev: Device to handle.
  * @old_delay: The former autosuspend_delay value.
@@ -1199,4 +1226,6 @@ void pm_runtime_remove(struct device *de
 	/* Change the status back to 'suspended' to match the initial status. */
 	if (dev->power.runtime_status == RPM_ACTIVE)
 		pm_runtime_set_suspended(dev);
+	if (dev->power.irq_safe && dev->parent)
+		pm_runtime_put_sync(dev->parent);
 }
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,15 @@ 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 use the pm_runtime_irq_safe() helper function
+to tell the PM core that a device's ->runtime_suspend() and ->runtime_resume()
+callbacks should be invoked in atomic context with interrupts disabled
+(->runtime_idle() is still invoked the default way).  This implies that these
+callback routines must not block or sleep, but it also means that the
+synchronous helper functions listed at the end of Section 4 can be used within
+an interrupt handler or in an atomic context.
+
 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 +246,10 @@ defined in include/linux/pm.h:
       Section 8); it may be modified only by the pm_runtime_no_callbacks()
       helper function
 
+  unsigned int irq_safe;
+    - indicates that the ->runtime_suspend() and ->runtime_resume() callbacks
+      will be invoked with the spinlock held and 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
@@ -344,6 +357,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_suspend(struct device *dev);
+    - decrement the device's usage counter; if the result is 0 then run
+      pm_runtime_suspend(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 +414,11 @@ 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_irq_safe(struct device *dev);
+    - set the power.irq_safe flag for the device, causing the runtime-PM
+      suspend and resume callbacks (but not the idle callback) 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
 
@@ -438,6 +460,15 @@ pm_runtime_suspended()
 pm_runtime_mark_last_busy()
 pm_runtime_autosuspend_expiration()
 
+If pm_runtime_irq_safe() has been called for a device then the following helper
+functions may also be used in interrupt context:
+
+pm_runtime_suspend()
+pm_runtime_autosuspend()
+pm_runtime_resume()
+pm_runtime_get_sync()
+pm_runtime_put_sync_suspend()
+
 5. Run-time PM Initialization, Device Probing and Removal
 
 Initially, the run-time PM is disabled for all devices, which means that the

Re: [linux-pm] [PATCH ver. 3] PM: add synchronous runtime interface for interrupt handlers

[Oliver Neukum]

Am Donnerstag, 25. November 2010, 16:52:39 schrieb Alan Stern:
> When a device is declared irq-safe in this way, the PM core increments
> the parent's usage count, so the parent will never be runtime
> suspended.  This prevents difficult situations in which an irq-safe
> device can't resume because it is forced to wait for its non-irq-safe
> parent.

Shouldn't you walk further up the tree if the parent itself is irq-safe?
It seems like a waste of power to not suspend a paren, that can be
woken in irq.

	Regards
		Oliver

Re: [linux-pm] [PATCH ver. 3] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Thursday, November 25, 2010, Oliver Neukum wrote:
> Am Donnerstag, 25. November 2010, 16:52:39 schrieb Alan Stern:
> > When a device is declared irq-safe in this way, the PM core increments
> > the parent's usage count, so the parent will never be runtime
> > suspended.  This prevents difficult situations in which an irq-safe
> > device can't resume because it is forced to wait for its non-irq-safe
> > parent.
> 
> Shouldn't you walk further up the tree if the parent itself is irq-safe?
> It seems like a waste of power to not suspend a paren, that can be
> woken in irq.

Then we should walk up the tree if the parent's parent is irq-safe and so
on, which would make us spend uncertain amount of time in the ISR (or generally
with interrupts off).  I think it's safer not to do that.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thursday, November 25, 2010, Alan Stern wrote:
> This patch (as1431c) makes the synchronous runtime-PM interface
> suitable for use in interrupt handlers.  Subsystems can call the new
> pm_runtime_irq_safe() function to tell the PM core that a device's
> runtime_suspend and runtime_resume callbacks should be invoked with
> interrupts disabled and the spinlock held.  This permits the
> pm_runtime_get_sync() and the new pm_runtime_put_sync_suspend()
> routines to be called from within interrupt handlers.
> 
> When a device is declared irq-safe in this way, the PM core increments
> the parent's usage count, so the parent will never be runtime
> suspended.  This prevents difficult situations in which an irq-safe
> device can't resume because it is forced to wait for its non-irq-safe
> parent.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Applied to suspend-2.6/linux-next.

Thanks,
Rafael


> ---
> 
> Index: usb-2.6/include/linux/pm.h
> ===================================================================
> --- usb-2.6.orig/include/linux/pm.h
> +++ usb-2.6/include/linux/pm.h
> @@ -486,6 +486,7 @@ struct dev_pm_info {
>  	unsigned int		run_wake:1;
>  	unsigned int		runtime_auto:1;
>  	unsigned int		no_callbacks:1;
> +	unsigned int		irq_safe: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
> @@ -40,6 +40,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_irq_safe(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 +124,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_irq_safe(struct device *dev) {}
>  
>  static inline void pm_runtime_mark_last_busy(struct device *dev) {}
>  static inline void __pm_runtime_use_autosuspend(struct device *dev,
> @@ -195,6 +197,11 @@ static inline int pm_runtime_put_sync(st
>  	return __pm_runtime_idle(dev, RPM_GET_PUT);
>  }
>  
> +static inline int pm_runtime_put_sync_suspend(struct device *dev)
> +{
> +	return __pm_runtime_suspend(dev, RPM_GET_PUT);
> +}
> +
>  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
> @@ -250,13 +250,16 @@ static int rpm_callback(int (*cb)(struct
>  	if (!cb)
>  		return -ENOSYS;
>  
> -	spin_unlock_irq(&dev->power.lock);
> +	if (dev->power.irq_safe) {
> +		retval = cb(dev);
> +	} else {
> +		spin_unlock_irq(&dev->power.lock);
>  
> -	retval = cb(dev);
> +		retval = cb(dev);
>  
> -	spin_lock_irq(&dev->power.lock);
> +		spin_lock_irq(&dev->power.lock);
> +	}
>  	dev->power.runtime_error = retval;
> -
>  	return retval;
>  }
>  
> @@ -404,7 +407,7 @@ static int rpm_suspend(struct device *de
>  		goto out;
>  	}
>  
> -	if (parent && !parent->power.ignore_children) {
> +	if (parent && !parent->power.ignore_children && !dev->power.irq_safe) {
>  		spin_unlock_irq(&dev->power.lock);
>  
>  		pm_request_idle(parent);
> @@ -527,10 +530,13 @@ static int rpm_resume(struct device *dev
>  
>  	if (!parent && dev->parent) {
>  		/*
> -		 * Increment the parent's resume counter and resume it if
> -		 * necessary.
> +		 * Increment the parent's usage counter and resume it if
> +		 * necessary.  Not needed if dev is irq-safe; then the
> +		 * parent is permanently resumed.
>  		 */
>  		parent = dev->parent;
> +		if (dev->power.irq_safe)
> +			goto skip_parent;
>  		spin_unlock(&dev->power.lock);
>  
>  		pm_runtime_get_noresume(parent);
> @@ -553,6 +559,7 @@ static int rpm_resume(struct device *dev
>  			goto out;
>  		goto repeat;
>  	}
> + skip_parent:
>  
>  	if (dev->power.no_callbacks)
>  		goto no_callback;	/* Assume success. */
> @@ -584,7 +591,7 @@ static int rpm_resume(struct device *dev
>  		rpm_idle(dev, RPM_ASYNC);
>  
>   out:
> -	if (parent) {
> +	if (parent && !dev->power.irq_safe) {
>  		spin_unlock_irq(&dev->power.lock);
>  
>  		pm_runtime_put(parent);
> @@ -1065,7 +1072,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)
>  {
> @@ -1078,6 +1084,27 @@ void pm_runtime_no_callbacks(struct devi
>  EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks);
>  
>  /**
> + * pm_runtime_irq_safe - Leave interrupts disabled during callbacks.
> + * @dev: Device to handle
> + *
> + * Set the power.irq_safe flag, which tells the PM core that the
> + * ->runtime_suspend() and ->runtime_resume() callbacks for this device should
> + * always be invoked with the spinlock held and interrupts disabled.  It also
> + * causes the parent's usage counter to be permanently incremented, preventing
> + * the parent from runtime suspending -- otherwise an irq-safe child might have
> + * to wait for a non-irq-safe parent.
> + */
> +void pm_runtime_irq_safe(struct device *dev)
> +{
> +	if (dev->parent)
> +		pm_runtime_get_sync(dev->parent);
> +	spin_lock_irq(&dev->power.lock);
> +	dev->power.irq_safe = 1;
> +	spin_unlock_irq(&dev->power.lock);
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_irq_safe);
> +
> +/**
>   * update_autosuspend - Handle a change to a device's autosuspend settings.
>   * @dev: Device to handle.
>   * @old_delay: The former autosuspend_delay value.
> @@ -1199,4 +1226,6 @@ void pm_runtime_remove(struct device *de
>  	/* Change the status back to 'suspended' to match the initial status. */
>  	if (dev->power.runtime_status == RPM_ACTIVE)
>  		pm_runtime_set_suspended(dev);
> +	if (dev->power.irq_safe && dev->parent)
> +		pm_runtime_put_sync(dev->parent);
>  }
> 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,15 @@ 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 use the pm_runtime_irq_safe() helper function
> +to tell the PM core that a device's ->runtime_suspend() and ->runtime_resume()
> +callbacks should be invoked in atomic context with interrupts disabled
> +(->runtime_idle() is still invoked the default way).  This implies that these
> +callback routines must not block or sleep, but it also means that the
> +synchronous helper functions listed at the end of Section 4 can be used within
> +an interrupt handler or in an atomic context.
> +
>  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 +246,10 @@ defined in include/linux/pm.h:
>        Section 8); it may be modified only by the pm_runtime_no_callbacks()
>        helper function
>  
> +  unsigned int irq_safe;
> +    - indicates that the ->runtime_suspend() and ->runtime_resume() callbacks
> +      will be invoked with the spinlock held and 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
> @@ -344,6 +357,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_suspend(struct device *dev);
> +    - decrement the device's usage counter; if the result is 0 then run
> +      pm_runtime_suspend(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 +414,11 @@ 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_irq_safe(struct device *dev);
> +    - set the power.irq_safe flag for the device, causing the runtime-PM
> +      suspend and resume callbacks (but not the idle callback) 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
>  
> @@ -438,6 +460,15 @@ pm_runtime_suspended()
>  pm_runtime_mark_last_busy()
>  pm_runtime_autosuspend_expiration()
>  
> +If pm_runtime_irq_safe() has been called for a device then the following helper
> +functions may also be used in interrupt context:
> +
> +pm_runtime_suspend()
> +pm_runtime_autosuspend()
> +pm_runtime_resume()
> +pm_runtime_get_sync()
> +pm_runtime_put_sync_suspend()
> +
>  5. Run-time PM Initialization, Device Probing and Removal
>  
>  Initially, the run-time PM is disabled for all devices, which means that the
> 
> 
> 

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

[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

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: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Kevin Hilman]

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

> On Mon, 27 Sep 2010, Rafael J. Wysocki wrote:
>
>> > How about adding another flag to the dev_pm_info structure, to indicate 
>> > that the runtime callbacks may be called in interrupt context?
>> > 
>> > Maybe that will lead to problems I haven't thought of.  But if it seems 
>> > okay to you, I can code it up easily enough.
>> 
>> Hmm.  I was thinking about adding a new RPM_ flag for that, like RPM_FASTPATH,
>> telling the PM core to assume the callbacks will not sleep and that the call
>> might be from interrupt handler.
>
> That might appear more flexible, but is there any advantage to it?  
> That is, would there be a situation where the driver knows that the 
> next callback won't sleep but in general the callback might sleep 
> sometimes?  It doesn't seem very likely.
>
> Kevin, do you have any feelings about this?

For flexibility, I like the RPM_FASTPATH approach.

At least in the drivers I'm currently tinkering with, most of the
callers can sleep.  It's only the occasional callback triggered from the
ISR that can't sleep.

During this "normal" (non-ISR) usage, there might very well be
concurrent or pending requests, some of which might have to sleep.  So,
I think it's best to have the fastpath settable on a per-caller basis.

Kevin

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Alan Stern]

On Fri, 24 Sep 2010, Kevin Hilman wrote:

> >> So, what is the "right" thing to do here?
> >
> > You should call pm_runtime_get(), turn off the interrupt source, and
> > return.  Then your resume routine should check for an outstanding
> > interrupt or wakeup request and handle it (the easiest way may be 
> > simply to call the ISR).
> 
> For a "normal" device driver, your solution makes complete sense.  The
> only catch is that it introduces potentically significant latency in the
> interrupt handling and it requires the interrupt source to be masked,
> potentially loosing other interrupts, while waiting for the runtime PM
> workqueue to schedule.  For chained handlers in particular, this means
> that *all* interrupts managed by the chained handler would be masked for
> this additional time.  Not good.

Masking an interrupt source doesn't cause any interrupts to be lost, if 
you mask it at a time when you couldn't handle the interrupts anyway.

> The problematic device for me as an on-chip GPIO controller, and the ISR
> in question is a chained handler (run with interrupts disabled) which
> does the GPIO demux and then dispatches to the actual ISR.  Following the
> above approach means that all GPIO interrupts (in that bank) would be
> masked until ->runtime_resume() is called.  For a GPIO bank with
> multiple edge-triggered IRQs, masked IRQs for that amount of time could
> mean several missed interrupts while waiting.

Wait a minute, you're confusing me.  I take it that the GPIO controller
is the device being runtime suspended, right?  And you said that while
it is suspended you can't access its registers.  So then how can you
mask it?  

Do you mean that you have to mask the entire IRQ line because there's
no way to turn off the interrupt-request source in the GPIO controller?  
That's different from what you wrote above.

>  Hoever, this isn't a
> major concern as we don't (currently) have IRQF_DISABLED handlers hooked
> up to GPIO IRQs (that I know of.)

Isn't IRQF_DISABLED on its way out, anyway?

> It may seem like I'm trying to fight the design, but I'm actually trying
> to find ways to use it.  I want to use the API (and we're using it
> successfully in most of our drivers now.)  The problem is only in a few
> of these corner cases where using it introduces significant changes from
> previous behavior like introducing long, unbounded windows for missed
> interrupts.

Assuming the problem of missed interrupts didn't exist, would you still 
be unhappy about the latency issue?

In the general case there's no way to avoid it.  Even though a device
like your GPIO controller may be able to return to full power very
quickly, the fact that it was suspended may have led the PM core to
suspend its parent as well.  And the parent may be slow to resume,
requiring a full process context.

It seems as though what you really need is a way to tell the PM core 
that your device can change its power state quickly with no need for a 
process context.  Given that, pm_runtime_get() could invoke your 
runtime_resume callback directly; you wouldn't have to wait for the 
workqueue.  (Unless your device had a suspended parent that _did_ need 
a long time to resume.)

Would that solve your problem?  It seems like a reasonable sort of 
feature to add.

Alan Stern

Re: [linux-pm] runtime_pm_get_sync() from ISR with IRQs disabled?

[Kevin Hilman]

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

> On Fri, 24 Sep 2010, Kevin Hilman wrote:
>
>> >> So, what is the "right" thing to do here?
>> >
>> > You should call pm_runtime_get(), turn off the interrupt source, and
>> > return.  Then your resume routine should check for an outstanding
>> > interrupt or wakeup request and handle it (the easiest way may be 
>> > simply to call the ISR).
>> 
>> For a "normal" device driver, your solution makes complete sense.  The
>> only catch is that it introduces potentically significant latency in the
>> interrupt handling and it requires the interrupt source to be masked,
>> potentially loosing other interrupts, while waiting for the runtime PM
>> workqueue to schedule.  For chained handlers in particular, this means
>> that *all* interrupts managed by the chained handler would be masked for
>> this additional time.  Not good.
>
> Masking an interrupt source doesn't cause any interrupts to be lost, if 
> you mask it at a time when you couldn't handle the interrupts anyway.

Sure, but the "mask in ISR, handle in ->runtime_resume()" proposal
would keeping them masked while we actually could be handling them.

Even in our current GPIO code, we have a tiny (but bounded) window where
we might miss an edge-triggered GPIO interrupt while we're still
detecting the previous one.  However, this is a very small window in an
interrupts-disabled chained handler.  

>> The problematic device for me as an on-chip GPIO controller, and the ISR
>> in question is a chained handler (run with interrupts disabled) which
>> does the GPIO demux and then dispatches to the actual ISR.  Following the
>> above approach means that all GPIO interrupts (in that bank) would be
>> masked until ->runtime_resume() is called.  For a GPIO bank with
>> multiple edge-triggered IRQs, masked IRQs for that amount of time could
>> mean several missed interrupts while waiting.
>
> Wait a minute, you're confusing me.  I take it that the GPIO controller
> is the device being runtime suspended, right?  

Right.

> And you said that while it is suspended you can't access its
> registers.  So then how can you mask it?

The interrupt to be masked would be the main GPIO bank IRQ (for the
chained handler) and it would be masked at the IRQ controller, which
is not (yet) suspended.

> Do you mean that you have to mask the entire IRQ line because there's
> no way to turn off the interrupt-request source in the GPIO controller?  

Right. Because in order to determine the exact interrupt source, you
have to access the GPIO controller registers and figure out which GPIO
in the GPIO bank was the source.  And then, the only way to turn off the
source is to know which device is connected to that IRQ, and disable it
via the driver for that device (e.g. network driver using a GPIO IRQ.)

IOW, When the bank IRQ fires, all we know is that (at least) one of the 32
GPIOs in the GPIO bank has an interrupt pending.  In order to determine
which one it was, we have to read GPIO controller registers, but we
can't do that until ->runtime_resume().  In order for the bank IRQ not
to continually re-fire, it would have to be kept masked until
the ISR is triggered from ->runtime_resume().

> That's different from what you wrote above.

Right, I should have said the GPIO bank IRQ needs to be masked instead
of all GPIOs in the bank need to be masked.

>>  Hoever, this isn't a
>> major concern as we don't (currently) have IRQF_DISABLED handlers hooked
>> up to GPIO IRQs (that I know of.)
>
> Isn't IRQF_DISABLED on its way out, anyway?

Yes, that's why it's not really a concern.

>> It may seem like I'm trying to fight the design, but I'm actually trying
>> to find ways to use it.  I want to use the API (and we're using it
>> successfully in most of our drivers now.)  The problem is only in a few
>> of these corner cases where using it introduces significant changes from
>> previous behavior like introducing long, unbounded windows for missed
>> interrupts.
>
> Assuming the problem of missed interrupts didn't exist, would you still 
> be unhappy about the latency issue?

Yes.

> In the general case there's no way to avoid it.  Even though a device
> like your GPIO controller may be able to return to full power very
> quickly, the fact that it was suspended may have led the PM core to
> suspend its parent as well.  And the parent may be slow to resume,
> requiring a full process context.

In the general case, I agree.

> It seems as though what you really need is a way to tell the PM core 
> that your device can change its power state quickly with no need for a 
> process context.  Given that, pm_runtime_get() could invoke your 
> runtime_resume callback directly; you wouldn't have to wait for the 
> workqueue.  (Unless your device had a suspended parent that _did_ need 
> a long time to resume.)
>
> Would that solve your problem?  It seems like a reasonable sort of 
> feature to add.

Yes, that would definitely solve the problem, and is basically the
hack/workaround I'm using locally:

  pm_runtime_get_noresume()
  dev->pm->runtime_resume()
  /* handle IRQ */
  pm_runtime_put();

because I know this on-chip device is 1) already suspended, 2) has no
runtime PM capable parents and 3) not runtime_resume'd elsewhere.

Kevin