[PATCH v2 2/6] ACPI: scan: Stop calling acpi_bus_init_power() early

[PATCH v2 2/6] ACPI: scan: Stop calling acpi_bus_init_power() early

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

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.  Later, if
acpi_bus_attach() running for that device notices that it has missing
dependencies, the enumeration of it will be deferred and its
power_manageable flag will be cleared, even though it is still in D0
at that point.

After the dependencies in question have been met, acpi_bus_attach()
runs again for the device and now it calls acpi_bus_init_power() that
takes additional references to the power resources used by the device
in D0.  These additional references prevent the power resources from
being turned off when the device goes into D3hot/D3cold.

Another problem, related to the previous one, is that ACPI power state
initialization may be carried out for devices whose parents are not
ready for enumeration which may lead to initialization ordering issues.

To address both, stop calling acpi_bus_init_power() from
acpi_bus_get_power_flags(), but also take the initialization of
PCI devices into account, which needs to be done because they
are initialized and bound to their ACPI companions before
acpi_bus_attach() is called for the latter.

To that end, notice that each PCI device discovered on the bus is
put into power state D0 via pci_power_up() which involves invoking
acpi_device_set_power() if the given PCI device has an ACPI companion
with flags.power_manageable set.  The initial ACPI power state of
the device needs to be known at that point to carry out the power
transition of it properly, so modify acpi_device_set_power() to
call acpi_bus_init_power() upfront if the device's ACPI power
state is still unknown.

Also use the ACPI power state tracking to decide whether or not
the device's power state needs to be initialized in acpi_bus_attach()
instead of using the "initialized" flag of the ACPI device object
for this purpose, which is fragile and inconvenient.  Also stop
clearing the power_manageable flag for devices with unmet
dependencies and poison the power state as "disabled" if the
initialization of it fails, which may not be recoverable.

While at it, add a debug message printing statement to
acpi_bus_init_power() to facilitate diagnostics.

Fixes: 9d9bcae47fd5 ("ACPI: delay enumeration of devices with a _DEP pointing to an INT3472 device")
Link: https://lore.kernel.org/linux-acpi/20260820-acpi-power-resource-ref-fix-v2-1-29818173ea13@linux.spacemit.com/ [1]
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v2:
   * Address Sashiko review comments:
     https://lore.kernel.org/linux-pci/20260831201426.92F091F000E9@smtp.kernel.org/

---
 drivers/acpi/device_pm.c | 56 +++++++++++++++++++++++++++++++++-------
 drivers/acpi/scan.c      | 15 +++--------
 2 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 4269735aadde..e13096cfd790 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -23,6 +23,8 @@
 #include "fan.h"
 #include "internal.h"
 
+#define ACPI_D_STATE_DISABLED	ACPI_D_STATE_COUNT
+
 /**
  * acpi_power_state_string - String representation of ACPI device power state.
  * @state: ACPI device power state to return the string representation of.
@@ -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;
+	}
+
 	acpi_handle_debug(device->handle, "Power state change: %s -> %s\n",
 			  acpi_power_state_string(device->power.state),
 			  acpi_power_state_string(state));
@@ -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)
 {
 	int state;
 	int result;
 
-	if (!device)
-		return -EINVAL;
-
-	device->power.state = ACPI_STATE_UNKNOWN;
-	if (!acpi_device_is_present(device)) {
-		device->flags.initialized = false;
-		return -ENXIO;
-	}
-
 	result = acpi_device_get_power(device, &state);
 	if (result)
 		return result;
@@ -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);
+
+	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;
+		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 f48715ed827c..4586f1798685 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1145,8 +1145,7 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
 			device->power.states[ACPI_STATE_D3_COLD].flags.valid = 1;
 	}
 
-	if (acpi_bus_init_power(device))
-		device->flags.power_manageable = 0;
+	device->power.state = ACPI_STATE_UNKNOWN;
 }
 
 static void acpi_bus_get_flags(struct acpi_device *device)
@@ -2354,9 +2353,7 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
 	acpi_bus_get_status(device);
 	/* Skip devices that are not ready for enumeration (e.g. not present) */
 	if (!acpi_dev_ready_for_enumeration(device)) {
-		device->flags.initialized = false;
 		acpi_device_clear_enumerated(device);
-		device->flags.power_manageable = 0;
 		return 0;
 	}
 	if (device->handler)
