Re: [PATCH v1 1/7] ACPI: scan: Stop calling acpi_bus_init_power() early

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Transient failures in early PCI enumeration due to unmet ACPI dependencies permanently disable ACPI power management for the device.
- [High] Concurrent execution of `acpi_device_set_power` and `acpi_bus_attach` leads to power resource reference count leaks.
- [High] Bitfield data race on `struct acpi_device_flags` leading to corrupted device flags.
- [High] Unprivileged sysfs access to a child's power state bypasses the parent's power resource initialization, leading to reference count underflow and permanent power leaks.
--

commit 00db710bd9860a36a16f28aa782d074b11d43aab
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

ACPI: scan: Stop calling acpi_bus_init_power() early

This commit prevents premature transition into the D0 power state for
devices with missing dependencies by changing when acpi_bus_init_power()
is called. It updates acpi_device_set_power() to initialize power upfront
and adjusts acpi_bus_attach() to use ACPI power state tracking instead of
the initialized flag.

Link: https://lore.kernel.org/linux-acpi/20260820-acpi-power-resource-ref-fix-v2-1-29818173ea13@linux.spacemit.com/ [1]

> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index aa55ecfc2923c..a680e6972a8c2 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c

[ ... ]

> @@ -168,6 +168,12 @@ int acpi_device_set_power(struct acpi_device *device, int state)
>  	    || (state < ACPI_STATE_D0) || (state > ACPI_STATE_D3_COLD))
>  		return -EINVAL;
>  
> +	if (device->power.state == ACPI_STATE_UNKNOWN &&
> +	    acpi_bus_init_power(device)) {
> +		device->flags.power_manageable = 0;

[Severity: High]
If acpi_bus_init_power() fails transiently here because dependencies are
unmet (for instance, _PSC or _ON evaluations fail during early PCI
discovery), does this permanently disable ACPI power management for the
device? Since the prior restoration logic was removed from
acpi_bus_attach(), it appears there is no mechanism to recover from this
transient failure when dependencies are met later.

[Severity: High]
Because struct acpi_device_flags is a bitfield, does modifying
power_manageable here without mutual exclusion risk a data race? 

If acpi_device_set_power() runs concurrently with async enumeration
(such as acpi_bus_attach() calling acpi_device_set_enumerated()), they
could perform concurrent read-modify-write operations on the bitfield.
This could cause one of the flag updates (like the visited or enumerated
flag) to be silently lost.

> +		return -ENODEV;
> +	}
> +
>  	acpi_handle_debug(device->handle, "Power state change: %s -> %s\n",
>  			  acpi_power_state_string(device->power.state),
>  			  acpi_power_state_string(state));

[ ... ]

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f48715ed827c7..1ad8dffc2dafe 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c

[ ... ]

> @@ -2364,16 +2361,13 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
>  
>  	acpi_ec_register_opregions(device);
>  
> -	if (!device->flags.initialized) {
> -		device->flags.power_manageable =
> -			device->power.states[ACPI_STATE_D0].flags.valid;
> -		if (acpi_bus_init_power(device))
> -			device->flags.power_manageable = 0;
> +	if (device->flags.power_manageable &&
> +	    device->power.state == ACPI_STATE_UNKNOWN &&
> +	    acpi_bus_init_power(device))

[Severity: High]
Is it possible for acpi_bus_attach() (running asynchronously via
acpi_scan_clear_dep_fn()) and acpi_device_set_power() (reachable via
sysfs power/control) to execute concurrently? 

Both paths now perform a lockless check of
device->power.state == ACPI_STATE_UNKNOWN and subsequently call
acpi_bus_init_power(). Could this lead to both threads acquiring power
resource references via acpi_power_on_resources(), keeping the hardware
powered on indefinitely when the second set of references is leaked?

[Severity: High]
Could an unprivileged user reading the real_power_state sysfs attribute
of a child device bypass this parent initialization?

If a sysfs read calls acpi_device_get_power() on the child, it can
locklessly mutate the parent's state to ACPI_STATE_D0:

acpi_device_get_power() {
    ...
    if (!device->power.flags.ignore_parent && parent &&
        parent->power.state == ACPI_STATE_UNKNOWN &&
        result == ACPI_STATE_D0)
            parent->power.state = ACPI_STATE_D0;
    ...
}

If this occurs before acpi_bus_attach() runs for the parent, the
lockless check here in acpi_bus_attach() will see a state other than
ACPI_STATE_UNKNOWN and skip calling acpi_bus_init_power(). 

Does this result in the parent's power resources never being initialized,
leading to a reference count underflow when the device later transitions
to D3?

> +		device->flags.power_manageable = 0;
>  
> -		device->flags.initialized = true;
> -	} else if (device->flags.visited) {
> +	if (device->flags.visited)
>  		goto ok;
> -	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/3435655.aeNJFYEL58@rafael.j.wysocki?part=1