[RFT][PATCH v1 0/3] ACPI: scan: Defer enumeration of devices with significant dependencies

[RFT][PATCH v1 0/3] ACPI: scan: Defer enumeration of devices with significant dependencies

[Rafael J. Wysocki]

Hi,

This series addresses some enumeration ordering issues by using information
from _DEP to defer the enumeration of devices that are likely to depend on
operation region (OpRegion) handlers supplied by the drivers of other
devices.

This allows the OpRegion suppliers to be probed and start working before the
devices depending on them are enumerated.

Please see the patch changelogs for details.

Hans, please test this series on the system with OpRegion dependencies in
control methods used for device enumeration.

Thanks!

[RFT][PATCH v1 1/3] ACPI: scan: Evaluate _DEP before adding the device

[Rafael J. Wysocki]

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

Evaluate _DEP before calling acpi_add_single_object() from
acpi_bus_check_add() and do that only for ACPI_BUS_TYPE_DEVICE
objects.

While at it, rename acpi_device_dep_initialize() to
acpi_scan_check_dep(), fix up a memory allocation statement in
that function, consistently treat memory allocation failures in
there as intermittent errors and make some related janitorial
changes in it.

This change will help to avoid calling acpi_add_single_object() if
there are unmet _DEP dependencies in the future, as that may cause
some control methods, potentially depending on the presence of
operation regions supplied by other devices, to be evaluated
prematurely.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1842,32 +1842,30 @@ static void acpi_scan_init_hotplug(struc
 	}
 }
 