@@ -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);
 
-		device->flags.initialized = true;
-	} else if (device->flags.visited) {
+	if (device->flags.visited)
 		goto ok;
-	}
 
 	ret = acpi_scan_attach_handler(device);
 	if (ret < 0)
-- 
2.51.0

[PATCH v2 3/6] ACPI: scan: Combine two conditionals in acpi_bus_attach()

[Rafael J. Wysocki]

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

v1 -> v2:
   * Reverse checks to avoid multiple negations (Andy)
   * Rebase on top of the new [2/6]

Link to the v1:
https://lore.kernel.org/linux-pci/2021470.taCxCBeP46@rafael.j.wysocki/

---
 drivers/acpi/scan.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 4586f1798685..7208cc515866 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2370,13 +2370,8 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
 	if (ret < 0)
 		return 0;
 
-	if (ret > 0 && !device->flags.enumeration_by_parent) {
-		acpi_device_set_enumerated(device);
-		goto ok;
-	}
-
-	if (device->pnp.type.platform_id || device->pnp.type.backlight ||
-	    device->flags.enumeration_by_parent)
+	if (device->flags.enumeration_by_parent ||
+	    (!ret && (device->pnp.type.platform_id || device->pnp.type.backlight)))
 		acpi_default_enumeration(device);
 	else
 		acpi_device_set_enumerated(device);
-- 
2.51.0

[PATCH v2 4/6] ACPI: scan: Add ACPI device enumerated marker

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Currently, the "visited" flag of ACPI device objects has two roles.
One of them is to indicate that the given ACPI device object has been
enumerated, which basically means that either it has been associated
with a "physical" device representation, or there is no matching
"physical" device representation for it.  The other one is to
indicate that acpi_bus_attach() has processed the device.  That dual
purpose leads to some confusion regarding when that flag should be set
and cleared and by whom.

To address that confusion, add a new bool field called "enumerated"
to struct acpi_device for the purpose of indicating whether or not
the given ACPI device object has been enumerated.  Accordingly, switch
over acpi_device_enumerated() and acpi_device_set/clear_enumerated() to
operate on that field and the "visited" flag will now be only used
internally by the core ACPI device enumeration code.

Namely, acpi_bus_attach() will set flags.visited for a given ACPI
device object after it has been completely processed by that function
and regardless of its current status it needs to be "unattached", for
example by acpi_scan_check_and_detach(), so that acpi_bus_attach() can
process it again.

The rule regarding flags.visited and the "enumerated" field is that the
latter cannot be set until the former has been set and they need to be
cleared in reverse order.

This allows a couple of checks to be dropped from acpi_bus_attach()
and its first_pass argument is not needed any more.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Peixin Xie <peixin.xie@linux.spacemit.com>
---

v1 -> v2:
   * Rebase on top of the new [2-3/6]
   * Drop quote characters from the subject (unnecessary and may get
     in the way in future)
   * Add tag from Peixin Xie

Link to the v1:
https://lore.kernel.org/linux-pci/48339705.fMDQidcC6G@rafael.j.wysocki/

---
 drivers/acpi/scan.c     | 28 +++++++++++++---------------
 include/acpi/acpi_bus.h |  3 ++-
 include/linux/acpi.h    |  4 ++--
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 7208cc515866..2e853add5e60 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -266,8 +266,8 @@ static int acpi_scan_check_and_detach(struct acpi_device *adev, void *p)
 		if (acpi_device_is_enabled(adev))
 			return 0;
 
-		/* Skip device that have not been enumerated. */
-		if (!acpi_device_enumerated(adev)) {
+		/* Skip device that have not been prepared for enumeration. */
+		if (!adev->flags.visited) {
 			dev_dbg(&adev->dev, "Still not enumerated\n");
 			return 0;
 		}
@@ -287,6 +287,7 @@ static int acpi_scan_check_and_detach(struct acpi_device *adev, void *p)
 	if (!(flags & ACPI_SCAN_CHECK_FLAG_EJECT)) {
 		adev->handler = NULL;
 		acpi_device_clear_enumerated(adev);
+		adev->flags.visited = 0;
 	}
 	return 0;
 }
