Skip to content

Commit

Permalink
ACPI / PM: Take unusual configurations of power resources into account
Browse files Browse the repository at this point in the history
Commit d2e5f0c (ACPI / PCI: Rework the setup and cleanup of device
wakeup) moved the initial disabling of system wakeup for PCI devices
into a place where it can actually work and that exposed a hidden old
issue with crap^Wunusual system designs where the same power
resources are used for both wakeup power and device power control at
run time.

Namely, say there is one power resource such that the ACPI power
state D0 of a PCI device depends on that power resource (i.e. the
device is in D0 when that power resource is "on") and it is used
as a wakeup power resource for the same device.  Then, calling
acpi_pci_sleep_wake(pci_dev, false) for the device in question will
cause the reference counter of that power resource to drop to 0,
which in turn will cause it to be turned off.  As a result, the
device will go into D3cold at that point, although it should have
stayed in D0.

As it turns out, that happens to USB controllers on some laptops
and USB becomes unusable on those machines as a result, which is
a major regression from v3.8.

To fix this problem, (1) increment the reference counters of wakup
power resources during their initialization if they are "on"
initially, (2) prevent acpi_disable_wakeup_device_power() from
decrementing the reference counters of wakeup power resources that
were not enabled for wakeup power previously, and (3) prevent
acpi_enable_wakeup_device_power() from incrementing the reference
counters of wakeup power resources that already are enabled for
wakeup power.

In addition to that, if it is impossible to determine the initial
states of wakeup power resources, avoid enabling wakeup for devices
whose wakeup power depends on those power resources.

Reported-by: Dave Jones <davej@redhat.com>
Reported-by: Fabio Baltieri <fabio.baltieri@linaro.org>
Tested-by: Fabio Baltieri <fabio.baltieri@linaro.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
  • Loading branch information
rafaeljw committed Feb 23, 2013
1 parent 511d5c4 commit b5d667e
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 29 deletions.
2 changes: 1 addition & 1 deletion drivers/acpi/internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ int acpi_extract_power_resources(union acpi_object *package, unsigned int start,
struct list_head *list);
int acpi_add_power_resource(acpi_handle handle);
void acpi_power_add_remove_device(struct acpi_device *adev, bool add);
int acpi_power_min_system_level(struct list_head *list);
int acpi_power_wakeup_list_init(struct list_head *list, int *system_level);
int acpi_device_sleep_wake(struct acpi_device *dev,
int enable, int sleep_state, int dev_state);
int acpi_power_get_inferred_state(struct acpi_device *device, int *state);
Expand Down
112 changes: 85 additions & 27 deletions drivers/acpi/power.c
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ struct acpi_power_resource {
u32 system_level;
u32 order;
unsigned int ref_count;
bool wakeup_enabled;
struct mutex resource_lock;
};

Expand Down Expand Up @@ -272,11 +273,9 @@ static int __acpi_power_on(struct acpi_power_resource *resource)
return 0;
}

static int acpi_power_on(struct acpi_power_resource *resource)
static int acpi_power_on_unlocked(struct acpi_power_resource *resource)
{
int result = 0;;

mutex_lock(&resource->resource_lock);
int result = 0;

if (resource->ref_count++) {
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
Expand All @@ -293,9 +292,16 @@ static int acpi_power_on(struct acpi_power_resource *resource)
schedule_work(&dep->work);
}
}
return result;
}

mutex_unlock(&resource->resource_lock);
static int acpi_power_on(struct acpi_power_resource *resource)
{
int result;

mutex_lock(&resource->resource_lock);
result = acpi_power_on_unlocked(resource);
mutex_unlock(&resource->resource_lock);
return result;
}

Expand All @@ -313,17 +319,15 @@ static int __acpi_power_off(struct acpi_power_resource *resource)
return 0;
}

static int acpi_power_off(struct acpi_power_resource *resource)
static int acpi_power_off_unlocked(struct acpi_power_resource *resource)
{
int result = 0;

mutex_lock(&resource->resource_lock);

if (!resource->ref_count) {
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
"Power resource [%s] already off",
resource->name));
goto unlock;
return 0;
}

if (--resource->ref_count) {
Expand All @@ -335,10 +339,16 @@ static int acpi_power_off(struct acpi_power_resource *resource)
if (result)
resource->ref_count++;
}
return result;
}

unlock:
mutex_unlock(&resource->resource_lock);
static int acpi_power_off(struct acpi_power_resource *resource)
{
int result;

mutex_lock(&resource->resource_lock);
result = acpi_power_off_unlocked(resource);
mutex_unlock(&resource->resource_lock);
return result;
}

