[RFC PATCH 0/3] ACPI/Power: Fix infinite loop of runtime resume and suspend caused by turning on power resource

[RFC PATCH 0/3] ACPI/Power: Fix infinite loop of runtime resume and suspend caused by turning on power resource

[Lan Tianyu]

Currently, clearing usb port(empty port) PM Qos NO_POWER_OFF flag twice will lead
the usb port to fall into the infinite loop of runtime pm resume and
suspend. This is caused by pm_request_resume() in the acpi_power_resume_dependent().
Detail is in the following link.
http://marc.info/?l=linux-acpi&m=138182749202885&w=2

This patchset is to fix the issue. Patch 1~2 adds new state_lock
to mutex power resource's _ON, _OFF and _STA instead of resource_lock
in order to avoid dead lock when call acpi_power_get_inferred_state()
under resource_lock. Prepare for Patch 3.

Patch 3 is to make pm_request_resume() in the acpi_power_resume_dependent()
able to run just after turning on power and check the right physical device
runtime pm status. 

Lan Tianyu (3):
  ACPI/Power: Add state_lock to mutex power resource's _ON, _OFF and
    _STA
  ACPI/POWER: Rework acpi_power_get_state()
  ACPI/POWER: Call acpi_power_resume_dependent() directly in the
    acpi_power_on_unlocked()

 drivers/acpi/power.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

-- 
1.8.2.1

[RFC PATCH 1/3] ACPI/Power: Add state_lock to mutex power resource's _ON, _OFF and _STA

[Lan Tianyu]

