On Thu, Jun 13, 2019 at 2:52 PM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Jun 13, 2019 at 12:38:50AM +0200, Rafael J. Wysocki wrote:
> > Basically, at the pci_acpi_setup() time dev and adev need to be passed to a function that
> > will add dev as a "dependent device" for each of the power resources in the adev's D0
> > list.
> >
> > Next whenever a power resource with a list of "dependent devices" goes _ON successfully,
> > pm_request_resume() needs to be called for each device in that list.
> >
> > Finally, at the pci_acpi_cleanup() time, dev needs to be removed from the lists of
> > "dependent devices" for all power resources in its ACPI companion's D0 list.
> >
> > At least that's how I see that.
>
> Thanks for the suggestion. This seems to make it work only for PCI
> devices, though. Is that the intention?
I'm not sure to what extent the D0-uninitialized concept applies to
non-PCI devices. It may just not be necessary to do this for non-PCI
device in general.
> I went for the all ACPI devices path instead where we add all devices
> sharing the power resource as "consumers" for that resource. I haven't
> fully tested the approach yet but the draft patch is below. I have no
> issues doing what you say above, though :)
>
> ----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----8<----
>
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index f6157d4d637a..e840299c3293 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -127,10 +127,10 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev,
> Power Resource
> -------------------------------------------------------------------------- */
> int acpi_power_init(void);
> -void acpi_power_resources_list_free(struct list_head *list);
> -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_resources_list_free(struct acpi_device *device, struct list_head *list);
> +int acpi_extract_power_resources(struct acpi_device *device,
> + union acpi_object *package, unsigned int start, struct list_head *list);
> +int acpi_add_power_resource(struct acpi_device *adev, acpi_handle handle);
> void acpi_power_add_remove_device(struct acpi_device *adev, bool add);
> int acpi_power_wakeup_list_init(struct list_head *list, int *system_level);
> int acpi_device_sleep_wake(struct acpi_device *dev,
> diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
> index a916417b9e70..31817f931381 100644
> --- a/drivers/acpi/power.c
> +++ b/drivers/acpi/power.c
> @@ -45,6 +45,8 @@ ACPI_MODULE_NAME("power");
> struct acpi_power_resource {
> struct acpi_device device;
> struct list_head list_node;
> + struct list_head consumers;
> + struct mutex consumer_lock;
Why do you need this extra lock?
> char *name;
> u32 system_level;
> u32 order;
> @@ -58,6 +60,11 @@ struct acpi_power_resource_entry {
> struct acpi_power_resource *resource;
> };
>
> +struct acpi_power_resource_consumer {
> + struct list_head node;
> + struct acpi_device *consumer;
I would put the "physical" device pointer here.
> +};
> +
> static LIST_HEAD(acpi_power_resource_list);
> static DEFINE_MUTEX(power_resource_list_lock);
>
> @@ -81,6 +88,111 @@ static struct acpi_power_resource *acpi_power_get_context(acpi_handle handle)
> return to_power_resource(device);
> }
>
> +static int acpi_power_add_consumer(struct acpi_power_resource *resource,
> + struct acpi_device *device)
> +{
> + struct acpi_power_resource_consumer *consumer;
> + int ret = 0;
> +
> + if (!device)
> + return 0;
> +
> + mutex_lock(&resource->consumer_lock);
All of this could be done under the resource mutex instead, I think.
> +
> + list_for_each_entry(consumer, &resource->consumers, node) {
> + /* Don't add it twice */
> + if (consumer->consumer == device)
> + goto unlock;
> + }
> +
> + consumer = kzalloc(sizeof(*consumer), GFP_KERNEL);
> + if (!consumer) {
> + ret = -ENOMEM;
> + goto unlock;
> + }
> +
> + consumer->consumer = device;
> + list_add_tail(&consumer->node, &resource->consumers);
> +
> + acpi_handle_debug(device->handle, "added dependency to %s\n",
> + resource->name);
> +
> +unlock:
> + mutex_unlock(&resource->consumer_lock);
> + return ret;
> +}
> +
> +static void acpi_power_remove_consumer(struct acpi_power_resource *resource,
> + struct acpi_device *device)
> +{
> + struct acpi_power_resource_consumer *consumer;
> +
> + mutex_lock(&resource->consumer_lock);
> + list_for_each_entry(consumer, &resource->consumers, node) {
> + if (consumer->consumer == device) {
> + list_del(&consumer->node);
> + kfree(consumer);
> +
> + acpi_handle_debug(device->handle,
> + "removed dependency to %s\n",
> + resource->name);
> + break;
> + }
> + }
> + mutex_unlock(&resource->consumer_lock);
> +}
> +
> +static void
> +acpi_power_resume_consumer(const struct acpi_power_resource *resource,
> + struct acpi_device *device)
> +{
> + struct device *dev;
> +
> + /*
> + * If the device is prepared to wake we need to resume it now so
> + * that the driver can re-program it to do so. For non-wake devices
> + * we can leave them as is. The driver then restores the device
> + * when it is needed next time.
> + */
> + if (!acpi_device_can_wakeup(device) || !device->wakeup.prepare_count)
> + return;
Checking prepare_count should not be necessary here.
Arguably, the power resource cannot go off without suspending all
devices that depend on it and for PM-runtime wakeup is always enabled.
> +
> + dev = acpi_get_first_physical_node(device);
And that would be unnecessary if you stored dev in struct
acpi_power_resource_consumer.
> + if (dev) {
> + acpi_handle_debug(device->handle,
> + "resuming %s because %s was turned on\n",
> + dev_name(dev), resource->name);
> + pm_runtime_resume(dev);
pm_request_resume()
This needs to be asynchronous as it should be done under resource_lock
(see below).
> + }
> +}
> +
> +static int acpi_power_resume_consumers(struct acpi_power_resource *resource,
> + struct acpi_device *device)
> +{
> + struct acpi_power_resource_consumer *consumer;
> + int ret = 0;
> +
> + mutex_lock(&resource->consumer_lock);
> +
> + list_for_each_entry(consumer, &resource->consumers, node) {
> + struct acpi_device *adev = consumer->consumer;
> + int state, ret;
> +
> + /* Skip the device that originated the power on request */
> + if (adev == device)
> + continue;
> +
> + ret = acpi_power_get_inferred_state(consumer->consumer, &state);
This isn't necessary AFAICS.
The resource was off previously, so none of its consumers can be in D0.
> + if (!ret && adev->power.state > ACPI_STATE_D0 &&
> + state == ACPI_STATE_D0) {
> + acpi_power_resume_consumer(resource, adev);
> + }
> + }
> +
> + mutex_unlock(&resource->consumer_lock);
> + return ret;
> +}
> +
> static int acpi_power_resources_list_add(acpi_handle handle,
> struct list_head *list)
> {
> @@ -108,12 +220,14 @@ static int acpi_power_resources_list_add(acpi_handle handle,
> return 0;
> }
>
> -void acpi_power_resources_list_free(struct list_head *list)
> +void acpi_power_resources_list_free(struct acpi_device *device, struct list_head *list)
> {
> struct acpi_power_resource_entry *entry, *e;
>
> list_for_each_entry_safe(entry, e, list, node) {
> list_del(&entry->node);
> + if (device)
> + acpi_power_remove_consumer(entry->resource, device);
> kfree(entry);
> }
> }
> @@ -135,8 +249,8 @@ static bool acpi_power_resource_is_dup(union acpi_object *package,
> return false;
> }
>
> -int acpi_extract_power_resources(union acpi_object *package, unsigned int start,
> - struct list_head *list)
> +int acpi_extract_power_resources(struct acpi_device *device,
> + union acpi_object *package, unsigned int start, struct list_head *list)
> {
> unsigned int i;
> int err = 0;
> @@ -159,7 +273,7 @@ int acpi_extract_power_resources(union acpi_object *package, unsigned int start,
> if (acpi_power_resource_is_dup(package, start, i))
> continue;
>
> - err = acpi_add_power_resource(rhandle);
> + err = acpi_add_power_resource(device, rhandle);
> if (err)
> break;
>
> @@ -168,7 +282,7 @@ int acpi_extract_power_resources(union acpi_object *package, unsigned int start,
> break;
> }
> if (err)
> - acpi_power_resources_list_free(list);
> + acpi_power_resources_list_free(device, list);
>
> return err;
> }
> @@ -258,18 +372,33 @@ static int acpi_power_on_unlocked(struct acpi_power_resource *resource)
> result = __acpi_power_on(resource);
> if (result)
> resource->ref_count--;
> + else
> + result = 1;
> }
> return result;
> }
>
> -static int acpi_power_on(struct acpi_power_resource *resource)
> +static int acpi_power_on(struct acpi_device *device,
> + 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;
> +
> + if (result <= 0)
> + return result;
> +
> + /*
> + * The power resource was physically turned on. Because of this
> + * some of the devices sharing it may have been transitioned into
> + * D0 so we need to runtime resume them to make sure their driver
> + * re-initializes them properly. This is important for PCI devices
> + * that go into D0uninitialized and lose their wakeup settings
> + * otherwise.
So you only talk about PCI devices here, which is why I think that, at
least for now, this needs to be done only for PCI devices.
> + */
> + return acpi_power_resume_consumers(resource, device);
Resuming consumers technically belongs to the "on" operation, so it
should be done under resource_lock (or there is nothing to prevent the
resource from going off immediately in a different thread in theory,
although that is unlikely due to the way device PM code works).
> }
>
> static int __acpi_power_off(struct acpi_power_resource *resource)
> @@ -319,7 +448,7 @@ static int acpi_power_off(struct acpi_power_resource *resource)
> return result;
> }
>
> -static int acpi_power_off_list(struct list_head *list)
> +static int acpi_power_off_list(struct acpi_device *device, struct list_head *list)
> {
> struct acpi_power_resource_entry *entry;
> int result = 0;
> @@ -333,18 +462,18 @@ static int acpi_power_off_list(struct list_head *list)
>
> err:
> list_for_each_entry_continue(entry, list, node)
> - acpi_power_on(entry->resource);
> + acpi_power_on(device, entry->resource);
>
> return result;
> }
>
> -static int acpi_power_on_list(struct list_head *list)
> +static int acpi_power_on_list(struct acpi_device *device, struct list_head *list)
> {
> struct acpi_power_resource_entry *entry;
> int result = 0;
>
> list_for_each_entry(entry, list, node) {
> - result = acpi_power_on(entry->resource);
> + result = acpi_power_on(device, entry->resource);
> if (result)
> goto err;
> }
> @@ -582,7 +711,7 @@ int acpi_enable_wakeup_device_power(struct acpi_device *dev, int sleep_state)
>
> if (!resource->wakeup_enabled) {
> err = acpi_power_on_unlocked(resource);
> - if (!err)
> + if (err >= 0)
> resource->wakeup_enabled = true;
> }
>
> @@ -703,7 +832,7 @@ int acpi_power_on_resources(struct acpi_device *device, int state)
> if (!device || state < ACPI_STATE_D0 || state > ACPI_STATE_D3_HOT)
> return -EINVAL;
>
> - return acpi_power_on_list(&device->power.states[state].resources);
> + return acpi_power_on_list(device, &device->power.states[state].resources);
> }
>
> int acpi_power_transition(struct acpi_device *device, int state)
> @@ -726,11 +855,11 @@ int acpi_power_transition(struct acpi_device *device, int state)
> * we dereference all power resources used in the current list.
> */
> if (state < ACPI_STATE_D3_COLD)
> - result = acpi_power_on_list(
> + result = acpi_power_on_list(device,
> &device->power.states[state].resources);
>
> if (!result && device->power.state < ACPI_STATE_D3_COLD)
> - acpi_power_off_list(
> + acpi_power_off_list(device,
> &device->power.states[device->power.state].resources);
>
> /* We shouldn't change the state unless the above operations succeed. */
> @@ -788,7 +917,7 @@ static void acpi_power_add_resource_to_list(struct acpi_power_resource *resource
> mutex_unlock(&power_resource_list_lock);
> }
>
> -int acpi_add_power_resource(acpi_handle handle)
> +int acpi_add_power_resource(struct acpi_device *adev, acpi_handle handle)
> {
> struct acpi_power_resource *resource;
> struct acpi_device *device = NULL;
> @@ -798,8 +927,10 @@ int acpi_add_power_resource(acpi_handle handle)
> int state, result = -ENODEV;
>
> acpi_bus_get_device(handle, &device);
> - if (device)
> - return 0;
> + if (device) {
> + resource = to_power_resource(device);
> + goto add_consumer;
> + }
>
> resource = kzalloc(sizeof(*resource), GFP_KERNEL);
> if (!resource)
> @@ -810,6 +941,8 @@ int acpi_add_power_resource(acpi_handle handle)
> ACPI_STA_DEFAULT);
> mutex_init(&resource->resource_lock);
> INIT_LIST_HEAD(&resource->list_node);
> + INIT_LIST_HEAD(&resource->consumers);
> + mutex_init(&resource->consumer_lock);
> resource->name = device->pnp.bus_id;
> strcpy(acpi_device_name(device), ACPI_POWER_DEVICE_NAME);
> strcpy(acpi_device_class(device), ACPI_POWER_CLASS);
> @@ -840,7 +973,11 @@ int acpi_add_power_resource(acpi_handle handle)
>
> acpi_power_add_resource_to_list(resource);
> acpi_device_add_finalize(device);
> - return 0;
> +
> + add_consumer:
Note that this only needs to be done if the device's D0 list contains
the resource.
> + result = acpi_power_add_consumer(resource, adev);
> + if (!result)
> + return 0;
>
> err:
> acpi_release_power_resource(&device->dev);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 6153030451eb..3af0abe5b5e2 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -451,14 +451,14 @@ static void acpi_free_power_resources_lists(struct acpi_device *device)
> int i;
>
> if (device->wakeup.flags.valid)
> - acpi_power_resources_list_free(&device->wakeup.resources);
> + acpi_power_resources_list_free(NULL, &device->wakeup.resources);
>
> if (!device->power.flags.power_resources)
> return;
>
> for (i = ACPI_STATE_D0; i <= ACPI_STATE_D3_HOT; i++) {
> struct acpi_device_power_state *ps = &device->power.states[i];
> - acpi_power_resources_list_free(&ps->resources);
> + acpi_power_resources_list_free(device, &ps->resources);
> }
> }
>
> @@ -816,7 +816,7 @@ static int acpi_bus_extract_wakeup_device_power_package(struct acpi_device *dev)
>
> wakeup->sleep_state = element->integer.value;
>
> - err = acpi_extract_power_resources(package, 2, &wakeup->resources);
> + err = acpi_extract_power_resources(NULL, package, 2, &wakeup->resources);
> if (err)
> goto out;
>
> @@ -828,7 +828,7 @@ static int acpi_bus_extract_wakeup_device_power_package(struct acpi_device *dev)
> if (err) {
> acpi_handle_warn(handle, "Retrieving current states "
> "of wakeup power resources failed\n");
> - acpi_power_resources_list_free(&wakeup->resources);
> + acpi_power_resources_list_free(NULL, &wakeup->resources);
> goto out;
> }
> if (sleep_state < wakeup->sleep_state) {
> @@ -920,7 +920,7 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
> if (buffer.length && package
> && package->type == ACPI_TYPE_PACKAGE
> && package->package.count) {
> - int err = acpi_extract_power_resources(package, 0,
> + int err = acpi_extract_power_resources(device, package, 0,
> &ps->resources);
> if (!err)
> device->power.flags.power_resources = 1;
> @@ -1867,7 +1867,7 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
> return AE_OK;
>
> if (type == ACPI_BUS_TYPE_POWER) {
> - acpi_add_power_resource(handle);
> + acpi_add_power_resource(NULL, handle);
> return AE_OK;
> }
>
> --
> 2.20.1
>
