[v2,0/6] ACPI: scan: Adjust power management initialization and PCI devices handling

Hi All,

This is an update of

https://lore.kernel.org/linux-acpi/3435655.aeNJFYEL58@rafael.j.wysocki/

that adds one patch, drops two patches, and addresses review comments in
some other patches in the series.

The new patch drops an unneeded parent power state update that may lead to
issues in theory from acpi_device_get_power().

One of the patches dropped from the original series is not necessary any
more after addressing review comments in patch [2/6], and the other one
might introduced some issues pointed out by Sashiko.

Overall, the series aims at addressing a few issues present in the core ACPI
device enumeration code, mostly related to the acpi_bus_attach() function.

The first one is that, on some systems, ACPI power management is initialized
twice for devices that have missing dependencies to start with.  An attempt
to address this issue had been made during the 7.3 merge window, but it had
to be reverted [1].  The approach used here is roughly the same as in the
reverted commit, but it takes PCI devices (which are a special case) into
account.

In addition to the above, ACPI power management may be initialized prematurely
for device objects whose parents are not ready for enumeration.

The other issues are that flags are unnecessarily cleared by acpi_bus_attach()
for devices with missing dependencies, the "initialized" ACPI device object
flag is not particularly useful, the "visited" ACPI device object flag is
used for two different things which is confusing at best, and PCI devices
are handled by acpi_bus_attach() like any other devices which is a mistake.

Please see the changelogs of individual patches for details.

Thanks!


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

> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> The parent state update in acpi_device_get_power(), that occurs when the
> child device turns out to be in D0 and the parent's power state is still
> unknown, is highly questionable because it may cause the reference
> counters of the power resources used by the parent in D0 (if any) to
> underflow when the parent goes into a low-power state later.  Moreover,
> there is no reason to do it on reads from the real_power_state sysfs
> attribute of the child.
> 
> That check had been added by commit 8f7412a792bc ("ACPI / PM: Infer
> parent power state from child if unknown, v2") before starting to
> handle the "missing _PSC and no power resources" case directly in
> acpi_bus_init_power(), as of commit b3785492268f ("ACPI / PM: Do not
> power manage devices in unknown initial states").  It is not necessary
> any more and commit b3785492268f should have removed it.
> 
> Drop it now.
> 
> Fixes: b3785492268f ("ACPI / PM: Do not power manage devices in unknown initial states")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Global mutex init_power_lock in acpi_bus_init_power causes deadlocks when power initialization of one device indirectly triggers initialization of another.
- [High] The poisoned ACPI_D_STATE_DISABLED state is silently overwritten by acpi_device_update_power() for devices without power resources.
- [High] Leaving device->flags.power_manageable = 1 when power initialization fails aborts system suspend.
- [High] TOCTOU race in acpi_device_set_power allows bypassing the disabled power state safety check.

Pre-existing issues:
- [High] Unreleased ACPI power resource references if acpi_dev_pm_explicit_set() fails.
--

commit 6095d5c97ad32b03d70b1c09e3f9a5916aa85e11
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

    ACPI: scan: Stop calling acpi_bus_init_power() early

    There is a problem, introduced by commit 9d9bcae47fd5 ("ACPI: delay
    enumeration of devices with a _DEP pointing to an INT3472 device")
    inadvertently, that devices with missing dependencies may be put
    into power state D0 prematurely [1].

    Namely, acpi_bus_init_power() called by acpi_bus_get_power_flags()
    during the early initialization of ACPI device objects, may discover
    that all of the power resources needed by the given device to be in
    power state D0 are initially on, so it will reference count those
    power resources and transition the device into D0.

    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 4269735aadde5..e13096cfd7909 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
[ ... ]
> @@ -157,6 +159,15 @@ 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_D_STATE_DISABLED)
> +		return -ENXIO;
> +
> +	if (device->power.state == ACPI_STATE_UNKNOWN) {
> +		result = acpi_bus_init_power(device);
> +		if (result)
> +			return result;
> +	}
> +

[Severity: High]
Since device->power.state is checked outside of the init_power_lock, can a
TOCTOU race occur here?

If two threads enter acpi_device_set_power() concurrently for a device in
ACPI_STATE_UNKNOWN:

1. Thread 1 calls acpi_bus_init_power(), fails, and poisons the state to
ACPI_D_STATE_DISABLED.
2. Thread 2 acquires init_power_lock in acpi_bus_init_power(), sees the
state is no longer ACPI_STATE_UNKNOWN, and returns 0 (success).
3. Thread 2 then proceeds to apply power transitions to a device that Thread
1 just poisoned.

[ ... ]
> @@ -293,20 +304,11 @@ int acpi_bus_set_power(acpi_handle handle, int state)
>  }
>  EXPORT_SYMBOL(acpi_bus_set_power);
>  
> -int acpi_bus_init_power(struct acpi_device *device)
> +static int acpi_device_init_power(struct acpi_device *device)
>  {

[Severity: High]
This is a pre-existing issue, but does this function leak power resource
references?

If acpi_power_on_resources() succeeds but the subsequent call to
acpi_dev_pm_explicit_set(device, state) fails, it returns the error
immediately without dropping the power resource references acquired just
before.

[ ... ]
> @@ -340,9 +342,43 @@ int acpi_bus_init_power(struct acpi_device *device)
>  		state = ACPI_STATE_D0;
>  	}
>  	device->power.state = state;
> +
> +	acpi_handle_debug(device->handle, "Initial power state: %s\n",
> +			  acpi_power_state_string(state));
> +
>  	return 0;
>  }
>  
> +int acpi_bus_init_power(struct acpi_device *device)
> +{
> +	static DEFINE_MUTEX(init_power_lock);
> +	int result;
> +
> +	/*
> +	 * This is done to prevent power state initialization from being carried
> +	 * out twice in parallel for the same device (not impossible, but very
> +	 * unlikely).
> +	 */
> +	guard(mutex)(&init_power_lock);

[Severity: High]
Can this global non-recursive mutex cause deadlocks?

If acpi_device_init_power() (called while holding this lock) evaluates ACPI
methods (e.g., _ON) that access an ACPI Operation Region, it can trigger a
runtime resume of the controller device. If the controller device's power
state is ACPI_STATE_UNKNOWN, its runtime resume path (e.g.,
acpi_device_set_power()) will recursively call acpi_bus_init_power(), leading
to a deadlock on this static mutex.

> +
> +	if (device->power.state != ACPI_STATE_UNKNOWN)
> +		return 0;
> +
> +	/*
> +	 * The ACPI device power state can be only initialized once.  If this
> +	 * fails, ACPI power management will not be used for the device going
> +	 * forward.
> +	 */
> +	result = acpi_device_init_power(device);
> +	if (result) {
> +		device->power.state = ACPI_D_STATE_DISABLED;

[Severity: High]
Is this disabled state fully protected from being overwritten by other
functions?

If acpi_bus_init_power() fails, the state is set to ACPI_D_STATE_DISABLED.
Later, if acpi_device_update_power() is called, it sees the state is not
ACPI_STATE_UNKNOWN, bypasses the initialization check, and calls
acpi_device_get_power(). If the device has no power resources, it executes
device->power.state = state;, unconditionally wiping out the
ACPI_D_STATE_DISABLED marker.

> +		acpi_handle_info(device->handle,
> +			"Failed to determine initial power state, ACPI PM disabled\n");
> +	}
> +
> +	return result;
> +}
> +
>  /**
>   * acpi_device_fix_up_power - Force device with missing _PSC into D0.
>   * @device: Device object whose power state is to be fixed up.
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f48715ed827c7..4586f1798685f 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
[ ... ]
> @@ -2364,16 +2361,10 @@ 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;
> +	acpi_bus_init_power(device);

[Severity: High]
Does ignoring the return value of acpi_bus_init_power() here break system
suspend?

By no longer clearing device->flags.power_manageable when initialization
fails, the device remains power manageable but its state is marked
ACPI_D_STATE_DISABLED. During system suspend, acpi_dev_pm_low_power() will
see it is power manageable and call acpi_device_set_power(). Encountering
ACPI_D_STATE_DISABLED, acpi_device_set_power() returns -ENXIO, which
propagates to the PM core and aborts the suspend process entirely.

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

> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> There are two conditionals in acpi_bus_attach() that can be combined,
> which slightly reduces the overhead and makes the code a bit easier
> to follow, so do that.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Sashiko has reviewed this patch and found no issues. It looks great!