Re: [linux-pm] [PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled
"Rafael J. Wysocki" writes: > On Thursday, August 19, 2010, Kevin Hilman wrote: >> "Rafael J. Wysocki" 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
Alan Stern 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
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 > > Signed-off-by: Kevin Hilman > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
On Thursday, August 19, 2010, Kevin Hilman wrote: > "Rafael J. Wysocki" 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
"Rafael J. Wysocki" 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
Alan Stern 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 >> Signed-off-by: Kevin Hilman >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
> -Original Message- > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- > ow...@vger.kernel.org] On Behalf Of Rafael J. Wysocki > Sent: Tuesday, August 17, 2010 2:48 AM > To: Kevin Hilman > Cc: linux...@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 majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@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
2010/8/11 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() 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 > Signed-off-by: Kevin Hilman > --- > 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 majord...@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
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 > Signed-off-by: Kevin Hilman > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] PM: runtime PM + idle: allow usage when interrupts are disabled
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
Signed-off-by: Kevin Hilman
---
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.run