-static void acpi_device_dep_initialize(struct acpi_device *adev)
+static u32 acpi_scan_check_dep(acpi_handle handle)
 {
-	struct acpi_dep_data *dep;
 	struct acpi_handle_list dep_devices;
 	acpi_status status;
+	u32 count;
 	int i;
 
-	adev->dep_unmet = 0;
+	if (!acpi_has_method(handle, "_DEP"))
+		return 0;
 
-	if (!acpi_has_method(adev->handle, "_DEP"))
-		return;
-
-	status = acpi_evaluate_reference(adev->handle, "_DEP", NULL,
-					&dep_devices);
+	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
 	if (ACPI_FAILURE(status)) {
-		dev_dbg(&adev->dev, "Failed to evaluate _DEP.\n");
-		return;
+		acpi_handle_debug(handle, "Failed to evaluate _DEP.\n");
+		return 0;
 	}
 
-	for (i = 0; i < dep_devices.count; i++) {
+	for (count = 0, i = 0; i < dep_devices.count; i++) {
 		struct acpi_device_info *info;
-		int skip;
+		struct acpi_dep_data *dep;
+		bool skip;
 
 		status = acpi_get_object_info(dep_devices.handles[i], &info);
 		if (ACPI_FAILURE(status)) {
-			dev_dbg(&adev->dev, "Error reading _DEP device info\n");
+			acpi_handle_debug(handle, "Error reading _DEP device info\n");
 			continue;
 		}
 
@@ -1877,26 +1875,30 @@ static void acpi_device_dep_initialize(s
 		if (skip)
 			continue;
 
-		dep = kzalloc(sizeof(struct acpi_dep_data), GFP_KERNEL);
+		dep = kzalloc(sizeof(*dep), GFP_KERNEL);
 		if (!dep)
-			return;
+			continue;
+
+		count++;
 
 		dep->supplier = dep_devices.handles[i];
-		dep->consumer  = adev->handle;
-		adev->dep_unmet++;
+		dep->consumer = handle;
 
 		mutex_lock(&acpi_dep_list_lock);
 		list_add_tail(&dep->node , &acpi_dep_list);
 		mutex_unlock(&acpi_dep_list_lock);
 	}
+
+	return count;
 }
 
 static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
 				      void *not_used, void **return_value)
 {
 	struct acpi_device *device = NULL;
-	int type;
+	u32 dep_count = 0;
 	unsigned long long sta;
+	int type;
 	int result;
 
 	acpi_bus_get_device(handle, &device);
@@ -1912,12 +1914,16 @@ static acpi_status acpi_bus_check_add(ac
 		return AE_OK;
 	}
 
+	if (type == ACPI_BUS_TYPE_DEVICE)
+		dep_count = acpi_scan_check_dep(handle);
+
 	acpi_add_single_object(&device, handle, type, sta);
 	if (!device)
 		return AE_CTRL_DEPTH;
 
+	device->dep_unmet = dep_count;
+
 	acpi_scan_init_hotplug(device);
-	acpi_device_dep_initialize(device);
 
  out:
 	if (!*return_value)

[RFT][PATCH v1 2/3] ACPI: scan: Defer enumeration of devices with _DEP lists

[Rafael J. Wysocki]

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

In some cases ACPI control methods used during device enumeration
(such as _HID or _STA) may rely on Operation Region handlers
supplied by the drivers of other devices [1]:

 An example of this is the Acer Switch 10E SW3-016 model. The _HID
 method of the ACPI node for the UART attached Bluetooth, reads
 GPIOs to detect the installed wifi chip and update the _HID for the
 Bluetooth's ACPI node accordingly. The current ACPI scan code calls
 _HID before the GPIO controller's OpRegions are available, leading
 to the wrong _HID being used and Bluetooth not working.

In principle, in those cases there should be a _DEP control method
under the device object with OpRegion enumeration dependencies, so
deferring the enumeration of devices with _DEP returning a non-empty
list of suppliers of OpRegions depended on by the given device
(modulo some known exceptions that don't really supply any OpRegions
and are listed by _DEP for other reasons irrelevant for Linux) should
at least address the first-order dependencies by allowing the OpRegion
suppliers to be enumerated before their consumers.

Implement the above idea by modifying acpi_bus_scan() to enumerate
devices in the given scope of the ACPI namespace in two passes,
where the first pass covers the devices without "significant" lists
of dependencies coming from _DEP only and the second pass covers
all of the devices that were not enumerated in the first pass.

Take _DEP into account only for device objects with _HID, mostly in
order to avoid deferring the creation of ACPI device objects that
represent PCI devices and must be present during the enumeration
of the PCI bus (which takes place during the processing of the ACPI
device object that represents the host bridge), so that they can
be properly associated with the corresponding PCI devices.

Link: https://lore.kernel.org/linux-acpi/20201121203040.146252-1-hdegoede@redhat.com/ # [1]
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |  103 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 78 insertions(+), 25 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1635,8 +1635,6 @@ void acpi_init_device_object(struct acpi
 	device_initialize(&device->dev);
 	dev_set_uevent_suppress(&device->dev, true);
 	acpi_init_coherency(device);
-	/* Assume there are unmet deps until acpi_device_dep_initialize() runs */
-	device->dep_unmet = 1;
 }
 
 void acpi_device_add_finalize(struct acpi_device *device)
@@ -1849,7 +1847,13 @@ static u32 acpi_scan_check_dep(acpi_hand
 	u32 count;
 	int i;
 
-	if (!acpi_has_method(handle, "_DEP"))
+	/*
+	 * Check for _HID here to avoid deferring the enumeration of:
+	 * 1. PCI devices.
+	 * 2. ACPI nodes describing USB ports.
+	 * Still, checking for _HID catches more then just these cases ...
+	 */
+	if (!acpi_has_method(handle, "_DEP") || !acpi_has_method(handle, "_HID"))
 		return 0;
 
 	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
@@ -1892,11 +1896,24 @@ static u32 acpi_scan_check_dep(acpi_hand
 	return count;
 }
 
-static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
-				      void *not_used, void **return_value)
+static void acpi_scan_dep_init(struct acpi_device *adev)
+{
+	struct acpi_dep_data *dep;
+
+	mutex_lock(&acpi_dep_list_lock);
+
+	list_for_each_entry(dep, &acpi_dep_list, node) {
+		if (dep->consumer == adev->handle)
+			adev->dep_unmet++;
+	}
+
+	mutex_unlock(&acpi_dep_list_lock);
+}
+
+static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
+				      struct acpi_device **adev_p)
 {
 	struct acpi_device *device = NULL;
-	u32 dep_count = 0;
 	unsigned long long sta;
 	int type;
 	int result;
@@ -1914,24 +1931,40 @@ static acpi_status acpi_bus_check_add(ac
 		return AE_OK;
 	}
 
-	if (type == ACPI_BUS_TYPE_DEVICE)
-		dep_count = acpi_scan_check_dep(handle);
+	if (type == ACPI_BUS_TYPE_DEVICE && check_dep) {
+		u32 count = acpi_scan_check_dep(handle);
+		/* Bail out if the number of recorded dependencies is not 0. */
+		if (count > 0)
+			return AE_CTRL_DEPTH;
+	}
 
 	acpi_add_single_object(&device, handle, type, sta);
 	if (!device)
 		return AE_CTRL_DEPTH;
 
-	device->dep_unmet = dep_count;
-
 	acpi_scan_init_hotplug(device);
+	if (!check_dep)
+		acpi_scan_dep_init(device);
 
- out:
-	if (!*return_value)
-		*return_value = device;
+out:
+	if (!*adev_p)
+		*adev_p = device;
 
 	return AE_OK;
 }
 
+static acpi_status acpi_bus_check_add_1(acpi_handle handle, u32 lvl_not_used,
+					void *not_used, void **ret_p)
+{
+	return acpi_bus_check_add(handle, true, (struct acpi_device **)ret_p);
+}
+
+static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
+					void *not_used, void **ret_p)
+{
+	return acpi_bus_check_add(handle, false, (struct acpi_device **)ret_p);
+}
+
 static void acpi_default_enumeration(struct acpi_device *device)
 {
 	/*
@@ -1999,12 +2032,16 @@ static int acpi_scan_attach_handler(stru
 	return ret;
 }
 
-static void acpi_bus_attach(struct acpi_device *device)
+static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
 {
 	struct acpi_device *child;
+	bool skip = !first_pass && device->flags.visited;
 	acpi_handle ejd;
 	int ret;
 
+	if (skip)
+		goto ok;
+
 	if (ACPI_SUCCESS(acpi_bus_get_ejd(device->handle, &ejd)))
 		register_dock_dependent_device(device, ejd);
 
@@ -2051,9 +2088,9 @@ static void acpi_bus_attach(struct acpi_
 
  ok:
 	list_for_each_entry(child, &device->children, node)
-		acpi_bus_attach(child);
+		acpi_bus_attach(child, first_pass);
 
-	if (device->handler && device->handler->hotplug.notify_online)
+	if (!skip && device->handler && device->handler->hotplug.notify_online)
 		device->handler->hotplug.notify_online(device);
 }
 
@@ -2071,7 +2108,8 @@ void acpi_walk_dep_device_list(acpi_hand
 
 			adev->dep_unmet--;
 			if (!adev->dep_unmet)
-				acpi_bus_attach(adev);
+				acpi_bus_attach(adev, true);
+
 			list_del(&dep->node);
 			kfree(dep);
 		}
@@ -2096,17 +2134,32 @@ EXPORT_SYMBOL_GPL(acpi_walk_dep_device_l
  */
 int acpi_bus_scan(acpi_handle handle)
 {
-	void *device = NULL;
+	struct acpi_device *device = NULL;
 
-	if (ACPI_SUCCESS(acpi_bus_check_add(handle, 0, NULL, &device)))
+	/* Pass 1: Avoid enumerating devices with missing dependencies. */
+
+	if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &device)))
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-				    acpi_bus_check_add, NULL, NULL, &device);
+				    acpi_bus_check_add_1, NULL, NULL,
+				    (void **)&device);
 