@@ -305,6 +306,7 @@ static int acpi_bus_post_eject(struct acpi_device *adev, void *not_used)
 	}
 
 	acpi_device_clear_enumerated(adev);
+	adev->flags.visited = 0;
 
 	return 0;
 }
@@ -2338,46 +2340,42 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
 	return ret;
 }
 
-static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
+static int acpi_bus_attach(struct acpi_device *device, void *not_used)
 {
-	bool skip = !first_pass && device->flags.visited;
 	acpi_handle ejd;
+	bool skip;
 	int ret;
 
+	skip = !!device->flags.visited;
 	if (skip)
-		goto ok;
+		goto children;
 
 	if (ACPI_SUCCESS(acpi_bus_get_ejd(device->handle, &ejd)))
 		register_dock_dependent_device(device, ejd);
 
 	acpi_bus_get_status(device);
 	/* Skip devices that are not ready for enumeration (e.g. not present) */
-	if (!acpi_dev_ready_for_enumeration(device)) {
-		acpi_device_clear_enumerated(device);
+	if (!acpi_dev_ready_for_enumeration(device))
 		return 0;
-	}
-	if (device->handler)
-		goto ok;
 
 	acpi_ec_register_opregions(device);
 
 	acpi_bus_init_power(device);
 
-	if (device->flags.visited)
-		goto ok;
-
 	ret = acpi_scan_attach_handler(device);
 	if (ret < 0)
 		return 0;
 
+	device->flags.visited = 1;
+
 	if (device->flags.enumeration_by_parent ||
 	    (!ret && (device->pnp.type.platform_id || device->pnp.type.backlight)))
 		acpi_default_enumeration(device);
 	else
 		acpi_device_set_enumerated(device);
 
-ok:
-	acpi_dev_for_each_child(device, acpi_bus_attach, first_pass);
+children:
+	acpi_dev_for_each_child(device, acpi_bus_attach, NULL);
 
 	if (!skip && device->handler && device->handler->hotplug.notify_online)
 		device->handler->hotplug.notify_online(device);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 1a45e0d521d8..46ac4a33ae2c 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -468,6 +468,7 @@ struct acpi_device {
 	struct list_head physical_node_list;
 	struct mutex physical_node_lock;
 	void (*remove)(struct acpi_device *);
+	bool enumerated;
 };
 
 /* Non-device subnode */
@@ -654,7 +655,7 @@ void acpi_set_modalias(struct acpi_device *adev, const char *default_id,
 
 static inline bool acpi_device_enumerated(struct acpi_device *adev)
 {
-	return adev && adev->flags.initialized && adev->flags.visited;
+	return adev && adev->flags.initialized && adev->enumerated;
 }
 
 /*
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index ddacac812094..2e9e49e2e665 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -782,12 +782,12 @@ struct platform_device *acpi_create_platform_device(struct acpi_device *,
 
 static inline void acpi_device_set_enumerated(struct acpi_device *adev)
 {
-	adev->flags.visited = true;
+	adev->enumerated = true;
 }
 
 static inline void acpi_device_clear_enumerated(struct acpi_device *adev)
 {
-	adev->flags.visited = false;
+	adev->enumerated = false;
 }
 
 enum acpi_reconfig_event  {
-- 
2.51.0

[PATCH v2 5/6] ACPI: scan: Adjust and rename acpi_bus_attach()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

After previous changes, the second argument of acpi_bus_attach() is
ignored and none of its callers checks its return value, so rename
it to attach_subtree(), add a void wrapper around it called
acpi_scan_attach(), and adjust its callers to invoke that wrapper.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Peixin Xie <peixin.xie@linux.spacemit.com>
---

v1 -> v2:
   * Add tag from Peixin Xie

---
 drivers/acpi/scan.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 2e853add5e60..f4718b0207e0 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2340,7 +2340,7 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
 	return ret;
 }
 
-static int acpi_bus_attach(struct acpi_device *device, void *not_used)
+static int attach_subtree(struct acpi_device *device, void *not_used)
 {
 	acpi_handle ejd;
 	bool skip;
@@ -2374,8 +2374,10 @@ static int acpi_bus_attach(struct acpi_device *device, void *not_used)
 	else
 		acpi_device_set_enumerated(device);
 
+	acpi_handle_debug(device->handle, "Scanning complete\n");
+
 children:
-	acpi_dev_for_each_child(device, acpi_bus_attach, NULL);
+	acpi_dev_for_each_child(device, attach_subtree, NULL);
 
 	if (!skip && device->handler && device->handler->hotplug.notify_online)
 		device->handler->hotplug.notify_online(device);
@@ -2383,6 +2385,11 @@ static int acpi_bus_attach(struct acpi_device *device, void *not_used)
 	return 0;
 }
 
+static void acpi_scan_attach(struct acpi_device *adev)
+{
+	attach_subtree(adev, NULL);
+}
+
 static int acpi_dev_get_next_consumer_dev_cb(struct acpi_dep_data *dep, void *data)
 {
 	struct acpi_device **adev_p = data;
@@ -2414,7 +2421,7 @@ static void acpi_scan_clear_dep_fn(void *dev, async_cookie_t cookie)
 	struct acpi_device *adev = to_acpi_device(dev);
 
 	acpi_scan_lock_acquire();
-	acpi_bus_attach(adev, (void *)true);
+	acpi_scan_attach(adev);
 	acpi_scan_lock_release();
 
 	acpi_dev_put(adev);
@@ -2427,7 +2434,7 @@ static bool acpi_scan_clear_dep_queue(struct acpi_device *adev)
 
 	/*
 	 * Async schedule the deferred acpi_scan_clear_dep_fn() since:
-	 * - acpi_bus_attach() needs to hold acpi_scan_lock which cannot
+	 * - acpi_scan_attach() needs to run under acpi_scan_lock which cannot
 	 *   be acquired under acpi_dep_list_lock (held here)
 	 * - the deferred work at boot stage is ensured to be finished
 	 *   before userspace init task by the async_synchronize_full()
@@ -2568,7 +2575,7 @@ static void acpi_scan_postponed_branch(acpi_handle handle)
 	 */
 	acpi_mipi_init_crs_csi2_swnodes();
 
-	acpi_bus_attach(adev, NULL);
+	acpi_scan_attach(adev);
 }
 
 static void acpi_scan_postponed(void)
@@ -2725,7 +2732,7 @@ int acpi_bus_scan(acpi_handle handle)
 	acpi_mipi_scan_crs_csi2();
 	acpi_mipi_init_crs_csi2_swnodes();
 
-	acpi_bus_attach(device, (void *)true);
+	acpi_scan_attach(device);
 
 	/* Pass 2: Enumerate all of the remaining devices. */
 
-- 
2.51.0

[PATCH v2 6/6] ACPI: scan: Take PCI device enumeration into account directly

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

The ACPI companions of PCI devices are associated with the corresponding
PCI devices before being processed by acpi_scan_attach() and by the time
they are passed to attach_subtree(), the PCI devices associated with
them have been already enumerated and initialized.

Accordingly, it is not necessary or even useful to check their status in
attach_subtree(), so do not do that.

Fixes: 2c22e6520ac8 ("ACPI / scan: Use direct recurrence for device hierarchy walks")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Peixin Xie <peixin.xie@linux.spacemit.com>
---

v1 -> v2:
   * Use pci_name() (Andy)
   * Add tag from Peixin Xie

---
 drivers/acpi/scan.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f4718b0207e0..34f7f477a25a 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -20,6 +20,7 @@
 #include <linux/kthread.h>
 #include <linux/dmi.h>
 #include <linux/dma-map-ops.h>
+#include <linux/pci.h>
 #include <linux/platform_data/x86/apple.h>
 #include <linux/pgtable.h>
 #include <linux/crc32.h>
@@ -2342,6 +2343,7 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
 
 static int attach_subtree(struct acpi_device *device, void *not_used)
 {
+	struct pci_dev *pci;
 	acpi_handle ejd;
 	bool skip;
 	int ret;
@@ -2353,10 +2355,26 @@ static int attach_subtree(struct acpi_device *device, void *not_used)
 	if (ACPI_SUCCESS(acpi_bus_get_ejd(device->handle, &ejd)))
 		register_dock_dependent_device(device, ejd);
 
-	acpi_bus_get_status(device);
-	/* Skip devices that are not ready for enumeration (e.g. not present) */
-	if (!acpi_dev_ready_for_enumeration(device))
-		return 0;
+	/*
+	 * If the given ACPI device object has been already associated with a
+	 * PCI device found on the bus, its status is effectively "present
+	 * and functional".
+	 */
+	pci = acpi_dev_get_pci_dev(device);
+	if (pci) {
+		acpi_handle_debug(device->handle, "PCI companion %s found\n",
+				  pci_name(pci));
+
+		pci_dev_put(pci);
+	} else {
+		acpi_bus_get_status(device);
+		/*
+		 * Skip devices that are not ready for enumeration (e.g. not
+		 * present).
+		 */
+		if (!acpi_dev_ready_for_enumeration(device))
+			return 0;
+	}
 
 	acpi_ec_register_opregions(device);
 
-- 
2.51.0

Re: [PATCH v2 1/6] ACPI: PM: Drop parent state update from acpi_device_get_power()

[sashiko-bot]

> 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!

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

Re: [PATCH v2 2/6] ACPI: scan: Stop calling acpi_bus_init_power() early

[sashiko-bot]

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)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5144065.31r3eYUQgx@rafael.j.wysocki?part=2

Re: [PATCH v2 3/6] ACPI: scan: Combine two conditionals in acpi_bus_attach()

[sashiko-bot]

> 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!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5144065.31r3eYUQgx@rafael.j.wysocki?part=3

Re: [PATCH v2 4/6] ACPI: scan: Add ACPI device enumerated marker

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message establishes a strict structural invariant: 'The rule regarding flags.visited and the "enumerated" field is that the latter cannot be set until the former has been set and they need to be cleared in reverse order.' However, the implementation violates this rule in multiple paths. For early and fixed devices, `acpi_bus_register_early_device()` and `acpi_bus_add_fixed_device_object()` call `acpi_default_enumeration()`, which sets `enumerated = true`, but their `flags.visited` is never set to 1. Additionally, for generic devices, `acpi_scan_attach_handler()` calls `acpi_generic_device_attach()`, which triggers `acpi_default_enumeration()`, setting `enumerated = true` before `acpi_bus_attach()` sets `device->flags.visited = 1`.
--

commit 193e84bf6b51af7db08c641525537f0a2b63b3c2
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

ACPI: scan: Add ACPI device enumerated marker

Currently, the "visited" flag of ACPI device objects has two roles.
One of them is to indicate that the given ACPI device object has been
enumerated, and the other is to indicate that acpi_bus_attach() has
processed the device. This commit adds a new "enumerated" bool field to
struct acpi_device to separate these purposes.

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 7208cc5158660..2e853add5e605 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
[ ... ]
> @@ -2338,46 +2340,42 @@ static int acpi_scan_attach_handler(struct acpi_device *device)
>  	return ret;
>  }
>  
> -static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
> +static int acpi_bus_attach(struct acpi_device *device, void *not_used)
>  {
[ ... ]
>  	acpi_bus_init_power(device);
>  
> -	if (device->flags.visited)
> -		goto ok;
> -
>  	ret = acpi_scan_attach_handler(device);
>  	if (ret < 0)
>  		return 0;
>  
> +	device->flags.visited = 1;

[Severity: Medium]
The commit message establishes a strict structural invariant: 'The rule
regarding flags.visited and the "enumerated" field is that the latter cannot
be set until the former has been set and they need to be cleared in reverse
order.'

Does the implementation violate this rule in multiple paths?

For generic devices, acpi_scan_attach_handler() (called just above) calls
acpi_generic_device_attach(), which triggers acpi_default_enumeration().
This sets enumerated = true before acpi_bus_attach() reaches this line to
set device->flags.visited = 1.

Additionally, for early and fixed devices, acpi_bus_register_early_device()
and acpi_bus_add_fixed_device_object() call acpi_default_enumeration(),
which sets enumerated = true, but their flags.visited is never set to 1.

> +
>  	if (device->flags.enumeration_by_parent ||
>  	    (!ret && (device->pnp.type.platform_id || device->pnp.type.backlight)))
>  		acpi_default_enumeration(device);
>  	else
>  		acpi_device_set_enumerated(device);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5144065.31r3eYUQgx@rafael.j.wysocki?part=4

Re: [PATCH v2 5/6] ACPI: scan: Adjust and rename acpi_bus_attach()

[sashiko-bot]

> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> After previous changes, the second argument of acpi_bus_attach() is
> ignored and none of its callers checks its return value, so rename
> it to attach_subtree(), add a void wrapper around it called
> acpi_scan_attach(), and adjust its callers to invoke that wrapper.
> 
> 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!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5144065.31r3eYUQgx@rafael.j.wysocki?part=5

Re: [PATCH v2 6/6] ACPI: scan: Take PCI device enumeration into account directly

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Bypassing `acpi_bus_get_status()` for PCI companions leaves `device->status` stale, which can cause `acpi_device_is_present()` to incorrectly return false.
--

commit 7f00077fea1f2ada305a470b1a050f31934232c4
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

ACPI: scan: Take PCI device enumeration into account directly

This commit skips evaluating ACPI device status during hierarchy walks
for devices already associated with a PCI companion, assuming they are
present and functional because the PCI devices are already enumerated.

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index f4718b0207e0e..34f7f477a25ad 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
[ ... ]
> @@ -2353,10 +2355,26 @@ static int attach_subtree(struct acpi_device *device, void *not_used)
>  	if (ACPI_SUCCESS(acpi_bus_get_ejd(device->handle, &ejd)))
>  		register_dock_dependent_device(device, ejd);
>  
> -	acpi_bus_get_status(device);
> -	/* Skip devices that are not ready for enumeration (e.g. not present) */
> -	if (!acpi_dev_ready_for_enumeration(device))
> -		return 0;
> +	/*
> +	 * If the given ACPI device object has been already associated with a
> +	 * PCI device found on the bus, its status is effectively "present
> +	 * and functional".
> +	 */
> +	pci = acpi_dev_get_pci_dev(device);
> +	if (pci) {
> +		acpi_handle_debug(device->handle, "PCI companion %s found\n",
> +				  pci_name(pci));
> +
> +		pci_dev_put(pci);
> +	} else {

[Severity: High]
Bypassing acpi_bus_get_status() for PCI companions leaves device->status
stale. Could this cause acpi_device_is_present() to incorrectly return false?

If the initial evaluation of the device status returned 0 (e.g., due to unmet
dependencies at boot or firmware quirks), device->status.present will remain
0 permanently because it is not explicitly updated here to reflect the
"present and functional" assumption.

This stale status might prevent ACPI power management features, such as
acpi_device_fix_up_power(), and other subsystems relying on
acpi_device_is_present() from properly operating on the device.

> +		acpi_bus_get_status(device);
> +		/*
> +		 * Skip devices that are not ready for enumeration (e.g. not
> +		 * present).
> +		 */
> +		if (!acpi_dev_ready_for_enumeration(device))
> +			return 0;
> +	}
>  
>  	acpi_ec_register_opregions(device);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5144065.31r3eYUQgx@rafael.j.wysocki?part=6

Re: [PATCH v2 3/6] ACPI: scan: Combine two conditionals in acpi_bus_attach()

[Andy Shevchenko]

On Wed, Sep 02, 2026 at 09:30:00PM +0200, Rafael J. Wysocki wrote:

> 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.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] ACPI: scan: Take PCI device enumeration into account directly

[Andy Shevchenko]

On Wed, Sep 02, 2026 at 09:36:54PM +0200, Rafael J. Wysocki wrote:

> The ACPI companions of PCI devices are associated with the corresponding
> PCI devices before being processed by acpi_scan_attach() and by the time
> they are passed to attach_subtree(), the PCI devices associated with
> them have been already enumerated and initialized.
> 
> Accordingly, it is not necessary or even useful to check their status in
> attach_subtree(), so do not do that.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko