[PATCH] ACPI: PM: postpone bringing devices to D0 unless we need them

[PATCH] ACPI: PM: postpone bringing devices to D0 unless we need them

[Dmitry Torokhov]

Currently ACPI power domain brings devices into D0 state in the "resume
early" phase. Normally this does not cause any issues, as powering up
happens quickly. However there are peripherals that have certain timing
requirements for powering on, for example some models of Elan
touchscreens need 300msec after powering up/releasing reset line before
they can accept commands from the host. Such devices will dominate
the time spent in early resume phase and cause increase in overall
resume time as we wait for early resume to complete before we can
proceed to the normal resume stage.

There are ways for a driver to indicate that it can tolerate device
being in the low power mode and that it knows how to power the device
back up when resuming, bit that requires changes to individual drivers
that may not really care about details of ACPI controlled power
management.

This change attempts to solve this issue at ACPI power domain level, by
postponing powering up device until we get to the normal resume stage,
unless there is early resume handler defined for the device, or device
does not declare any resume handlers, in which case we continue powering
up such devices early. This allows us to shave off several hundred
milliseconds of resume time on affected systems.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/acpi/device_pm.c | 46 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 096153761ebc..00b412ccb2e0 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1131,17 +1131,52 @@ static int acpi_subsys_resume_noirq(struct device *dev)
  *
  * Use ACPI to put the given device into the full-power state and carry out the
  * generic early resume procedure for it during system transition into the