-	if (device) {
-		acpi_bus_attach(device);
-		return 0;
-	}
-	return -ENODEV;
+	if (!device)
+		return -ENODEV;
+
+	acpi_bus_attach(device, true);
+
+	/* Pass 2: Enumerate all of the remaining devices. */
+
+	device = NULL;
+
+	if (ACPI_SUCCESS(acpi_bus_check_add(handle, false, &device)))
+		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				    acpi_bus_check_add_2, NULL, NULL,
+				    (void **)&device);
+
+	acpi_bus_attach(device, false);
+
+	return 0;
 }
 EXPORT_SYMBOL(acpi_bus_scan);
 

[RFT][PATCH v1 3/3] ACPI: scan: Avoid unnecessary second pass in acpi_bus_scan()

[Rafael J. Wysocki]

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

If there are no devices whose enumeration has been deferred after
the first pass in acpi_bus_scan(), the second pass is not necssary,
so avoid it with the help of a new static variable.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1910,6 +1910,8 @@ static void acpi_scan_dep_init(struct ac
 	mutex_unlock(&acpi_dep_list_lock);
 }
 
+static bool acpi_bus_scan_second_pass;
+
 static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
 				      struct acpi_device **adev_p)
 {
@@ -1934,8 +1936,10 @@ static acpi_status acpi_bus_check_add(ac
 	if (type == ACPI_BUS_TYPE_DEVICE && check_dep) {
 		u32 count = acpi_scan_check_dep(handle);
 		/* Bail out if the number of recorded dependencies is not 0. */
-		if (count > 0)
+		if (count > 0) {
+			acpi_bus_scan_second_pass = true;
 			return AE_CTRL_DEPTH;
+		}
 	}
 
 	acpi_add_single_object(&device, handle, type, sta);
@@ -2136,6 +2140,8 @@ int acpi_bus_scan(acpi_handle handle)
 {
 	struct acpi_device *device = NULL;
 
+	acpi_bus_scan_second_pass = false;
+
 	/* Pass 1: Avoid enumerating devices with missing dependencies. */
 
 	if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &device)))
@@ -2148,6 +2154,9 @@ int acpi_bus_scan(acpi_handle handle)
 
 	acpi_bus_attach(device, true);
 
+	if (!acpi_bus_scan_second_pass)
+		return 0;
+
 	/* Pass 2: Enumerate all of the remaining devices. */
 
 	device = NULL;

Re: [RFT][PATCH v1 0/3] ACPI: scan: Defer enumeration of devices with significant dependencies

[Hans de Goede]

Hi,

On 12/14/20 9:23 PM, Rafael J. Wysocki wrote:
> Hi,
> 
> This series addresses some enumeration ordering issues by using information
> from _DEP to defer the enumeration of devices that are likely to depend on
> operation region (OpRegion) handlers supplied by the drivers of other
> devices.
> 
> This allows the OpRegion suppliers to be probed and start working before the
> devices depending on them are enumerated.
> 
> Please see the patch changelogs for details.
> 
> Hans, please test this series on the system with OpRegion dependencies in
> control methods used for device enumeration.

Thank you for you work on this!

I started with reviewing the series so that I would know what to
expect during testing. All 3 patches look good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

As is the series does not fix the Bluetooth HID misreporting on
the system with OpRegion dependencies in its BT ACPI node's _HID,
but ...

This is expected because that _HID has a _DEP on the GPO1 GPIO
controller, which in turn has a _DEP pointing to the
"Intel Baytrail Mailbox Device" ("INT33BD")

We (Linux) don't do anything with the INT33BD device, so it can
simply be added to the acpi_ignore_dep_ids list. With this done the
3 patches from this RFT fix the BT _HID issue. IOW everything works
as expected / as we want.

While at it I also booted the kernel with these patches on a
Lenovo Yoga X1 Carbon 8th gen. I'm not seeing any adverse side
effects there, so please add my tested-by to the entire series:

Tested-by: Hans de Goede <hdegoede@redhat.com>

I will send out a patch adding "INT33BD" to the
acpi_ignore_dep_ids list (to be applied on top of this series)
right away.

Regards,

Hans

Re: [RFT][PATCH v1 0/3] ACPI: scan: Defer enumeration of devices with significant dependencies

[Mika Westerberg]

Hi Rafael,

On Mon, Dec 14, 2020 at 09:23:47PM +0100, Rafael J. Wysocki wrote:
> Hi,
> 
> This series addresses some enumeration ordering issues by using information
> from _DEP to defer the enumeration of devices that are likely to depend on
> operation region (OpRegion) handlers supplied by the drivers of other
> devices.
> 
> This allows the OpRegion suppliers to be probed and start working before the
> devices depending on them are enumerated.

For the whole series,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>