* [PATCH] drivers/acpi: Turn off power resources while entering S5
@ 2019-04-23 23:39 Furquan Shaikh
2019-04-23 23:39 ` Furquan Shaikh
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Furquan Shaikh @ 2019-04-23 23:39 UTC (permalink / raw)
To: Rafael J . Wysocki, Len Brown
Cc: linux-acpi, linux-kernel, rajatja, dlaurie, furquan
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@google.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] drivers/acpi: Turn off power resources while entering S5
2019-04-23 23:39 [PATCH] drivers/acpi: Turn off power resources while entering S5 Furquan Shaikh
@ 2019-04-23 23:39 ` Furquan Shaikh
2019-05-09 6:11 ` Furquan Shaikh
2019-05-31 10:13 ` Rafael J. Wysocki
2 siblings, 0 replies; 6+ messages in thread
From: Furquan Shaikh @ 2019-04-23 23:39 UTC (permalink / raw)
To: Rafael J . Wysocki, Len Brown
Cc: linux-acpi, linux-kernel, rajatja, dlaurie, furquan
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@google.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers/acpi: Turn off power resources while entering S5
2019-04-23 23:39 [PATCH] drivers/acpi: Turn off power resources while entering S5 Furquan Shaikh
2019-04-23 23:39 ` Furquan Shaikh
@ 2019-05-09 6:11 ` Furquan Shaikh
2019-05-09 7:41 ` Rafael J. Wysocki
2019-05-31 10:13 ` Rafael J. Wysocki
2 siblings, 1 reply; 6+ messages in thread
From: Furquan Shaikh @ 2019-05-09 6:11 UTC (permalink / raw)
To: Rafael J . Wysocki, Len Brown
Cc: ACPI Devel Maling List, Linux Kernel Mailing List, Rajat Jain,
Duncan Laurie, Todd Broch
On Tue, Apr 23, 2019 at 4:39 PM Furquan Shaikh <furquan@google.com> 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@google.com>
> ---
> 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.
Thanks,
Furquan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers/acpi: Turn off power resources while entering S5
2019-05-09 6:11 ` Furquan Shaikh
@ 2019-05-09 7:41 ` Rafael J. Wysocki
0 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-05-09 7:41 UTC (permalink / raw)
To: Furquan Shaikh
Cc: Rafael J . Wysocki, Len Brown, ACPI Devel Maling List,
Linux Kernel Mailing List, Rajat Jain, Duncan Laurie, Todd Broch
On Thu, May 9, 2019 at 8:12 AM Furquan Shaikh <furquan@google.com> wrote:
>
> On Tue, Apr 23, 2019 at 4:39 PM Furquan Shaikh <furquan@google.com> 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@google.com>
> > ---
> > 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers/acpi: Turn off power resources while entering S5
2019-04-23 23:39 [PATCH] drivers/acpi: Turn off power resources while entering S5 Furquan Shaikh
2019-04-23 23:39 ` Furquan Shaikh
2019-05-09 6:11 ` Furquan Shaikh
@ 2019-05-31 10:13 ` Rafael J. Wysocki
2019-06-14 2:08 ` Furquan Shaikh
2 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2019-05-31 10:13 UTC (permalink / raw)
To: Furquan Shaikh; +Cc: Len Brown, linux-acpi, linux-kernel, rajatja, dlaurie
On Wednesday, April 24, 2019 1:39:04 AM CEST Furquan Shaikh 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.
I'm not sure I can follow you here. Any details?
> 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.
Well, I would say that this is not just about power resources. ACPI PM methods should be
invoked for the devices as well, so generally there should be subject to the normal PM
during S5 transitions.
Generally speaking, S5 should follow the code flow of the last phase of hibernation.
> 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.
Quite frankly, this looks like a hack causing a particular platform to behave
as expected, but it very well may not be applicable to other platforms.
> Signed-off-by: Furquan Shaikh <furquan@google.com>
> ---
> 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;
Instead of doing this you could add a routing decrementing the recfount for all of
the power resources in the ON state and call that, before the original
acpi_turn_off_unused_power_resources(), in acpi_power_off_prepare().
That said I don't think the approach is valid in general as stated above.
> + } 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)
> {
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drivers/acpi: Turn off power resources while entering S5
2019-05-31 10:13 ` Rafael J. Wysocki
@ 2019-06-14 2:08 ` Furquan Shaikh
0 siblings, 0 replies; 6+ messages in thread
From: Furquan Shaikh @ 2019-06-14 2:08 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, ACPI Devel Maling List, Linux Kernel Mailing List,
Rajat Jain, Duncan Laurie
On Fri, May 31, 2019 at 3:13 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Wednesday, April 24, 2019 1:39:04 AM CEST Furquan Shaikh 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.
>
> I'm not sure I can follow you here. Any details?
On the platforms that I am currently testing, I noticed that ACPI _OFF
routine does not get called for the touchscreen I2C device. Since the
device has power-on and power-down sequencing requirements, not
calling _OFF routine results in its power-down sequence being violated
when the system is entering S5. On further debug, I identified that
this is true for all the ACPI devices on the platform i.e. _OFF
routine does not get called when entering S5.
In case of S3/S0ix, I see that the _OFF ACPI routine gets called using
the following code flow:
pm_suspend()
...
--> suspend_devices_and_enter()
...
--> dpm_suspend() for every device
...
--> acpi_power_off() for every ACPI device
On the other hand, in case of S5, _OFF routine never gets called
because of the following code flow:
kernel_poweroff()
...
--> acpi_power_off_prepare()
--> acpi_sleep_prepare()
--> acpi_disable_all_gpes()
--> acpi_os_wait_events_complete()
...
--> machine_power_off()
In this sequence acpi_power_off is not executed for the 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.
>
> Well, I would say that this is not just about power resources. ACPI PM methods should be
> invoked for the devices as well, so generally there should be subject to the normal PM
> during S5 transitions.
Agree.
>
> Generally speaking, S5 should follow the code flow of the last phase of hibernation.
By last phase of hibernation, do you mean HIBERNATION_SHUTDOWN?
My platforms do not support hibernation, so I can't really test it,
but from my reading of the code sequence below:
hibernate()
...
--> power_down()
...
It seems the "last phase of hibernation" i.e. HIBERNATION_SHUTDOWN is
also going to have the same issue i.e. it will not lead to calling the
ACPI _OFF routine.
HIBERNATION_PLATFORM which calls hibernation_platform_enter() actually
makes calls to dpm_suspend() and dpm_suspend_end() which will end up
calling acpi_power_off.
I'm sure I'm missing something here as I'm not too familiar with this
code. Can you please help me by pointing me to the code sequence you
expect should result in getting the _OFF ACPI routine called for:
(1) Hibernation
(2) Shutdown (S5)
Many Thanks!
>
> > 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.
>
> Quite frankly, this looks like a hack causing a particular platform to behave
> as expected, but it very well may not be applicable to other platforms.
>
> > Signed-off-by: Furquan Shaikh <furquan@google.com>
> > ---
> > 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;
>
> Instead of doing this you could add a routing decrementing the recfount for all of
> the power resources in the ON state and call that, before the original
> acpi_turn_off_unused_power_resources(), in acpi_power_off_prepare().
>
> That said I don't think the approach is valid in general as stated above.
>
> > + } 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)
> > {
> >
>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-14 2:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-23 23:39 [PATCH] drivers/acpi: Turn off power resources while entering S5 Furquan Shaikh
2019-04-23 23:39 ` Furquan Shaikh
2019-05-09 6:11 ` Furquan Shaikh
2019-05-09 7:41 ` Rafael J. Wysocki
2019-05-31 10:13 ` Rafael J. Wysocki
2019-06-14 2:08 ` Furquan Shaikh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).