[PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Kevin Hilman]

When using runtime PM in combination with CPUidle, the runtime PM
transtions of some devices may be triggered during the idle path.
Late in the idle sequence, interrupts will likely be disabled when
runtime PM for these devices is initiated.

Currently, the runtime PM core assumes methods are called with
interrupts enabled.  However, if it is called with interrupts
disabled, the internal locking unconditionally enables interrupts, for
example:

pm_runtime_put_sync()
    __pm_runtime_put()
        pm_runtime_idle()
            spin_lock_irq()
            __pm_runtime_idle()
                spin_unlock_irq()   <--- interrupts unconditionally enabled
                dev->bus->pm->runtime_idle()
                spin_lock_irq()
             spin_unlock_irq()

Unconditionally enabling interrupts late in the idle sequence is not
desired behavior.  To fix, use the save/restore versions of the
spinlock API.

Reported-by: Partha Basak <p-basak2@ti.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
RFC: I'm not crazy about having the 'flags' in struct dev_pm_info, but
since the locks are taken and released in separate functions, this
seems better than changing the function APIs to pass around the flags.

 drivers/base/power/runtime.c |   68 ++++++++++++++++++++++-------------------
 include/linux/pm.h           |    1 +
 2 files changed, 37 insertions(+), 32 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b78c401..0caaef1 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -80,24 +80,24 @@ static int __pm_runtime_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);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		dev->bus->pm->runtime_idle(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	} else if (dev->type && dev->type->pm && dev->type->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		dev->type->pm->runtime_idle(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	} else if (dev->class && dev->class->pm
 	    && dev->class->pm->runtime_idle) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		dev->class->pm->runtime_idle(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	}
 
 	dev->power.idle_notification = false;
@@ -115,9 +115,9 @@ int pm_runtime_idle(struct device *dev)
 {
 	int retval;
 
-	spin_lock_irq(&dev->power.lock);
+	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	retval = __pm_runtime_idle(dev);
-	spin_unlock_irq(&dev->power.lock);
+	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 	return retval;
 }
@@ -226,11 +226,13 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq)
 			if (dev->power.runtime_status != RPM_SUSPENDING)
 				break;
 
-			spin_unlock_irq(&dev->power.lock);
+			spin_unlock_irqrestore(&dev->power.lock,
+					       dev->power.lock_flags);
 
 			schedule();
 
-			spin_lock_irq(&dev->power.lock);
+			spin_lock_irqsave(&dev->power.lock,
+					  dev->power.lock_flags);
 		}
 		finish_wait(&dev->power.wait_queue, &wait);
 		goto repeat;
