Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
Alan Stern st...@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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
Rafael J. Wysocki r...@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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 st...@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
--
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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 -- 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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 st...@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 intrun_wake:1;
unsigned intruntime_auto:1;
unsigned intno_callbacks:1;
+ unsigned intirq_safe:1;
unsigned intuse_autosuspend:1;
unsigned inttimer_autosuspends:1;
enum rpm_requestrequest;
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
--
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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 st...@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
--
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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 st...@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
--
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 ver. 2] PM: add synchronous runtime interface for interrupt handlers
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 st...@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 intrun_wake:1;
unsigned intruntime_auto:1;
unsigned intno_callbacks:1;
+ unsigned intirq_safe:1;
unsigned intuse_autosuspend:1;
unsigned inttimer_autosuspends:1;
enum rpm_requestrequest;
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
