| Message ID | 20241114220921.2529905-2-saravanak@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | Optimize async device suspend/resume | expand |
On Thu, Nov 14, 2024 at 02:09:15PM -0800, Saravana Kannan wrote: > Some devices might have their is_suspended flag set to false. In these > cases, dpm_resume() should skip doing anything for those devices. > However, runtime PM enable and a few others steps are done before > checking for this flag. Fix it so that we do things in the right order. > > Signed-off-by: Saravana Kannan <saravanak@google.com> This looks like a nice generic fix as well, should it go to older kernels? thanks, greg k-h
On Fri, Nov 15, 2024 at 11:43 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Nov 14, 2024 at 02:09:15PM -0800, Saravana Kannan wrote: > > Some devices might have their is_suspended flag set to false. In these > > cases, dpm_resume() should skip doing anything for those devices. > > However, runtime PM enable and a few others steps are done before > > checking for this flag. Fix it so that we do things in the right order. > > > > Signed-off-by: Saravana Kannan <saravanak@google.com> > > This looks like a nice generic fix as well, should it go to older > kernels? Yeah, it's meant to be a generic fix. But I'll feel better about it if someone familiar with this code can give it a Reviewed-by. And as I look at it... I have a bug in there. I think it should be goto Complete and not Unlock! No idea how my devices didn't get freed after a few suspend aborts! I can send it out as a separate patch too if you want. And depending on when it lands, I can keep it or drop it from v2 of this series. Thanks, Saravana
Trim CC list. On Thu, Nov 14, 2024 at 11:09 PM Saravana Kannan <saravanak@google.com> wrote: > > Some devices might have their is_suspended flag set to false. In these > cases, dpm_resume() should skip doing anything for those devices. Not really. This is particularly untrue for devices with power.direct_complete set that have power.is_suspended clear. > However, runtime PM enable and a few others steps are done before > checking for this flag. Fix it so that we do things in the right order. I don't see the bug this is fixing, but I can see bugs introduced by it. I think that you want power.is_suspended to be checked before waiting for the superiors. Fair enough, since for devices with power.is_suspended clear, there should be no superiors to wait for, so the two checks can be done in any order and checking power.is_suspended first would be less overhead. And that's it AFAICS. > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/base/power/main.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 4a67e83300e1..86e51b9fefab 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -913,6 +913,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async) > if (dev->power.syscore) > goto Complete; > > + if (!dev->power.is_suspended) > + goto Unlock; > + > if (dev->power.direct_complete) { > /* Match the pm_runtime_disable() in __device_suspend(). */ > pm_runtime_enable(dev); > @@ -931,9 +934,6 @@ static void device_resume(struct device *dev, pm_message_t state, bool async) > */ > dev->power.is_prepared = false; > > - if (!dev->power.is_suspended) > - goto Unlock; > - > if (dev->pm_domain) { > info = "power domain "; > callback = pm_op(&dev->pm_domain->ops, state); > -- > 2.47.0.338.g60cca15819-goog >
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 4a67e83300e1..86e51b9fefab 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -913,6 +913,9 @@ static void device_resume(struct device *dev, pm_message_t state, bool async) if (dev->power.syscore) goto Complete; + if (!dev->power.is_suspended) + goto Unlock; + if (dev->power.direct_complete) { /* Match the pm_runtime_disable() in __device_suspend(). */ pm_runtime_enable(dev); @@ -931,9 +934,6 @@ static void device_resume(struct device *dev, pm_message_t state, bool async) */ dev->power.is_prepared = false; - if (!dev->power.is_suspended) - goto Unlock; - if (dev->pm_domain) { info = "power domain "; callback = pm_op(&dev->pm_domain->ops, state);
Some devices might have their is_suspended flag set to false. In these cases, dpm_resume() should skip doing anything for those devices. However, runtime PM enable and a few others steps are done before checking for this flag. Fix it so that we do things in the right order. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/base/power/main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)