Re: [PATCH] drivers/acpi: Turn off power resources while entering S5

[Rafael J. Wysocki]

On Thu, May 9, 2019 at 8:12 AM Furquan Shaikh <furquan@xxxxxxxxxx> wrote:
>
> On Tue, Apr 23, 2019 at 4:39 PM Furquan Shaikh <furquan@xxxxxxxxxx> wrote:
> >
> > During boot-up, ACPI bus scan enables all power resources so that
> > respective device drivers can talk to their device. This causes acpi
> > ref_count for the power resource to be incremented to 1. When system
> > suspends (S3/S0ix) or hibernates(S4), DPM is responsible for calling
> > power off on all power resources whose ref_count is 1 i.e. no other
> > user of this power resource and thus resulting in _OFF routine being
> > called for it.
> >
> > However, in case of poweroff, DPM is not involved and so the power
> > resources are left on when the system is entering S5. This results in
> > the violation of power down sequencing for certain devices
> > e.g. touchscreen or digitizer I2C devices. In order to ensure that the
> > power down sequencing does the right thing, it is necessary for ACPI
> > to disable all power resources while preparing for S5.
> >
> > This change updates the function acpi_turn_off_unused_power_resources
> > to accept a parameter acpi_system_state, This function turns off power
> > resources according to the targeted system ACPI state:
> > 1. For S0: Unused power resources are turned off i.e. power resources
> > whose ref_count is already 0.
> > 2. For S5: ref_count is decremented first to undo the increment
> > performed during ACPI bus scan and then power resources with ref_count
> > 0 are turned off.
> > 3. All other suspend/hibernate states: No action is required since DPM
> > takes care of turning off power resources.
> >
> > This change should not affect the wake capable devices since:
> > 1. If wake capable devices are enabled before this call, their
> > refcount should be greater than 1. Thus, they won't be turned off.
> > 2. If wake capable devices are not enabled yet when this call is made,
> > they would eventually get turned on by call to
> > acpi_enable_wakeup_devices.
> >
> > Signed-off-by: Furquan Shaikh <furquan@xxxxxxxxxx>
> > ---
> >  drivers/acpi/power.c | 47 ++++++++++++++++++++++++++++++++++++++------
> >  drivers/acpi/sleep.c |  5 ++++-
> >  drivers/acpi/sleep.h |  2 +-
> >  3 files changed, 46 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> > index 665e93ca0b40f..945db762861a3 100644
> > --- a/drivers/acpi/power.c
> > +++ b/drivers/acpi/power.c
> > @@ -889,10 +889,42 @@ void acpi_resume_power_resources(void)
> >
> >         mutex_unlock(&power_resource_list_lock);
> >  }
> > +#endif
> >
> > -void acpi_turn_off_unused_power_resources(void)
> > +/**
> > + * acpi_turn_off_power_resources: This function is used to turn off power
> > + * resources in provided ACPI system state.
> > + *
> > + * Behavior differs based on the target system state:
> > + * ACPI_STATE_S0: Turn off unused power resources i.e. turn off power resources
> > + *                with ref_count zero.
> > + * ACPI_STATE_S5: Decrement ref_count first and turn off power resources with
> > + *                ref_count zero. This is done to ensure that the ref_count
> > + *                incremented during ACPI bus scan is undone and any power
> > + *                resources that are not required during S5 are turned off.
> > + * ACPI_STATE_Sx: No action required. DPM is responsible for turning off power
> > + *                resources while suspending/hibernating.
> > + */
> > +void acpi_turn_off_power_resources(int acpi_system_state)
> >  {
> >         struct acpi_power_resource *resource;
> > +       int decrement;
> > +
> > +       if (acpi_system_state == ACPI_STATE_S0) {
> > +               /*
> > +                * In case of ACPI_STATE_S0, turn off only unused power
> > +                * resources. So, no need to decrement ref_count.
> > +                */
> > +               decrement = 0;
> > +       } else if (acpi_system_state == ACPI_STATE_S5) {
> > +               /*
> > +                * In case of ACPI_STATE_S5, ref_count needs to be decremented
> > +                * first before checking if it is okay to power off the
> > +                * resource.
> > +                */
> > +               decrement = 1;
> > +       } else
> > +               return;
> >
> >         mutex_lock(&power_resource_list_lock);
> >
> > @@ -907,10 +939,14 @@ void acpi_turn_off_unused_power_resources(void)
> >                         continue;
> >                 }
> >
> > -               if (state == ACPI_POWER_RESOURCE_STATE_ON
> > -                   && !resource->ref_count) {
> > -                       dev_info(&resource->device.dev, "Turning OFF\n");
> > -                       __acpi_power_off(resource);
> > +               if (state == ACPI_POWER_RESOURCE_STATE_ON) {
> > +                       if (resource->ref_count)
> > +                               resource->ref_count -= decrement;
> > +
> > +                       if (!resource->ref_count) {
> > +                               dev_info(&resource->device.dev, "Turning OFF\n");
> > +                               __acpi_power_off(resource);
> > +                       }
> >                 }
> >
> >                 mutex_unlock(&resource->resource_lock);
> > @@ -918,4 +954,3 @@ void acpi_turn_off_unused_power_resources(void)
> >
> >         mutex_unlock(&power_resource_list_lock);
> >  }
> > -#endif
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index 403c4ff153498..fb6b3ca0eeb91 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -75,6 +75,9 @@ static int acpi_sleep_prepare(u32 acpi_state)
> >         printk(KERN_INFO PREFIX "Preparing to enter system sleep state S%d\n",
> >                 acpi_state);
> >         acpi_enable_wakeup_devices(acpi_state);
> > +
> > +       acpi_turn_off_power_resources(acpi_state);
> > +
> >         acpi_enter_sleep_state_prep(acpi_state);
> >         return 0;
> >  }
> > @@ -524,7 +527,7 @@ static void acpi_pm_start(u32 acpi_state)
> >   */
> >  static void acpi_pm_end(void)
> >  {
> > -       acpi_turn_off_unused_power_resources();
> > +       acpi_turn_off_power_resources(ACPI_STATE_S0);
> >         acpi_scan_lock_release();
> >         /*
> >          * This is necessary in case acpi_pm_finish() is not called during a
> > diff --git a/drivers/acpi/sleep.h b/drivers/acpi/sleep.h
> > index 41675d24a9bc0..a495c91e2bf3b 100644
> > --- a/drivers/acpi/sleep.h
> > +++ b/drivers/acpi/sleep.h
> > @@ -7,7 +7,7 @@ extern struct list_head acpi_wakeup_device_list;
> >  extern struct mutex acpi_device_lock;
> >
> >  extern void acpi_resume_power_resources(void);
> > -extern void acpi_turn_off_unused_power_resources(void);
> > +extern void acpi_turn_off_power_resources(int acpi_system_state);
> >
> >  static inline acpi_status acpi_set_waking_vector(u32 wakeup_address)
> >  {
> > --
> > 2.21.0.593.g511ec345e18-goog
> >
>
> Hi Rafael,
>
> Gentle ping. Just checking if you got a chance to look at this one. It
> is actually causing problems in one of our devices since power off
> sequencing is not honored when entering S5.

I have looked at it and I didn't like it.

I'll post my comments on this patch later.