From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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;
}
On Thu, Sep 22, 2022 at 08:04:40PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
Hi,
On Thu, Sep 22, 2022 at 11:04 AM Rafael J. Wysocki <[email protected]> wrote:
>
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/base/power/runtime.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
Seems reasonable to me.
Reviewed-by: Douglas Anderson <[email protected]>
On Thu, Sep 22, 2022 at 9:32 PM Alan Stern <[email protected]> wrote:
>
> On Thu, Sep 22, 2022 at 08:04:40PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks!
On Thu, 22 Sept 2022 at 20:04, Rafael J. Wysocki <[email protected]> wrote:
>
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
Looks good to me!
Reviewed-by: Ulf Hansson <[email protected]>
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;
> }
>
>
>
>
On Fri, Sep 23, 2022 at 3:26 PM Ulf Hansson <[email protected]> wrote:
>
> On Thu, 22 Sept 2022 at 20:04, Rafael J. Wysocki <[email protected]> wrote:
> >
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
>
> Looks good to me!
>
> Reviewed-by: Ulf Hansson <[email protected]>
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;
> > }
> >
> >
> >
> >
On Fri, 23 Sept 2022 at 17:53, Rafael J. Wysocki <[email protected]> wrote:
>
> On Fri, Sep 23, 2022 at 3:26 PM Ulf Hansson <[email protected]> wrote:
> >
> > On Thu, 22 Sept 2022 at 20:04, Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > 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