Expand Down Expand Up @@ -521,18 +531,35 @@ void acpi_power_add_remove_device(struct acpi_device *adev, bool add)
}
}

int acpi_power_min_system_level(struct list_head *list)
int acpi_power_wakeup_list_init(struct list_head *list, int *system_level_p)
{
struct acpi_power_resource_entry *entry;
int system_level = 5;

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);

result = acpi_power_get_state(handle, &state);
if (result) {
mutex_unlock(&resource->resource_lock);
return result;
}
if (state == ACPI_POWER_RESOURCE_STATE_ON) {
resource->ref_count++;
resource->wakeup_enabled = true;
}
if (system_level > resource->system_level)
system_level = resource->system_level;

mutex_unlock(&resource->resource_lock);
}
return system_level;
*system_level_p = system_level;
return 0;
}

/* --------------------------------------------------------------------------
Expand Down Expand Up @@ -610,6 +637,7 @@ int acpi_device_sleep_wake(struct acpi_device *dev,
*/
int acpi_enable_wakeup_device_power(struct acpi_device *dev, int sleep_state)
{
struct acpi_power_resource_entry *entry;
int err = 0;

if (!dev || !dev->wakeup.flags.valid)
Expand All @@ -620,17 +648,31 @@ int acpi_enable_wakeup_device_power(struct acpi_device *dev, int sleep_state)
if (dev->wakeup.prepare_count++)
goto out;

err = acpi_power_on_list(&dev->wakeup.resources);
if (err) {
dev_err(&dev->dev, "Cannot turn wakeup power resources on\n");
dev->wakeup.flags.valid = 0;
} else {
/*
* Passing 3 as the third argument below means the device may be
* put into arbitrary power state afterward.
*/
err = acpi_device_sleep_wake(dev, 1, sleep_state, 3);
list_for_each_entry(entry, &dev->wakeup.resources, node) {
struct acpi_power_resource *resource = entry->resource;

mutex_lock(&resource->resource_lock);

if (!resource->wakeup_enabled) {
err = acpi_power_on_unlocked(resource);
if (!err)
resource->wakeup_enabled = true;
}

mutex_unlock(&resource->resource_lock);

if (err) {
dev_err(&dev->dev,
"Cannot turn wakeup power resources on\n");
dev->wakeup.flags.valid = 0;
goto out;
}
}
/*
* Passing 3 as the third argument below means the device may be
* put into arbitrary power state afterward.
*/
err = acpi_device_sleep_wake(dev, 1, sleep_state, 3);
if (err)
dev->wakeup.prepare_count = 0;

Expand All @@ -647,6 +689,7 @@ int acpi_enable_wakeup_device_power(struct acpi_device *dev, int sleep_state)
*/
int acpi_disable_wakeup_device_power(struct acpi_device *dev)
{
struct acpi_power_resource_entry *entry;
int err = 0;

if (!dev || !dev->wakeup.flags.valid)
Expand All @@ -668,10 +711,25 @@ int acpi_disable_wakeup_device_power(struct acpi_device *dev)
if (err)
goto out;

err = acpi_power_off_list(&dev->wakeup.resources);
if (err) {
dev_err(&dev->dev, "Cannot turn wakeup power resources off\n");
dev->wakeup.flags.valid = 0;
list_for_each_entry(entry, &dev->wakeup.resources, node) {
struct acpi_power_resource *resource = entry->resource;

mutex_lock(&resource->resource_lock);

if (resource->wakeup_enabled) {
err = acpi_power_off_unlocked(resource);
if (!err)
resource->wakeup_enabled = false;
}

mutex_unlock(&resource->resource_lock);

if (err) {
dev_err(&dev->dev,
"Cannot turn wakeup power resources off\n");
dev->wakeup.flags.valid = 0;
break;
}
}

out:
Expand Down
9 changes: 8 additions & 1 deletion drivers/acpi/scan.c
Original file line number Diff line number Diff line change
Expand Up @@ -1004,7 +1004,14 @@ static int acpi_bus_extract_wakeup_device_power_package(acpi_handle handle,
if (!list_empty(&wakeup->resources)) {
int sleep_state;

sleep_state = acpi_power_min_system_level(&wakeup->resources);
err = acpi_power_wakeup_list_init(&wakeup->resources,
&sleep_state);
if (err) {
acpi_handle_warn(handle, "Retrieving current states "
"of wakeup power resources failed\n");
acpi_power_resources_list_free(&wakeup->resources);
goto out;
}
if (sleep_state < wakeup->sleep_state) {
acpi_handle_warn(handle, "Overriding _PRW sleep state "
"(S%d) by S%d from power resources\n",
Expand Down

0 comments on commit b5d667e

Please sign in to comment.