[PATCH v2 0/3] PM: runtime: Update behaviour for no callbacks

[PATCH v2 0/3] PM: runtime: Update behaviour for no callbacks

[Ulf Hansson]

While reviewing a patch on the mmc-list, I ended up inspecting the behaviour of
how we deal with the no callback case for runtime PM.

A couple of observations:

*) When pm_runtime_no_callbacks() have been called, it allows the PM core to
takes a quicker path, but at the same time, consumer/supplier device links are
being skipped in rpm_resume|suspend().

**) Calling pm_runtime_no_callbacks() to avoid boiler plate code (assigning
empty functions to ->runtime_suspend|resume()), doesn't work if there could be
consumer/supplier device link being used or a platform dependent PM domain that
could get attached to the device.

Therefore, this series suggests to change the behaviour in the PM core, to
allow the ->runtime_suspend|resume() callbacks to be unassigned. This is already
supported for ->runtime_idle() callbacks, so it would also move things into a
more consistent behaviour.

I have looked at various error paths, in the kernel of callers of
pm_runtime_get_sync(). I couldn't find anyone that made sense, that looked for
the special error code, -ENOSYS, which is the error code getting returned when a
callback is missing. Whether that is sufficient proof that these changes are
100% safe, I can't guarantee, but I think it would be worth a try as the
benefits of avoiding boilerplate code and the corresponding additional code
paths are quite nice, if you ask me.

Kind regards
Ulf Hansson


Ulf Hansson (3):
  PM: runtime: Improve path in rpm_idle() when no callback
  PM: runtime: Allow unassigned ->runtime_suspend|resume callbacks
  PM: runtime: Clarify documentation when callbacks are unassigned

 Documentation/power/runtime_pm.rst |  8 ++++++++
 drivers/base/power/runtime.c       | 18 ++++++++----------
 2 files changed, 16 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] PM: runtime: Improve path in rpm_idle() when no callback

[Ulf Hansson]

When pm_runtime_no_callbacks() has been called for a struct device to set
the dev->power.no_callbacks flag for it, it enables rpm_idle() to take a
slightly quicker path by assuming that a ->runtime_idle() callback would
have returned 0 to indicate success.

A device that does not have the dev->power.no_callbacks flag set for it,
may still be missing a corresponding ->runtime_idle() callback, in which
case the slower path in rpm_idle() is taken. Let's improve the behaviour
for this case, by aligning code to the quicker path.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- None.

---
 drivers/base/power/runtime.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index b570848d23e0..68bebbf81347 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -446,7 +446,10 @@ static int rpm_idle(struct device *dev, int rpmflags)
 	/* Pending requests need to be canceled. */
 	dev->power.request = RPM_REQ_NONE;
 
-	if (dev->power.no_callbacks)
+	callback = RPM_GET_CALLBACK(dev, runtime_idle);
+
+	/* If no callback assume success. */
+	if (!callback || dev->power.no_callbacks)
 		goto out;
 
 	/* Carry out an asynchronous or a synchronous idle notification. */
@@ -462,10 +465,7 @@ static int rpm_idle(struct device *dev, int rpmflags)
 
 	dev->power.idle_notification = true;
 
-	callback = RPM_GET_CALLBACK(dev, runtime_idle);
-
-	if (callback)
-		retval = __rpm_callback(callback, dev);
+	retval = __rpm_callback(callback, dev);
 
 	dev->power.idle_notification = false;
 	wake_up_all(&dev->power.wait_queue);
-- 
2.25.1

[PATCH v2 2/3] PM: runtime: Allow unassigned ->runtime_suspend|resume callbacks

[Ulf Hansson]

We are currently allowing ->runtime_idle() callbacks to be unassigned
without returning an error code from rpm_idle(). This has been useful to
avoid boilerplate code in drivers. Let's take this approach a step further,
by allowing also unassigned ->runtime_suspend|resume() callbacks.

In this way, a consumer/supplier device link can be used to let a consumer
device be power managed through its supplier device, without requiring
assigned ->runtime_suspend|resume() callbacks for the consumer device, for
example.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Small updates to commit message.

