| Message ID | 12079576.O9o76ZdvQC@kreacher (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | PM: runtime: Return -EINPROGRESS from rpm_resume() in the RPM_NOWAIT case | expand |
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>
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!
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>
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; > } > > > >
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; > > } > > > > > > > >
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
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; }