| Message ID | 201108202132.13304.rjw@sisk.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, 20 Aug 2011, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw@sisk.pl> > > Add a helper function allowing drivers and subsystems to clear > the power.irq_safe device flag. > --- linux.orig/drivers/base/power/runtime.c > +++ linux/drivers/base/power/runtime.c > @@ -1109,22 +1109,23 @@ void pm_runtime_no_callbacks(struct devi > EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks); > > /** > - * pm_runtime_irq_safe - Leave interrupts disabled during callbacks. > + * __pm_runtime_irq_safe - Manipulate a device's power.irq_safe flag. > * @dev: Device to handle > + * @irq_safe: Whether or not to leave interrupts disabled during callbacks. > * > - * Set the power.irq_safe flag, which tells the PM core that the > + * Set or unset the power.irq_safe flag, which tells the PM core that the > * ->runtime_suspend() and ->runtime_resume() callbacks for this device should > * always be invoked with the spinlock held and interrupts disabled. It also > * causes the parent's usage counter to be permanently incremented, preventing > * the parent from runtime suspending -- otherwise an irq-safe child might have > * to wait for a non-irq-safe parent. > */ > -void pm_runtime_irq_safe(struct device *dev) > +void __pm_runtime_irq_safe(struct device *dev, bool irq_safe) > { > if (dev->parent) > pm_runtime_get_sync(dev->parent); > spin_lock_irq(&dev->power.lock); > - dev->power.irq_safe = 1; > + dev->power.irq_safe = irq_safe; > spin_unlock_irq(&dev->power.lock); It's not quite this easy. There are two important aspects that must be considered. Firstly, I originally envisioned pm_runtime_irq_safe() being called just once, before the device is enabled for runtime PM. If you allow the flag to be turned on and off like this, you raise the possibility of races with runtime PM callbacks. (That is, if a callback occurs at about the same time as the irq_safe flag is changed, nobody can predict whether the callback will be invoked with interrupts enabled.) Maybe that's something the driver needs to take care of, but it should at least be mentioned in the documentation. Secondly, this doesn't manage the parent's usage counter correctly. Do the pm_runtime_get_sync(dev->parent) at the beginning only when the irq_safe flag was off and is being turned on. And at the end, if the irq_safe flag was on and is being turned off, do pm_runtime_put_sync(dev->parent). See pm_runtime_remove() for why this matters. (Also update the documentation; the change to the parent isn't necessarily permanent any more.) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday, August 21, 2011, Alan Stern wrote: > On Sat, 20 Aug 2011, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rjw@sisk.pl> > > > > Add a helper function allowing drivers and subsystems to clear > > the power.irq_safe device flag. > > > --- linux.orig/drivers/base/power/runtime.c > > +++ linux/drivers/base/power/runtime.c > > @@ -1109,22 +1109,23 @@ void pm_runtime_no_callbacks(struct devi > > EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks); > > > > /** > > - * pm_runtime_irq_safe - Leave interrupts disabled during callbacks. > > + * __pm_runtime_irq_safe - Manipulate a device's power.irq_safe flag. > > * @dev: Device to handle > > + * @irq_safe: Whether or not to leave interrupts disabled during callbacks. > > * > > - * Set the power.irq_safe flag, which tells the PM core that the > > + * Set or unset the power.irq_safe flag, which tells the PM core that the > > * ->runtime_suspend() and ->runtime_resume() callbacks for this device should > > * always be invoked with the spinlock held and interrupts disabled. It also > > * causes the parent's usage counter to be permanently incremented, preventing > > * the parent from runtime suspending -- otherwise an irq-safe child might have > > * to wait for a non-irq-safe parent. > > */ > > -void pm_runtime_irq_safe(struct device *dev) > > +void __pm_runtime_irq_safe(struct device *dev, bool irq_safe) > > { > > if (dev->parent) > > pm_runtime_get_sync(dev->parent); > > spin_lock_irq(&dev->power.lock); > > - dev->power.irq_safe = 1; > > + dev->power.irq_safe = irq_safe; > > spin_unlock_irq(&dev->power.lock); > > It's not quite this easy. There are two important aspects that must be > considered. > > Firstly, I originally envisioned pm_runtime_irq_safe() being called > just once, before the device is enabled for runtime PM. If you allow > the flag to be turned on and off like this, you raise the possibility > of races with runtime PM callbacks. (That is, if a callback occurs at > about the same time as the irq_safe flag is changed, nobody can predict > whether the callback will be invoked with interrupts enabled.) Maybe > that's something the driver needs to take care of, but it should at > least be mentioned in the documentation. Good point. Perhaps I should make it possible only if runtime PM is disabled. > Secondly, this doesn't manage the parent's usage counter correctly. Right, I forgot about that. > Do the pm_runtime_get_sync(dev->parent) at the beginning only when the > irq_safe flag was off and is being turned on. And at the end, if the > irq_safe flag was on and is being turned off, do > pm_runtime_put_sync(dev->parent). See pm_runtime_remove() for why this > matters. (Also update the documentation; the change to the parent > isn't necessarily permanent any more.) I'll try to figure out an alternative approach without the $subject change first. If I can't, I'll revisit this idea. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux/include/linux/pm_runtime.h =================================================================== --- linux.orig/include/linux/pm_runtime.h +++ linux/include/linux/pm_runtime.h @@ -40,7 +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_irq_safe(struct device *dev, bool irq_safe); 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); @@ -102,6 +102,16 @@ static inline void pm_runtime_mark_last_ ACCESS_ONCE(dev->power.last_busy) = jiffies; } +static inline void pm_runtime_irq_safe(struct device *dev) +{ + __pm_runtime_irq_safe(dev, true); +} + +static inline void pm_runtime_irq_unsafe(struct device *dev) +{ + __pm_runtime_irq_safe(dev, false); +} + #else /* !CONFIG_PM_RUNTIME */ static inline int __pm_runtime_idle(struct device *dev, int rpmflags) @@ -143,6 +153,7 @@ static inline int pm_generic_runtime_sus 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_irq_unsafe(struct device *dev) {} static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; } static inline void pm_runtime_mark_last_busy(struct device *dev) {} Index: linux/drivers/base/power/runtime.c =================================================================== --- linux.orig/drivers/base/power/runtime.c +++ linux/drivers/base/power/runtime.c @@ -1109,22 +1109,23 @@ void pm_runtime_no_callbacks(struct devi EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks); /** - * pm_runtime_irq_safe - Leave interrupts disabled during callbacks. + * __pm_runtime_irq_safe - Manipulate a device's power.irq_safe flag. * @dev: Device to handle + * @irq_safe: Whether or not to leave interrupts disabled during callbacks. * - * Set the power.irq_safe flag, which tells the PM core that the + * Set or unset the power.irq_safe flag, which tells the PM core that the * ->runtime_suspend() and ->runtime_resume() callbacks for this device should * always be invoked with the spinlock held and interrupts disabled. It also * causes the parent's usage counter to be permanently incremented, preventing * the parent from runtime suspending -- otherwise an irq-safe child might have * to wait for a non-irq-safe parent. */ -void pm_runtime_irq_safe(struct device *dev) +void __pm_runtime_irq_safe(struct device *dev, bool irq_safe) { if (dev->parent) pm_runtime_get_sync(dev->parent); spin_lock_irq(&dev->power.lock); - dev->power.irq_safe = 1; + dev->power.irq_safe = irq_safe; spin_unlock_irq(&dev->power.lock); } EXPORT_SYMBOL_GPL(pm_runtime_irq_safe); Index: linux/Documentation/power/runtime_pm.txt =================================================================== --- linux.orig/Documentation/power/runtime_pm.txt +++ linux/Documentation/power/runtime_pm.txt @@ -434,6 +434,10 @@ drivers/base/power/runtime.c and include suspend and resume callbacks (but not the idle callback) to be invoked with interrupts disabled + void pm_runtime_irq_unsafe(struct device *dev); + - clear the power.irq_safe flag for the device, causing the runtime-PM + callbacks to be invoked with interrupts enabled + void pm_runtime_mark_last_busy(struct device *dev); - set the power.last_busy field to the current time