---
 drivers/base/power/runtime.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 68bebbf81347..8a66eaf731e4 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -345,7 +345,7 @@ static void rpm_suspend_suppliers(struct device *dev)
 static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 	__releases(&dev->power.lock) __acquires(&dev->power.lock)
 {
-	int retval, idx;
+	int retval = 0, idx;
 	bool use_links = dev->power.links_count > 0;
 
 	if (dev->power.irq_safe) {
@@ -373,7 +373,8 @@ static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
 		}
 	}
 
-	retval = cb(dev);
+	if (cb)
+		retval = cb(dev);
 
 	if (dev->power.irq_safe) {
 		spin_lock(&dev->power.lock);
@@ -484,9 +485,6 @@ static int rpm_callback(int (*cb)(struct device *), struct device *dev)
 {
 	int retval;
 
-	if (!cb)
-		return -ENOSYS;
-
 	if (dev->power.memalloc_noio) {
 		unsigned int noio_flag;
 
-- 
2.25.1

[PATCH v2 3/3] PM: runtime: Clarify documentation when callbacks are unassigned

[Ulf Hansson]

Recent changes to the PM core allows ->runtime_suspend|resume callbacks to
be unassigned.

In the earlier behaviour the PM core would return -ENOSYS, when trying to
runtime resume a device, for example. Let's update the documentation to
clarify this.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v2:
	- Added a new patch for the updating the docs, as pointed out by Alan.

---
 Documentation/power/runtime_pm.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
index 18ae21bf7f92..3d09c9fd450d 100644
--- a/Documentation/power/runtime_pm.rst
+++ b/Documentation/power/runtime_pm.rst
@@ -827,6 +827,14 @@ or driver about runtime power changes.  Instead, the driver for the device's
 parent must take responsibility for telling the device's driver when the
 parent's power state changes.
 
+Note that, in some cases it may not be desirable for subsystems/drivers to call
+pm_runtime_no_callbacks() for their devices. This could be because a subset of
+the runtime PM callbacks needs to be implemented, a platform dependent PM
+domain could get attached to the device or that the device is power manged
+through a supplier device link. For these reasons and to avoid boilerplate code
+in subsystems/drivers, the PM core allows runtime PM callbacks to be
+unassigned.
+
 9. Autosuspend, or automatically-delayed suspends
 =================================================
 
-- 
2.25.1

Re: [PATCH v2 3/3] PM: runtime: Clarify documentation when callbacks are unassigned

[Alan Stern]

On Tue, Jun 08, 2021 at 11:02:50AM +0200, Ulf Hansson wrote:
> Recent changes to the PM core allows ->runtime_suspend|resume callbacks to
> be unassigned.
> 
> In the earlier behaviour the PM core would return -ENOSYS, when trying to
> runtime resume a device, for example. Let's update the documentation to
> clarify this.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v2:
> 	- Added a new patch for the updating the docs, as pointed out by Alan.
> 
> ---
>  Documentation/power/runtime_pm.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> index 18ae21bf7f92..3d09c9fd450d 100644
> --- a/Documentation/power/runtime_pm.rst
> +++ b/Documentation/power/runtime_pm.rst
> @@ -827,6 +827,14 @@ or driver about runtime power changes.  Instead, the driver for the device's
>  parent must take responsibility for telling the device's driver when the
>  parent's power state changes.
>  
> +Note that, in some cases it may not be desirable for subsystems/drivers to call
> +pm_runtime_no_callbacks() for their devices. This could be because a subset of
> +the runtime PM callbacks needs to be implemented, a platform dependent PM
> +domain could get attached to the device or that the device is power manged
> +through a supplier device link. For these reasons and to avoid boilerplate code
> +in subsystems/drivers, the PM core allows runtime PM callbacks to be
> +unassigned.
> +

You should also mention that if a callback pointer is NULL, the 
runtime PM core will act as though there was a callback and it 
returned 0.  That's an important consideration.

Also, notice that this file was carefully edited to make sure that 
none of the lines exceed 80 characters.  Your new addition should 
be the same.

Alan Stern

Re: [PATCH v2 0/3] PM: runtime: Update behaviour for no callbacks

[Alan Stern]

On Tue, Jun 08, 2021 at 11:02:47AM +0200, Ulf Hansson wrote:
> While reviewing a patch on the mmc-list, I ended up inspecting the behaviour of
> how we deal with the no callback case for runtime PM.
> 
> A couple of observations:
> 
> *) When pm_runtime_no_callbacks() have been called, it allows the PM core to
> takes a quicker path, but at the same time, consumer/supplier device links are
> being skipped in rpm_resume|suspend().
> 
> **) Calling pm_runtime_no_callbacks() to avoid boiler plate code (assigning
> empty functions to ->runtime_suspend|resume()), doesn't work if there could be
> consumer/supplier device link being used or a platform dependent PM domain that
> could get attached to the device.
> 
> Therefore, this series suggests to change the behaviour in the PM core, to
> allow the ->runtime_suspend|resume() callbacks to be unassigned. This is already
> supported for ->runtime_idle() callbacks, so it would also move things into a
> more consistent behaviour.
> 
> I have looked at various error paths, in the kernel of callers of
> pm_runtime_get_sync(). I couldn't find anyone that made sense, that looked for
> the special error code, -ENOSYS, which is the error code getting returned when a
> callback is missing. Whether that is sufficient proof that these changes are
> 100% safe, I can't guarantee, but I think it would be worth a try as the
> benefits of avoiding boilerplate code and the corresponding additional code
> paths are quite nice, if you ask me.

In principle I have no objection to these changes.  It's likely 
that if any problems do crop up, we'll be able to fix them pretty 
easily.  For patches 1 and 2:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

Re: [PATCH v2 3/3] PM: runtime: Clarify documentation when callbacks are unassigned

[Ulf Hansson]

On Tue, 8 Jun 2021 at 16:23, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, Jun 08, 2021 at 11:02:50AM +0200, Ulf Hansson wrote:
> > Recent changes to the PM core allows ->runtime_suspend|resume callbacks to
> > be unassigned.
> >
> > In the earlier behaviour the PM core would return -ENOSYS, when trying to
> > runtime resume a device, for example. Let's update the documentation to
> > clarify this.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes in v2:
> >       - Added a new patch for the updating the docs, as pointed out by Alan.
> >
> > ---
> >  Documentation/power/runtime_pm.rst | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> > index 18ae21bf7f92..3d09c9fd450d 100644
> > --- a/Documentation/power/runtime_pm.rst
> > +++ b/Documentation/power/runtime_pm.rst
> > @@ -827,6 +827,14 @@ or driver about runtime power changes.  Instead, the driver for the device's
> >  parent must take responsibility for telling the device's driver when the
> >  parent's power state changes.
> >
> > +Note that, in some cases it may not be desirable for subsystems/drivers to call
> > +pm_runtime_no_callbacks() for their devices. This could be because a subset of
> > +the runtime PM callbacks needs to be implemented, a platform dependent PM
> > +domain could get attached to the device or that the device is power manged
> > +through a supplier device link. For these reasons and to avoid boilerplate code
> > +in subsystems/drivers, the PM core allows runtime PM callbacks to be
> > +unassigned.
> > +
>
> You should also mention that if a callback pointer is NULL, the
> runtime PM core will act as though there was a callback and it
> returned 0.  That's an important consideration.

Good point, let me add it.

I send a new version of $subject patch, unless Rafael is happy to do
the amending when/if applying?

>
> Also, notice that this file was carefully edited to make sure that
> none of the lines exceed 80 characters.  Your new addition should
> be the same.

Absolutely, but it should be okay already, no?

>
> Alan Stern

Kind regards
Uffe

Re: [PATCH v2 3/3] PM: runtime: Clarify documentation when callbacks are unassigned

[Alan Stern]

On Tue, Jun 08, 2021 at 04:30:48PM +0200, Ulf Hansson wrote:
> On Tue, 8 Jun 2021 at 16:23, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Tue, Jun 08, 2021 at 11:02:50AM +0200, Ulf Hansson wrote:
> > > Recent changes to the PM core allows ->runtime_suspend|resume callbacks to
> > > be unassigned.
> > >
> > > In the earlier behaviour the PM core would return -ENOSYS, when trying to
> > > runtime resume a device, for example. Let's update the documentation to
> > > clarify this.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >
> > > Changes in v2:
> > >       - Added a new patch for the updating the docs, as pointed out by Alan.
> > >
> > > ---
> > >  Documentation/power/runtime_pm.rst | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> > > index 18ae21bf7f92..3d09c9fd450d 100644
> > > --- a/Documentation/power/runtime_pm.rst
> > > +++ b/Documentation/power/runtime_pm.rst
> > > @@ -827,6 +827,14 @@ or driver about runtime power changes.  Instead, the driver for the device's
> > >  parent must take responsibility for telling the device's driver when the
> > >  parent's power state changes.
> > >
> > > +Note that, in some cases it may not be desirable for subsystems/drivers to call

More than 80 chars.

> > > +pm_runtime_no_callbacks() for their devices. This could be because a subset of

More than 80 chars.

> > > +the runtime PM callbacks needs to be implemented, a platform dependent PM
> > > +domain could get attached to the device or that the device is power manged

s/manged/managed/

> > > +through a supplier device link. For these reasons and to avoid boilerplate code

More than 80 chars.

> > > +in subsystems/drivers, the PM core allows runtime PM callbacks to be
> > > +unassigned.
> > > +
> >
> > You should also mention that if a callback pointer is NULL, the
> > runtime PM core will act as though there was a callback and it
> > returned 0.  That's an important consideration.
> 
> Good point, let me add it.
> 
> I send a new version of $subject patch, unless Rafael is happy to do
> the amending when/if applying?
> 
> >
> > Also, notice that this file was carefully edited to make sure that
> > none of the lines exceed 80 characters.  Your new addition should
> > be the same.
> 
> Absolutely, but it should be okay already, no?

See above.

Alan

Re: [PATCH v2 3/3] PM: runtime: Clarify documentation when callbacks are unassigned

[Ulf Hansson]

On Tue, 8 Jun 2021 at 16:49, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, Jun 08, 2021 at 04:30:48PM +0200, Ulf Hansson wrote:
> > On Tue, 8 Jun 2021 at 16:23, Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Tue, Jun 08, 2021 at 11:02:50AM +0200, Ulf Hansson wrote:
> > > > Recent changes to the PM core allows ->runtime_suspend|resume callbacks to
> > > > be unassigned.
> > > >
> > > > In the earlier behaviour the PM core would return -ENOSYS, when trying to
> > > > runtime resume a device, for example. Let's update the documentation to
> > > > clarify this.
> > > >
> > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > ---
> > > >
> > > > Changes in v2:
> > > >       - Added a new patch for the updating the docs, as pointed out by Alan.
> > > >
> > > > ---
> > > >  Documentation/power/runtime_pm.rst | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/Documentation/power/runtime_pm.rst b/Documentation/power/runtime_pm.rst
> > > > index 18ae21bf7f92..3d09c9fd450d 100644
> > > > --- a/Documentation/power/runtime_pm.rst
> > > > +++ b/Documentation/power/runtime_pm.rst
> > > > @@ -827,6 +827,14 @@ or driver about runtime power changes.  Instead, the driver for the device's
> > > >  parent must take responsibility for telling the device's driver when the
> > > >  parent's power state changes.
> > > >
> > > > +Note that, in some cases it may not be desirable for subsystems/drivers to call
>
> More than 80 chars.

Perhaps it's the email client that messes up the patch in some way.
This above line in the patch is 79 chars.

If you have a look at the patch in patchwork [1], you should see (I
hope) that it respects the 80 chars per line!?

>
> > > > +pm_runtime_no_callbacks() for their devices. This could be because a subset of
>
> More than 80 chars.
>
> > > > +the runtime PM callbacks needs to be implemented, a platform dependent PM
> > > > +domain could get attached to the device or that the device is power manged
>
> s/manged/managed/

Thanks for spotting this, my spell checker accepted "manged". :-)

>
> > > > +through a supplier device link. For these reasons and to avoid boilerplate code
>
> More than 80 chars.
>
> > > > +in subsystems/drivers, the PM core allows runtime PM callbacks to be
> > > > +unassigned.
> > > > +
> > >
> > > You should also mention that if a callback pointer is NULL, the
> > > runtime PM core will act as though there was a callback and it
> > > returned 0.  That's an important consideration.
> >
> > Good point, let me add it.
> >
> > I send a new version of $subject patch, unless Rafael is happy to do
> > the amending when/if applying?
> >
> > >
> > > Also, notice that this file was carefully edited to make sure that
> > > none of the lines exceed 80 characters.  Your new addition should
> > > be the same.
> >
> > Absolutely, but it should be okay already, no?
>
> See above.
>
> Alan

Kind regards
Uffe

[1]
https://patchwork.kernel.org/project/linux-pm/patch/20210608090250.85256-4-ulf.hansson@linaro.org/