According commit d0515d9f, power resource's _ON, _OFF and _STA should
be mutexed. This patch is to add state_lock to do this instead of
using resource_lock.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/acpi/power.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index 0c1c3ec..fa89d16 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -75,6 +75,7 @@ struct acpi_power_resource {
 	unsigned int ref_count;
 	bool wakeup_enabled;
 	struct mutex resource_lock;
+	struct mutex state_lock;
 };
 
 struct acpi_power_resource_entry {
@@ -216,9 +217,9 @@ static int acpi_power_get_list_state(struct list_head *list, int *state)
 		acpi_handle handle = resource->device.handle;
 		int result;
 
-		mutex_lock(&resource->resource_lock);
+		mutex_lock(&resource->state_lock);
 		result = acpi_power_get_state(handle, &cur_state);
-		mutex_unlock(&resource->resource_lock);
+		mutex_unlock(&resource->state_lock);
 		if (result)
 			return result;
 
@@ -263,7 +264,10 @@ static int __acpi_power_on(struct acpi_power_resource *resource)
 {
 	acpi_status status = AE_OK;
 
+	mutex_lock(&resource->state_lock);
 	status = acpi_evaluate_object(resource->device.handle, "_ON", NULL, NULL);
+	mutex_unlock(&resource->state_lock);
+
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
@@ -309,8 +313,11 @@ static int __acpi_power_off(struct acpi_power_resource *resource)
 {
 	acpi_status status;
 
+	mutex_lock(&resource->state_lock);
 	status = acpi_evaluate_object(resource->device.handle, "_OFF",
 				      NULL, NULL);
+	mutex_unlock(&resource->state_lock);
+
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
@@ -564,7 +571,9 @@ int acpi_power_wakeup_list_init(struct list_head *list, int *system_level_p)
 
 		mutex_lock(&resource->resource_lock);
 
+		mutex_lock(&resource->state_lock);
 		result = acpi_power_get_state(handle, &state);
+		mutex_unlock(&resource->state_lock);
 		if (result) {
 			mutex_unlock(&resource->resource_lock);
 			return result;
@@ -881,6 +890,7 @@ int acpi_add_power_resource(acpi_handle handle)
 	device = &resource->device;
 	acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER,
 				ACPI_STA_DEFAULT);
+	mutex_init(&resource->state_lock);
 	mutex_init(&resource->resource_lock);
 	INIT_LIST_HEAD(&resource->dependent);
 	INIT_LIST_HEAD(&resource->list_node);
@@ -935,7 +945,10 @@ void acpi_resume_power_resources(void)
 
 		mutex_lock(&resource->resource_lock);
 
+		mutex_lock(&resource->state_lock);
 		result = acpi_power_get_state(resource->device.handle, &state);
+		mutex_unlock(&resource->state_lock);
+
 		if (result) {
 			mutex_unlock(&resource->resource_lock);
 			continue;
-- 
1.8.2.1

[RFC PATCH 2/3] ACPI/POWER: Rework acpi_power_get_state()

[Lan Tianyu]

Make acpi_power_get_state() to accept struct acpi_power_resource as param
instead of acpi_handle and hold state_lock inside of acpi_power_get_state().

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/acpi/power.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index fa89d16..3fc2873 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -176,8 +176,10 @@ int acpi_extract_power_resources(union acpi_object *package, unsigned int start,
 	return err;
 }
 
-static int acpi_power_get_state(acpi_handle handle, int *state)
+static int acpi_power_get_state(struct acpi_power_resource *resource,
+				int *state)
 {
+	acpi_handle handle = resource->device.handle;
 	acpi_status status = AE_OK;
 	unsigned long long sta = 0;
 	char node_name[5];
@@ -187,7 +189,10 @@ static int acpi_power_get_state(acpi_handle handle, int *state)
 	if (!handle || !state)
 		return -EINVAL;
 
+	mutex_lock(&resource->state_lock);
 	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+	mutex_unlock(&resource->state_lock);
+
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
@@ -213,13 +218,9 @@ static int acpi_power_get_list_state(struct list_head *list, int *state)
 
 	/* The state of the list is 'on' IFF all resources are 'on'. */
 	list_for_each_entry(entry, list, node) {
-		struct acpi_power_resource *resource = entry->resource;
-		acpi_handle handle = resource->device.handle;
 		int result;
 
-		mutex_lock(&resource->state_lock);
-		result = acpi_power_get_state(handle, &cur_state);
-		mutex_unlock(&resource->state_lock);
+		result = acpi_power_get_state(entry->resource, &cur_state);
 		if (result)
 			return result;
 
@@ -565,15 +566,12 @@ int acpi_power_wakeup_list_init(struct list_head *list, int *system_level_p)
 
 	list_for_each_entry(entry, list, node) {
 		struct acpi_power_resource *resource = entry->resource;
-		acpi_handle handle = resource->device.handle;
 		int result;
 		int state;
 
 		mutex_lock(&resource->resource_lock);
 
-		mutex_lock(&resource->state_lock);
-		result = acpi_power_get_state(handle, &state);
-		mutex_unlock(&resource->state_lock);
+		result = acpi_power_get_state(entry->resource, &state);
 		if (result) {
 			mutex_unlock(&resource->resource_lock);
 			return result;
@@ -907,7 +905,7 @@ int acpi_add_power_resource(acpi_handle handle)
 	resource->system_level = acpi_object.power_resource.system_level;
 	resource->order = acpi_object.power_resource.resource_order;
 
-	result = acpi_power_get_state(handle, &state);
+	result = acpi_power_get_state(resource, &state);
 	if (result)
 		goto err;
 
@@ -945,10 +943,7 @@ void acpi_resume_power_resources(void)
 
 		mutex_lock(&resource->resource_lock);
 
-		mutex_lock(&resource->state_lock);
-		result = acpi_power_get_state(resource->device.handle, &state);
-		mutex_unlock(&resource->state_lock);
-
+		result = acpi_power_get_state(resource, &state);
 		if (result) {
 			mutex_unlock(&resource->resource_lock);
 			continue;
-- 
1.8.2.1

[RFC PATCH 3/3] ACPI/POWER: Call acpi_power_resume_dependent() directly in the acpi_power_on_unlocked()

[Lan Tianyu]

acpi_power_resume_dependent() purposes to resume power resource's dependent
physical devices after turning on the related power resource. But current
it runs in the workqueue, pm_request_resume() can't check the right
runtime pm state just after turning on power resource. This will cause
infinite loop between resume and suspend in some cases that turn on power
resource in the pm runtime resume callback(e,g usb port). This patch is to
fix the issue via calling acpi_power_resume_dependent() directly instead
of running it in a workqueue.

Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
---
 drivers/acpi/power.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index 3fc2873..5e4991b 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -62,7 +62,6 @@ ACPI_MODULE_NAME("power");
 struct acpi_power_dependent_device {
 	struct list_head node;
 	struct acpi_device *adev;
-	struct work_struct work;
 };
 
 struct acpi_power_resource {
@@ -235,14 +234,12 @@ static int acpi_power_get_list_state(struct list_head *list, int *state)
 	return 0;
 }
 
-static void acpi_power_resume_dependent(struct work_struct *work)
+static void acpi_power_resume_dependent(struct acpi_power_dependent_device *dep)
 {
-	struct acpi_power_dependent_device *dep;
 	struct acpi_device_physical_node *pn;
 	struct acpi_device *adev;
 	int state;
 
-	dep = container_of(work, struct acpi_power_dependent_device, work);
 	adev = dep->adev;
 	if (acpi_power_get_inferred_state(adev, &state))
 		return;
@@ -294,7 +291,7 @@ static int acpi_power_on_unlocked(struct acpi_power_resource *resource)
 			struct acpi_power_dependent_device *dep;
 
 			list_for_each_entry(dep, &resource->dependent, node)
-				schedule_work(&dep->work);
+				acpi_power_resume_dependent(dep);
 		}
 	}
 	return result;
@@ -414,7 +411,6 @@ static void acpi_power_add_dependent(struct acpi_power_resource *resource,
 		goto out;
 
 	dep->adev = adev;
-	INIT_WORK(&dep->work, acpi_power_resume_dependent);
 	list_add_tail(&dep->node, &resource->dependent);
 
  out:
@@ -425,23 +421,16 @@ static void acpi_power_remove_dependent(struct acpi_power_resource *resource,
 					struct acpi_device *adev)
 {
 	struct acpi_power_dependent_device *dep;
-	struct work_struct *work = NULL;
 
 	mutex_lock(&resource->resource_lock);
 
 	list_for_each_entry(dep, &resource->dependent, node)
 		if (dep->adev == adev) {
 			list_del(&dep->node);
-			work = &dep->work;
 			break;
 		}
 
 	mutex_unlock(&resource->resource_lock);
-
-	if (work) {
-		cancel_work_sync(work);
-		kfree(dep);
-	}
 }
 
 static struct attribute *attrs[] = {
-- 
1.8.2.1

Re: [RFC PATCH 3/3] ACPI/POWER: Call acpi_power_resume_dependent() directly in the acpi_power_on_unlocked()

[Lan Tianyu]

On 10/15/2013 11:04 PM, Lan Tianyu wrote:
> acpi_power_resume_dependent() purposes to resume power resource's dependent
> physical devices after turning on the related power resource. But current
> it runs in the workqueue, pm_request_resume() can't check the right
> runtime pm state just after turning on power resource. This will cause
> infinite loop between resume and suspend in some cases that turn on power
> resource in the pm runtime resume callback(e,g usb port). This patch is to
> fix the issue via calling acpi_power_resume_dependent() directly instead
> of running it in a workqueue.
>
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>   drivers/acpi/power.c | 15 ++-------------
>   1 file changed, 2 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> index 3fc2873..5e4991b 100644
> --- a/drivers/acpi/power.c
> +++ b/drivers/acpi/power.c
> @@ -62,7 +62,6 @@ ACPI_MODULE_NAME("power");
>   struct acpi_power_dependent_device {
>   	struct list_head node;
>   	struct acpi_device *adev;
> -	struct work_struct work;
>   };
>
>   struct acpi_power_resource {
> @@ -235,14 +234,12 @@ static int acpi_power_get_list_state(struct list_head *list, int *state)
>   	return 0;
>   }
>
> -static void acpi_power_resume_dependent(struct work_struct *work)
> +static void acpi_power_resume_dependent(struct acpi_power_dependent_device *dep)
>   {
> -	struct acpi_power_dependent_device *dep;
>   	struct acpi_device_physical_node *pn;
>   	struct acpi_device *adev;
>   	int state;
>
> -	dep = container_of(work, struct acpi_power_dependent_device, work);
>   	adev = dep->adev;
>   	if (acpi_power_get_inferred_state(adev, &state))
>   		return;
> @@ -294,7 +291,7 @@ static int acpi_power_on_unlocked(struct acpi_power_resource *resource)
>   			struct acpi_power_dependent_device *dep;
>
>   			list_for_each_entry(dep, &resource->dependent, node)
> -				schedule_work(&dep->work);
> +				acpi_power_resume_dependent(dep);
>   		}
>   	}
>   	return result;
> @@ -414,7 +411,6 @@ static void acpi_power_add_dependent(struct acpi_power_resource *resource,
>   		goto out;
>
>   	dep->adev = adev;
> -	INIT_WORK(&dep->work, acpi_power_resume_dependent);
>   	list_add_tail(&dep->node, &resource->dependent);
>
>    out:
> @@ -425,23 +421,16 @@ static void acpi_power_remove_dependent(struct acpi_power_resource *resource,
>   					struct acpi_device *adev)
>   {
>   	struct acpi_power_dependent_device *dep;
> -	struct work_struct *work = NULL;
>
>   	mutex_lock(&resource->resource_lock);
>
>   	list_for_each_entry(dep, &resource->dependent, node)
>   		if (dep->adev == adev) {
>   			list_del(&dep->node);
> -			work = &dep->work;

Sorry, Just find forget to free dep Here. Will update if this patchset 
is acceptable.

>   			break;
>   		}
>
>   	mutex_unlock(&resource->resource_lock);
> -
> -	if (work) {
> -		cancel_work_sync(work);
> -		kfree(dep);
> -	}
>   }
>
>   static struct attribute *attrs[] = {
>

Re: [RFC PATCH 0/3] ACPI/Power: Fix infinite loop of runtime resume and suspend caused by turning on power resource

[Rafael J. Wysocki]

On Tuesday, October 15, 2013 11:04:50 PM Lan Tianyu wrote:
> Currently, clearing usb port(empty port) PM Qos NO_POWER_OFF flag twice will lead
> the usb port to fall into the infinite loop of runtime pm resume and
> suspend. This is caused by pm_request_resume() in the acpi_power_resume_dependent().
> Detail is in the following link.
> http://marc.info/?l=linux-acpi&m=138182749202885&w=2

As far as I can say, the bug is in USB, so I'm not quite sure why you want
to address it in the ACPI core?

Rafael