| Message ID | 6238546.EnOBWIOf9o@aspire.rjw.lan (mailing list archive) |
|---|---|
| State | Mainlined |
| Delegated to: | Rafael Wysocki |
| Headers | show |
Hello, thanks for for your effort and the patch. Is this eligible for stable? Best regards Am 22.05.2018 um 13:02 schrieb Rafael J. Wysocki: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE > driver flags) inadvertently prevented the power.direct_complete flag > from being set for devices without PM callbacks and with disabled > runtime PM which also prevents power.direct_complete from being set > for their parents. That led to problems including a resume crash on > HP ZBook 14u. > > Restore the previous behavior by causing power.direct_complete to be > set for those devices again, but do that in a more direct way to > avoid overlooking that case in the future. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693 > Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) > Reported-by: Thomas Martitz <kugel@rockbox.org> > Tested-by: Thomas Martitz <kugel@rockbox.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/base/power/main.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/drivers/base/power/main.c > @@ -1920,10 +1920,8 @@ static int device_prepare(struct device > > dev->power.wakeup_path = false; > > - if (dev->power.no_pm_callbacks) { > - ret = 1; /* Let device go direct_complete */ > + if (dev->power.no_pm_callbacks) > goto unlock; > - } > > if (dev->pm_domain) > callback = dev->pm_domain->ops.prepare; > @@ -1957,7 +1955,8 @@ unlock: > */ > spin_lock_irq(&dev->power.lock); > dev->power.direct_complete = state.event == PM_EVENT_SUSPEND && > - pm_runtime_suspended(dev) && ret > 0 && > + ((pm_runtime_suspended(dev) && ret > 0) || > + dev->power.no_pm_callbacks) && > !dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP); > spin_unlock_irq(&dev->power.lock); > return 0; >
On Tuesday, May 22, 2018 1:12:40 PM CEST Thomas Martitz wrote: > Hello, > > thanks for for your effort and the patch. > > Is this eligible for stable? Yes, it is. Thanks, Rafael > Am 22.05.2018 um 13:02 schrieb Rafael J. Wysocki: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE > > driver flags) inadvertently prevented the power.direct_complete flag > > from being set for devices without PM callbacks and with disabled > > runtime PM which also prevents power.direct_complete from being set > > for their parents. That led to problems including a resume crash on > > HP ZBook 14u. > > > > Restore the previous behavior by causing power.direct_complete to be > > set for those devices again, but do that in a more direct way to > > avoid overlooking that case in the future. > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693 > > Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) > > Reported-by: Thomas Martitz <kugel@rockbox.org> > > Tested-by: Thomas Martitz <kugel@rockbox.org> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/base/power/main.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > Index: linux-pm/drivers/base/power/main.c > > =================================================================== > > --- linux-pm.orig/drivers/base/power/main.c > > +++ linux-pm/drivers/base/power/main.c > > @@ -1920,10 +1920,8 @@ static int device_prepare(struct device > > > > dev->power.wakeup_path = false; > > > > - if (dev->power.no_pm_callbacks) { > > - ret = 1; /* Let device go direct_complete */ > > + if (dev->power.no_pm_callbacks) > > goto unlock; > > - } > > > > if (dev->pm_domain) > > callback = dev->pm_domain->ops.prepare; > > @@ -1957,7 +1955,8 @@ unlock: > > */ > > spin_lock_irq(&dev->power.lock); > > dev->power.direct_complete = state.event == PM_EVENT_SUSPEND && > > - pm_runtime_suspended(dev) && ret > 0 && > > + ((pm_runtime_suspended(dev) && ret > 0) || > > + dev->power.no_pm_callbacks) && > > !dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP); > > spin_unlock_irq(&dev->power.lock); > > return 0; > > > >
On 22 May 2018 at 13:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE > driver flags) inadvertently prevented the power.direct_complete flag > from being set for devices without PM callbacks and with disabled > runtime PM which also prevents power.direct_complete from being set > for their parents. That led to problems including a resume crash on > HP ZBook 14u. > > Restore the previous behavior by causing power.direct_complete to be > set for those devices again, but do that in a more direct way to > avoid overlooking that case in the future. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693 > Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) > Reported-by: Thomas Martitz <kugel@rockbox.org> > Tested-by: Thomas Martitz <kugel@rockbox.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> It seems like the resume path of HP ZBook 14u is kind of fragile, in case it *requires* dev->power.direct_complete to be set for devices like these. And that makes me wonder, that perhaps we should try to address that issue as well, no? In either way: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/main.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > Index: linux-pm/drivers/base/power/main.c > =================================================================== > --- linux-pm.orig/drivers/base/power/main.c > +++ linux-pm/drivers/base/power/main.c > @@ -1920,10 +1920,8 @@ static int device_prepare(struct device > > dev->power.wakeup_path = false; > > - if (dev->power.no_pm_callbacks) { > - ret = 1; /* Let device go direct_complete */ > + if (dev->power.no_pm_callbacks) > goto unlock; > - } > > if (dev->pm_domain) > callback = dev->pm_domain->ops.prepare; > @@ -1957,7 +1955,8 @@ unlock: > */ > spin_lock_irq(&dev->power.lock); > dev->power.direct_complete = state.event == PM_EVENT_SUSPEND && > - pm_runtime_suspended(dev) && ret > 0 && > + ((pm_runtime_suspended(dev) && ret > 0) || > + dev->power.no_pm_callbacks) && > !dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP); > spin_unlock_irq(&dev->power.lock); > return 0; >
On Tue, May 22, 2018 at 01:02:17PM +0200, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE > driver flags) inadvertently prevented the power.direct_complete flag > from being set for devices without PM callbacks and with disabled > runtime PM which also prevents power.direct_complete from being set > for their parents. That led to problems including a resume crash on > HP ZBook 14u. > > Restore the previous behavior by causing power.direct_complete to be > set for those devices again, but do that in a more direct way to > avoid overlooking that case in the future. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693 > Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) I stumbled over this the other day as well and tracked it down to the above commit. In my case (child devices to serdev clients), this was mostly benign although it did prevent the direct-complete optimisation during suspend. Never got around to reporting or fixing it myself, but your analysis and fix matches my initial findings. > Reported-by: Thomas Martitz <kugel@rockbox.org> > Tested-by: Thomas Martitz <kugel@rockbox.org> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Reviewed-by: Johan Hovold <johan@kernel.org> As already suggested elsewhere in the thread, I think a stable is warranted too. Thanks, Johan
On Tuesday, May 22, 2018 1:41:06 PM CEST Ulf Hansson wrote: > On 22 May 2018 at 13:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Commit 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE > > driver flags) inadvertently prevented the power.direct_complete flag > > from being set for devices without PM callbacks and with disabled > > runtime PM which also prevents power.direct_complete from being set > > for their parents. That led to problems including a resume crash on > > HP ZBook 14u. > > > > Restore the previous behavior by causing power.direct_complete to be > > set for those devices again, but do that in a more direct way to > > avoid overlooking that case in the future. > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=199693 > > Fixes: 08810a4119aa (PM / core: Add NEVER_SKIP and SMART_PREPARE driver flags) > > Reported-by: Thomas Martitz <kugel@rockbox.org> > > Tested-by: Thomas Martitz <kugel@rockbox.org> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > It seems like the resume path of HP ZBook 14u is kind of fragile, Yes, it is. > in case it *requires* dev->power.direct_complete to be set for devices > like these. And that makes me wonder, that perhaps we should try to > address that issue as well, no? Yes, in principle. But since the direct_complete handling needs to be fixed anyway, it doesn't matter a lot in practice, because the resume issue on HP ZBook 14u will not be reproducible anyway then. And since the dependency clearly is on a device with no callbacks, I'm not worried too much about that.
Index: linux-pm/drivers/base/power/main.c =================================================================== --- linux-pm.orig/drivers/base/power/main.c +++ linux-pm/drivers/base/power/main.c @@ -1920,10 +1920,8 @@ static int device_prepare(struct device dev->power.wakeup_path = false; - if (dev->power.no_pm_callbacks) { - ret = 1; /* Let device go direct_complete */ + if (dev->power.no_pm_callbacks) goto unlock; - } if (dev->pm_domain) callback = dev->pm_domain->ops.prepare; @@ -1957,7 +1955,8 @@ unlock: */ spin_lock_irq(&dev->power.lock); dev->power.direct_complete = state.event == PM_EVENT_SUSPEND && - pm_runtime_suspended(dev) && ret > 0 && + ((pm_runtime_suspended(dev) && ret > 0) || + dev->power.no_pm_callbacks) && !dev_pm_test_driver_flags(dev, DPM_FLAG_NEVER_SKIP); spin_unlock_irq(&dev->power.lock); return 0;