Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Laurent Pinchart]

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

Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Alan Stern]

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

Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Sakari Ailus]

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

Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Alan Stern]

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

Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Laurent Pinchart]

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

Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Alan Stern]

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

Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Laurent Pinchart]

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

Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM

[Alan Stern]

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