@@ -240,27 +242,27 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq)
 	dev->power.deferred_resume = false;
 
 	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->bus->pm->runtime_suspend(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else if (dev->type && dev->type->pm
 	    && dev->type->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->type->pm->runtime_suspend(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else if (dev->class && dev->class->pm
 	    && dev->class->pm->runtime_suspend) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->class->pm->runtime_suspend(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else {
 		retval = -ENOSYS;
@@ -296,11 +298,11 @@ int __pm_runtime_suspend(struct device *dev, bool from_wq)
 		__pm_runtime_idle(dev);
 
 	if (parent && !parent->power.ignore_children) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		pm_request_idle(parent);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	}
 
  out:
@@ -317,9 +319,9 @@ int pm_runtime_suspend(struct device *dev)
 {
 	int retval;
 
-	spin_lock_irq(&dev->power.lock);
+	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	retval = __pm_runtime_suspend(dev, false);
-	spin_unlock_irq(&dev->power.lock);
+	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 	return retval;
 }
@@ -381,11 +383,13 @@ int __pm_runtime_resume(struct device *dev, bool from_wq)
 			    && dev->power.runtime_status != RPM_SUSPENDING)
 				break;
 
-			spin_unlock_irq(&dev->power.lock);
+			spin_unlock_irqrestore(&dev->power.lock,
+					       dev->power.lock_flags);
 
 			schedule();
 
-			spin_lock_irq(&dev->power.lock);
+			spin_lock_irqsave(&dev->power.lock,
+					  dev->power.lock_flags);
 		}
 		finish_wait(&dev->power.wait_queue, &wait);
 		goto repeat;
@@ -423,27 +427,27 @@ int __pm_runtime_resume(struct device *dev, bool from_wq)
 	__update_runtime_status(dev, RPM_RESUMING);
 
 	if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->bus->pm->runtime_resume(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else if (dev->type && dev->type->pm
 	    && dev->type->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->type->pm->runtime_resume(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else if (dev->class && dev->class->pm
 	    && dev->class->pm->runtime_resume) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		retval = dev->class->pm->runtime_resume(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 		dev->power.runtime_error = retval;
 	} else {
 		retval = -ENOSYS;
@@ -464,11 +468,11 @@ int __pm_runtime_resume(struct device *dev, bool from_wq)
 
  out:
 	if (parent) {
-		spin_unlock_irq(&dev->power.lock);
+		spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 		pm_runtime_put(parent);
 
-		spin_lock_irq(&dev->power.lock);
+		spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	}
 
 	dev_dbg(dev, "__pm_runtime_resume() returns %d!\n", retval);
@@ -484,9 +488,9 @@ int pm_runtime_resume(struct device *dev)
 {
 	int retval;
 
-	spin_lock_irq(&dev->power.lock);
+	spin_lock_irqsave(&dev->power.lock, dev->power.lock_flags);
 	retval = __pm_runtime_resume(dev, false);
-	spin_unlock_irq(&dev->power.lock);
+	spin_unlock_irqrestore(&dev->power.lock, dev->power.lock_flags);
 
 	return retval;
 }
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 52e8c55..8515153 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -465,6 +465,7 @@ struct dev_pm_info {
 	struct work_struct	work;
 	wait_queue_head_t	wait_queue;
 	spinlock_t		lock;
+	unsigned long           lock_flags;
 	atomic_t		usage_count;
 	atomic_t		child_count;
 	unsigned int		disable_depth:3;
-- 
1.7.2.1

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Alan Stern]

On Tue, 10 Aug 2010, Kevin Hilman wrote:

> When using runtime PM in combination with CPUidle, the runtime PM
> transtions of some devices may be triggered during the idle path.
> Late in the idle sequence, interrupts will likely be disabled when
> runtime PM for these devices is initiated.
> 
> Currently, the runtime PM core assumes methods are called with
> interrupts enabled.  However, if it is called with interrupts
> disabled, the internal locking unconditionally enables interrupts, for
> example:

...

> Unconditionally enabling interrupts late in the idle sequence is not
> desired behavior.  To fix, use the save/restore versions of the
> spinlock API.
> 
> Reported-by: Partha Basak <p-basak2@ti.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
> RFC: I'm not crazy about having the 'flags' in struct dev_pm_info, but
> since the locks are taken and released in separate functions, this
> seems better than changing the function APIs to pass around the flags.

There are restrictions on what you're allowed to do with the flags, but 
I don't remember exactly what they are.

In any case, I don't really like this change.  It seems that we would 
be better off preventing the runtime PM calls from occurring in the 
first place while interrupts are disabled.  In fact, it's hard to see 
what could cause this to happen at all.

Alan Stern

Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Alan Stern]

On Tue, 10 Aug 2010, Kevin Hilman wrote:

> When using runtime PM in combination with CPUidle, the runtime PM
> transtions of some devices may be triggered during the idle path.
> Late in the idle sequence, interrupts will likely be disabled when
> runtime PM for these devices is initiated.
> 
> Currently, the runtime PM core assumes methods are called with
> interrupts enabled.  However, if it is called with interrupts
> disabled, the internal locking unconditionally enables interrupts, for
> example:

...

> Unconditionally enabling interrupts late in the idle sequence is not
> desired behavior.  To fix, use the save/restore versions of the
> spinlock API.
> 
> Reported-by: Partha Basak <p-basak2@ti.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
> RFC: I'm not crazy about having the 'flags' in struct dev_pm_info, but
> since the locks are taken and released in separate functions, this
> seems better than changing the function APIs to pass around the flags.

There are restrictions on what you're allowed to do with the flags, but 
I don't remember exactly what they are.

In any case, I don't really like this change.  It seems that we would 
be better off preventing the runtime PM calls from occurring in the 
first place while interrupts are disabled.  In fact, it's hard to see 
what could cause this to happen at all.

Alan Stern

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Ming Lei]

2010/8/11 Kevin Hilman <khilman@deeprootsystems.com>:
> When using runtime PM in combination with CPUidle, the runtime PM
> transtions of some devices may be triggered during the idle path.
> Late in the idle sequence, interrupts will likely be disabled when
> runtime PM for these devices is initiated.
>
> Currently, the runtime PM core assumes methods are called with
> interrupts enabled.  However, if it is called with interrupts
> disabled, the internal locking unconditionally enables interrupts, for
> example:
>
> pm_runtime_put_sync()

It is always called from process context,  why do you have to disable irq
before calling pm_runtime_put_sync?

>    __pm_runtime_put()
>        pm_runtime_idle()
>            spin_lock_irq()
>            __pm_runtime_idle()
>                spin_unlock_irq()   <--- interrupts unconditionally enabled
>                dev->bus->pm->runtime_idle()
>                spin_lock_irq()
>             spin_unlock_irq()
>
> Unconditionally enabling interrupts late in the idle sequence is not
> desired behavior.  To fix, use the save/restore versions of the
> spinlock API.
>
> Reported-by: Partha Basak <p-basak2@ti.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
> RFC: I'm not crazy about having the 'flags' in struct dev_pm_info, but
> since the locks are taken and released in separate functions, this
> seems better than changing the function APIs to pass around the flags.
>

thanks,

-- 
Lei Ming

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Ming Lei]

2010/8/11 Kevin Hilman <khilman@deeprootsystems.com>:
> When using runtime PM in combination with CPUidle, the runtime PM
> transtions of some devices may be triggered during the idle path.
> Late in the idle sequence, interrupts will likely be disabled when
> runtime PM for these devices is initiated.
>
> Currently, the runtime PM core assumes methods are called with
> interrupts enabled.  However, if it is called with interrupts
> disabled, the internal locking unconditionally enables interrupts, for
> example:
>
> pm_runtime_put_sync()

It is always called from process context,  why do you have to disable irq
before calling pm_runtime_put_sync?

>    __pm_runtime_put()
>        pm_runtime_idle()
>            spin_lock_irq()
>            __pm_runtime_idle()
>                spin_unlock_irq()   <--- interrupts unconditionally enabled
>                dev->bus->pm->runtime_idle()
>                spin_lock_irq()
>             spin_unlock_irq()
>
> Unconditionally enabling interrupts late in the idle sequence is not
> desired behavior.  To fix, use the save/restore versions of the
> spinlock API.
>
> Reported-by: Partha Basak <p-basak2@ti.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
> RFC: I'm not crazy about having the 'flags' in struct dev_pm_info, but
> since the locks are taken and released in separate functions, this
> seems better than changing the function APIs to pass around the flags.
>

thanks,

-- 
Lei Ming
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Rafael J. Wysocki]

On Tuesday, August 10, 2010, Kevin Hilman wrote:
> When using runtime PM in combination with CPUidle, the runtime PM
> transtions of some devices may be triggered during the idle path.
> Late in the idle sequence, interrupts will likely be disabled when
> runtime PM for these devices is initiated.
> 
> Currently, the runtime PM core assumes methods are called with
> interrupts enabled.  However, if it is called with interrupts
> disabled, the internal locking unconditionally enables interrupts, for
> example:
> 
> pm_runtime_put_sync()

Please don't use that from interrupt context.  There's pm_runtime_put()
exactly for this purpose that puts the _idle() call into a workqueue.

>     __pm_runtime_put()
>         pm_runtime_idle()
>             spin_lock_irq()
>             __pm_runtime_idle()
>                 spin_unlock_irq()   <--- interrupts unconditionally enabled

That's because __pm_runtime_idle() has to be called from process context.

>                 dev->bus->pm->runtime_idle()
>                 spin_lock_irq()
>              spin_unlock_irq()

Thanks,
Rafael

Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Rafael J. Wysocki]

On Tuesday, August 10, 2010, Kevin Hilman wrote:
> When using runtime PM in combination with CPUidle, the runtime PM
> transtions of some devices may be triggered during the idle path.
> Late in the idle sequence, interrupts will likely be disabled when
> runtime PM for these devices is initiated.
> 
> Currently, the runtime PM core assumes methods are called with
> interrupts enabled.  However, if it is called with interrupts
> disabled, the internal locking unconditionally enables interrupts, for
> example:
> 
> pm_runtime_put_sync()

Please don't use that from interrupt context.  There's pm_runtime_put()
exactly for this purpose that puts the _idle() call into a workqueue.

>     __pm_runtime_put()
>         pm_runtime_idle()
>             spin_lock_irq()
>             __pm_runtime_idle()
>                 spin_unlock_irq()   <--- interrupts unconditionally enabled

That's because __pm_runtime_idle() has to be called from process context.

>                 dev->bus->pm->runtime_idle()
>                 spin_lock_irq()
>              spin_unlock_irq()

Thanks,
Rafael

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Basak, Partha]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: Tuesday, August 17, 2010 2:48 AM
> To: Kevin Hilman
> Cc: linux-pm@lists.linux-foundation.org; linux-omap@vger.kernel.org; Alan
> Stern; Ming Lei
> Subject: Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when
> interrupts are disabled
> 
> On Tuesday, August 10, 2010, Kevin Hilman wrote:
> > When using runtime PM in combination with CPUidle, the runtime PM
> > transtions of some devices may be triggered during the idle path.
> > Late in the idle sequence, interrupts will likely be disabled when
> > runtime PM for these devices is initiated.
> >
> > Currently, the runtime PM core assumes methods are called with
> > interrupts enabled.  However, if it is called with interrupts
> > disabled, the internal locking unconditionally enables interrupts, for
> > example:
> >
> > pm_runtime_put_sync()
> 
> Please don't use that from interrupt context.  There's pm_runtime_put()
> exactly for this purpose that puts the _idle() call into a workqueue.

