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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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,
> --