PM: runtime: Return -EINPROGRESS from rpm_resume() in the RPM_NOWAIT case

[PATCH] PM: runtime: Return -EINPROGRESS from rpm_resume() in the RPM_NOWAIT case

Posted by Rafael J. Wysocki 3 years, 6 months ago

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The prospective callers of rpm_resume() passing RPM_NOWAIT to it may
be confused when it returns 0 without actually resuming the device
which may happen if the device is suspending at the given time and it
will only resume when the suspend in progress has completed.  To avoid
that confusion, return -EINPROGRESS from rpm_resume() in that case.

Since none of the current callers passing RPM_NOWAIT to rpm_resume()
check its return value, this change has no functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/power/runtime.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -792,10 +792,13 @@ static int rpm_resume(struct device *dev
 		DEFINE_WAIT(wait);
 
 		if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
-			if (dev->power.runtime_status == RPM_SUSPENDING)
+			if (dev->power.runtime_status == RPM_SUSPENDING) {
 				dev->power.deferred_resume = true;
-			else
+				if (rpmflags & RPM_NOWAIT)
+					retval = -EINPROGRESS;
+			} else {
 				retval = -EINPROGRESS;
+			}
 			goto out;
 		}

Re: [PATCH] PM: runtime: Return -EINPROGRESS from rpm_resume() in the RPM_NOWAIT case

Posted by Ulf Hansson 3 years, 6 months ago

On Thu, 22 Sept 2022 at 20:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The prospective callers of rpm_resume() passing RPM_NOWAIT to it may
> be confused when it returns 0 without actually resuming the device
> which may happen if the device is suspending at the given time and it
> will only resume when the suspend in progress has completed.  To avoid
> that confusion, return -EINPROGRESS from rpm_resume() in that case.
>
> Since none of the current callers passing RPM_NOWAIT to rpm_resume()
> check its return value, this change has no functional impact.

Sounds like there are additional improvements that can be made around
this, right?

>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Looks good to me!

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

Kind regards
Uffe

> ---
>  drivers/base/power/runtime.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -792,10 +792,13 @@ static int rpm_resume(struct device *dev
>                 DEFINE_WAIT(wait);
>
>                 if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
> -                       if (dev->power.runtime_status == RPM_SUSPENDING)
> +                       if (dev->power.runtime_status == RPM_SUSPENDING) {
>                                 dev->power.deferred_resume = true;
> -                       else
> +                               if (rpmflags & RPM_NOWAIT)
> +                                       retval = -EINPROGRESS;
> +                       } else {
>                                 retval = -EINPROGRESS;
> +                       }
>                         goto out;
>                 }
>
>
>
>

Re: [PATCH] PM: runtime: Return -EINPROGRESS from rpm_resume() in the RPM_NOWAIT case

Posted by Rafael J. Wysocki 3 years, 6 months ago

On Fri, Sep 23, 2022 at 3:26 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 22 Sept 2022 at 20:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The prospective callers of rpm_resume() passing RPM_NOWAIT to it may
> > be confused when it returns 0 without actually resuming the device
> > which may happen if the device is suspending at the given time and it
> > will only resume when the suspend in progress has completed.  To avoid
> > that confusion, return -EINPROGRESS from rpm_resume() in that case.
> >
> > Since none of the current callers passing RPM_NOWAIT to rpm_resume()
> > check its return value, this change has no functional impact.
>
> Sounds like there are additional improvements that can be made around
> this, right?

This allows RPM_NOWAIT to be used in places where the caller doesn't
want to wait, because it might deadlock or similar, but they still
need to know whether or not the device can be accessed safely.

Or do you mean something else?

> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Looks good to me!
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Thanks!

> > ---
> >  drivers/base/power/runtime.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -792,10 +792,13 @@ static int rpm_resume(struct device *dev
> >                 DEFINE_WAIT(wait);
> >
> >                 if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
> > -                       if (dev->power.runtime_status == RPM_SUSPENDING)
> > +                       if (dev->power.runtime_status == RPM_SUSPENDING) {
> >                                 dev->power.deferred_resume = true;
> > -                       else
> > +                               if (rpmflags & RPM_NOWAIT)
> > +                                       retval = -EINPROGRESS;
> > +                       } else {
> >                                 retval = -EINPROGRESS;
> > +                       }
> >                         goto out;
> >                 }
> >
> >
> >
> >

Re: [PATCH] PM: runtime: Return -EINPROGRESS from rpm_resume() in the RPM_NOWAIT case

Posted by Ulf Hansson 3 years, 6 months ago

