* [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
@ 2016-11-18 13:50 Sakari Ailus
2016-11-18 16:09 ` Arnd Bergmann
0 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2016-11-18 13:50 UTC (permalink / raw)
To: linux-media; +Cc: arnd
Power on the sensor when the module is loaded and power it off when it is
removed.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi Arnd and others,
The patch is tested with CONFIG_PM set, as the system does I was testing
on did not boot with CONFIG_PM disabled. I'm not really too worried about
this though, the patch is very simple.
Kind regards,
Sakari
drivers/media/i2c/smiapp/smiapp-core.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 59872b3..8624dc4 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2741,8 +2741,6 @@ static const struct v4l2_subdev_internal_ops smiapp_internal_ops = {
* I2C Driver
*/
-#ifdef CONFIG_PM
-
static int smiapp_suspend(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -2783,13 +2781,6 @@ static int smiapp_resume(struct device *dev)
return rval;
}
-#else
-
-#define smiapp_suspend NULL
-#define smiapp_resume NULL
-
-#endif /* CONFIG_PM */
-
static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
{
struct smiapp_hwconfig *hwcfg;
@@ -2915,7 +2906,11 @@ static int smiapp_probe(struct i2c_client *client,
pm_runtime_enable(&client->dev);
+#ifdef CONFIG_PM
rval = pm_runtime_get_sync(&client->dev);
+#else
+ rval = smiapp_power_on(&client->dev);
+#endif
if (rval < 0) {
rval = -ENODEV;
goto out_power_off;
@@ -3113,7 +3108,11 @@ static int smiapp_probe(struct i2c_client *client,
smiapp_cleanup(sensor);
out_power_off:
+#ifdef CONFIG_PM
pm_runtime_put(&client->dev);
+#else
+ smiapp_power_off(&client->dev);
+#endif
pm_runtime_disable(&client->dev);
return rval;
@@ -3127,7 +3126,11 @@ static int smiapp_remove(struct i2c_client *client)
v4l2_async_unregister_subdev(subdev);
+#ifdef CONFIG_PM
pm_runtime_suspend(&client->dev);
+#else
+ smiapp_power_off(&client->dev);
+#endif
pm_runtime_disable(&client->dev);
for (i = 0; i < sensor->ssds_used; i++) {
--
2.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-18 13:50 [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM Sakari Ailus
@ 2016-11-18 16:09 ` Arnd Bergmann
2016-11-22 18:31 ` Laurent Pinchart
0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2016-11-18 16:09 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media
On Friday, November 18, 2016 3:50:16 PM CET Sakari Ailus wrote:
> Power on the sensor when the module is loaded and power it off when it is
> removed.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Arnd and others,
>
> The patch is tested with CONFIG_PM set, as the system does I was testing
> on did not boot with CONFIG_PM disabled. I'm not really too worried about
> this though, the patch is very simple.
>
> static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
> {
> struct smiapp_hwconfig *hwcfg;
> @@ -2915,7 +2906,11 @@ static int smiapp_probe(struct i2c_client *client,
>
> pm_runtime_enable(&client->dev);
>
> +#ifdef CONFIG_PM
> rval = pm_runtime_get_sync(&client->dev);
> +#else
> + rval = smiapp_power_on(&client->dev);
> +#endif
> if (rval < 0) {
> rval = -ENODEV;
> goto out_power_off;
I would suggest writing this as
if (IS_ENABLED(CONFIG_PM))
rval = pm_runtime_get_sync(&client->dev);
else
rval = smiapp_power_on(&client->dev);
though that is a purely cosmetic change.
I think you are missing one other warning: with CONFIG_PM=y and
CONFIG_PM_SLEEP=n, the smiapp_suspend/smiapp_resume functions
are now unused and need to be marked as __maybe_unused.
Arnd
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-18 16:09 ` Arnd Bergmann
@ 2016-11-22 18:31 ` Laurent Pinchart
2016-11-22 20:58 ` Arnd Bergmann
0 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2016-11-22 18:31 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Sakari Ailus, linux-media
Hi Arnd,
On Friday 18 Nov 2016 17:09:01 Arnd Bergmann wrote:
> On Friday, November 18, 2016 3:50:16 PM CET Sakari Ailus wrote:
> > Power on the sensor when the module is loaded and power it off when it is
> > removed.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Arnd and others,
> >
> > The patch is tested with CONFIG_PM set, as the system does I was testing
> > on did not boot with CONFIG_PM disabled. I'm not really too worried about
> > this though, the patch is very simple.
> >
> > static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
> > {
> > struct smiapp_hwconfig *hwcfg;
> > @@ -2915,7 +2906,11 @@ static int smiapp_probe(struct i2c_client *client,
> >
> > pm_runtime_enable(&client->dev);
> >
> > +#ifdef CONFIG_PM
> > rval = pm_runtime_get_sync(&client->dev);
> > +#else
> > + rval = smiapp_power_on(&client->dev);
> > +#endif
> >
> > if (rval < 0) {
> > rval = -ENODEV;
> > goto out_power_off;
>
> I would suggest writing this as
>
> if (IS_ENABLED(CONFIG_PM))
> rval = pm_runtime_get_sync(&client->dev);
> else
> rval = smiapp_power_on(&client->dev);
>
> though that is a purely cosmetic change.
Are all drivers really supposed to code this kind of construct ? Shouldn't
this be handled in the PM core ? A very naive approach would be to call
.runtime_resume() and .runtime_suspend() from the non-CONFIG_PM versions of
pm_runtime_enable() and pm_runtime_disable() respectively. I assume that would
break things, but can't we implement something similar to that that wouldn't
require all drivers to open-code it ?
> I think you are missing one other warning: with CONFIG_PM=y and
> CONFIG_PM_SLEEP=n, the smiapp_suspend/smiapp_resume functions
> are now unused and need to be marked as __maybe_unused.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-22 18:31 ` Laurent Pinchart
@ 2016-11-22 20:58 ` Arnd Bergmann
2016-11-25 0:43 ` Laurent Pinchart
0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2016-11-22 20:58 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media
On Tuesday, November 22, 2016 8:31:42 PM CET Laurent Pinchart wrote:
> > > @@ -2915,7 +2906,11 @@ static int smiapp_probe(struct i2c_client *client,
> > >
> > > pm_runtime_enable(&client->dev);
> > >
> > > +#ifdef CONFIG_PM
> > > rval = pm_runtime_get_sync(&client->dev);
> > > +#else
> > > + rval = smiapp_power_on(&client->dev);
> > > +#endif
> > >
> > > if (rval < 0) {
> > > rval = -ENODEV;
> > > goto out_power_off;
> >
> > I would suggest writing this as
> >
> > if (IS_ENABLED(CONFIG_PM))
> > rval = pm_runtime_get_sync(&client->dev);
> > else
> > rval = smiapp_power_on(&client->dev);
> >
> > though that is a purely cosmetic change.
>
> Are all drivers really supposed to code this kind of construct ? Shouldn't
> this be handled in the PM core ? A very naive approach would be to call
> .runtime_resume() and .runtime_suspend() from the non-CONFIG_PM versions of
> pm_runtime_enable() and pm_runtime_disable() respectively. I assume that would
> break things, but can't we implement something similar to that that wouldn't
> require all drivers to open-code it ?
I know nothing about the details of how the suspend/resume code should
do this, I was just commenting on the syntax above, preferring an
IS_ENABLED() check over an #ifdef.
Arnd
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-22 20:58 ` Arnd Bergmann
@ 2016-11-25 0:43 ` Laurent Pinchart
2016-11-25 2:15 ` Alan Stern
0 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2016-11-25 0:43 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: Sakari Ailus, linux-media, linux-pm
Hello,
(CC'ing the linux-pm mailing list)
On Tuesday 22 Nov 2016 21:58:32 Arnd Bergmann wrote:
> On Tuesday, November 22, 2016 8:31:42 PM CET Laurent Pinchart wrote:
> >>> @@ -2915,7 +2906,11 @@ static int smiapp_probe(struct i2c_client
> >>> *client,
> >>>
> >>> pm_runtime_enable(&client->dev);
> >>>
> >>> +#ifdef CONFIG_PM
> >>> rval = pm_runtime_get_sync(&client->dev);
> >>> +#else
> >>> + rval = smiapp_power_on(&client->dev);
> >>> +#endif
> >>> if (rval < 0) {
> >>> rval = -ENODEV;
> >>> goto out_power_off;
> >>
> >> I would suggest writing this as
> >>
> >> if (IS_ENABLED(CONFIG_PM))
> >> rval = pm_runtime_get_sync(&client->dev);
> >> else
> >> rval = smiapp_power_on(&client->dev);
> >>
> >> though that is a purely cosmetic change.
> >
> > Are all drivers really supposed to code this kind of construct ? Shouldn't
> > this be handled in the PM core ? A very naive approach would be to call
> > .runtime_resume() and .runtime_suspend() from the non-CONFIG_PM versions
> > of pm_runtime_enable() and pm_runtime_disable() respectively. I assume
> > that would break things, but can't we implement something similar to that
> > that wouldn't require all drivers to open-code it ?
>
> I know nothing about the details of how the suspend/resume code should
> do this, I was just commenting on the syntax above, preferring an
> IS_ENABLED() check over an #ifdef.
Dear linux-pm developers, what's the suggested way to ensure that a runtime-
pm-enabled driver can run fine on a system with CONFIG_PM disabled ?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-25 0:43 ` Laurent Pinchart
@ 2016-11-25 2:15 ` Alan Stern
0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-11-25 2:15 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> Dear linux-pm developers, what's the suggested way to ensure that a runtime-
> pm-enabled driver can run fine on a system with CONFIG_PM disabled ?
The exact point of your question isn't entirely clear. In the most
literal sense, the best ways to ensure this are (1) audit the code, and
(2) actually try it.
I have a feeling this doesn't quite answer your question, however. :-)
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
@ 2016-11-25 2:15 ` Alan Stern
0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-11-25 2:15 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> Dear linux-pm developers, what's the suggested way to ensure that a runtime-
> pm-enabled driver can run fine on a system with CONFIG_PM disabled ?
The exact point of your question isn't entirely clear. In the most
literal sense, the best ways to ensure this are (1) audit the code, and
(2) actually try it.
I have a feeling this doesn't quite answer your question, however. :-)
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-25 2:15 ` Alan Stern
(?)
@ 2016-11-25 7:48 ` Sakari Ailus
2016-11-25 15:21 ` Alan Stern
-1 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2016-11-25 7:48 UTC (permalink / raw)
To: Alan Stern
Cc: Laurent Pinchart, Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
Hi Alan and others,
On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote:
> On Fri, 25 Nov 2016, Laurent Pinchart wrote:
>
> > Dear linux-pm developers, what's the suggested way to ensure that a runtime-
> > pm-enabled driver can run fine on a system with CONFIG_PM disabled ?
>
> The exact point of your question isn't entirely clear. In the most
> literal sense, the best ways to ensure this are (1) audit the code, and
> (2) actually try it.
>
> I have a feeling this doesn't quite answer your question, however. :-)
The question is related to devices that require certain power-up and
power-down sequences that are now implemented as PM runtime hooks that,
without CONFIG_PM defined, will not be executed. Is there a better way than
to handle this than have an implementation in the driver for the PM runtime
and non-PM runtime case separately?
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-25 7:48 ` Sakari Ailus
@ 2016-11-25 15:21 ` Alan Stern
0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-11-25 15:21 UTC (permalink / raw)
To: Sakari Ailus
Cc: Laurent Pinchart, Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
On Fri, 25 Nov 2016, Sakari Ailus wrote:
> Hi Alan and others,
>
> On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote:
> > On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> >
> > > Dear linux-pm developers, what's the suggested way to ensure that a runtime-
> > > pm-enabled driver can run fine on a system with CONFIG_PM disabled ?
> >
> > The exact point of your question isn't entirely clear. In the most
> > literal sense, the best ways to ensure this are (1) audit the code, and
> > (2) actually try it.
> >
> > I have a feeling this doesn't quite answer your question, however. :-)
>
> The question is related to devices that require certain power-up and
> power-down sequences that are now implemented as PM runtime hooks that,
> without CONFIG_PM defined, will not be executed. Is there a better way than
> to handle this than have an implementation in the driver for the PM runtime
> and non-PM runtime case separately?
Yes, there is a better way. For the initial power-up and final
power-down sequences, don't rely on the PM core to invoke the
callbacks. Just call them directly, yourself.
For example, as part of the probe routine, instead of doing this:
pm_runtime_set_suspended(dev);
pm_runtime_enable(dev);
pm_runtime_get_sync(dev);
Do this:
pm_runtime_set_active(dev);
pm_runtime_get_noresume(dev);
pm_runtime_enable(dev);
/*
* In case CONFIG_PM is disabled, invoke the runtime-resume
* callback directly.
*/
my_runtime_resume(dev);
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
@ 2016-11-25 15:21 ` Alan Stern
0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-11-25 15:21 UTC (permalink / raw)
To: Sakari Ailus
Cc: Laurent Pinchart, Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
On Fri, 25 Nov 2016, Sakari Ailus wrote:
> Hi Alan and others,
>
> On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote:
> > On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> >
> > > Dear linux-pm developers, what's the suggested way to ensure that a runtime-
> > > pm-enabled driver can run fine on a system with CONFIG_PM disabled ?
> >
> > The exact point of your question isn't entirely clear. In the most
> > literal sense, the best ways to ensure this are (1) audit the code, and
> > (2) actually try it.
> >
> > I have a feeling this doesn't quite answer your question, however. :-)
>
> The question is related to devices that require certain power-up and
> power-down sequences that are now implemented as PM runtime hooks that,
> without CONFIG_PM defined, will not be executed. Is there a better way than
> to handle this than have an implementation in the driver for the PM runtime
> and non-PM runtime case separately?
Yes, there is a better way. For the initial power-up and final
power-down sequences, don't rely on the PM core to invoke the
callbacks. Just call them directly, yourself.
For example, as part of the probe routine, instead of doing this:
pm_runtime_set_suspended(dev);
pm_runtime_enable(dev);
pm_runtime_get_sync(dev);
Do this:
pm_runtime_set_active(dev);
pm_runtime_get_noresume(dev);
pm_runtime_enable(dev);
/*
* In case CONFIG_PM is disabled, invoke the runtime-resume
* callback directly.
*/
my_runtime_resume(dev);
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-25 15:21 ` Alan Stern
(?)
@ 2016-11-25 19:34 ` Laurent Pinchart
2016-11-26 20:10 ` Alan Stern
-1 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2016-11-25 19:34 UTC (permalink / raw)
To: Alan Stern
Cc: Sakari Ailus, Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
Hi Alan,
On Friday 25 Nov 2016 10:21:21 Alan Stern wrote:
> On Fri, 25 Nov 2016, Sakari Ailus wrote:
> > On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote:
> >> On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> >>> Dear linux-pm developers, what's the suggested way to ensure that a
> >>> runtime- pm-enabled driver can run fine on a system with CONFIG_PM
> >>> disabled ?
> >>
> >> The exact point of your question isn't entirely clear. In the most
> >> literal sense, the best ways to ensure this are (1) audit the code, and
> >> (2) actually try it.
> >>
> >> I have a feeling this doesn't quite answer your question, however. :-)
> >
> > The question is related to devices that require certain power-up and
> > power-down sequences that are now implemented as PM runtime hooks that,
> > without CONFIG_PM defined, will not be executed. Is there a better way
> > than to handle this than have an implementation in the driver for the PM
> > runtime and non-PM runtime case separately?
>
> Yes, there is a better way. For the initial power-up and final
> power-down sequences, don't rely on the PM core to invoke the
> callbacks. Just call them directly, yourself.
>
> For example, as part of the probe routine, instead of doing this:
>
> pm_runtime_set_suspended(dev);
> pm_runtime_enable(dev);
> pm_runtime_get_sync(dev);
>
> Do this:
>
> pm_runtime_set_active(dev);
> pm_runtime_get_noresume(dev);
> pm_runtime_enable(dev);
> /*
> * In case CONFIG_PM is disabled, invoke the runtime-resume
> * callback directly.
> */
> my_runtime_resume(dev);
Wouldn't it be cleaner for drivers not to have to handle this manually (which
gives an opportunity to get it wrong) but instead have pm_runtime_enable()
call the runtime resume callback when CONFIG_PM is disabled ?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-25 19:34 ` Laurent Pinchart
@ 2016-11-26 20:10 ` Alan Stern
0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-11-26 20:10 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> Hi Alan,
Hello.
> On Friday 25 Nov 2016 10:21:21 Alan Stern wrote:
> > On Fri, 25 Nov 2016, Sakari Ailus wrote:
> > > On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote:
> > >> On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> > >>> Dear linux-pm developers, what's the suggested way to ensure that a
> > >>> runtime- pm-enabled driver can run fine on a system with CONFIG_PM
> > >>> disabled ?
> > >>
> > >> The exact point of your question isn't entirely clear. In the most
> > >> literal sense, the best ways to ensure this are (1) audit the code, and
> > >> (2) actually try it.
> > >>
> > >> I have a feeling this doesn't quite answer your question, however. :-)
> > >
> > > The question is related to devices that require certain power-up and
> > > power-down sequences that are now implemented as PM runtime hooks that,
> > > without CONFIG_PM defined, will not be executed. Is there a better way
> > > than to handle this than have an implementation in the driver for the PM
> > > runtime and non-PM runtime case separately?
> >
> > Yes, there is a better way. For the initial power-up and final
> > power-down sequences, don't rely on the PM core to invoke the
> > callbacks. Just call them directly, yourself.
> >
> > For example, as part of the probe routine, instead of doing this:
> >
> > pm_runtime_set_suspended(dev);
> > pm_runtime_enable(dev);
> > pm_runtime_get_sync(dev);
> >
> > Do this:
> >
> > pm_runtime_set_active(dev);
> > pm_runtime_get_noresume(dev);
> > pm_runtime_enable(dev);
> > /*
> > * In case CONFIG_PM is disabled, invoke the runtime-resume
> > * callback directly.
> > */
> > my_runtime_resume(dev);
>
> Wouldn't it be cleaner for drivers not to have to handle this manually (which
> gives an opportunity to get it wrong) but instead have pm_runtime_enable()
> call the runtime resume callback when CONFIG_PM is disabled ?
Well, I admit it would be nicer if drivers didn't have to worry about
whether or not CONFIG_PM was enabled. A slightly cleaner approach
from the one outlined above would have the probe routine do this:
my_power_up(dev);
pm_runtime_set_active(dev);
pm_runtime_get_noresume(dev);
pm_runtime_enable(dev);
and have the runtime-resume callback routine call my_power_up() to do
its work. (Or make my_power_up() actually be the runtime-resume
callback routine.) That's pretty straightforward and hard to mess up.
In theory, we could have pm_runtime_enable() invoke the runtime-resume
callback when CONFIG_PM is disabled. In practice, it would be rather
awkward. drivers/base/power/runtime.c, which is where
pm_runtime_enable() is defined and the runtime-PM callbacks are
invoked, doesn't even get compiled if CONFIG_PM is off.
(Also, it would run against the grain. CONFIG_PM=n means the kernel
ignores runtime PM, so pm_runtime_enable() shouldn't do anything.)
There's a corollary aspect to this. If you depend on runtime PM for
powering up your device during probe, does that mean you also depend on
runtime PM for powering down the device during remove? That is likely
not to work, because the user can prevent runtime suspends by writing
to /sys/.../power/control.
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
@ 2016-11-26 20:10 ` Alan Stern
0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-11-26 20:10 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> Hi Alan,
Hello.
> On Friday 25 Nov 2016 10:21:21 Alan Stern wrote:
> > On Fri, 25 Nov 2016, Sakari Ailus wrote:
> > > On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote:
> > >> On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> > >>> Dear linux-pm developers, what's the suggested way to ensure that a
> > >>> runtime- pm-enabled driver can run fine on a system with CONFIG_PM
> > >>> disabled ?
> > >>
> > >> The exact point of your question isn't entirely clear. In the most
> > >> literal sense, the best ways to ensure this are (1) audit the code, and
> > >> (2) actually try it.
> > >>
> > >> I have a feeling this doesn't quite answer your question, however. :-)
> > >
> > > The question is related to devices that require certain power-up and
> > > power-down sequences that are now implemented as PM runtime hooks that,
> > > without CONFIG_PM defined, will not be executed. Is there a better way
> > > than to handle this than have an implementation in the driver for the PM
> > > runtime and non-PM runtime case separately?
> >
> > Yes, there is a better way. For the initial power-up and final
> > power-down sequences, don't rely on the PM core to invoke the
> > callbacks. Just call them directly, yourself.
> >
> > For example, as part of the probe routine, instead of doing this:
> >
> > pm_runtime_set_suspended(dev);
> > pm_runtime_enable(dev);
> > pm_runtime_get_sync(dev);
> >
> > Do this:
> >
> > pm_runtime_set_active(dev);
> > pm_runtime_get_noresume(dev);
> > pm_runtime_enable(dev);
> > /*
> > * In case CONFIG_PM is disabled, invoke the runtime-resume
> > * callback directly.
> > */
> > my_runtime_resume(dev);
>
> Wouldn't it be cleaner for drivers not to have to handle this manually (which
> gives an opportunity to get it wrong) but instead have pm_runtime_enable()
> call the runtime resume callback when CONFIG_PM is disabled ?
Well, I admit it would be nicer if drivers didn't have to worry about
whether or not CONFIG_PM was enabled. A slightly cleaner approach
from the one outlined above would have the probe routine do this:
my_power_up(dev);
pm_runtime_set_active(dev);
pm_runtime_get_noresume(dev);
pm_runtime_enable(dev);
and have the runtime-resume callback routine call my_power_up() to do
its work. (Or make my_power_up() actually be the runtime-resume
callback routine.) That's pretty straightforward and hard to mess up.
In theory, we could have pm_runtime_enable() invoke the runtime-resume
callback when CONFIG_PM is disabled. In practice, it would be rather
awkward. drivers/base/power/runtime.c, which is where
pm_runtime_enable() is defined and the runtime-PM callbacks are
invoked, doesn't even get compiled if CONFIG_PM is off.
(Also, it would run against the grain. CONFIG_PM=n means the kernel
ignores runtime PM, so pm_runtime_enable() shouldn't do anything.)
There's a corollary aspect to this. If you depend on runtime PM for
powering up your device during probe, does that mean you also depend on
runtime PM for powering down the device during remove? That is likely
not to work, because the user can prevent runtime suspends by writing
to /sys/.../power/control.
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-26 20:10 ` Alan Stern
(?)
@ 2016-11-28 7:58 ` Laurent Pinchart
2016-11-28 15:45 ` Alan Stern
-1 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2016-11-28 7:58 UTC (permalink / raw)
To: Alan Stern
Cc: Sakari Ailus, Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
Hi Alan,
On Saturday 26 Nov 2016 15:10:28 Alan Stern wrote:
> On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> > On Friday 25 Nov 2016 10:21:21 Alan Stern wrote:
> >> On Fri, 25 Nov 2016, Sakari Ailus wrote:
> >>> On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote:
> >>>> On Fri, 25 Nov 2016, Laurent Pinchart wrote:
> >>>>> Dear linux-pm developers, what's the suggested way to ensure that a
> >>>>> runtime- pm-enabled driver can run fine on a system with CONFIG_PM
> >>>>> disabled ?
> >>>>
> >>>> The exact point of your question isn't entirely clear. In the most
> >>>> literal sense, the best ways to ensure this are (1) audit the code,
> >>>> and (2) actually try it.
> >>>>
> >>>> I have a feeling this doesn't quite answer your question, however.
> >>>> :-)
> >>>
> >>> The question is related to devices that require certain power-up and
> >>> power-down sequences that are now implemented as PM runtime hooks
> >>> that, without CONFIG_PM defined, will not be executed. Is there a
> >>> better way than to handle this than have an implementation in the
> >>> driver for the PM runtime and non-PM runtime case separately?
> >>
> >> Yes, there is a better way. For the initial power-up and final
> >> power-down sequences, don't rely on the PM core to invoke the
> >> callbacks. Just call them directly, yourself.
> >>
> >> For example, as part of the probe routine, instead of doing this:
> >> pm_runtime_set_suspended(dev);
> >> pm_runtime_enable(dev);
> >> pm_runtime_get_sync(dev);
> >>
> >> Do this:
> >> pm_runtime_set_active(dev);
> >> pm_runtime_get_noresume(dev);
> >> pm_runtime_enable(dev);
> >> /*
> >> * In case CONFIG_PM is disabled, invoke the runtime-resume
> >> * callback directly.
> >> */
> >> my_runtime_resume(dev);
> >
> > Wouldn't it be cleaner for drivers not to have to handle this manually
> > (which gives an opportunity to get it wrong) but instead have
> > pm_runtime_enable() call the runtime resume callback when CONFIG_PM is
> > disabled ?
>
> Well, I admit it would be nicer if drivers didn't have to worry about
> whether or not CONFIG_PM was enabled. A slightly cleaner approach
> from the one outlined above would have the probe routine do this:
>
> my_power_up(dev);
> pm_runtime_set_active(dev);
> pm_runtime_get_noresume(dev);
> pm_runtime_enable(dev);
>
> and have the runtime-resume callback routine call my_power_up() to do
> its work. (Or make my_power_up() actually be the runtime-resume
> callback routine.) That's pretty straightforward and hard to mess up.
You'd be surprised how easy drivers can mess simple things up ;-) We'd still
have to get the message out there, that would be the most difficult part.
> In theory, we could have pm_runtime_enable() invoke the runtime-resume
> callback when CONFIG_PM is disabled. In practice, it would be rather
> awkward. drivers/base/power/runtime.c, which is where
> pm_runtime_enable() is defined and the runtime-PM callbacks are
> invoked, doesn't even get compiled if CONFIG_PM is off.
Sure, but that can easily be fixed.
> (Also, it would run against the grain. CONFIG_PM=n means the kernel
> ignores runtime PM, so pm_runtime_enable() shouldn't do anything.)
I'd argue that CONFIG_PM=n should mean that the runtime PM API doesn't perform
runtime PM, not that it should do absolutely nothing. If semantics is the
biggest concern, we could introduce a helper (whose name is TBD) that would
enable runtime PM when CONFIG_PM=y or power on the device when CONFIG_PM=n
I want to make it as easy as possible for drivers to make sure they won't get
this wrong, which in my opinion requires a simple and straightforward API with
no code in the driver that would depend on the value of CONFIG_PM.
> There's a corollary aspect to this. If you depend on runtime PM for
> powering up your device during probe, does that mean you also depend on
> runtime PM for powering down the device during remove? That is likely
> not to work, because the user can prevent runtime suspends by writing
> to /sys/.../power/control.
Yes, I do, and I expect most runtime PM-enabled driver to do the same. When
runtime suspend is disabled through /sys/.../power/control does
pm_runtime_disable() invoke the runtime PM suspend handler if the device is
powered on ?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
2016-11-28 7:58 ` Laurent Pinchart
@ 2016-11-28 15:45 ` Alan Stern
0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-11-28 15:45 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
On Mon, 28 Nov 2016, Laurent Pinchart wrote:
> > Well, I admit it would be nicer if drivers didn't have to worry about
> > whether or not CONFIG_PM was enabled. A slightly cleaner approach
> > from the one outlined above would have the probe routine do this:
> >
> > my_power_up(dev);
> > pm_runtime_set_active(dev);
> > pm_runtime_get_noresume(dev);
> > pm_runtime_enable(dev);
> >
> > and have the runtime-resume callback routine call my_power_up() to do
> > its work. (Or make my_power_up() actually be the runtime-resume
> > callback routine.) That's pretty straightforward and hard to mess up.
>
> You'd be surprised how easy drivers can mess simple things up ;-)
No -- I wouldn't! :-)
> We'd still
> have to get the message out there, that would be the most difficult part.
Agreed.
> > In theory, we could have pm_runtime_enable() invoke the runtime-resume
> > callback when CONFIG_PM is disabled. In practice, it would be rather
> > awkward. drivers/base/power/runtime.c, which is where
> > pm_runtime_enable() is defined and the runtime-PM callbacks are
> > invoked, doesn't even get compiled if CONFIG_PM is off.
>
> Sure, but that can easily be fixed.
>
> > (Also, it would run against the grain. CONFIG_PM=n means the kernel
> > ignores runtime PM, so pm_runtime_enable() shouldn't do anything.)
>
> I'd argue that CONFIG_PM=n should mean that the runtime PM API doesn't perform
> runtime PM, not that it should do absolutely nothing. If semantics is the
> biggest concern, we could introduce a helper (whose name is TBD) that would
> enable runtime PM when CONFIG_PM=y or power on the device when CONFIG_PM=n
Or have the driver call _both_ the helper routine and
pm_runtime_enable() -- the helper would do nothing if CONFIG_PM=y, and
it would invoke the runtime-resume callback if CONFIG_PM=n.
Either way would be a good approach. Having pm_runtime_enable() call
the runtime-resume handler wouldn't work well if the driver has already
powered-up the device or the device starts out in the power-on state
(which is often the case).
> I want to make it as easy as possible for drivers to make sure they won't get
> this wrong, which in my opinion requires a simple and straightforward API with
> no code in the driver that would depend on the value of CONFIG_PM.
Well, the approach I outlined above is pretty simple and it doesn't
depend on the value of CONFIG_PM.
Your proposal is just as simple, but it does require drivers to
remember to call the new helper routine.
> > There's a corollary aspect to this. If you depend on runtime PM for
> > powering up your device during probe, does that mean you also depend on
> > runtime PM for powering down the device during remove? That is likely
> > not to work, because the user can prevent runtime suspends by writing
> > to /sys/.../power/control.
>
> Yes, I do, and I expect most runtime PM-enabled driver to do the same. When
> runtime suspend is disabled through /sys/.../power/control does
> pm_runtime_disable() invoke the runtime PM suspend handler if the device is
> powered on ?
No, it doesn't, and neither does pm_runtime_put(). After all, if the
user has told the system not to do runtime PM on that device, it
doesn't make sense to call the runtime-suspend handler. But you can
always blame the user when this happens. :-)
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
@ 2016-11-28 15:45 ` Alan Stern
0 siblings, 0 replies; 16+ messages in thread
From: Alan Stern @ 2016-11-28 15:45 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sakari Ailus, Arnd Bergmann, Sakari Ailus, linux-media, linux-pm
On Mon, 28 Nov 2016, Laurent Pinchart wrote:
> > Well, I admit it would be nicer if drivers didn't have to worry about
> > whether or not CONFIG_PM was enabled. A slightly cleaner approach
> > from the one outlined above would have the probe routine do this:
> >
> > my_power_up(dev);
> > pm_runtime_set_active(dev);
> > pm_runtime_get_noresume(dev);
> > pm_runtime_enable(dev);
> >
> > and have the runtime-resume callback routine call my_power_up() to do
> > its work. (Or make my_power_up() actually be the runtime-resume
> > callback routine.) That's pretty straightforward and hard to mess up.
>
> You'd be surprised how easy drivers can mess simple things up ;-)
No -- I wouldn't! :-)
> We'd still
> have to get the message out there, that would be the most difficult part.
Agreed.
> > In theory, we could have pm_runtime_enable() invoke the runtime-resume
> > callback when CONFIG_PM is disabled. In practice, it would be rather
> > awkward. drivers/base/power/runtime.c, which is where
> > pm_runtime_enable() is defined and the runtime-PM callbacks are
> > invoked, doesn't even get compiled if CONFIG_PM is off.
>
> Sure, but that can easily be fixed.
>
> > (Also, it would run against the grain. CONFIG_PM=n means the kernel
> > ignores runtime PM, so pm_runtime_enable() shouldn't do anything.)
>
> I'd argue that CONFIG_PM=n should mean that the runtime PM API doesn't perform
> runtime PM, not that it should do absolutely nothing. If semantics is the
> biggest concern, we could introduce a helper (whose name is TBD) that would
> enable runtime PM when CONFIG_PM=y or power on the device when CONFIG_PM=n
Or have the driver call _both_ the helper routine and
pm_runtime_enable() -- the helper would do nothing if CONFIG_PM=y, and
it would invoke the runtime-resume callback if CONFIG_PM=n.
Either way would be a good approach. Having pm_runtime_enable() call
the runtime-resume handler wouldn't work well if the driver has already
powered-up the device or the device starts out in the power-on state
(which is often the case).
> I want to make it as easy as possible for drivers to make sure they won't get
> this wrong, which in my opinion requires a simple and straightforward API with
> no code in the driver that would depend on the value of CONFIG_PM.
Well, the approach I outlined above is pretty simple and it doesn't
depend on the value of CONFIG_PM.
Your proposal is just as simple, but it does require drivers to
remember to call the new helper routine.
> > There's a corollary aspect to this. If you depend on runtime PM for
> > powering up your device during probe, does that mean you also depend on
> > runtime PM for powering down the device during remove? That is likely
> > not to work, because the user can prevent runtime suspends by writing
> > to /sys/.../power/control.
>
> Yes, I do, and I expect most runtime PM-enabled driver to do the same. When
> runtime suspend is disabled through /sys/.../power/control does
> pm_runtime_disable() invoke the runtime PM suspend handler if the device is
> powered on ?
No, it doesn't, and neither does pm_runtime_put(). After all, if the
user has told the system not to do runtime PM on that device, it
doesn't make sense to call the runtime-suspend handler. But you can
always blame the user when this happens. :-)
Alan Stern
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-11-28 15:45 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-18 13:50 [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM Sakari Ailus
2016-11-18 16:09 ` Arnd Bergmann
2016-11-22 18:31 ` Laurent Pinchart
2016-11-22 20:58 ` Arnd Bergmann
2016-11-25 0:43 ` Laurent Pinchart
2016-11-25 2:15 ` Alan Stern
2016-11-25 2:15 ` Alan Stern
2016-11-25 7:48 ` Sakari Ailus
2016-11-25 15:21 ` Alan Stern
2016-11-25 15:21 ` Alan Stern
2016-11-25 19:34 ` Laurent Pinchart
2016-11-26 20:10 ` Alan Stern
2016-11-26 20:10 ` Alan Stern
2016-11-28 7:58 ` Laurent Pinchart
2016-11-28 15:45 ` Alan Stern
2016-11-28 15:45 ` Alan Stern
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.