[PATCH v1 0/7] ACPI: scan: Address power management initialization and PCI devices handling

[PATCH v1 0/7] ACPI: scan: Address power management initialization and PCI devices handling

[Rafael J. Wysocki]

Hi All,

This series aims at addressing a few issues present in the core ACPI device
enumeration code, mostly related to the acpi_bus_attach() function (that
gets renamed in one of the patches).

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.

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

I have tested this series on a couple of machines, but let's see what
Sashiko has to say about it.

Thanks!


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

[PATCH v1 1/7] 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 causes devices with missing dependencies to be
put into power state D0 prematurely on some systems [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()
will run again for the device and now it will call
acpi_bus_init_power() that will take 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.

To address this, 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
calling acpi_bus_attach() 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().  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
acpi_bus_init_power() needs to be called by acpi_bus_attach()
instead of using the "initialized" flag of the ACPI device object
for this purpose, which is fragile and inconvenient, and stop
clearing the power_manageable flag for devices with unmet
dependencies, which is not necessary.

While at it, add a debug message pringing 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>
---
 drivers/acpi/device_pm.c | 19 ++++++++++---------
 drivers/acpi/scan.c      | 18 ++++++------------
 2 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index aa55ecfc2923..a680e6972a8c 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;
+		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));
@@ -309,15 +315,6 @@ int acpi_bus_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;
@@ -351,6 +348,10 @@ 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;
 }
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f48715ed827c..1ad8dffc2daf 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,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))
+		device->flags.power_manageable = 0;
 
-		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 v1 2/7] ACPI: PM: Introduce acpi_device_init_power()

[Rafael J. Wysocki]

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

Two out of three callers of acpi_bus_init_power() need to clear
flags.power_manageable for the target device on errors, which
is somewhat cumbersome, so rename the function to
__acpi_device_init_power(), add a wrapper called
acpi_device_init_power() around it that will take care
of the flags.power_manageable clearing, and make the two
callers of acpi_bus_init_power() in question invoke that
wrapper.

While at it, clean up the declaration of local variables
in __acpi_device_init_power().

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/device_pm.c | 22 +++++++++++++++-------
 drivers/acpi/scan.c      |  5 ++---
 include/acpi/acpi_bus.h  |  2 +-
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index a680e6972a8c..7fd780125177 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -169,10 +169,8 @@ int acpi_device_set_power(struct acpi_device *device, int state)
 		return -EINVAL;
 
 	if (device->power.state == ACPI_STATE_UNKNOWN &&
-	    acpi_bus_init_power(device)) {
-		device->flags.power_manageable = 0;
+	    acpi_device_init_power(device))
 		return -ENODEV;
-	}
 
 	acpi_handle_debug(device->handle, "Power state change: %s -> %s\n",
 			  acpi_power_state_string(device->power.state),
@@ -310,10 +308,9 @@ 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;
+	int result, state;
 
 	result = acpi_device_get_power(device, &state);
 	if (result)
@@ -355,6 +352,17 @@ int acpi_bus_init_power(struct acpi_device *device)
 	return 0;
 }
 
+int acpi_device_init_power(struct acpi_device *device)
+{
+	int ret;
+
+	ret = __acpi_device_init_power(device);
+	if (ret)
+		device->flags.power_manageable = 0;
+
+	return ret;
+}
+
 /**
  * acpi_device_fix_up_power - Force device with missing _PSC into D0.
  * @device: Device object whose power state is to be fixed up.
@@ -417,7 +425,7 @@ int acpi_device_update_power(struct acpi_device *device, int *state_p)
 	int result;
 
 	if (device->power.state == ACPI_STATE_UNKNOWN) {
-		result = acpi_bus_init_power(device);
+		result = __acpi_device_init_power(device);
 		if (!result && state_p)
 			*state_p = device->power.state;
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1ad8dffc2daf..5aa3ecb000e1 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2362,9 +2362,8 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
 	acpi_ec_register_opregions(device);
 
 	if (device->flags.power_manageable &&
-	    device->power.state == ACPI_STATE_UNKNOWN &&
-	    acpi_bus_init_power(device))
-		device->flags.power_manageable = 0;
+	    device->power.state == ACPI_STATE_UNKNOWN)
+		acpi_device_init_power(device);
 
 	if (device->flags.visited)
 		goto ok;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 1a45e0d521d8..b0d9057ccb8a 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -618,7 +618,7 @@ int acpi_bus_get_status(struct acpi_device *device);
 int acpi_bus_set_power(acpi_handle handle, int state);
 const char *acpi_power_state_string(int state);
 int acpi_device_set_power(struct acpi_device *device, int state);
-int acpi_bus_init_power(struct acpi_device *device);
+int acpi_device_init_power(struct acpi_device *device);
 int acpi_device_fix_up_power(struct acpi_device *device);
 void acpi_device_fix_up_power_extended(struct acpi_device *adev);
 void acpi_device_fix_up_power_children(struct acpi_device *adev);
-- 
2.51.0

[PATCH v1 3/7] ACPI: bus: Drop initialized flag from struct acpi_device_flags

[Rafael J. Wysocki]

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

After previous changes, the ACPI device object "initialized" flag is
not particularly useful.  It is set almost all the time and it is
only checked along with the "visited" flag, but checking the latter
is sufficient.

Drop it accordingly.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c     | 2 --
 include/acpi/acpi_bus.h | 5 ++---
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 5aa3ecb000e1..019a43e3b5d7 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -281,7 +281,6 @@ static int acpi_scan_check_and_detach(struct acpi_device *adev, void *p)
 	 * that.
 	 */
 	acpi_device_set_power(adev, ACPI_STATE_D3_COLD);
-	adev->flags.initialized = false;
 
 	/* For eject this is deferred to acpi_bus_post_eject() */
 	if (!(flags & ACPI_SCAN_CHECK_FLAG_EJECT)) {
@@ -1827,7 +1826,6 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	acpi_set_pnp_ids(handle, &device->pnp, type);
 	acpi_init_properties(device);
 	acpi_bus_get_flags(device);
-	device->flags.initialized = true;
 	device->flags.enumeration_by_parent =
 		acpi_device_enumeration_by_parent(device);
 	acpi_device_clear_enumerated(device);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index b0d9057ccb8a..596fbd748e88 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -180,7 +180,6 @@ struct acpi_device_flags {
 	u32 removable:1;
 	u32 ejectable:1;
 	u32 power_manageable:1;
-	u32 initialized:1;
 	u32 visited:1;
 	u32 hotplug_notify:1;
 	u32 is_dock_station:1;
@@ -189,7 +188,7 @@ struct acpi_device_flags {
 	u32 cca_seen:1;
 	u32 enumeration_by_parent:1;
 	u32 honor_deps:1;
-	u32 reserved:19;
+	u32 reserved:20;
 };
 
 /* File System */
@@ -654,7 +653,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.visited;
 }
 
 /*
-- 
2.51.0

[PATCH v1 4/7] 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>
---
 drivers/acpi/scan.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 019a43e3b5d7..8c5a2fcef582 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2370,16 +2370,11 @@ 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) {
+	if (!device->flags.enumeration_by_parent && (ret > 0 ||
+	    (!device->pnp.type.platform_id && !device->pnp.type.backlight)))
 		acpi_device_set_enumerated(device);
-		goto ok;
-	}
-
-	if (device->pnp.type.platform_id || device->pnp.type.backlight ||
-	    device->flags.enumeration_by_parent)
-		acpi_default_enumeration(device);
 	else
-		acpi_device_set_enumerated(device);
+		acpi_default_enumeration(device);
 
 ok:
 	acpi_dev_for_each_child(device, acpi_bus_attach, first_pass);
-- 
2.51.0

[PATCH v1 5/7] 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>
---
 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 8c5a2fcef582..153c66ca9217 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;
 		}
@@ -286,6 +286,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;
 }
@@ -304,6 +305,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;
 }
@@ -2336,26 +2338,23 @@ 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);
 
@@ -2363,21 +2362,20 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
 	    device->power.state == ACPI_STATE_UNKNOWN)
 		acpi_device_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 > 0 ||
 	    (!device->pnp.type.platform_id && !device->pnp.type.backlight)))
 		acpi_device_set_enumerated(device);
 	else
 		acpi_default_enumeration(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 596fbd748e88..4392e3d6cd7a 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -467,6 +467,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 */
@@ -653,7 +654,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.visited;
+	return adev && 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 v1 6/7] 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>
---
 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 153c66ca9217..517126fa2d50 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2338,7 +2338,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_default_enumeration(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 v1 7/7] 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 (including power
management initialization).

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>
---
 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 517126fa2d50..53e11cbb79ce 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>
@@ -2340,6 +2341,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;
@@ -2351,10 +2353,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, the device can be regarded as present
+	 * and functional.
+	 */
+	pci = acpi_dev_get_pci_dev(device);
+	if (pci) {
+		acpi_handle_debug(device->handle, "PCI companion %s found\n",
+				  dev_name(&pci->dev));
+
+		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 v1 1/7] ACPI: scan: Stop calling acpi_bus_init_power() early