Rafael, we execute this little before executing idle instruction with interrupts locked. So, we cannot call pm_runtime_put() as we want it to take effect immediately.
Kevin??

> 
> >     __pm_runtime_put()
> >         pm_runtime_idle()
> >             spin_lock_irq()
> >             __pm_runtime_idle()
> >                 spin_unlock_irq()   <--- interrupts unconditionally
> enabled
> 
> That's because __pm_runtime_idle() has to be called from process context.
> 
> >                 dev->bus->pm->runtime_idle()
> >                 spin_lock_irq()
> >              spin_unlock_irq()
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Basak, Partha]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: Tuesday, August 17, 2010 2:48 AM
> To: Kevin Hilman
> Cc: linux-pm@lists.linux-foundation.org; linux-omap@vger.kernel.org; Alan
> Stern; Ming Lei
> Subject: Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when
> interrupts are disabled
> 
> On Tuesday, August 10, 2010, Kevin Hilman wrote:
> > When using runtime PM in combination with CPUidle, the runtime PM
> > transtions of some devices may be triggered during the idle path.
> > Late in the idle sequence, interrupts will likely be disabled when
> > runtime PM for these devices is initiated.
> >
> > Currently, the runtime PM core assumes methods are called with
> > interrupts enabled.  However, if it is called with interrupts
> > disabled, the internal locking unconditionally enables interrupts, for
> > example:
> >
> > pm_runtime_put_sync()
> 
> Please don't use that from interrupt context.  There's pm_runtime_put()
> exactly for this purpose that puts the _idle() call into a workqueue.