- * working state.
+ * working state, but only do that if device either defines early resume
+ * handler, or does not define power operations at all. Otherwise powering up
+ * of the device is postponed to the normal resume phase.
  */
 static int acpi_subsys_resume_early(struct device *dev)
 {
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
 	int ret;
 
-	if (dev_pm_skip_resume(dev))
-		return 0;
+	if (dev_pm_skip_resume(dev)) {
+		ret = 0;
+	} else if (!pm || pm->resume_early) {
+		ret = acpi_dev_resume(dev);
+		if (!ret)
+			ret = pm_generic_resume_early(dev);
+	} else {
+		if (adev)
+			acpi_device_wakeup_disable(adev);
+
+		dev_dbg(dev, "postponing D0 transition to normal resume stage\n");
+		ret = 0;
+	}
+
+	return ret;
+}
+
+/**
+ * acpi_subsys_resume - Resume device using ACPI.
+ * @dev: Device to Resume.
+ *
+ * Use ACPI to put the given device into the full-power state if it has not been
+ * powered up during early resume phase, and carry out the generic resume
+ * procedure for it during system transition into the working state.
+ */
+static int acpi_subsys_resume(struct device *dev)
+{
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int ret = 0;
+
+	if (!dev_pm_skip_resume(dev) && pm && !pm->resume_early) {
+		dev_dbg(dev, "executing postponed D0 transition\n");
+		ret = acpi_dev_resume(dev);
+	}
 
-	ret = acpi_dev_resume(dev);
-	return ret ? ret : pm_generic_resume_early(dev);
+	return ret ? ret : pm_generic_resume(dev);
 }
 
 /**
@@ -1236,6 +1271,7 @@ static struct dev_pm_domain acpi_general_pm_domain = {
 		.prepare = acpi_subsys_prepare,
 		.complete = acpi_subsys_complete,
 		.suspend = acpi_subsys_suspend,
+		.resume = acpi_subsys_resume,
 		.suspend_late = acpi_subsys_suspend_late,
 		.suspend_noirq = acpi_subsys_suspend_noirq,
 		.resume_noirq = acpi_subsys_resume_noirq,
-- 
2.32.0.288.g62a8d224e6-goog


-- 
Dmitry

Re: [PATCH] ACPI: PM: postpone bringing devices to D0 unless we need them

[Rafael J. Wysocki]

On Tue, Jun 22, 2021 at 12:20 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Currently ACPI power domain brings devices into D0 state in the "resume
> early" phase. Normally this does not cause any issues, as powering up
> happens quickly. However there are peripherals that have certain timing
> requirements for powering on, for example some models of Elan
> touchscreens need 300msec after powering up/releasing reset line before
> they can accept commands from the host. Such devices will dominate
> the time spent in early resume phase and cause increase in overall
> resume time as we wait for early resume to complete before we can
> proceed to the normal resume stage.
>
> There are ways for a driver to indicate that it can tolerate device
> being in the low power mode and that it knows how to power the device
> back up when resuming, bit that requires changes to individual drivers
> that may not really care about details of ACPI controlled power
> management.
>
> This change attempts to solve this issue at ACPI power domain level, by
> postponing powering up device until we get to the normal resume stage,
> unless there is early resume handler defined for the device, or device
> does not declare any resume handlers, in which case we continue powering
> up such devices early. This allows us to shave off several hundred
> milliseconds of resume time on affected systems.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/acpi/device_pm.c | 46 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 096153761ebc..00b412ccb2e0 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1131,17 +1131,52 @@ static int acpi_subsys_resume_noirq(struct device *dev)
>   *
>   * Use ACPI to put the given device into the full-power state and carry out the
>   * generic early resume procedure for it during system transition into the
> - * working state.
> + * working state, but only do that if device either defines early resume
> + * handler, or does not define power operations at all. Otherwise powering up
> + * of the device is postponed to the normal resume phase.
>   */
>  static int acpi_subsys_resume_early(struct device *dev)
>  {
> +       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       struct acpi_device *adev = ACPI_COMPANION(dev);
>         int ret;
>
> -       if (dev_pm_skip_resume(dev))
> -               return 0;

The above doesn't need to be changed AFAICS.

> +       if (dev_pm_skip_resume(dev)) {
> +               ret = 0;
> +       } else if (!pm || pm->resume_early) {

This is rather tricky, but I don't see a particular reason why it wouldn't work.

> +               ret = acpi_dev_resume(dev);
> +               if (!ret)
> +                       ret = pm_generic_resume_early(dev);
> +       } else {
> +               if (adev)
> +                       acpi_device_wakeup_disable(adev);

This isn't necessary here.

> +
> +               dev_dbg(dev, "postponing D0 transition to normal resume stage\n");
> +               ret = 0;
> +       }
> +
> +       return ret;
> +}
> +
> +/**
> + * acpi_subsys_resume - Resume device using ACPI.
> + * @dev: Device to Resume.
> + *
> + * Use ACPI to put the given device into the full-power state if it has not been
> + * powered up during early resume phase, and carry out the generic resume
> + * procedure for it during system transition into the working state.
> + */
> +static int acpi_subsys_resume(struct device *dev)
> +{
> +       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       int ret = 0;
> +
> +       if (!dev_pm_skip_resume(dev) && pm && !pm->resume_early) {
> +               dev_dbg(dev, "executing postponed D0 transition\n");
> +               ret = acpi_dev_resume(dev);
> +       }
>
> -       ret = acpi_dev_resume(dev);
> -       return ret ? ret : pm_generic_resume_early(dev);
> +       return ret ? ret : pm_generic_resume(dev);
>  }
>
>  /**
> @@ -1236,6 +1271,7 @@ static struct dev_pm_domain acpi_general_pm_domain = {
>                 .prepare = acpi_subsys_prepare,
>                 .complete = acpi_subsys_complete,
>                 .suspend = acpi_subsys_suspend,
> +               .resume = acpi_subsys_resume,
>                 .suspend_late = acpi_subsys_suspend_late,
>                 .suspend_noirq = acpi_subsys_suspend_noirq,
>                 .resume_noirq = acpi_subsys_resume_noirq,
> --

Re: [PATCH] ACPI: PM: postpone bringing devices to D0 unless we need them

[Dmitry Torokhov]

Hi Rafael,

On Tue, Jun 22, 2021 at 03:40:05PM +0200, Rafael J. Wysocki wrote:
> On Tue, Jun 22, 2021 at 12:20 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Currently ACPI power domain brings devices into D0 state in the "resume
> > early" phase. Normally this does not cause any issues, as powering up
> > happens quickly. However there are peripherals that have certain timing
> > requirements for powering on, for example some models of Elan
> > touchscreens need 300msec after powering up/releasing reset line before
> > they can accept commands from the host. Such devices will dominate
> > the time spent in early resume phase and cause increase in overall
> > resume time as we wait for early resume to complete before we can
> > proceed to the normal resume stage.
> >
> > There are ways for a driver to indicate that it can tolerate device
> > being in the low power mode and that it knows how to power the device
> > back up when resuming, bit that requires changes to individual drivers
> > that may not really care about details of ACPI controlled power
> > management.
> >
> > This change attempts to solve this issue at ACPI power domain level, by
> > postponing powering up device until we get to the normal resume stage,
> > unless there is early resume handler defined for the device, or device
> > does not declare any resume handlers, in which case we continue powering
> > up such devices early. This allows us to shave off several hundred
> > milliseconds of resume time on affected systems.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/acpi/device_pm.c | 46 +++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 41 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index 096153761ebc..00b412ccb2e0 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -1131,17 +1131,52 @@ static int acpi_subsys_resume_noirq(struct device *dev)
> >   *
> >   * Use ACPI to put the given device into the full-power state and carry out the
> >   * generic early resume procedure for it during system transition into the
> > - * working state.
> > + * working state, but only do that if device either defines early resume
> > + * handler, or does not define power operations at all. Otherwise powering up
> > + * of the device is postponed to the normal resume phase.
> >   */
> >  static int acpi_subsys_resume_early(struct device *dev)
> >  {
> > +       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +       struct acpi_device *adev = ACPI_COMPANION(dev);
> >         int ret;
> >
> > -       if (dev_pm_skip_resume(dev))
> > -               return 0;
> 
> The above doesn't need to be changed AFAICS.

I was trying to have if string if/else if/else, but I can keep it as it
was.

> 
> > +       if (dev_pm_skip_resume(dev)) {
> > +               ret = 0;
> > +       } else if (!pm || pm->resume_early) {
> 
> This is rather tricky, but I don't see a particular reason why it wouldn't work.
> 
> > +               ret = acpi_dev_resume(dev);
> > +               if (!ret)
> > +                       ret = pm_generic_resume_early(dev);
> > +       } else {
> > +               if (adev)
> > +                       acpi_device_wakeup_disable(adev);
> 
> This isn't necessary here.

Just to confirm - you are saying that disabling the device as a wakeup
source can be safely postponed till the normal resume stage? I was
trying to keep as much of the original behavior as possible and this is
a part of acpi_dev_resume() that we are now postponing.

Thanks.

-- 
Dmitry

Re: [PATCH] ACPI: PM: postpone bringing devices to D0 unless we need them

[Rafael J. Wysocki]

Hi,

On Tue, Jun 22, 2021 at 8:13 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Rafael,
>
> On Tue, Jun 22, 2021 at 03:40:05PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Jun 22, 2021 at 12:20 AM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Currently ACPI power domain brings devices into D0 state in the "resume
> > > early" phase. Normally this does not cause any issues, as powering up
> > > happens quickly. However there are peripherals that have certain timing
> > > requirements for powering on, for example some models of Elan
> > > touchscreens need 300msec after powering up/releasing reset line before
> > > they can accept commands from the host. Such devices will dominate
> > > the time spent in early resume phase and cause increase in overall
> > > resume time as we wait for early resume to complete before we can
> > > proceed to the normal resume stage.
> > >
> > > There are ways for a driver to indicate that it can tolerate device
> > > being in the low power mode and that it knows how to power the device
> > > back up when resuming, bit that requires changes to individual drivers
> > > that may not really care about details of ACPI controlled power
> > > management.
> > >
> > > This change attempts to solve this issue at ACPI power domain level, by
> > > postponing powering up device until we get to the normal resume stage,
> > > unless there is early resume handler defined for the device, or device
> > > does not declare any resume handlers, in which case we continue powering
> > > up such devices early. This allows us to shave off several hundred
> > > milliseconds of resume time on affected systems.
> > >
> > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > ---
> > >  drivers/acpi/device_pm.c | 46 +++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 41 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > > index 096153761ebc..00b412ccb2e0 100644
> > > --- a/drivers/acpi/device_pm.c
> > > +++ b/drivers/acpi/device_pm.c
> > > @@ -1131,17 +1131,52 @@ static int acpi_subsys_resume_noirq(struct device *dev)
> > >   *
> > >   * Use ACPI to put the given device into the full-power state and carry out the
> > >   * generic early resume procedure for it during system transition into the
> > > - * working state.
> > > + * working state, but only do that if device either defines early resume
> > > + * handler, or does not define power operations at all. Otherwise powering up
> > > + * of the device is postponed to the normal resume phase.
> > >   */
> > >  static int acpi_subsys_resume_early(struct device *dev)
> > >  {
> > > +       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > > +       struct acpi_device *adev = ACPI_COMPANION(dev);
> > >         int ret;
> > >
> > > -       if (dev_pm_skip_resume(dev))
> > > -               return 0;
> >
> > The above doesn't need to be changed AFAICS.
>
> I was trying to have if string if/else if/else, but I can keep it as it
> was.
>
> >
> > > +       if (dev_pm_skip_resume(dev)) {
> > > +               ret = 0;
> > > +       } else if (!pm || pm->resume_early) {
> >
> > This is rather tricky, but I don't see a particular reason why it wouldn't work.
> >
> > > +               ret = acpi_dev_resume(dev);
> > > +               if (!ret)
> > > +                       ret = pm_generic_resume_early(dev);
> > > +       } else {
> > > +               if (adev)
> > > +                       acpi_device_wakeup_disable(adev);
> >
> > This isn't necessary here.
>
> Just to confirm - you are saying that disabling the device as a wakeup
> source can be safely postponed till the normal resume stage?

Yes, it should be safe.  Moreover, it may be unsafe to change the
ordering between acpi_dev_pm_full_power() and
acpi_device_wakeup_disable().

> I was trying to keep as much of the original behavior as possible and this is
> a part of acpi_dev_resume() that we are now postponing.

I would postpone the whole thing.

Thanks!