[Andy Shevchenko]

On Mon, Aug 31, 2026 at 06:24:45PM +0200, Rafael J. Wysocki wrote:
> 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 causes devices with missing dependencies to be
> put into power state D0 prematurely on some systems [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()
> will run again for the device and now it will call
> acpi_bus_init_power() that will take 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.
> 
> To address this, 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
> calling acpi_bus_attach() 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().  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
> acpi_bus_init_power() needs to be called by acpi_bus_attach()
> instead of using the "initialized" flag of the ACPI device object
> for this purpose, which is fragile and inconvenient, and stop
> clearing the power_manageable flag for devices with unmet
> dependencies, which is not necessary.
> 
> While at it, add a debug message pringing statement to
> acpi_bus_init_power() to facilitate diagnostics.

...

> +	if (device->power.state == ACPI_STATE_UNKNOWN &&
> +	    acpi_bus_init_power(device)) {
> +		device->flags.power_manageable = 0;
> +		return -ENODEV;
> +	}

In this form it might be harder to catch the side effect of the conditional.
I would split it into two:


	if (device->power.state == ACPI_STATE_UNKNOWN) {
		int result;

		result = acpi_bus_init_power(device);
		if (result) {
			device->flags.power_manageable = 0;
			return result; // shouldn't we instead of return -ENODEV?
		}
	}

...

> +	if (device->flags.power_manageable &&
> +	    device->power.state == ACPI_STATE_UNKNOWN &&
> +	    acpi_bus_init_power(device))
> +		device->flags.power_manageable = 0;

In the similar way. And it might be even worth to have a helper to deduplicate
this check and setting?

static inline int ...()
{
	int result;

	if (device->power.state != ACPI_STATE_UNKNOWN)
		return 0;

	result = acpi_bus_init_power(device);
	if (result)
		device->flags.power_manageable = 0;

	return result;
}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/7] ACPI: PM: Introduce acpi_device_init_power()

[Andy Shevchenko]

On Mon, Aug 31, 2026 at 06:25:52PM +0200, Rafael J. Wysocki wrote:

> Two out of three callers of acpi_bus_init_power() need to clear
> flags.power_manageable for the target device on errors, which
> is somewhat cumbersome, so rename the function to
> __acpi_device_init_power(), add a wrapper called
> acpi_device_init_power() around it that will take care
> of the flags.power_manageable clearing, and make the two
> callers of acpi_bus_init_power() in question invoke that
> wrapper.
> 
> While at it, clean up the declaration of local variables
> in __acpi_device_init_power().

...

>  {
> -	int state;
> -	int result;
> +	int result, state;

Not sure if this change is required.

>  	result = acpi_device_get_power(device, &state);
>  	if (result)

...

> +int acpi_device_init_power(struct acpi_device *device)

Ah, here is a helper!

> +{
> +	int ret;

Elsewhere in the file it's called 'result'.

> +
> +	ret = __acpi_device_init_power(device);
> +	if (ret)
> +		device->flags.power_manageable = 0;
> +
> +	return ret;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/7] ACPI: scan: Combine two conditionals in acpi_bus_attach()

[Andy Shevchenko]

On Mon, Aug 31, 2026 at 07:59:32PM +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.

...

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

I would leave a longer line (having logical split)

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

Or even going further and cleaning too many negations (if I'm not mistaken in
the logic)

	if (device->flags.enumeration_by_parent ||
// not sure what the expected ret values here, maybe < 0 or == 0 part is not needed
	    (ret <= 0 && (device->pnp.type.platform_id || device->pnp.type.backlight)))
		acpi_default_enumeration(device);
	else
		acpi_device_set_enumerated(device);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 7/7] ACPI: scan: Take PCI device enumeration into account directly

[Andy Shevchenko]

On Mon, Aug 31, 2026 at 07:59:48PM +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 (including power
> management initialization).
> 
> Accordingly, it is not necessary or even useful to check their status in
> attach_subtree(), so do not do that.

...

> +	if (pci) {
> +		acpi_handle_debug(device->handle, "PCI companion %s found\n",
> +				  dev_name(&pci->dev));

We have pci_name().

> +
> +		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;
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/7] ACPI: PM: Introduce acpi_device_init_power()

[Rafael J. Wysocki (Intel)]

On Tue, Sep 1, 2026 at 10:29 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Aug 31, 2026 at 06:25:52PM +0200, Rafael J. Wysocki wrote:
>
> > Two out of three callers of acpi_bus_init_power() need to clear
> > flags.power_manageable for the target device on errors, which
> > is somewhat cumbersome, so rename the function to
> > __acpi_device_init_power(), add a wrapper called
> > acpi_device_init_power() around it that will take care
> > of the flags.power_manageable clearing, and make the two
> > callers of acpi_bus_init_power() in question invoke that
> > wrapper.
> >
> > While at it, clean up the declaration of local variables
> > in __acpi_device_init_power().
>
> ...
>
> >  {
> > -     int state;
> > -     int result;
> > +     int result, state;
>
> Not sure if this change is required.

Nope, but I think that it's useful.

> >       result = acpi_device_get_power(device, &state);
> >       if (result)
>
> ...
>
> > +int acpi_device_init_power(struct acpi_device *device)
>
> Ah, here is a helper!

Yeah, I guess it's better to fold it into the first patch.

> > +{
> > +     int ret;
>
> Elsewhere in the file it's called 'result'.

Yes, it is.

So I guess you're suggesting to also use that name here, which is fair enough.

> > +
> > +     ret = __acpi_device_init_power(device);
> > +     if (ret)
> > +             device->flags.power_manageable = 0;
> > +
> > +     return ret;
> > +}

Re: [PATCH v1 4/7] ACPI: scan: Combine two conditionals in acpi_bus_attach()

[Rafael J. Wysocki (Intel)]

On Tue, Sep 1, 2026 at 10:40 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Aug 31, 2026 at 07:59:32PM +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.
>
> ...
>
> > -     if (ret > 0 && !device->flags.enumeration_by_parent) {
> > +     if (!device->flags.enumeration_by_parent && (ret > 0 ||
> > +         (!device->pnp.type.platform_id && !device->pnp.type.backlight)))
> >               acpi_device_set_enumerated(device);
> > -             goto ok;
> > -     }
> > -
> > -     if (device->pnp.type.platform_id || device->pnp.type.backlight ||
> > -         device->flags.enumeration_by_parent)
> > -             acpi_default_enumeration(device);
> >       else
> > -             acpi_device_set_enumerated(device);
> > +             acpi_default_enumeration(device);
>
> I would leave a longer line (having logical split)
>
>         if (!device->flags.enumeration_by_parent &&
>             (ret > 0 || (!device->pnp.type.platform_id && !device->pnp.type.backlight)))
>                 acpi_device_set_enumerated(device);
>         else
>                 acpi_default_enumeration(device);
>
> Or even going further and cleaning too many negations (if I'm not mistaken in
> the logic)
>
>         if (device->flags.enumeration_by_parent ||
> // not sure what the expected ret values here, maybe < 0 or == 0 part is not needed

!ret should be fine.

>             (ret <= 0 && (device->pnp.type.platform_id || device->pnp.type.backlight)))
>                 acpi_default_enumeration(device);
>         else
>                 acpi_device_set_enumerated(device);

Yes, it looks better this way.

Re: [PATCH v1 7/7] ACPI: scan: Take PCI device enumeration into account directly

[Rafael J. Wysocki (Intel)]

On Tue, Sep 1, 2026 at 10:44 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Aug 31, 2026 at 07:59:48PM +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 (including power
> > management initialization).
> >
> > Accordingly, it is not necessary or even useful to check their status in
> > attach_subtree(), so do not do that.
>
> ...
>
> > +     if (pci) {
> > +             acpi_handle_debug(device->handle, "PCI companion %s found\n",
> > +                               dev_name(&pci->dev));
>
> We have pci_name().

Ah, OK

Re: [PATCH v1 0/7] ACPI: scan: Address power management initialization and PCI devices handling

[Peixin Xie]

Hi Rafael,

On Mon, Aug 31, 2026 at 06:20:42PM +0200, Rafael J. Wysocki wrote:
> Hi All,
> 
> This series aims at addressing a few issues present in the core ACPI device
> enumeration code, mostly related to the acpi_bus_attach() function (that
> gets renamed in one of the patches).
> 
> 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.
> 
> 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 that 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.
> 
> I have tested this series on a couple of machines, but let's see what
> Sashiko has to say about it.
> 
> Thanks!

I tested the complete series on the SpacemiT K3 RISC-V Pico-ITX platform
where the original duplicate ACPI PowerResource reference issue was
observed.

The power resource left on by firmware is now turned off as unused after
the namespace scan. After the device dependency is satisfied, the power
resource is turned on once for the device and is turned off normally
during runtime suspend. Runtime resume also works correctly.

Tested-by: Peixin Xie <peixin.xie@linux.spacemit.com>

-- 
Best Regards,
Peixin Xie