Rafael, we execute this little before executing idle instruction with interrupts locked. So, we cannot call pm_runtime_put() as we want it to take effect immediately.
Kevin??

> 
> >     __pm_runtime_put()
> >         pm_runtime_idle()
> >             spin_lock_irq()
> >             __pm_runtime_idle()
> >                 spin_unlock_irq()   <--- interrupts unconditionally
> enabled
> 
> That's because __pm_runtime_idle() has to be called from process context.
> 
> >                 dev->bus->pm->runtime_idle()
> >                 spin_lock_irq()
> >              spin_unlock_irq()
> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Kevin Hilman]

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

> On Tue, 10 Aug 2010, Kevin Hilman wrote:
>
>> When using runtime PM in combination with CPUidle, the runtime PM
>> transtions of some devices may be triggered during the idle path.
>> Late in the idle sequence, interrupts will likely be disabled when
>> runtime PM for these devices is initiated.
>> 
>> Currently, the runtime PM core assumes methods are called with
>> interrupts enabled.  However, if it is called with interrupts
>> disabled, the internal locking unconditionally enables interrupts, for
>> example:
>
> ...
>
>> Unconditionally enabling interrupts late in the idle sequence is not
>> desired behavior.  To fix, use the save/restore versions of the
>> spinlock API.
>> 
>> Reported-by: Partha Basak <p-basak2@ti.com>
>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> ---
>> RFC: I'm not crazy about having the 'flags' in struct dev_pm_info, but
>> since the locks are taken and released in separate functions, this
>> seems better than changing the function APIs to pass around the flags.
>
> There are restrictions on what you're allowed to do with the flags, but 
> I don't remember exactly what they are.
>
> In any case, I don't really like this change.  It seems that we would
> be better off preventing the runtime PM calls from occurring in the
> first place while interrupts are disabled.  

Why? 

> In fact, it's hard to see what could cause this to happen at all.

As I mentioned in the changelog, this happens when trying to use runtime
PM in combination with CPUidle.  As has been suggested elsewhere[1],
there is a need to do runtime PM on some devices in combination with CPU
idle transitions managed by CPUidle.  However, late in the idle path,
at the time we want to manage these IO devices, interrupts are disabled.

Currently, on OMAP, we are already managing the power state of certain
IO devices along with CPUidle transitions using more brute force
methods.  IMO, using runtime PM for this would be a much cleaner
approach.  The only obstacle is the assumption that the API must be
called with interrupts enabled.

Kevin

[1] http://www.linuxplumbersconf.org/2010/ocw/proposals/717

Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Kevin Hilman]

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

> On Tue, 10 Aug 2010, Kevin Hilman wrote:
>
>> When using runtime PM in combination with CPUidle, the runtime PM
>> transtions of some devices may be triggered during the idle path.
>> Late in the idle sequence, interrupts will likely be disabled when
>> runtime PM for these devices is initiated.
>> 
>> Currently, the runtime PM core assumes methods are called with
>> interrupts enabled.  However, if it is called with interrupts
>> disabled, the internal locking unconditionally enables interrupts, for
>> example:
>
> ...
>
>> Unconditionally enabling interrupts late in the idle sequence is not
>> desired behavior.  To fix, use the save/restore versions of the
>> spinlock API.
>> 
>> Reported-by: Partha Basak <p-basak2@ti.com>
>> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
>> ---
>> RFC: I'm not crazy about having the 'flags' in struct dev_pm_info, but
>> since the locks are taken and released in separate functions, this
>> seems better than changing the function APIs to pass around the flags.
>
> There are restrictions on what you're allowed to do with the flags, but 
> I don't remember exactly what they are.
>
> In any case, I don't really like this change.  It seems that we would
> be better off preventing the runtime PM calls from occurring in the
> first place while interrupts are disabled.  

Why? 

> In fact, it's hard to see what could cause this to happen at all.

As I mentioned in the changelog, this happens when trying to use runtime
PM in combination with CPUidle.  As has been suggested elsewhere[1],
there is a need to do runtime PM on some devices in combination with CPU
idle transitions managed by CPUidle.  However, late in the idle path,
at the time we want to manage these IO devices, interrupts are disabled.

Currently, on OMAP, we are already managing the power state of certain
IO devices along with CPUidle transitions using more brute force
methods.  IMO, using runtime PM for this would be a much cleaner
approach.  The only obstacle is the assumption that the API must be
called with interrupts enabled.

Kevin

[1] http://www.linuxplumbersconf.org/2010/ocw/proposals/717

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Kevin Hilman]

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

> On Tuesday, August 10, 2010, Kevin Hilman wrote:
>> When using runtime PM in combination with CPUidle, the runtime PM
>> transtions of some devices may be triggered during the idle path.
>> Late in the idle sequence, interrupts will likely be disabled when
>> runtime PM for these devices is initiated.
>> 
>> Currently, the runtime PM core assumes methods are called with
>> interrupts enabled.  However, if it is called with interrupts
>> disabled, the internal locking unconditionally enables interrupts, for
>> example:
>> 
>> pm_runtime_put_sync()
>
> Please don't use that from interrupt context.  

I'm not using this in interrupt context.  I'm using it in process
context where interrupts are disabled, specifically, the idle thread.

> There's pm_runtime_put() exactly for this purpose that puts the
> _idle() call into a workqueue.

If I'm in my CPU's idle path, I don't want to activate a workqueue
because then I'll no longer be idle.

Kevin

Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Kevin Hilman]

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

> On Tuesday, August 10, 2010, Kevin Hilman wrote:
>> When using runtime PM in combination with CPUidle, the runtime PM
>> transtions of some devices may be triggered during the idle path.
>> Late in the idle sequence, interrupts will likely be disabled when
>> runtime PM for these devices is initiated.
>> 
>> Currently, the runtime PM core assumes methods are called with
>> interrupts enabled.  However, if it is called with interrupts
>> disabled, the internal locking unconditionally enables interrupts, for
>> example:
>> 
>> pm_runtime_put_sync()
>
> Please don't use that from interrupt context.  

I'm not using this in interrupt context.  I'm using it in process
context where interrupts are disabled, specifically, the idle thread.

> There's pm_runtime_put() exactly for this purpose that puts the
> _idle() call into a workqueue.

If I'm in my CPU's idle path, I don't want to activate a workqueue
because then I'll no longer be idle.

Kevin

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Alan Stern]

On Thu, 19 Aug 2010, Kevin Hilman wrote:

> > In any case, I don't really like this change.  It seems that we would
> > be better off preventing the runtime PM calls from occurring in the
> > first place while interrupts are disabled.  
> 
> Why? 

Because that's how the Runtime PM framework was designed.  Drivers 
expect interrupts to be enabled when their runtime PM callbacks are 
invoked.  And the framework internally depends on it as well.

> > In fact, it's hard to see what could cause this to happen at all.
> 
> As I mentioned in the changelog, this happens when trying to use runtime
> PM in combination with CPUidle.  As has been suggested elsewhere[1],
> there is a need to do runtime PM on some devices in combination with CPU
> idle transitions managed by CPUidle.  However, late in the idle path,
> at the time we want to manage these IO devices, interrupts are disabled.

Then it isn't really feasible to use the runtime PM framework for those 
devices.  Not unless the framework is extended with new functions meant 
to be used without interrupts enabled (in which case it doesn't seem to 
matter much whether you are in process context or not).

> Currently, on OMAP, we are already managing the power state of certain
> IO devices along with CPUidle transitions using more brute force
> methods.  IMO, using runtime PM for this would be a much cleaner
> approach.  The only obstacle is the assumption that the API must be
> called with interrupts enabled.

That's a big obstacle.  Why can't you manage these devices earlier, 
while interrupts are still enabled?

Alan Stern

Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Alan Stern]

On Thu, 19 Aug 2010, Kevin Hilman wrote:

> > In any case, I don't really like this change.  It seems that we would
> > be better off preventing the runtime PM calls from occurring in the
> > first place while interrupts are disabled.  
> 
> Why? 

Because that's how the Runtime PM framework was designed.  Drivers 
expect interrupts to be enabled when their runtime PM callbacks are 
invoked.  And the framework internally depends on it as well.

> > In fact, it's hard to see what could cause this to happen at all.
> 
> As I mentioned in the changelog, this happens when trying to use runtime
> PM in combination with CPUidle.  As has been suggested elsewhere[1],
> there is a need to do runtime PM on some devices in combination with CPU
> idle transitions managed by CPUidle.  However, late in the idle path,
> at the time we want to manage these IO devices, interrupts are disabled.

Then it isn't really feasible to use the runtime PM framework for those 
devices.  Not unless the framework is extended with new functions meant 
to be used without interrupts enabled (in which case it doesn't seem to 
matter much whether you are in process context or not).

> Currently, on OMAP, we are already managing the power state of certain
> IO devices along with CPUidle transitions using more brute force
> methods.  IMO, using runtime PM for this would be a much cleaner
> approach.  The only obstacle is the assumption that the API must be
> called with interrupts enabled.

That's a big obstacle.  Why can't you manage these devices earlier, 
while interrupts are still enabled?

Alan Stern

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Rafael J. Wysocki]

On Thursday, August 19, 2010, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Tuesday, August 10, 2010, Kevin Hilman wrote:
> >> When using runtime PM in combination with CPUidle, the runtime PM
> >> transtions of some devices may be triggered during the idle path.
> >> Late in the idle sequence, interrupts will likely be disabled when
> >> runtime PM for these devices is initiated.
> >> 
> >> Currently, the runtime PM core assumes methods are called with
> >> interrupts enabled.  However, if it is called with interrupts
> >> disabled, the internal locking unconditionally enables interrupts, for
> >> example:
> >> 
> >> pm_runtime_put_sync()
> >
> > Please don't use that from interrupt context.  
> 
> I'm not using this in interrupt context.  I'm using it in process
> context where interrupts are disabled, specifically, the idle thread.
> 
> > There's pm_runtime_put() exactly for this purpose that puts the
> > _idle() call into a workqueue.
> 
> If I'm in my CPU's idle path, I don't want to activate a workqueue
> because then I'll no longer be idle.

Well, what about:

-> idle
   -> check if devices have been suspended
       - enter idle if so
       - call pm_request_idle() for devices

The workqueue will activate and put the devices into low-power states and
then your idle callback will be called again, with the devices suspended.

Thanks,
Rafael

Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Rafael J. Wysocki]

On Thursday, August 19, 2010, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Tuesday, August 10, 2010, Kevin Hilman wrote:
> >> When using runtime PM in combination with CPUidle, the runtime PM
> >> transtions of some devices may be triggered during the idle path.
> >> Late in the idle sequence, interrupts will likely be disabled when
> >> runtime PM for these devices is initiated.
> >> 
> >> Currently, the runtime PM core assumes methods are called with
> >> interrupts enabled.  However, if it is called with interrupts
> >> disabled, the internal locking unconditionally enables interrupts, for
> >> example:
> >> 
> >> pm_runtime_put_sync()
> >
> > Please don't use that from interrupt context.  
> 
> I'm not using this in interrupt context.  I'm using it in process
> context where interrupts are disabled, specifically, the idle thread.
> 
> > There's pm_runtime_put() exactly for this purpose that puts the
> > _idle() call into a workqueue.
> 
> If I'm in my CPU's idle path, I don't want to activate a workqueue
> because then I'll no longer be idle.