On Fri, 23 Sept 2022 at 17:53, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Fri, Sep 23, 2022 at 3:26 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Thu, 22 Sept 2022 at 20:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > The prospective callers of rpm_resume() passing RPM_NOWAIT to it may
> > > be confused when it returns 0 without actually resuming the device
> > > which may happen if the device is suspending at the given time and it
> > > will only resume when the suspend in progress has completed.  To avoid
> > > that confusion, return -EINPROGRESS from rpm_resume() in that case.
> > >
> > > Since none of the current callers passing RPM_NOWAIT to rpm_resume()
> > > check its return value, this change has no functional impact.
> >
> > Sounds like there are additional improvements that can be made around
> > this, right?
>
> This allows RPM_NOWAIT to be used in places where the caller doesn't
> want to wait, because it might deadlock or similar, but they still
> need to know whether or not the device can be accessed safely.
>
> Or do you mean something else?

Nope, I was mostly wondering if you are planning to make those
improvements too. Sooner or later.

[...]

Kind regards
Uffe

Re: [PATCH] PM: runtime: Return -EINPROGRESS from rpm_resume() in the RPM_NOWAIT case

Posted by Doug Anderson 3 years, 6 months ago

Hi,

On Thu, Sep 22, 2022 at 11:04 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The prospective callers of rpm_resume() passing RPM_NOWAIT to it may
> be confused when it returns 0 without actually resuming the device
> which may happen if the device is suspending at the given time and it
> will only resume when the suspend in progress has completed.  To avoid
> that confusion, return -EINPROGRESS from rpm_resume() in that case.
>
> Since none of the current callers passing RPM_NOWAIT to rpm_resume()
> check its return value, this change has no functional impact.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/runtime.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Seems reasonable to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH] PM: runtime: Return -EINPROGRESS from rpm_resume() in the RPM_NOWAIT case

Posted by Alan Stern 3 years, 6 months ago

On Thu, Sep 22, 2022 at 08:04:40PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The prospective callers of rpm_resume() passing RPM_NOWAIT to it may
> be confused when it returns 0 without actually resuming the device
> which may happen if the device is suspending at the given time and it
> will only resume when the suspend in progress has completed.  To avoid
> that confusion, return -EINPROGRESS from rpm_resume() in that case.
> 
> Since none of the current callers passing RPM_NOWAIT to rpm_resume()
> check its return value, this change has no functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/base/power/runtime.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -792,10 +792,13 @@ static int rpm_resume(struct device *dev
>  		DEFINE_WAIT(wait);
>  
>  		if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {

Hmmm, and what if a caller sets both of these flags?  I guess in that 
case he gets what he deserves.

> -			if (dev->power.runtime_status == RPM_SUSPENDING)
> +			if (dev->power.runtime_status == RPM_SUSPENDING) {
>  				dev->power.deferred_resume = true;
> -			else
> +				if (rpmflags & RPM_NOWAIT)
> +					retval = -EINPROGRESS;
> +			} else {
>  				retval = -EINPROGRESS;
> +			}
>  			goto out;
>  		}

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

Re: [PATCH] PM: runtime: Return -EINPROGRESS from rpm_resume() in the RPM_NOWAIT case

Posted by Rafael J. Wysocki 3 years, 6 months ago

On Thu, Sep 22, 2022 at 9:32 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, Sep 22, 2022 at 08:04:40PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The prospective callers of rpm_resume() passing RPM_NOWAIT to it may
> > be confused when it returns 0 without actually resuming the device
> > which may happen if the device is suspending at the given time and it
> > will only resume when the suspend in progress has completed.  To avoid
> > that confusion, return -EINPROGRESS from rpm_resume() in that case.
> >
> > Since none of the current callers passing RPM_NOWAIT to rpm_resume()
> > check its return value, this change has no functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/base/power/runtime.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > Index: linux-pm/drivers/base/power/runtime.c
> > ===================================================================
> > --- linux-pm.orig/drivers/base/power/runtime.c
> > +++ linux-pm/drivers/base/power/runtime.c
> > @@ -792,10 +792,13 @@ static int rpm_resume(struct device *dev
> >               DEFINE_WAIT(wait);
> >
> >               if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) {
>
> Hmmm, and what if a caller sets both of these flags?  I guess in that
> case he gets what he deserves.

Exactly.

> > -                     if (dev->power.runtime_status == RPM_SUSPENDING)
> > +                     if (dev->power.runtime_status == RPM_SUSPENDING) {
> >                               dev->power.deferred_resume = true;
> > -                     else
> > +                             if (rpmflags & RPM_NOWAIT)
> > +                                     retval = -EINPROGRESS;
> > +                     } else {
> >                               retval = -EINPROGRESS;
> > +                     }
> >                       goto out;
> >               }
>
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Thanks!