| Message ID | 1411566612-32277-2-git-send-email-k.kozlowski@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote: > Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime > PM IRQ safe was set or not. > > Various bus drivers implementing runtime PM may use choose to suspend > differently based on IRQ safeness status of child driver (e.g. do not > unprepare the clock if IRQ safe is not set). > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Are you sure this is good interface? "Tell me if another function works this or that way". That's certainly not traditional interface, and it seems dangerous to me. Callbacks now have different semantic requirements based on value of some flag... Would it be possible to have two sets of callbacks, one irq safe and one not? Pavel
On Wed, 24 Sep 2014, Pavel Machek wrote: > On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote: > > Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime > > PM IRQ safe was set or not. > > > > Various bus drivers implementing runtime PM may use choose to suspend > > differently based on IRQ safeness status of child driver (e.g. do not > > unprepare the clock if IRQ safe is not set). > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > Are you sure this is good interface? > > "Tell me if another function works this or that way". > > That's certainly not traditional interface, and it seems dangerous to > me. Callbacks now have different semantic requirements based on value > of some flag... > > Would it be possible to have two sets of callbacks, one irq safe and > one not? Or maybe add a flag to the bus-specific device structures, indicating specifically whether or not the clock should be unprepared during a runtime suspend. Then individual drivers could set this flag or not, independent of the irq-safe setting. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 24, 2014 at 03:47:17PM -0400, Alan Stern wrote: > On Wed, 24 Sep 2014, Pavel Machek wrote: > > > On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote: > > > Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime > > > PM IRQ safe was set or not. > > > > > > Various bus drivers implementing runtime PM may use choose to suspend > > > differently based on IRQ safeness status of child driver (e.g. do not > > > unprepare the clock if IRQ safe is not set). > > > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > > > Are you sure this is good interface? > > > > "Tell me if another function works this or that way". > > > > That's certainly not traditional interface, and it seems dangerous to > > me. Callbacks now have different semantic requirements based on value > > of some flag... > > > > Would it be possible to have two sets of callbacks, one irq safe and > > one not? > > Or maybe add a flag to the bus-specific device structures, indicating > specifically whether or not the clock should be unprepared during a > runtime suspend. Then individual drivers could set this flag or not, > independent of the irq-safe setting. What you're proposing is _less_ safe, because with your proposal, you now have the possibility that drivers will tell runtime PM that it has IRQ safe callbacks, but the bus code tries to prepare/unprepare the clock, which causes a might-sleep-if warning. This is fragile.
On ?ro, 2014-09-24 at 19:53 +0200, Pavel Machek wrote: > On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote: > > Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime > > PM IRQ safe was set or not. > > > > Various bus drivers implementing runtime PM may use choose to suspend > > differently based on IRQ safeness status of child driver (e.g. do not > > unprepare the clock if IRQ safe is not set). > > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> > > Are you sure this is good interface? > > "Tell me if another function works this or that way". > > That's certainly not traditional interface, and it seems dangerous to > me. Callbacks now have different semantic requirements based on value > of some flag... Yes... but to me that semantic change looks similar to the PM runtime code which behaves differently when the flag is set. Some PM runtime functions might sleep or might not... depending on the flag. > Would it be possible to have two sets of callbacks, one irq safe and > one not? That would require changing the callbacks during probe of each driver. It is possible - I'm fine with that. However we still would need some kind of interface for selecting the callbacks according to irq safe status - something like pm_runtime_is_irq_safe(): static int amba_probe(struct device *dev) { ... if (pm_runtime_is_irq_safe()) dev->driver->pm = &amba_pm_irq_safe; else dev->driver->pm = &amba_pm; Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24 September 2014 21:52, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Sep 24, 2014 at 03:47:17PM -0400, Alan Stern wrote: >> On Wed, 24 Sep 2014, Pavel Machek wrote: >> >> > On Wed 2014-09-24 15:50:08, Krzysztof Kozlowski wrote: >> > > Add a simple getter pm_runtime_is_irq_safe() for quering whether runtime >> > > PM IRQ safe was set or not. >> > > >> > > Various bus drivers implementing runtime PM may use choose to suspend >> > > differently based on IRQ safeness status of child driver (e.g. do not >> > > unprepare the clock if IRQ safe is not set). >> > > >> > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> >> > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> >> > >> > Are you sure this is good interface? >> > >> > "Tell me if another function works this or that way". >> > >> > That's certainly not traditional interface, and it seems dangerous to >> > me. Callbacks now have different semantic requirements based on value >> > of some flag... >> > >> > Would it be possible to have two sets of callbacks, one irq safe and >> > one not? >> >> Or maybe add a flag to the bus-specific device structures, indicating >> specifically whether or not the clock should be unprepared during a >> runtime suspend. Then individual drivers could set this flag or not, >> independent of the irq-safe setting. > > What you're proposing is _less_ safe, because with your proposal, you > now have the possibility that drivers will tell runtime PM that it has > IRQ safe callbacks, but the bus code tries to prepare/unprepare the > clock, which causes a might-sleep-if warning. > > This is fragile. I agree. I would also like to feed another argument into this discussion on why I like the new API. Currently the generic power domain accesses "dev->power.irq_safe" directly. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt index f32ce5419573..397b81593142 100644 --- a/Documentation/power/runtime_pm.txt +++ b/Documentation/power/runtime_pm.txt @@ -468,6 +468,10 @@ drivers/base/power/runtime.c and include/linux/pm_runtime.h: - set the power.irq_safe flag for the device, causing the runtime-PM callbacks to be invoked with interrupts off + bool pm_runtime_is_irq_safe(struct device *dev); + - return true if power.irq_safe flag was set for the device, causing + the runtime-PM callbacks to be invoked with interrupts off + void pm_runtime_mark_last_busy(struct device *dev); - set the power.last_busy field to the current time diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h index 367f49b9a1c9..44d74f0f182e 100644 --- a/include/linux/pm_runtime.h +++ b/include/linux/pm_runtime.h @@ -128,6 +128,11 @@ static inline void pm_runtime_mark_last_busy(struct device *dev) ACCESS_ONCE(dev->power.last_busy) = jiffies; } +static inline bool pm_runtime_is_irq_safe(struct device *dev) +{ + return dev->power.irq_safe; +} + #else /* !CONFIG_PM_RUNTIME */ static inline int __pm_runtime_idle(struct device *dev, int rpmflags) @@ -167,6 +172,7 @@ static inline bool pm_runtime_enabled(struct device *dev) { return false; } static inline void pm_runtime_no_callbacks(struct device *dev) {} static inline void pm_runtime_irq_safe(struct device *dev) {} +static inline bool pm_runtime_is_irq_safe(struct device *dev) { return false; } static inline bool pm_runtime_callbacks_present(struct device *dev) { return false; } static inline void pm_runtime_mark_last_busy(struct device *dev) {}