Well, what about:

-> idle
   -> check if devices have been suspended
       - enter idle if so
       - call pm_request_idle() for devices

The workqueue will activate and put the devices into low-power states and
then your idle callback will be called again, with the devices suspended.

Thanks,
Rafael

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Pavel Machek]

Hi!

> > When using runtime PM in combination with CPUidle, the runtime PM
> > transtions of some devices may be triggered during the idle path.
> > Late in the idle sequence, interrupts will likely be disabled when
> > runtime PM for these devices is initiated.
> > 
> > Currently, the runtime PM core assumes methods are called with
> > interrupts enabled.  However, if it is called with interrupts
> > disabled, the internal locking unconditionally enables interrupts, for
> > example:
> 
> ...
> 
> > Unconditionally enabling interrupts late in the idle sequence is not
> > desired behavior.  To fix, use the save/restore versions of the
> > spinlock API.
> > 
> > Reported-by: Partha Basak <p-basak2@ti.com>
> > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> > ---
> > RFC: I'm not crazy about having the 'flags' in struct dev_pm_info, but
> > since the locks are taken and released in separate functions, this
> > seems better than changing the function APIs to pass around the flags.
> 
> There are restrictions on what you're allowed to do with the flags, but 
> I don't remember exactly what they are.

There used to be 'flags must local variable, and enable/disable must
happen in same function' restriction on sparc. Not sure if it is still
present. Ask davem?
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Pavel Machek]

Hi!

> > When using runtime PM in combination with CPUidle, the runtime PM
> > transtions of some devices may be triggered during the idle path.
> > Late in the idle sequence, interrupts will likely be disabled when
> > runtime PM for these devices is initiated.
> > 
> > Currently, the runtime PM core assumes methods are called with
> > interrupts enabled.  However, if it is called with interrupts
> > disabled, the internal locking unconditionally enables interrupts, for
> > example:
> 
> ...
> 
> > Unconditionally enabling interrupts late in the idle sequence is not
> > desired behavior.  To fix, use the save/restore versions of the
> > spinlock API.
> > 
> > Reported-by: Partha Basak <p-basak2@ti.com>
> > Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> > ---
> > RFC: I'm not crazy about having the 'flags' in struct dev_pm_info, but
> > since the locks are taken and released in separate functions, this
> > seems better than changing the function APIs to pass around the flags.
> 
> There are restrictions on what you're allowed to do with the flags, but 
> I don't remember exactly what they are.

There used to be 'flags must local variable, and enable/disable must
happen in same function' restriction on sparc. Not sure if it is still
present. Ask davem?
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Kevin Hilman]

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

> On Thu, 19 Aug 2010, Kevin Hilman wrote:
>
>> > In any case, I don't really like this change.  It seems that we would
>> > be better off preventing the runtime PM calls from occurring in the
>> > first place while interrupts are disabled.  
>> 
>> Why? 
>
> Because that's how the Runtime PM framework was designed.  Drivers 
> expect interrupts to be enabled when their runtime PM callbacks are 
> invoked.  And the framework internally depends on it as well.
>
>> > In fact, it's hard to see what could cause this to happen at all.
>> 
>> As I mentioned in the changelog, this happens when trying to use runtime
>> PM in combination with CPUidle.  As has been suggested elsewhere[1],
>> there is a need to do runtime PM on some devices in combination with CPU
>> idle transitions managed by CPUidle.  However, late in the idle path,
>> at the time we want to manage these IO devices, interrupts are disabled.
>
> Then it isn't really feasible to use the runtime PM framework for those 
> devices.  Not unless the framework is extended with new functions meant 
> to be used without interrupts enabled (in which case it doesn't seem to 
> matter much whether you are in process context or not).
>
>> Currently, on OMAP, we are already managing the power state of certain
>> IO devices along with CPUidle transitions using more brute force
>> methods.  IMO, using runtime PM for this would be a much cleaner
>> approach.  The only obstacle is the assumption that the API must be
>> called with interrupts enabled.
>
> That's a big obstacle.  Why can't you manage these devices earlier, 
> while interrupts are still enabled?

Indeed, that's a good question.  I'm exploring this more thoroughly now.

Kevin

Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Kevin Hilman]

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

> On Thu, 19 Aug 2010, Kevin Hilman wrote:
>
>> > In any case, I don't really like this change.  It seems that we would
>> > be better off preventing the runtime PM calls from occurring in the
>> > first place while interrupts are disabled.  
>> 
>> Why? 
>
> Because that's how the Runtime PM framework was designed.  Drivers 
> expect interrupts to be enabled when their runtime PM callbacks are 
> invoked.  And the framework internally depends on it as well.
>
>> > In fact, it's hard to see what could cause this to happen at all.
>> 
>> As I mentioned in the changelog, this happens when trying to use runtime
>> PM in combination with CPUidle.  As has been suggested elsewhere[1],
>> there is a need to do runtime PM on some devices in combination with CPU
>> idle transitions managed by CPUidle.  However, late in the idle path,
>> at the time we want to manage these IO devices, interrupts are disabled.
>
> Then it isn't really feasible to use the runtime PM framework for those 
> devices.  Not unless the framework is extended with new functions meant 
> to be used without interrupts enabled (in which case it doesn't seem to 
> matter much whether you are in process context or not).
>
>> Currently, on OMAP, we are already managing the power state of certain
>> IO devices along with CPUidle transitions using more brute force
>> methods.  IMO, using runtime PM for this would be a much cleaner
>> approach.  The only obstacle is the assumption that the API must be
>> called with interrupts enabled.
>
> That's a big obstacle.  Why can't you manage these devices earlier, 
> while interrupts are still enabled?

Indeed, that's a good question.  I'm exploring this more thoroughly now.

Kevin

Re: [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Kevin Hilman]

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

> On Thursday, August 19, 2010, Kevin Hilman wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> 
>> > On Tuesday, August 10, 2010, Kevin Hilman wrote:
>> >> When using runtime PM in combination with CPUidle, the runtime PM
>> >> transtions of some devices may be triggered during the idle path.
>> >> Late in the idle sequence, interrupts will likely be disabled when
>> >> runtime PM for these devices is initiated.
>> >> 
>> >> Currently, the runtime PM core assumes methods are called with
>> >> interrupts enabled.  However, if it is called with interrupts
>> >> disabled, the internal locking unconditionally enables interrupts, for
>> >> example:
>> >> 
>> >> pm_runtime_put_sync()
>> >
>> > Please don't use that from interrupt context.  
>> 
>> I'm not using this in interrupt context.  I'm using it in process
>> context where interrupts are disabled, specifically, the idle thread.
>> 
>> > There's pm_runtime_put() exactly for this purpose that puts the
>> > _idle() call into a workqueue.
>> *
>> If I'm in my CPU's idle path, I don't want to activate a workqueue
>> because then I'll no longer be idle.
>
> Well, what about:
>
> -> idle
>    -> check if devices have been suspended
>        - enter idle if so
>        - call pm_request_idle() for devices
>
> The workqueue will activate and put the devices into low-power states and
> then your idle callback will be called again, with the devices suspended.

The problem with this is that after we leave idle, the decision about
which C-state to enter will likely change, which will affect the set of
devices that need to be suspended.   I'm not sure this will be an
entirely predictable process with current CPUidle governors, but it
certainly merits some more experiments.

Kevin

Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled

[Kevin Hilman]

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

> On Thursday, August 19, 2010, Kevin Hilman wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> 
>> > On Tuesday, August 10, 2010, Kevin Hilman wrote:
>> >> When using runtime PM in combination with CPUidle, the runtime PM
>> >> transtions of some devices may be triggered during the idle path.
>> >> Late in the idle sequence, interrupts will likely be disabled when
>> >> runtime PM for these devices is initiated.
>> >> 
>> >> Currently, the runtime PM core assumes methods are called with
>> >> interrupts enabled.  However, if it is called with interrupts
>> >> disabled, the internal locking unconditionally enables interrupts, for
>> >> example:
>> >> 
>> >> pm_runtime_put_sync()
>> >
>> > Please don't use that from interrupt context.  
>> 
>> I'm not using this in interrupt context.  I'm using it in process
>> context where interrupts are disabled, specifically, the idle thread.
>> 
>> > There's pm_runtime_put() exactly for this purpose that puts the
>> > _idle() call into a workqueue.
>> *
>> If I'm in my CPU's idle path, I don't want to activate a workqueue
>> because then I'll no longer be idle.
>
> Well, what about:
>
> -> idle
>    -> check if devices have been suspended
>        - enter idle if so
>        - call pm_request_idle() for devices
>
> The workqueue will activate and put the devices into low-power states and
> then your idle callback will be called again, with the devices suspended.

The problem with this is that after we leave idle, the decision about
which C-state to enter will likely change, which will affect the set of
devices that need to be suspended.   I'm not sure this will be an
entirely predictable process with current CPUidle governors, but it
certainly merits some more experiments.

Kevin