[PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Hans de Goede]

Hi Rafael,

A while ago (almost 2 years ago) I discussed an issue with you about
some devices, where some of the methods used during device-addition
(such as _HID) may rely on OpRegions of other devices:

https://www.spinics.net/lists/linux-acpi/msg86303.html

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.

Last week I bought a second hand Acer device, not knowing it was this
exact model. Since I now have access to affected hardware I decided to
take a shot at fixing this.

In the discussion you suggested to split the acpi_bus_scan of the root
into 2 steps, first scan devices with an empty _DEP, putting other
acpi_handle-s on a list of deferred devices and then in step 2 scan the
rest.

I'm happy to report that, at least on the affected device, this works
nicely. While working on this I came up with a less drastic way to
deal with this. As you will see in patch 4 of this series, I decided
to first add a more KISS method of deciding which devices to defer
to the second scan step by matching by HID. This has the disadvantage
of not being a generic solution. But it has the advantage of being a
solution which does not potentially regress other devices.

Then in patch 5 I actually do add the option to defer or not based on
_DEP being empty. I've put this behind a kernel commandline option as
I'm not sure we should do this everywhere by default. At least no without
a lot more testing.

Patch 6 fixes an issue with patch 5 which causes battery devices to stop
working.

And patch 7 adds some extra HIDs to the list of HIDs which should be
ignored when checking if the _DEP list is empty from Linux' pov, iow
some extra HIDs which Linux does not bind to.

Please let me know what you think about this patch-set. I would be happy
to see just patches 1-4 merged.

If you dislike the HID match approach I can drop that and add a DMI-quirk
list of devices which need the new 2-step process (for now), to fix those
without regressing the OOTB experience on other devices.

Or we could just entirely switch to the new scheme in one big step, but
that seems a bit too adventurous.

Regards,

Hans

[PATCH 1/7] ACPI: scan: Add an acpi_info_matches_hids() helper

[Hans de Goede]

acpi_device_dep_initialize() skips _DEPS with a HID of "INT3396"
and checks this using an acpi_device_info struct.

The upcoming changes splitting the scanning of the root into 2 steps,
deferring the addition of some devices which need access to OpRegions of
other devices during scanning to the second step, need to extend the
HIDs to skip during _DEP checking to a list of HIDs; and need to check
an acpi_device_info struct against a list of HIDs in more places.

Add an acpi_info_matches_hids() helper which checks a list of HIDs
for this and switch acpi_device_dep_initialize() over to this helper.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index bc6a79e33220..e949265d5667 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -719,6 +719,28 @@ int acpi_device_add(struct acpi_device *device,
 /* --------------------------------------------------------------------------
                                  Device Enumeration
    -------------------------------------------------------------------------- */
+static bool acpi_info_matches_hids(struct acpi_device_info *info,
+				   const char * const hids[])
+{
+	int i;
+
+	if (!(info->valid & ACPI_VALID_HID))
+		return false;
+
+	for (i = 0; hids[i]; i++) {
+		if (!strcmp(info->hardware_id.string, hids[i]))
+			return true;
+	}
+
+	return false;
+}
+
+/* List of HIDs for which we ignore matching ACPI devices, when checking _DEP lists. */
+static const char * const acpi_ignore_dep_hids[] = {
+	"INT3396", /* Windows System Power Management Controller */
+	NULL
+};
+
 static struct acpi_device *acpi_bus_get_parent(acpi_handle handle)
 {
 	struct acpi_device *device = NULL;
@@ -1833,13 +1855,7 @@ static void acpi_device_dep_initialize(struct acpi_device *adev)
 			continue;
 		}
 
-		/*
-		 * Skip the dependency of Windows System Power
-		 * Management Controller
-		 */
-		skip = info->valid & ACPI_VALID_HID &&
-			!strcmp(info->hardware_id.string, "INT3396");
-
+		skip = acpi_info_matches_hids(info, acpi_ignore_dep_hids);
 		kfree(info);
 
 		if (skip)
-- 
2.28.0

[PATCH 2/7] ACPI: scan: Call acpi_get_object_info() from acpi_add_single_object()

[Hans de Goede]

Call acpi_get_object_info() from acpi_add_single_object() instead of
calling it from acpi_set_pnp_ids() and pass the result down to the
acpi_set_pnp_ids() call.

This is a preparation patch for splitting the scanning of the root
into 2 steps, deferring the addition of some devices which need access
to OpRegions of other devices during scanning to the second step.

Having the acpi_device_info available earlier is useful for making
the decision on whether to defer the device addition or not, without
needing to make a second acpi_get_object_info() call.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/internal.h |  3 ++-
 drivers/acpi/power.c    |  2 +-
 drivers/acpi/scan.c     | 22 ++++++++++++----------
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index e3638bafb941..cb229e24c563 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -105,7 +105,8 @@ struct acpi_device_bus_id {
 int acpi_device_add(struct acpi_device *device,
 		    void (*release)(struct device *));
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-			     int type, unsigned long long sta);
+			     int type, unsigned long long sta,
+			     struct acpi_device_info *info);
 int acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
 void acpi_device_add_finalize(struct acpi_device *device);
diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index 8048da85b7e0..189a0d4c6d06 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -939,7 +939,7 @@ int acpi_add_power_resource(acpi_handle handle)
 
 	device = &resource->device;
 	acpi_init_device_object(device, handle, ACPI_BUS_TYPE_POWER,
-				ACPI_STA_DEFAULT);
+				ACPI_STA_DEFAULT, NULL);
 	mutex_init(&resource->resource_lock);
 	INIT_LIST_HEAD(&resource->list_node);
 	INIT_LIST_HEAD(&resource->dependents);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e949265d5667..25a46ae24229 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1258,10 +1258,8 @@ static bool acpi_object_is_system_bus(acpi_handle handle)
 }
 
 static void acpi_set_pnp_ids(acpi_handle handle, struct acpi_device_pnp *pnp,
-				int device_type)
+				int device_type, struct acpi_device_info *info)
 {
-	acpi_status status;
-	struct acpi_device_info *info;
 	struct acpi_pnp_device_id_list *cid_list;
 	int i;
 
@@ -1272,8 +1270,7 @@ static void acpi_set_pnp_ids(acpi_handle handle, struct acpi_device_pnp *pnp,
 			break;
 		}
 
-		status = acpi_get_object_info(handle, &info);
-		if (ACPI_FAILURE(status)) {
+		if (!info) {
 			pr_err(PREFIX "%s: Error reading device info\n",
 					__func__);
 			return;
@@ -1298,8 +1295,6 @@ static void acpi_set_pnp_ids(acpi_handle handle, struct acpi_device_pnp *pnp,
 		if (info->valid & ACPI_VALID_CLS)
 			acpi_add_id(pnp, info->class_code.string);
 
-		kfree(info);
-
 		/*
 		 * Some devices don't reliably have _HIDs & _CIDs, so add
 		 * synthetic HIDs to make sure drivers can find them.
@@ -1605,7 +1600,8 @@ static bool acpi_device_enumeration_by_parent(struct acpi_device *device)
 }
 
 void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
-			     int type, unsigned long long sta)
+			     int type, unsigned long long sta,
+			     struct acpi_device_info *info)
 {
 	INIT_LIST_HEAD(&device->pnp.ids);
 	device->device_type = type;
@@ -1614,7 +1610,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	device->fwnode.ops = &acpi_device_fwnode_ops;
 	acpi_set_device_status(device, sta);
 	acpi_device_get_busid(device);
-	acpi_set_pnp_ids(handle, &device->pnp, type);
+	acpi_set_pnp_ids(handle, &device->pnp, type, info);
 	acpi_init_properties(device);
 	acpi_bus_get_flags(device);
 	device->flags.match_driver = false;
@@ -1642,14 +1638,20 @@ static int acpi_add_single_object(struct acpi_device **child,
 	int result;
 	struct acpi_device *device;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct acpi_device_info *info = NULL;
+
+	if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE)
+		acpi_get_object_info(handle, &info);
 
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
 	if (!device) {
 		printk(KERN_ERR PREFIX "Memory allocation error\n");
+		kfree(info);
 		return -ENOMEM;
 	}
 
-	acpi_init_device_object(device, handle, type, sta);
+	acpi_init_device_object(device, handle, type, sta, info);
+	kfree(info);
 	/*
 	 * For ACPI_BUS_TYPE_DEVICE getting the status is delayed till here so
 	 * that we can call acpi_bus_get_status() and use its quirk handling.
-- 
2.28.0

[PATCH 3/7] ACPI: scan: Add a separate cleanup exit-path to acpi_scan_init()

[Hans de Goede]

Add a separate cleanup exit-path to acpi_scan_init().
This is a preparation patch for splitting the scanning of the root
into 2 steps, deferring the addition of some devices which need access
to OpRegions of other devices during scanning to the second step.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 25a46ae24229..7dde66222648 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2250,27 +2250,31 @@ int __init acpi_scan_init(void)
 	 */
 	result = acpi_bus_scan(ACPI_ROOT_OBJECT);
 	if (result)
-		goto out;
+		goto cleanup;
 
 	result = acpi_bus_get_device(ACPI_ROOT_OBJECT, &acpi_root);
 	if (result)
-		goto out;
+		goto cleanup;
 
 	/* Fixed feature devices do not exist on HW-reduced platform */
 	if (!acpi_gbl_reduced_hardware) {
 		result = acpi_bus_scan_fixed();
-		if (result) {
-			acpi_detach_data(acpi_root->handle,
-					 acpi_scan_drop_device);
-			acpi_device_del(acpi_root);
-			put_device(&acpi_root->dev);
-			goto out;
-		}
+		if (result)
+			goto cleanup;
 	}
 
 	acpi_scan_initialized = true;
 
- out:
+	mutex_unlock(&acpi_scan_lock);
+	return 0;
+
+cleanup:
+	if (acpi_root) {
+		acpi_detach_data(acpi_root->handle, acpi_scan_drop_device);
+		acpi_device_del(acpi_root);
+		put_device(&acpi_root->dev);
+	}
+
 	mutex_unlock(&acpi_scan_lock);
 	return result;
 }
-- 
2.28.0

[PATCH 4/7] ACPI: scan: Split root scanning into 2 steps

[Hans de Goede]

The ACPI scan code currently does not honor _DEP lists, except
for ACPI battery nodes.

This is an issue on some devices, where some of the methods used during
device-addition may rely on OpRegions of other devices.

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.

This splits the scanning into 2 steps, deferring the addition of some
devices which need access to OpRegions of other devices during scanning
to the second step.

This initial implementation takes a very simple approach to identify
which devices need to have their device-addition deferred. It uses a
static lists of HIDs for this. This list is initially populated with
the HID reported for the Bluetooth on the Acer SW3-016. This uses the
HID reported when the GPIO OpRegions are not yet available!

A more elaborate approach, which actually looks at the _DEP list will
be added by a follow up patch. This other approach has a big chance
of causing regressions, so for now it will be hidden behind a kernel
commandline option. Which is why the KISS approach chosen here is needed
to fix the issue at hand, so that things will work OOTB on affected
devices.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1665610
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c | 65 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 7dde66222648..407c8536568b 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -42,6 +42,15 @@ DEFINE_MUTEX(acpi_device_lock);
 LIST_HEAD(acpi_wakeup_device_list);
 static DEFINE_MUTEX(acpi_hp_context_lock);
 
+/*
+ * Variables for the new 2 step scan scheme:
+ * Step 1. Add all devices for which acpi_should_defer_add() returns false
+ * Step 2. Add devices deferred during phase 1.
+ * These are protected by the acpi_scan_lock.
+ */
+static bool acpi_check_defer_add;
+static LIST_HEAD(acpi_deferred_devices);
+
 /*
  * The UART device described by the SPCR table is the only object which needs
  * special-casing. Everything else is covered by ACPI namespace paths in STAO
@@ -55,6 +64,11 @@ struct acpi_dep_data {
 	acpi_handle slave;
 };
 
+struct acpi_deferred_dev {
+	struct list_head node;
+	acpi_handle handle;
+};
+
 void acpi_scan_lock_acquire(void)
 {
 	mutex_lock(&acpi_scan_lock);
@@ -741,6 +755,16 @@ static const char * const acpi_ignore_dep_hids[] = {
 	NULL
 };
 
+/*
+ * List of HIDs for which we defer adding them to the second step of the
+ * scanning of the root, because some of their methods used during addition
+ * depend on OpRegions registered by the drivers for other ACPI devices.
+ */
+static const char * const acpi_defer_add_hids[] = {
+	"BCM2E5B", /* Acer SW3-016 bluetooth HID when GPIO OpRegs or not available yet */
+	NULL
+};
+
 static struct acpi_device *acpi_bus_get_parent(acpi_handle handle)
 {
 	struct acpi_device *device = NULL;
@@ -1631,18 +1655,42 @@ void acpi_device_add_finalize(struct acpi_device *device)
 	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 }
 
+static bool acpi_should_defer_add(acpi_handle handle, struct acpi_device_info *info)
+{
+	if (!acpi_check_defer_add || !info)
+		return false;
+
+	if (acpi_info_matches_hids(info, acpi_defer_add_hids))
+		return true;
+
+	return false;
+}
+
 static int acpi_add_single_object(struct acpi_device **child,
 				  acpi_handle handle, int type,
 				  unsigned long long sta)
 {
 	int result;
 	struct acpi_device *device;
+	struct acpi_deferred_dev *deferred_dev;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	struct acpi_device_info *info = NULL;
 
 	if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE)
 		acpi_get_object_info(handle, &info);
 
+	if (acpi_should_defer_add(handle, info)) {
+		kfree(info);
+
+		deferred_dev = kzalloc(sizeof(*deferred_dev), GFP_KERNEL);
+		if (!deferred_dev)
+			return -ENOMEM;
+
+		deferred_dev->handle = handle;
+		list_add_tail(&deferred_dev->node, &acpi_deferred_devices);
+		return -EPROBE_DEFER;
+	}
+
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
 	if (!device) {
 		printk(KERN_ERR PREFIX "Memory allocation error\n");
@@ -2201,6 +2249,7 @@ int __init acpi_scan_init(void)
 	int result;
 	acpi_status status;
 	struct acpi_table_stao *stao_ptr;
+	struct acpi_deferred_dev *deferred_dev, *tmp;
 
 	acpi_pci_root_init();
 	acpi_pci_link_init();
@@ -2248,7 +2297,9 @@ int __init acpi_scan_init(void)
 	/*
 	 * Enumerate devices in the ACPI namespace.
 	 */
+	acpi_check_defer_add = true;
 	result = acpi_bus_scan(ACPI_ROOT_OBJECT);
+	acpi_check_defer_add = false;
 	if (result)
 		goto cleanup;
 
@@ -2256,6 +2307,15 @@ int __init acpi_scan_init(void)
 	if (result)
 		goto cleanup;
 
+	list_for_each_entry_safe(deferred_dev, tmp, &acpi_deferred_devices, node) {
+		result = acpi_bus_scan(deferred_dev->handle);
+		if (result)
+			goto cleanup;
+
+		list_del(&deferred_dev->node);
+		kfree(deferred_dev);
+	}
+
 	/* Fixed feature devices do not exist on HW-reduced platform */
 	if (!acpi_gbl_reduced_hardware) {
 		result = acpi_bus_scan_fixed();
@@ -2275,6 +2335,11 @@ int __init acpi_scan_init(void)
 		put_device(&acpi_root->dev);
 	}
 
+	list_for_each_entry_safe(deferred_dev, tmp, &acpi_deferred_devices, node) {
+		list_del(&deferred_dev->node);
+		kfree(deferred_dev);
+	}
+
 	mutex_unlock(&acpi_scan_lock);
 	return result;
 }
-- 
2.28.0

[PATCH 5/7] ACPI: scan: Add support for deferring adding devices to the second scan phase based on the _DEP list

[Hans de Goede]

The current solution, of deferring adding of some devices because they
need access during the OpRegions of other devices while they are added,
is not very generic.

And support for making the decision to defer adding a device based on
its _DEP list, instead of the device's HID being in a fixed list of HIDs
to defer, which should be a more generic way to deal with this.

Since this is likely to cause issues on some hardware, this new method will
only be used if the new acpi.defer_scan_based_on_dep kernel commandline
option is set to 1.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 407c8536568b..9927036bfe77 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -31,6 +31,11 @@ extern struct acpi_device *acpi_root;
 
 #define INVALID_ACPI_HANDLE	((acpi_handle)empty_zero_page)
 
+static int defer_scan_based_on_dep = -1;
+module_param(defer_scan_based_on_dep, int, 0444);
+MODULE_PARM_DESC(defer_scan_based_on_dep,
+	"Use new scan-scheme deferring addition of devices with non empty _DEP list (-1=auto, 0=no, 1=yes)");
+
 static const char *dummy_hid = "device";
 
 static LIST_HEAD(acpi_dep_list);
@@ -1657,11 +1662,45 @@ void acpi_device_add_finalize(struct acpi_device *device)
 
 static bool acpi_should_defer_add(acpi_handle handle, struct acpi_device_info *info)
 {
+	struct acpi_handle_list dep_devices;
+	acpi_status status;
+	int i;
+
 	if (!acpi_check_defer_add || !info)
 		return false;
 
-	if (acpi_info_matches_hids(info, acpi_defer_add_hids))
+	if (!defer_scan_based_on_dep)
+		return acpi_info_matches_hids(info, acpi_defer_add_hids);
+
+	/*
+	 * We check for _ADR here to avoid deferring the adding of the following:
+	 * 1. PCI devices
+	 * 2. ACPI nodes describing USB ports
+	 * Note checking for _ADR catches more then just these cases...
+	 */
+	if (info->valid & ACPI_VALID_ADR)
+		return false;
+
+	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	for (i = 0; i < dep_devices.count; i++) {
+		struct acpi_device_info *dep_info;
+		bool ignore;
+
+		status = acpi_get_object_info(dep_devices.handles[i], &dep_info);
+		if (ACPI_FAILURE(status))
+			continue;
+
+		ignore = acpi_info_matches_hids(dep_info, acpi_ignore_dep_hids);
+		kfree(dep_info);
+
+		if (ignore)
+			continue;
+
 		return true;
+	}
 
 	return false;
 }
@@ -2251,6 +2290,10 @@ int __init acpi_scan_init(void)
 	struct acpi_table_stao *stao_ptr;
 	struct acpi_deferred_dev *deferred_dev, *tmp;
 
+	/* Currently no devices are known which need _dep based scan deferral */
+	if (defer_scan_based_on_dep == -1)
+		defer_scan_based_on_dep = 0;
+
 	acpi_pci_root_init();
 	acpi_pci_link_init();
 	acpi_processor_init();
-- 
2.28.0

[PATCH 6/7] ACPI: scan: Fix battery devices not working with acpi.defer_scan_based_on_dep=1

[Hans de Goede]

When battery devices get added during the second step of the now 2 step
scan-process, then acpi_walk_dep_device_list() may have already been
called for some deps during the first step.

In this case acpi_device_dep_initialize() should not add these deps to
the acpi_dep_list; and it should not increase adev->dep_unmet.

Add a check for already registered (and bound to a driver) devices to
acpi_device_dep_initialize(). This fixes battery devices (which honor the
dep_unmet value) not working with acpi.defer_scan_based_on_dep=1.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 9927036bfe77..44001610f6a4 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -17,6 +17,8 @@
 #include <linux/platform_data/x86/apple.h>
 #include <linux/pgtable.h>
 
+#include "acpica/accommon.h"
+#include "acpica/acnamesp.h"
 #include "internal.h"
 
 #define _COMPONENT		ACPI_BUS_COMPONENT
@@ -1935,7 +1937,10 @@ static void acpi_device_dep_initialize(struct acpi_device *adev)
 	}
 
 	for (i = 0; i < dep_devices.count; i++) {
+		struct acpi_device *dep_adev = NULL;
+		struct acpi_namespace_node *node;
 		struct acpi_device_info *info;
+		struct device *dep_dev;
 		int skip;
 
 		status = acpi_get_object_info(dep_devices.handles[i], &info);
@@ -1950,6 +1955,30 @@ static void acpi_device_dep_initialize(struct acpi_device *adev)
 		if (skip)
 			continue;
 
+		/*
+		 * Skip devices which already have a driver bound
+		 * and thus are already available for use.
+		 * We only need to do this during the second scan step,
+		 * when acpi_check_defer_add == false.
+		 */
+		if (!acpi_check_defer_add) {
+			/*
+			 * Cannot call acpi_bus_get_device here as we are called
+			 * with ACPI_MTX_NAMESPACE held.
+			 */
+			node = acpi_ns_validate_handle(dep_devices.handles[i]);
+			if (!node)
+				continue; /* Should never happen */
+
+			status = acpi_ns_get_attached_data(node, acpi_scan_drop_device,
+							   (void **)&dep_adev);
+			if (ACPI_SUCCESS(status) && dep_adev) {
+				dep_dev = acpi_get_first_physical_node(dep_adev);
+				if (dep_dev && dep_dev->driver)
+					continue;
+			}
+		}
+
 		dep = kzalloc(sizeof(struct acpi_dep_data), GFP_KERNEL);
 		if (!dep)
 			return;
-- 
2.28.0

[PATCH 7/7] ACPI: scan: Add some HIDs which are never bound on Cherry Trail devices to acpi_ignore_dep_hids

[Hans de Goede]

Add some HIDs which are found on Cherry Trail devices and which
Linux never bounds to acpi_ignore_dep_hids. This allows all root
level ACPI devices to be instantiated during step one of scanning
the ACPI root.

Note this is not strictly necessary, at least not on the one
Cherry Trail device this has been tested on so far.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/scan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 44001610f6a4..218a6e9e560e 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -759,6 +759,8 @@ static bool acpi_info_matches_hids(struct acpi_device_info *info,
 /* List of HIDs for which we ignore matching ACPI devices, when checking _DEP lists. */
 static const char * const acpi_ignore_dep_hids[] = {
 	"INT3396", /* Windows System Power Management Controller */
+	"INT33A4", /* Windows System Power Management Controller */
+	"INT33BD", /* Intel Baytrail Mailbox Device */
 	NULL
 };
 
-- 
2.28.0

Re: [PATCH 5/7] ACPI: scan: Add support for deferring adding devices to the second scan phase based on the _DEP list

[Rafael J. Wysocki]

On Sat, Nov 21, 2020 at 9:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The current solution, of deferring adding of some devices because they
> need access during the OpRegions of other devices while they are added,
> is not very generic.
>
> And support for making the decision to defer adding a device based on
> its _DEP list, instead of the device's HID being in a fixed list of HIDs
> to defer, which should be a more generic way to deal with this.

Thanks a lot for working on this!

I'll have a more thorough look at the series later this week, stay tuned.

> Since this is likely to cause issues on some hardware, this new method will
> only be used if the new acpi.defer_scan_based_on_dep kernel commandline
> option is set to 1.

However, I already can say that I don't like the new command line option.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/scan.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 407c8536568b..9927036bfe77 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -31,6 +31,11 @@ extern struct acpi_device *acpi_root;
>
>  #define INVALID_ACPI_HANDLE    ((acpi_handle)empty_zero_page)
>
> +static int defer_scan_based_on_dep = -1;
> +module_param(defer_scan_based_on_dep, int, 0444);
> +MODULE_PARM_DESC(defer_scan_based_on_dep,
> +       "Use new scan-scheme deferring addition of devices with non empty _DEP list (-1=auto, 0=no, 1=yes)");
> +
>  static const char *dummy_hid = "device";
>
>  static LIST_HEAD(acpi_dep_list);
> @@ -1657,11 +1662,45 @@ void acpi_device_add_finalize(struct acpi_device *device)
>
>  static bool acpi_should_defer_add(acpi_handle handle, struct acpi_device_info *info)
>  {
> +       struct acpi_handle_list dep_devices;
> +       acpi_status status;
> +       int i;
> +
>         if (!acpi_check_defer_add || !info)
>                 return false;
>
> -       if (acpi_info_matches_hids(info, acpi_defer_add_hids))
> +       if (!defer_scan_based_on_dep)
> +               return acpi_info_matches_hids(info, acpi_defer_add_hids);
> +
> +       /*
> +        * We check for _ADR here to avoid deferring the adding of the following:
> +        * 1. PCI devices
> +        * 2. ACPI nodes describing USB ports
> +        * Note checking for _ADR catches more then just these cases...

s/then/than/

> +        */
> +       if (info->valid & ACPI_VALID_ADR)
> +               return false;
> +
> +       status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
> +       if (ACPI_FAILURE(status))
> +               return false;
> +
> +       for (i = 0; i < dep_devices.count; i++) {
> +               struct acpi_device_info *dep_info;
> +               bool ignore;
> +
> +               status = acpi_get_object_info(dep_devices.handles[i], &dep_info);
> +               if (ACPI_FAILURE(status))
> +                       continue;
> +
> +               ignore = acpi_info_matches_hids(dep_info, acpi_ignore_dep_hids);
> +               kfree(dep_info);
> +
> +               if (ignore)
> +                       continue;
> +
>                 return true;
> +       }
>
>         return false;
>  }
> @@ -2251,6 +2290,10 @@ int __init acpi_scan_init(void)
>         struct acpi_table_stao *stao_ptr;
>         struct acpi_deferred_dev *deferred_dev, *tmp;
>
> +       /* Currently no devices are known which need _dep based scan deferral */
> +       if (defer_scan_based_on_dep == -1)
> +               defer_scan_based_on_dep = 0;
> +
>         acpi_pci_root_init();
>         acpi_pci_link_init();
>         acpi_processor_init();
> --
> 2.28.0
>

Re: [PATCH 5/7] ACPI: scan: Add support for deferring adding devices to the second scan phase based on the _DEP list

[Rafael J. Wysocki]

On Mon, Nov 23, 2020 at 2:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 11/23/20 1:17 PM, Rafael J. Wysocki wrote:
> > On Sat, Nov 21, 2020 at 9:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> The current solution, of deferring adding of some devices because they
> >> need access during the OpRegions of other devices while they are added,
> >> is not very generic.
> >>
> >> And support for making the decision to defer adding a device based on
> >> its _DEP list, instead of the device's HID being in a fixed list of HIDs
> >> to defer, which should be a more generic way to deal with this.
> >
> > Thanks a lot for working on this!
>
> You're welcome.
>
> > I'll have a more thorough look at the series later this week, stay tuned.
>
> Ok.
>
> >> Since this is likely to cause issues on some hardware, this new method will
> >> only be used if the new acpi.defer_scan_based_on_dep kernel commandline
> >> option is set to 1.
> >
> > However, I already can say that I don't like the new command line option.
>
> You don't like the name, or you don't like having a commandline option for this?

The latter.

> Anyways I'll wait till you have taken a closer look.

OK, thanks!

Re: [PATCH 5/7] ACPI: scan: Add support for deferring adding devices to the second scan phase based on the _DEP list

[Hans de Goede]

Hi,

On 11/23/20 1:17 PM, Rafael J. Wysocki wrote:
> On Sat, Nov 21, 2020 at 9:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The current solution, of deferring adding of some devices because they
>> need access during the OpRegions of other devices while they are added,
>> is not very generic.
>>
>> And support for making the decision to defer adding a device based on
>> its _DEP list, instead of the device's HID being in a fixed list of HIDs
>> to defer, which should be a more generic way to deal with this.
> 
> Thanks a lot for working on this!

You're welcome.

> I'll have a more thorough look at the series later this week, stay tuned.

Ok.

>> Since this is likely to cause issues on some hardware, this new method will
>> only be used if the new acpi.defer_scan_based_on_dep kernel commandline
>> option is set to 1.
> 
> However, I already can say that I don't like the new command line option.

You don't like the name, or you don't like having a commandline option for this?

Anyways I'll wait till you have taken a closer look.

Regards,

Hans


> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/acpi/scan.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 407c8536568b..9927036bfe77 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -31,6 +31,11 @@ extern struct acpi_device *acpi_root;
>>
>>  #define INVALID_ACPI_HANDLE    ((acpi_handle)empty_zero_page)
>>
>> +static int defer_scan_based_on_dep = -1;
>> +module_param(defer_scan_based_on_dep, int, 0444);
>> +MODULE_PARM_DESC(defer_scan_based_on_dep,
>> +       "Use new scan-scheme deferring addition of devices with non empty _DEP list (-1=auto, 0=no, 1=yes)");
>> +
>>  static const char *dummy_hid = "device";
>>
>>  static LIST_HEAD(acpi_dep_list);
>> @@ -1657,11 +1662,45 @@ void acpi_device_add_finalize(struct acpi_device *device)
>>
>>  static bool acpi_should_defer_add(acpi_handle handle, struct acpi_device_info *info)
>>  {
>> +       struct acpi_handle_list dep_devices;
>> +       acpi_status status;
>> +       int i;
>> +
>>         if (!acpi_check_defer_add || !info)
>>                 return false;
>>
>> -       if (acpi_info_matches_hids(info, acpi_defer_add_hids))
>> +       if (!defer_scan_based_on_dep)
>> +               return acpi_info_matches_hids(info, acpi_defer_add_hids);
>> +
>> +       /*
>> +        * We check for _ADR here to avoid deferring the adding of the following:
>> +        * 1. PCI devices
>> +        * 2. ACPI nodes describing USB ports
>> +        * Note checking for _ADR catches more then just these cases...
> 
> s/then/than/
> 
>> +        */
>> +       if (info->valid & ACPI_VALID_ADR)
>> +               return false;
>> +
>> +       status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
>> +       if (ACPI_FAILURE(status))
>> +               return false;
>> +
>> +       for (i = 0; i < dep_devices.count; i++) {
>> +               struct acpi_device_info *dep_info;
>> +               bool ignore;
>> +
>> +               status = acpi_get_object_info(dep_devices.handles[i], &dep_info);
>> +               if (ACPI_FAILURE(status))
>> +                       continue;
>> +
>> +               ignore = acpi_info_matches_hids(dep_info, acpi_ignore_dep_hids);
>> +               kfree(dep_info);
>> +
>> +               if (ignore)
>> +                       continue;
>> +
>>                 return true;
>> +       }
>>
>>         return false;
>>  }
>> @@ -2251,6 +2290,10 @@ int __init acpi_scan_init(void)
>>         struct acpi_table_stao *stao_ptr;
>>         struct acpi_deferred_dev *deferred_dev, *tmp;
>>
>> +       /* Currently no devices are known which need _dep based scan deferral */
>> +       if (defer_scan_based_on_dep == -1)
>> +               defer_scan_based_on_dep = 0;
>> +
>>         acpi_pci_root_init();
>>         acpi_pci_link_init();
>>         acpi_processor_init();
>> --
>> 2.28.0
>>
> 

Re: [PATCH 5/7] ACPI: scan: Add support for deferring adding devices to the second scan phase based on the _DEP list

[Hans de Goede]

Hi,

On 11/23/20 1:41 PM, Rafael J. Wysocki wrote:
> On Mon, Nov 23, 2020 at 2:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 11/23/20 1:17 PM, Rafael J. Wysocki wrote:
>>> On Sat, Nov 21, 2020 at 9:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> The current solution, of deferring adding of some devices because they
>>>> need access during the OpRegions of other devices while they are added,
>>>> is not very generic.
>>>>
>>>> And support for making the decision to defer adding a device based on
>>>> its _DEP list, instead of the device's HID being in a fixed list of HIDs
>>>> to defer, which should be a more generic way to deal with this.
>>>
>>> Thanks a lot for working on this!
>>
>> You're welcome.
>>
>>> I'll have a more thorough look at the series later this week, stay tuned.
>>
>> Ok.
>>
>>>> Since this is likely to cause issues on some hardware, this new method will
>>>> only be used if the new acpi.defer_scan_based_on_dep kernel commandline
>>>> option is set to 1.
>>>
>>> However, I already can say that I don't like the new command line option.
>>
>> You don't like the name, or you don't like having a commandline option for this?
> 
> The latter.

I already expected as much. Some initial thoughts on this. Note feel free to
respond later when you are reviewing the set:

I think that this is a bit adventurous. But this was a weekend project for
me and I only had time to test on the Acer Switch Sw3-016 so far, and that
worked well. So maybe it will work better then expected with some more testing.

If we want to do this be default from now on, then we need to take some
measures to avoid the acpi_ignore_dep_hids list added by one of the preparation
patches from growing endlessly. What would help here is extending the new 
acpi_info_matches_hids() helper to not only check the HIDs against
acpi_device_info.hardware_id but also the compatible_id_list and then replace
the System Power Management Controller HIDs in acpi_ignore_dep_hids list with
"PNP0D80" I believe that that should catch all PMC-s without needing to have
a HID per hardware/chipset generation.

Regards,

Hans

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Rafael J. Wysocki]

On Sat, Nov 21, 2020 at 9:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi Rafael,
>
> A while ago (almost 2 years ago) I discussed an issue with you about
> some devices, where some of the methods used during device-addition
> (such as _HID) may rely on OpRegions of other devices:
>
> https://www.spinics.net/lists/linux-acpi/msg86303.html
>
> 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.
>
> Last week I bought a second hand Acer device, not knowing it was this
> exact model. Since I now have access to affected hardware I decided to
> take a shot at fixing this.
>
> In the discussion you suggested to split the acpi_bus_scan of the root
> into 2 steps, first scan devices with an empty _DEP, putting other
> acpi_handle-s on a list of deferred devices and then in step 2 scan the
> rest.
>
> I'm happy to report that, at least on the affected device, this works
> nicely. While working on this I came up with a less drastic way to
> deal with this. As you will see in patch 4 of this series, I decided
> to first add a more KISS method of deciding which devices to defer
> to the second scan step by matching by HID. This has the disadvantage
> of not being a generic solution. But it has the advantage of being a
> solution which does not potentially regress other devices.
>
> Then in patch 5 I actually do add the option to defer or not based on
> _DEP being empty. I've put this behind a kernel commandline option as
> I'm not sure we should do this everywhere by default. At least no without
> a lot more testing.
>
> Patch 6 fixes an issue with patch 5 which causes battery devices to stop
> working.
>
> And patch 7 adds some extra HIDs to the list of HIDs which should be
> ignored when checking if the _DEP list is empty from Linux' pov, iow
> some extra HIDs which Linux does not bind to.
>
> Please let me know what you think about this patch-set. I would be happy
> to see just patches 1-4 merged.

I took patches 1 and 2, because IMO they are generally useful (I
rewrote the changelogs to avoid mentioning the rest of the series
though), but I have some reservations regarding the rest.

First off, I'm not really sure if failing acpi_add_single_object() for
devices with missing dependencies is a good idea.  IIRC there is
nothing in there that should depend on any opregions supplied by the
other devices, so it should be safe to allow it to complete.  That, in
turn, will allow the flags in struct acpi_device to be used to mark
the "deferred" devices without allocating more memory.

Next, in theory, devices with dependencies may also appear during
hotplug, so it would be prudent to handle that on every invocation of
acpi_bus_scan() and not just when it runs for the root object.

So my approach would be to allow the first namespace walk in
acpi_bus_scan() to complete, change acpi_bus_attach() to optionally
skip the devices with missing dependencies and return a result
indicating whether or not it has set flags.visited for any devices and
run it in a loop on the "root" device object until it says that no new
devices have been "attached".

Let me cut a prototype patch for that and get back to you.

Re: [PATCH 6/7] ACPI: scan: Fix battery devices not working with acpi.defer_scan_based_on_dep=1

[Rafael J. Wysocki]

On Sat, Nov 21, 2020 at 9:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> When battery devices get added during the second step of the now 2 step
> scan-process, then acpi_walk_dep_device_list() may have already been
> called for some deps during the first step.
>
> In this case acpi_device_dep_initialize() should not add these deps to
> the acpi_dep_list; and it should not increase adev->dep_unmet.
>
> Add a check for already registered (and bound to a driver) devices to
> acpi_device_dep_initialize(). This fixes battery devices (which honor the
> dep_unmet value) not working with acpi.defer_scan_based_on_dep=1.

I'd rather avoid having to fix this issue at all ...

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Rafael J. Wysocki]

On Wednesday, December 2, 2020 2:49:17 PM CET Rafael J. Wysocki wrote:
> On Sat, Nov 21, 2020 at 9:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi Rafael,
> >
> > A while ago (almost 2 years ago) I discussed an issue with you about
> > some devices, where some of the methods used during device-addition
> > (such as _HID) may rely on OpRegions of other devices:
> >
> > https://www.spinics.net/lists/linux-acpi/msg86303.html
> >
> > 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.
> >
> > Last week I bought a second hand Acer device, not knowing it was this
> > exact model. Since I now have access to affected hardware I decided to
> > take a shot at fixing this.
> >
> > In the discussion you suggested to split the acpi_bus_scan of the root
> > into 2 steps, first scan devices with an empty _DEP, putting other
> > acpi_handle-s on a list of deferred devices and then in step 2 scan the
> > rest.
> >
> > I'm happy to report that, at least on the affected device, this works
> > nicely. While working on this I came up with a less drastic way to
> > deal with this. As you will see in patch 4 of this series, I decided
> > to first add a more KISS method of deciding which devices to defer
> > to the second scan step by matching by HID. This has the disadvantage
> > of not being a generic solution. But it has the advantage of being a
> > solution which does not potentially regress other devices.
> >
> > Then in patch 5 I actually do add the option to defer or not based on
> > _DEP being empty. I've put this behind a kernel commandline option as
> > I'm not sure we should do this everywhere by default. At least no without
> > a lot more testing.
> >
> > Patch 6 fixes an issue with patch 5 which causes battery devices to stop
> > working.
> >
> > And patch 7 adds some extra HIDs to the list of HIDs which should be
> > ignored when checking if the _DEP list is empty from Linux' pov, iow
> > some extra HIDs which Linux does not bind to.
> >
> > Please let me know what you think about this patch-set. I would be happy
> > to see just patches 1-4 merged.
> 
> I took patches 1 and 2, because IMO they are generally useful (I
> rewrote the changelogs to avoid mentioning the rest of the series
> though), but I have some reservations regarding the rest.
> 
> First off, I'm not really sure if failing acpi_add_single_object() for
> devices with missing dependencies is a good idea.  IIRC there is
> nothing in there that should depend on any opregions supplied by the
> other devices, so it should be safe to allow it to complete.  That, in
> turn, will allow the flags in struct acpi_device to be used to mark
> the "deferred" devices without allocating more memory.
> 
> Next, in theory, devices with dependencies may also appear during
> hotplug, so it would be prudent to handle that on every invocation of
> acpi_bus_scan() and not just when it runs for the root object.
> 
> So my approach would be to allow the first namespace walk in
> acpi_bus_scan() to complete, change acpi_bus_attach() to optionally
> skip the devices with missing dependencies and return a result
> indicating whether or not it has set flags.visited for any devices and
> run it in a loop on the "root" device object until it says that no new
> devices have been "attached".
> 
> Let me cut a prototype patch for that and get back to you.

Maybe something like the patch below (untested).  I borrowed a few items from
your patches, hopefully not a problem.

The multiple passes idea would require using a static variable which would
be slightly inelegant, so this assumes that two passes should be sufficient.

---
 drivers/acpi/scan.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1961,12 +1961,50 @@ static int acpi_scan_attach_handler(stru
 	return ret;
 }
 
-static void acpi_bus_attach(struct acpi_device *device)
+/*
+ * List of IDs for which we defer adding them to the second pass of the
+ * scanning, because some of their methods used during addition depend on
+ * OpRegions registered by the drivers for other ACPI devices.
+ */
+static const struct acpi_device_id acpi_defer_attach_ids[] = {
+	{ "BCM2E5B", 0 }, /* Acer SW3-016 bluetooth vs GPIO OpRegs */
+	{"", 0},
+};
+
+static bool acpi_scan_should_defer_attach(struct acpi_device *adev)
+{
+	if (acpi_match_device_ids(adev, acpi_defer_attach_ids))
+		return true;
+
+	/*
+	 * We check for _ADR here to avoid deferring the adding of the following:
+	 * 1. PCI devices
+	 * 2. ACPI nodes describing USB ports
+	 * Note checking for _ADR catches more then just these cases ...
+	 */
+	if (adev->pnp.type.bus_address)
+		return false;
+
+	return adev->dep_unmet > 0;
+}
+
+static void __acpi_bus_attach(struct acpi_device *device, bool first_pass)
 {
 	struct acpi_device *child;
 	acpi_handle ejd;
 	int ret;
 
+	if (first_pass) {
+		if (acpi_scan_should_defer_attach(device))
+			return;
+	} else if (device->flags.visited) {
+		/*
+		 * This is not the first pass in the given scan and the device
+		 * has been "attached" already, so get to the children right away.
+		 */
+		goto ok;
+	}
+
 	if (ACPI_SUCCESS(acpi_bus_get_ejd(device->handle, &ejd)))
 		register_dock_dependent_device(device, ejd);
 
@@ -2013,12 +2051,23 @@ 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 (first_pass && device->handler &&
+	    device->handler->hotplug.notify_online)
 		device->handler->hotplug.notify_online(device);
 }
 
+static void acpi_bus_attach(struct acpi_device *device)
+{
+	/*
+	 * Assume two passes to be sufficient to satisfy all of the operation
+	 * region dependencies.
+	 */
+	__acpi_bus_attach(device, true);
+	__acpi_bus_attach(device, false);
+}
+
 void acpi_walk_dep_device_list(acpi_handle handle)
 {
 	struct acpi_dep_data *dep, *tmp;

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Hans de Goede]

Hi,

On 12/2/20 2:49 PM, Rafael J. Wysocki wrote:
> On Sat, Nov 21, 2020 at 9:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Rafael,
>>
>> A while ago (almost 2 years ago) I discussed an issue with you about
>> some devices, where some of the methods used during device-addition
>> (such as _HID) may rely on OpRegions of other devices:
>>
>> https://www.spinics.net/lists/linux-acpi/msg86303.html
>>
>> 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.
>>
>> Last week I bought a second hand Acer device, not knowing it was this
>> exact model. Since I now have access to affected hardware I decided to
>> take a shot at fixing this.
>>
>> In the discussion you suggested to split the acpi_bus_scan of the root
>> into 2 steps, first scan devices with an empty _DEP, putting other
>> acpi_handle-s on a list of deferred devices and then in step 2 scan the
>> rest.
>>
>> I'm happy to report that, at least on the affected device, this works
>> nicely. While working on this I came up with a less drastic way to
>> deal with this. As you will see in patch 4 of this series, I decided
>> to first add a more KISS method of deciding which devices to defer
>> to the second scan step by matching by HID. This has the disadvantage
>> of not being a generic solution. But it has the advantage of being a
>> solution which does not potentially regress other devices.
>>
>> Then in patch 5 I actually do add the option to defer or not based on
>> _DEP being empty. I've put this behind a kernel commandline option as
>> I'm not sure we should do this everywhere by default. At least no without
>> a lot more testing.
>>
>> Patch 6 fixes an issue with patch 5 which causes battery devices to stop
>> working.
>>
>> And patch 7 adds some extra HIDs to the list of HIDs which should be
>> ignored when checking if the _DEP list is empty from Linux' pov, iow
>> some extra HIDs which Linux does not bind to.
>>
>> Please let me know what you think about this patch-set. I would be happy
>> to see just patches 1-4 merged.
> 
> I took patches 1 and 2, because IMO they are generally useful (I
> rewrote the changelogs to avoid mentioning the rest of the series
> though),

That is fine. Thanks for taking those.

> but I have some reservations regarding the rest.
> 
> First off, I'm not really sure if failing acpi_add_single_object() for
> devices with missing dependencies is a good idea.  IIRC there is
> nothing in there that should depend on any opregions supplied by the
> other devices, so it should be safe to allow it to complete.

Actually acpi_add_single_object() does depend on ACPI methods
which may depend on opregions, that is the whole reason why
this series is necessary. Otherwise we could just delay the
binding of the driver based in dep_unmet which would be easier.

Here are 2 actual examples of acpi_add_single_object() calling
ACPI methods which may depend on opregions:

1. acpi_add_single_object() calls acpi_init_device_object() which 
calls acpi_set_pnp_ids() which fills a bunch if fields of
struct acpi_device with info returned by the acpi_get_object_info()
call.

Specifically it stores the value returned by the _HID method in
the acpi_device_pnp array for the device and that _HID method is
actually the problem in the example device which started this
series. The _HID method of the bluetooth device reads 2 GPIOs
to get a hw-id (0-3) and then translates the hwid to a _HID
string. If the GPIO opregion's handlers have not registered yet
then the reading of the GPIOs is correctly skipped, and hwid
0 is assumed, which is wrong in this case.

2. I've also seen examples where _STA depends on GPIOs in a similar
manner; and acpi_add_single_object() calls acpi_bus_get_power_flags()
which calls acpi_bus_init_power() which calls acpi_device_is_present()
which depends on _STA results.

> That, in
> turn, will allow the flags in struct acpi_device to be used to mark
> the "deferred" devices without allocating more memory.

See above, we would need to either redo a bunch of the device
addition; and/or potentially even undo it in the case of _STA
changing from present -> not present once the OpRegions have
registered.

> Next, in theory, devices with dependencies may also appear during
> hotplug, so it would be prudent to handle that on every invocation of
> acpi_bus_scan() and not just when it runs for the root object.
> 
> So my approach would be to allow the first namespace walk in
> acpi_bus_scan() to complete, change acpi_bus_attach() to optionally
> skip the devices with missing dependencies and return a result
> indicating whether or not it has set flags.visited for any devices and
> run it in a loop on the "root" device object until it says that no new
> devices have been "attached".
> 
> Let me cut a prototype patch for that and get back to you.

See above for why the prototype patch will not work. By the time
acpi_bus_attach() runs, the wrong HID has already been stored by
acpi_set_pnp_ids().

Note I'm not saying that your approach cannot work, we could
e.g. re-fetch the HID before running bus_attach.

But once we start doing extra work, replacing fields in the
earlier instantiated acpi_device, then there is not that much
difference between the 2 approaches and then the question becomes
which way is cleaner ?

I still favor my own approach because that simply delays the
entire instantiation, rather then doing it when we don't have
all the _DEPs yet and then "patching up" the acpi_device
later. Note I did consider the "patching up" approach, but
I rejected it (*).

The patching-up approach feels fragile / more error-prone to
me, which is why I chose to simply defer the entire
instantiation.

Regards,

Hans


*) In hindsight I should have probably written that down somewhere,
but the whole though process behind my choices it is not something
which I could quickly describe in 1 or 2 paragraphs in the
commit message.

Some other notes about this:

1. I considered the patching-up approach, but I did not implement it.

2. Another reason for delaying the entire instantiation is that we
have some quirk handling in various places which relies on _HID /
on the pnp-ids and since those are still in flux when the deps have
not been met yet, some of that quirk handling could (theoretically)
also go wonky if we instantiate the device too soon.

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Rafael J. Wysocki]

On Wednesday, December 2, 2020 4:51:59 PM CET Rafael J. Wysocki wrote:
> On Wednesday, December 2, 2020 2:49:17 PM CET Rafael J. Wysocki wrote:
> > On Sat, Nov 21, 2020 at 9:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > >
> > > Hi Rafael,
> > >
> > > A while ago (almost 2 years ago) I discussed an issue with you about
> > > some devices, where some of the methods used during device-addition
> > > (such as _HID) may rely on OpRegions of other devices:
> > >
> > > https://www.spinics.net/lists/linux-acpi/msg86303.html
> > >
> > > 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.
> > >
> > > Last week I bought a second hand Acer device, not knowing it was this
> > > exact model. Since I now have access to affected hardware I decided to
> > > take a shot at fixing this.
> > >
> > > In the discussion you suggested to split the acpi_bus_scan of the root
> > > into 2 steps, first scan devices with an empty _DEP, putting other
> > > acpi_handle-s on a list of deferred devices and then in step 2 scan the
> > > rest.
> > >
> > > I'm happy to report that, at least on the affected device, this works
> > > nicely. While working on this I came up with a less drastic way to
> > > deal with this. As you will see in patch 4 of this series, I decided
> > > to first add a more KISS method of deciding which devices to defer
> > > to the second scan step by matching by HID. This has the disadvantage
> > > of not being a generic solution. But it has the advantage of being a
> > > solution which does not potentially regress other devices.
> > >
> > > Then in patch 5 I actually do add the option to defer or not based on
> > > _DEP being empty. I've put this behind a kernel commandline option as
> > > I'm not sure we should do this everywhere by default. At least no without
> > > a lot more testing.
> > >
> > > Patch 6 fixes an issue with patch 5 which causes battery devices to stop
> > > working.
> > >
> > > And patch 7 adds some extra HIDs to the list of HIDs which should be
> > > ignored when checking if the _DEP list is empty from Linux' pov, iow
> > > some extra HIDs which Linux does not bind to.
> > >
> > > Please let me know what you think about this patch-set. I would be happy
> > > to see just patches 1-4 merged.
> > 
> > I took patches 1 and 2, because IMO they are generally useful (I
> > rewrote the changelogs to avoid mentioning the rest of the series
> > though), but I have some reservations regarding the rest.
> > 
> > First off, I'm not really sure if failing acpi_add_single_object() for
> > devices with missing dependencies is a good idea.  IIRC there is
> > nothing in there that should depend on any opregions supplied by the
> > other devices, so it should be safe to allow it to complete.  That, in
> > turn, will allow the flags in struct acpi_device to be used to mark
> > the "deferred" devices without allocating more memory.
> > 
> > Next, in theory, devices with dependencies may also appear during
> > hotplug, so it would be prudent to handle that on every invocation of
> > acpi_bus_scan() and not just when it runs for the root object.
> > 
> > So my approach would be to allow the first namespace walk in
> > acpi_bus_scan() to complete, change acpi_bus_attach() to optionally
> > skip the devices with missing dependencies and return a result
> > indicating whether or not it has set flags.visited for any devices and
> > run it in a loop on the "root" device object until it says that no new
> > devices have been "attached".
> > 
> > Let me cut a prototype patch for that and get back to you.
> 
> Maybe something like the patch below (untested).  I borrowed a few items from
> your patches, hopefully not a problem.
> 
> The multiple passes idea would require using a static variable which would
> be slightly inelegant, so this assumes that two passes should be sufficient.
> 

An update.

This one has been lightly tested, but it doesn't make any practical difference
on the system where it was run AFAICS.

I found a missing ! in acpi_scan_should_defer_attach() and then realized that
looking for _ADR wasn't really necessary, because _ADR devices should not be
affected by this in a meaningful way anyway (scan handlers and ACPI drivers
match on the _HID and/or _CID basis and the status check/power up in
__acpi_bus_attach() should be skipped for them, because they may be "glued"
to their "physical" counterparts before this code runs even - looks like a
bug).

---
 drivers/acpi/scan.c |   47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1979,12 +1979,42 @@ static int acpi_scan_attach_handler(stru
 	return ret;
 }
 
-static void acpi_bus_attach(struct acpi_device *device)
+/*
+ * List of IDs for which we defer adding them to the second pass of the
+ * scanning, because some of their methods used during addition depend on
+ * OpRegions registered by the drivers for other ACPI devices.
+ */
+static const struct acpi_device_id acpi_defer_attach_ids[] = {
+	{ "BCM2E5B", 0 }, /* Acer SW3-016 bluetooth vs GPIO OpRegs */
+	{"", 0},
+};
+
+static bool acpi_scan_should_defer_attach(struct acpi_device *adev)
+{
+	if (!acpi_match_device_ids(adev, acpi_defer_attach_ids))
+		return true;
+
+	return adev->dep_unmet > 0;
+}
+
+static void __acpi_bus_attach(struct acpi_device *device, bool first_pass)
 {
 	struct acpi_device *child;
 	acpi_handle ejd;
 	int ret;
 
+	if (first_pass) {
+		if (acpi_scan_should_defer_attach(device))
+			return;
+	} else if (device->flags.visited) {
+		/*
+		 * This is not the first pass in the given scan and the device
+		 * has been "attached" already, so get to the children right
+		 * away.
+		 */
+		goto ok;
+	}
+
 	if (ACPI_SUCCESS(acpi_bus_get_ejd(device->handle, &ejd)))
 		register_dock_dependent_device(device, ejd);
 
@@ -2031,12 +2061,23 @@ 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 (first_pass && device->handler &&
+	    device->handler->hotplug.notify_online)
 		device->handler->hotplug.notify_online(device);
 }
 
+static void acpi_bus_attach(struct acpi_device *device)
+{
+	/*
+	 * Assume two passes to be sufficient to satisfy all of the operation
+	 * region dependencies.
+	 */
+	__acpi_bus_attach(device, true);
+	__acpi_bus_attach(device, false);
+}
+
 void acpi_walk_dep_device_list(acpi_handle handle)
 {
 	struct acpi_dep_data *dep, *tmp;

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Rafael J. Wysocki]

On Wed, Dec 2, 2020 at 8:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/2/20 2:49 PM, Rafael J. Wysocki wrote:
> > On Sat, Nov 21, 2020 at 9:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi Rafael,
> >>
> >> A while ago (almost 2 years ago) I discussed an issue with you about
> >> some devices, where some of the methods used during device-addition
> >> (such as _HID) may rely on OpRegions of other devices:
> >>
> >> https://www.spinics.net/lists/linux-acpi/msg86303.html
> >>
> >> 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.
> >>
> >> Last week I bought a second hand Acer device, not knowing it was this
> >> exact model. Since I now have access to affected hardware I decided to
> >> take a shot at fixing this.
> >>
> >> In the discussion you suggested to split the acpi_bus_scan of the root
> >> into 2 steps, first scan devices with an empty _DEP, putting other
> >> acpi_handle-s on a list of deferred devices and then in step 2 scan the
> >> rest.
> >>
> >> I'm happy to report that, at least on the affected device, this works
> >> nicely. While working on this I came up with a less drastic way to
> >> deal with this. As you will see in patch 4 of this series, I decided
> >> to first add a more KISS method of deciding which devices to defer
> >> to the second scan step by matching by HID. This has the disadvantage
> >> of not being a generic solution. But it has the advantage of being a
> >> solution which does not potentially regress other devices.
> >>
> >> Then in patch 5 I actually do add the option to defer or not based on
> >> _DEP being empty. I've put this behind a kernel commandline option as
> >> I'm not sure we should do this everywhere by default. At least no without
> >> a lot more testing.
> >>
> >> Patch 6 fixes an issue with patch 5 which causes battery devices to stop
> >> working.
> >>
> >> And patch 7 adds some extra HIDs to the list of HIDs which should be
> >> ignored when checking if the _DEP list is empty from Linux' pov, iow
> >> some extra HIDs which Linux does not bind to.
> >>
> >> Please let me know what you think about this patch-set. I would be happy
> >> to see just patches 1-4 merged.
> >
> > I took patches 1 and 2, because IMO they are generally useful (I
> > rewrote the changelogs to avoid mentioning the rest of the series
> > though),
>
> That is fine. Thanks for taking those.
>
> > but I have some reservations regarding the rest.
> >
> > First off, I'm not really sure if failing acpi_add_single_object() for
> > devices with missing dependencies is a good idea.  IIRC there is
> > nothing in there that should depend on any opregions supplied by the
> > other devices, so it should be safe to allow it to complete.
>
> Actually acpi_add_single_object() does depend on ACPI methods
> which may depend on opregions, that is the whole reason why
> this series is necessary. Otherwise we could just delay the
> binding of the driver based in dep_unmet which would be easier.
>
> Here are 2 actual examples of acpi_add_single_object() calling
> ACPI methods which may depend on opregions:
>
> 1. acpi_add_single_object() calls acpi_init_device_object() which
> calls acpi_set_pnp_ids() which fills a bunch if fields of
> struct acpi_device with info returned by the acpi_get_object_info()
> call.
>
> Specifically it stores the value returned by the _HID method in
> the acpi_device_pnp array for the device and that _HID method is
> actually the problem in the example device which started this
> series. The _HID method of the bluetooth device reads 2 GPIOs
> to get a hw-id (0-3) and then translates the hwid to a _HID
> string. If the GPIO opregion's handlers have not registered yet
> then the reading of the GPIOs is correctly skipped, and hwid
> 0 is assumed, which is wrong in this case.
>
> 2. I've also seen examples where _STA depends on GPIOs in a similar
> manner; and acpi_add_single_object() calls acpi_bus_get_power_flags()
> which calls acpi_bus_init_power() which calls acpi_device_is_present()
> which depends on _STA results.

Well, this means that there is a bug in acpi_bus_attach() which
shouldn't call acpi_bus_init_power() which has been called already.

And it all means that either deferring acpi_add_single_object() is
needed and so there need to be 2 passes in acpi_bus_attach() overall,
or acpi_add_single_object() needs to avoid calling methods that may
depend on supplied opregions.  I guess the latter is rather
unrealistic, so the only practical choice is the former.

However, I still don't think that the extra list of "dependent
devices" is needed.

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Hans de Goede]

Hi,

On 12/2/20 8:57 PM, Rafael J. Wysocki wrote:
> On Wed, Dec 2, 2020 at 8:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 12/2/20 2:49 PM, Rafael J. Wysocki wrote:
>>> On Sat, Nov 21, 2020 at 9:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> Hi Rafael,
>>>>
>>>> A while ago (almost 2 years ago) I discussed an issue with you about
>>>> some devices, where some of the methods used during device-addition
>>>> (such as _HID) may rely on OpRegions of other devices:
>>>>
>>>> https://www.spinics.net/lists/linux-acpi/msg86303.html
>>>>
>>>> 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.
>>>>
>>>> Last week I bought a second hand Acer device, not knowing it was this
>>>> exact model. Since I now have access to affected hardware I decided to
>>>> take a shot at fixing this.
>>>>
>>>> In the discussion you suggested to split the acpi_bus_scan of the root
>>>> into 2 steps, first scan devices with an empty _DEP, putting other
>>>> acpi_handle-s on a list of deferred devices and then in step 2 scan the
>>>> rest.
>>>>
>>>> I'm happy to report that, at least on the affected device, this works
>>>> nicely. While working on this I came up with a less drastic way to
>>>> deal with this. As you will see in patch 4 of this series, I decided
>>>> to first add a more KISS method of deciding which devices to defer
>>>> to the second scan step by matching by HID. This has the disadvantage
>>>> of not being a generic solution. But it has the advantage of being a
>>>> solution which does not potentially regress other devices.
>>>>
>>>> Then in patch 5 I actually do add the option to defer or not based on
>>>> _DEP being empty. I've put this behind a kernel commandline option as
>>>> I'm not sure we should do this everywhere by default. At least no without
>>>> a lot more testing.
>>>>
>>>> Patch 6 fixes an issue with patch 5 which causes battery devices to stop
>>>> working.
>>>>
>>>> And patch 7 adds some extra HIDs to the list of HIDs which should be
>>>> ignored when checking if the _DEP list is empty from Linux' pov, iow
>>>> some extra HIDs which Linux does not bind to.
>>>>
>>>> Please let me know what you think about this patch-set. I would be happy
>>>> to see just patches 1-4 merged.
>>>
>>> I took patches 1 and 2, because IMO they are generally useful (I
>>> rewrote the changelogs to avoid mentioning the rest of the series
>>> though),
>>
>> That is fine. Thanks for taking those.
>>
>>> but I have some reservations regarding the rest.
>>>
>>> First off, I'm not really sure if failing acpi_add_single_object() for
>>> devices with missing dependencies is a good idea.  IIRC there is
>>> nothing in there that should depend on any opregions supplied by the
>>> other devices, so it should be safe to allow it to complete.
>>
>> Actually acpi_add_single_object() does depend on ACPI methods
>> which may depend on opregions, that is the whole reason why
>> this series is necessary. Otherwise we could just delay the
>> binding of the driver based in dep_unmet which would be easier.
>>
>> Here are 2 actual examples of acpi_add_single_object() calling
>> ACPI methods which may depend on opregions:
>>
>> 1. acpi_add_single_object() calls acpi_init_device_object() which
>> calls acpi_set_pnp_ids() which fills a bunch if fields of
>> struct acpi_device with info returned by the acpi_get_object_info()
>> call.
>>
>> Specifically it stores the value returned by the _HID method in
>> the acpi_device_pnp array for the device and that _HID method is
>> actually the problem in the example device which started this
>> series. The _HID method of the bluetooth device reads 2 GPIOs
>> to get a hw-id (0-3) and then translates the hwid to a _HID
>> string. If the GPIO opregion's handlers have not registered yet
>> then the reading of the GPIOs is correctly skipped, and hwid
>> 0 is assumed, which is wrong in this case.
>>
>> 2. I've also seen examples where _STA depends on GPIOs in a similar
>> manner; and acpi_add_single_object() calls acpi_bus_get_power_flags()
>> which calls acpi_bus_init_power() which calls acpi_device_is_present()
>> which depends on _STA results.
> 
> Well, this means that there is a bug in acpi_bus_attach() which
> shouldn't call acpi_bus_init_power() which has been called already.

I'm afraid we have a bit of a misunderstanding here, the problem is
not that acpi_bus_attach() calls acpi_bus_init_power(), the problem is
that acpi_bus_init_power() (which is called from acpi_add_single_object())
depends on the value returned by _STA and that in turn may depend on
some OpRegions being available. IOW it is the same problem as the _HID
problem.

> And it all means that either deferring acpi_add_single_object() is
> needed and so there need to be 2 passes in acpi_bus_attach() overall,
> or acpi_add_single_object() needs to avoid calling methods that may
> depend on supplied opregions.  I guess the latter is rather
> unrealistic, so the only practical choice is the former.

I agree.

> However, I still don't think that the extra list of "dependent
> devices" is needed.

I'm not sure what you are trying to say here? Do you mean this list:

+/*
+ * List of HIDs for which we defer adding them to the second step of the
+ * scanning of the root, because some of their methods used during addition
+ * depend on OpRegions registered by the drivers for other ACPI devices.
+ */
+static const char * const acpi_defer_add_hids[] = {
+	"BCM2E5B", /* Acer SW3-016 bluetooth HID when GPIO OpRegs or not available yet */
+	NULL
+};
+

?

That indeed is not necessary if you take the entire set and always enable the
new behavior instead of using the module option. I guess we could go that route
for 5.12, and get it into next as soon as 5.11-rc1 is available for maximum
testing.

Do you want me to do a new version of the series which drops the acpi_defer_add_hids[]
thing and the module option and simply always uses the new behavior?

Regards,

Hans

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Rafael J. Wysocki]

On Thu, Dec 3, 2020 at 10:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/2/20 8:57 PM, Rafael J. Wysocki wrote:
> > On Wed, Dec 2, 2020 at 8:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Hi,
> >>
> >> On 12/2/20 2:49 PM, Rafael J. Wysocki wrote:
> >>> On Sat, Nov 21, 2020 at 9:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>
> >>>> Hi Rafael,
> >>>>
> >>>> A while ago (almost 2 years ago) I discussed an issue with you about
> >>>> some devices, where some of the methods used during device-addition
> >>>> (such as _HID) may rely on OpRegions of other devices:
> >>>>
> >>>> https://www.spinics.net/lists/linux-acpi/msg86303.html
> >>>>
> >>>> 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.
> >>>>
> >>>> Last week I bought a second hand Acer device, not knowing it was this
> >>>> exact model. Since I now have access to affected hardware I decided to
> >>>> take a shot at fixing this.
> >>>>
> >>>> In the discussion you suggested to split the acpi_bus_scan of the root
> >>>> into 2 steps, first scan devices with an empty _DEP, putting other
> >>>> acpi_handle-s on a list of deferred devices and then in step 2 scan the
> >>>> rest.
> >>>>
> >>>> I'm happy to report that, at least on the affected device, this works
> >>>> nicely. While working on this I came up with a less drastic way to
> >>>> deal with this. As you will see in patch 4 of this series, I decided
> >>>> to first add a more KISS method of deciding which devices to defer
> >>>> to the second scan step by matching by HID. This has the disadvantage
> >>>> of not being a generic solution. But it has the advantage of being a
> >>>> solution which does not potentially regress other devices.
> >>>>
> >>>> Then in patch 5 I actually do add the option to defer or not based on
> >>>> _DEP being empty. I've put this behind a kernel commandline option as
> >>>> I'm not sure we should do this everywhere by default. At least no without
> >>>> a lot more testing.
> >>>>
> >>>> Patch 6 fixes an issue with patch 5 which causes battery devices to stop
> >>>> working.
> >>>>
> >>>> And patch 7 adds some extra HIDs to the list of HIDs which should be
> >>>> ignored when checking if the _DEP list is empty from Linux' pov, iow
> >>>> some extra HIDs which Linux does not bind to.
> >>>>
> >>>> Please let me know what you think about this patch-set. I would be happy
> >>>> to see just patches 1-4 merged.
> >>>
> >>> I took patches 1 and 2, because IMO they are generally useful (I
> >>> rewrote the changelogs to avoid mentioning the rest of the series
> >>> though),
> >>
> >> That is fine. Thanks for taking those.
> >>
> >>> but I have some reservations regarding the rest.
> >>>
> >>> First off, I'm not really sure if failing acpi_add_single_object() for
> >>> devices with missing dependencies is a good idea.  IIRC there is
> >>> nothing in there that should depend on any opregions supplied by the
> >>> other devices, so it should be safe to allow it to complete.
> >>
> >> Actually acpi_add_single_object() does depend on ACPI methods
> >> which may depend on opregions, that is the whole reason why
> >> this series is necessary. Otherwise we could just delay the
> >> binding of the driver based in dep_unmet which would be easier.
> >>
> >> Here are 2 actual examples of acpi_add_single_object() calling
> >> ACPI methods which may depend on opregions:
> >>
> >> 1. acpi_add_single_object() calls acpi_init_device_object() which
> >> calls acpi_set_pnp_ids() which fills a bunch if fields of
> >> struct acpi_device with info returned by the acpi_get_object_info()
> >> call.
> >>
> >> Specifically it stores the value returned by the _HID method in
> >> the acpi_device_pnp array for the device and that _HID method is
> >> actually the problem in the example device which started this
> >> series. The _HID method of the bluetooth device reads 2 GPIOs
> >> to get a hw-id (0-3) and then translates the hwid to a _HID
> >> string. If the GPIO opregion's handlers have not registered yet
> >> then the reading of the GPIOs is correctly skipped, and hwid
> >> 0 is assumed, which is wrong in this case.
> >>
> >> 2. I've also seen examples where _STA depends on GPIOs in a similar
> >> manner; and acpi_add_single_object() calls acpi_bus_get_power_flags()
> >> which calls acpi_bus_init_power() which calls acpi_device_is_present()
> >> which depends on _STA results.
> >
> > Well, this means that there is a bug in acpi_bus_attach() which
> > shouldn't call acpi_bus_init_power() which has been called already.
>
> I'm afraid we have a bit of a misunderstanding here, the problem is
> not that acpi_bus_attach() calls acpi_bus_init_power(), the problem is
> that acpi_bus_init_power() (which is called from acpi_add_single_object())
> depends on the value returned by _STA and that in turn may depend on
> some OpRegions being available. IOW it is the same problem as the _HID
> problem.

This was a side note.

Arguably, acpi_bus_init_power() should not be called twice in a row
for the same device, which is actually done by the current code.

Another side note: while we can avoid evaluating _STA for the devices
whose enumeration we want to defer, avoiding to evaluate _HID (or _CID
for that matter) for them will be rather hard.

> > And it all means that either deferring acpi_add_single_object() is
> > needed and so there need to be 2 passes in acpi_bus_attach() overall,
> > or acpi_add_single_object() needs to avoid calling methods that may
> > depend on supplied opregions.  I guess the latter is rather
> > unrealistic, so the only practical choice is the former.
>
> I agree.
>
> > However, I still don't think that the extra list of "dependent
> > devices" is needed.
>
> I'm not sure what you are trying to say here? Do you mean this list:
>
> +/*
> + * List of HIDs for which we defer adding them to the second step of the
> + * scanning of the root, because some of their methods used during addition
> + * depend on OpRegions registered by the drivers for other ACPI devices.
> + */
> +static const char * const acpi_defer_add_hids[] = {
> +       "BCM2E5B", /* Acer SW3-016 bluetooth HID when GPIO OpRegs or not available yet */
> +       NULL
> +};
> +
>
> ?

No, the one created by patch [4/7] in your series, acpi_deferred_devices.

> That indeed is not necessary if you take the entire set and always enable the
> new behavior instead of using the module option. I guess we could go that route
> for 5.12, and get it into next as soon as 5.11-rc1 is available for maximum
> testing.
>
> Do you want me to do a new version of the series which drops the acpi_defer_add_hids[]
> thing and the module option and simply always uses the new behavior?

No, something else.  Stay tuned.

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Rafael J. Wysocki]

On Thursday, December 3, 2020 3:27:27 PM CET Rafael J. Wysocki wrote:
> On Thu, Dec 3, 2020 at 10:53 AM Hans de Goede <hdegoede@redhat.com> wrote:

[cut]

> > That indeed is not necessary if you take the entire set and always enable the
> > new behavior instead of using the module option. I guess we could go that route
> > for 5.12, and get it into next as soon as 5.11-rc1 is available for maximum
> > testing.

I'd prefer to do the whole thing in 5.11-rc and possibly revert something if it
causes problems to occur.

> > Do you want me to do a new version of the series which drops the acpi_defer_add_hids[]
> > thing and the module option and simply always uses the new behavior?
> 
> No, something else.  Stay tuned.

The patch below illustrates what I'd like to do.  It at least doesn't kill my
test-bed system, but also it doesn't cause the enumeration ordering to change
on that system.

It really is three patches in one and (again) I borrowed some code from your
patches in the $subject series.  [It is on top of the "ACPI: scan: Add PNP0D80
to the _DEP exceptions list" patch I've just posted.]


Please let me know what you think.

---
 drivers/acpi/scan.c |  141 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 120 insertions(+), 21 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1646,17 +1646,77 @@ void acpi_device_add_finalize(struct acp
 	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
 }
 
-static int acpi_add_single_object(struct acpi_device **child,
-				  acpi_handle handle, int type,
-				  unsigned long long sta)
+/*
+ * List of IDs for which we defer enumeration them to the second pass, because
+ * some of their methods used during addition depend on OpRegions registered by
+ * the drivers for other ACPI devices.
+ */
+static const char * const acpi_defer_enumeration_ids[] = {
+	"BCM2E5B", /* Acer SW3-016 bluetooth HID vs GPIO OpRegs */
+	NULL
+};
+
+static bool acpi_should_defer_enumeration(acpi_handle handle,
+					  struct acpi_device_info *info)
+{
+	struct acpi_handle_list dep_devices;
+	acpi_status status;
+	int i;
+
+	if (acpi_info_matches_ids(info, acpi_defer_enumeration_ids))
+		return true;
+
+	/*
+	 * We check for _HID here to avoid deferring the enumeration of:
+	 * 1. PCI devices
+	 * 2. ACPI nodes describing USB ports
+	 * However, checking for _HID catches more then just these cases ...
+	 */
+	if (!(info->valid & ACPI_VALID_HID))
+		return false;
+
+	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	for (i = 0; i < dep_devices.count; i++) {
+		struct acpi_device_info *dep_info;
+		bool ignore;
+
+		status = acpi_get_object_info(dep_devices.handles[i], &dep_info);
+		if (ACPI_FAILURE(status))
+			continue;
+
+		ignore = acpi_info_matches_ids(dep_info, acpi_ignore_dep_ids);
+		kfree(dep_info);
+
+		if (ignore)
+			continue;
+
+		return true;
+	}
+
+	return false;
+}
+
+static int __acpi_add_single_object(struct acpi_device **child,
+				    acpi_handle handle, int type,
+				    unsigned long long sta, bool check_dep)
 {
 	int result;
 	struct acpi_device *device;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	struct acpi_device_info *info = NULL;
 
-	if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE)
+	if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE) {
 		acpi_get_object_info(handle, &info);
+		if (check_dep && info &&
+		    acpi_should_defer_enumeration(handle, info)) {
+			kfree(info);
+			acpi_handle_info(handle, "Missing dependencies\n");
+			return -EAGAIN;
+		}
+	}
 
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
 	if (!device) {
@@ -1696,6 +1756,13 @@ static int acpi_add_single_object(struct
 	return 0;
 }
 
+static int acpi_add_single_object(struct acpi_device **child,
+				  acpi_handle handle, int type,
+				  unsigned long long sta)
+{
+	return __acpi_add_single_object(child, handle, type, sta, false);
+}
+
 static acpi_status acpi_get_resource_memory(struct acpi_resource *ares,
 					    void *context)
 {
@@ -1892,8 +1959,8 @@ static void acpi_device_dep_initialize(s
 	}
 }
 
-static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
-				      void *not_used, void **return_value)
+static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
+				      struct acpi_device **adev_p)
 {
 	struct acpi_device *device = NULL;
 	int type;
@@ -1913,7 +1980,7 @@ static acpi_status acpi_bus_check_add(ac
 		return AE_OK;
 	}
 
-	acpi_add_single_object(&device, handle, type, sta);
+	__acpi_add_single_object(&device, handle, type, sta, check_dep);
 	if (!device)
 		return AE_CTRL_DEPTH;
 
@@ -1921,12 +1988,24 @@ static acpi_status acpi_bus_check_add(ac
 	acpi_device_dep_initialize(device);
 
  out:
-	if (!*return_value)
-		*return_value = device;
+	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)
 {
 	/*
@@ -1994,12 +2073,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);
 
@@ -2046,9 +2129,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);
 }
 
@@ -2066,7 +2149,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);
 		}
@@ -2091,17 +2175,32 @@ EXPORT_SYMBOL_GPL(acpi_walk_dep_device_l
  */
 int acpi_bus_scan(acpi_handle handle)
 {
-	void *device = NULL;
+	struct acpi_device *device = NULL;
+
+	/* Pass 1: Avoid enumerating devices with missing dependencies. */
 
-	if (ACPI_SUCCESS(acpi_bus_check_add(handle, 0, NULL, &device)))
+	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);
 

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Hans de Goede]

Hi,

On 12/5/20 4:44 PM, Rafael J. Wysocki wrote:
> On Thursday, December 3, 2020 3:27:27 PM CET Rafael J. Wysocki wrote:
>> On Thu, Dec 3, 2020 at 10:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> [cut]
> 
>>> That indeed is not necessary if you take the entire set and always enable the
>>> new behavior instead of using the module option. I guess we could go that route
>>> for 5.12, and get it into next as soon as 5.11-rc1 is available for maximum
>>> testing.
> 
> I'd prefer to do the whole thing in 5.11-rc and possibly revert something if it
> causes problems to occur.

Ok, that is you call to make, so that is fine with me.

>>> Do you want me to do a new version of the series which drops the acpi_defer_add_hids[]
>>> thing and the module option and simply always uses the new behavior?
>>
>> No, something else.  Stay tuned.
> 
> The patch below illustrates what I'd like to do.  It at least doesn't kill my
> test-bed system, but also it doesn't cause the enumeration ordering to change
> on that system.
> 
> It really is three patches in one and (again) I borrowed some code from your
> patches in the $subject series.

Borrowing some of my code is fine, no worries about that. I'm happy as some
fix for this gets upstream in some form :)

>  [It is on top of the "ACPI: scan: Add PNP0D80
> to the _DEP exceptions list" patch I've just posted.]
> 
> 
> Please let me know what you think.

I think this should work fine. I've 2 small remarks inline below, but nothing
big / important.

My list of kernel things to look into is longer then my available time
(something which I assume you are familiar with), so for now I've only taken
a good look at your proposed solution and not actually tested it.

Let me know if you want me to give this a spin on the device with the _HID
issue as is, or if you have something closer to a final version ready
which you want me to test.

> ---
>  drivers/acpi/scan.c |  141 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 120 insertions(+), 21 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1646,17 +1646,77 @@ void acpi_device_add_finalize(struct acp
>  	kobject_uevent(&device->dev.kobj, KOBJ_ADD);
>  }
>  
> -static int acpi_add_single_object(struct acpi_device **child,
> -				  acpi_handle handle, int type,
> -				  unsigned long long sta)
> +/*
> + * List of IDs for which we defer enumeration them to the second pass, because
> + * some of their methods used during addition depend on OpRegions registered by
> + * the drivers for other ACPI devices.
> + */
> +static const char * const acpi_defer_enumeration_ids[] = {
> +	"BCM2E5B", /* Acer SW3-016 bluetooth HID vs GPIO OpRegs */
> +	NULL
> +};

Note that since you defer if there are unmet _DEP-s, this won't be necessary:

This list was purely there as a safer way to select devices which to defer,
the kernel cmdline option in my patch-set would switch between either using
this list, or checking _DEP. Since we are going to fully go for using _DEP
now, this can be dropped.

> +
> +static bool acpi_should_defer_enumeration(acpi_handle handle,
> +					  struct acpi_device_info *info)
> +{
> +	struct acpi_handle_list dep_devices;
> +	acpi_status status;
> +	int i;
> +
> +	if (acpi_info_matches_ids(info, acpi_defer_enumeration_ids))
> +		return true;
> +
> +	/*
> +	 * We check for _HID here to avoid deferring the enumeration of:
> +	 * 1. PCI devices
> +	 * 2. ACPI nodes describing USB ports
> +	 * However, checking for _HID catches more then just these cases ...
> +	 */
> +	if (!(info->valid & ACPI_VALID_HID))
> +		return false;

Interesting approach (using _HID), I went with _ADR since _ADR indicates
(more or less) that the ACPI fwnode is a companion node for a device
which will be enumerated through other means (such as PCI devices), rather
then one where the ACPI code will instantiate a platform_device, or
i2c_client (etc) for it.

Maybe if we want to play it safe check both, and if there either is no
HID, or there is an ADR do not defer ?  Note just thinking out loud here,
I'm fine with either approach.


> +
> +	status = acpi_evaluate_reference(handle, "_DEP", NULL, &dep_devices);
> +	if (ACPI_FAILURE(status))
> +		return false;
> +
> +	for (i = 0; i < dep_devices.count; i++) {
> +		struct acpi_device_info *dep_info;
> +		bool ignore;
> +
> +		status = acpi_get_object_info(dep_devices.handles[i], &dep_info);
> +		if (ACPI_FAILURE(status))
> +			continue;
> +
> +		ignore = acpi_info_matches_ids(dep_info, acpi_ignore_dep_ids);
> +		kfree(dep_info);
> +
> +		if (ignore)
> +			continue;
> +
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int __acpi_add_single_object(struct acpi_device **child,
> +				    acpi_handle handle, int type,
> +				    unsigned long long sta, bool check_dep)
>  {
>  	int result;
>  	struct acpi_device *device;
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>  	struct acpi_device_info *info = NULL;
>  
> -	if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE)
> +	if (handle != ACPI_ROOT_OBJECT && type == ACPI_BUS_TYPE_DEVICE) {
>  		acpi_get_object_info(handle, &info);
> +		if (check_dep && info &&
> +		    acpi_should_defer_enumeration(handle, info)) {
> +			kfree(info);
> +			acpi_handle_info(handle, "Missing dependencies\n");
> +			return -EAGAIN;
> +		}
> +	}
>  
>  	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
>  	if (!device) {
> @@ -1696,6 +1756,13 @@ static int acpi_add_single_object(struct
>  	return 0;
>  }
>  
> +static int acpi_add_single_object(struct acpi_device **child,
> +				  acpi_handle handle, int type,
> +				  unsigned long long sta)
> +{
> +	return __acpi_add_single_object(child, handle, type, sta, false);
> +}
> +
>  static acpi_status acpi_get_resource_memory(struct acpi_resource *ares,
>  					    void *context)
>  {
> @@ -1892,8 +1959,8 @@ static void acpi_device_dep_initialize(s
>  	}
>  }
>  
> -static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl_not_used,
> -				      void *not_used, void **return_value)
> +static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
> +				      struct acpi_device **adev_p)
>  {
>  	struct acpi_device *device = NULL;
>  	int type;
> @@ -1913,7 +1980,7 @@ static acpi_status acpi_bus_check_add(ac
>  		return AE_OK;
>  	}
>  
> -	acpi_add_single_object(&device, handle, type, sta);
> +	__acpi_add_single_object(&device, handle, type, sta, check_dep);
>  	if (!device)
>  		return AE_CTRL_DEPTH;
>  
> @@ -1921,12 +1988,24 @@ static acpi_status acpi_bus_check_add(ac
>  	acpi_device_dep_initialize(device);
>  
>   out:
> -	if (!*return_value)
> -		*return_value = device;
> +	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)
>  {
>  	/*
> @@ -1994,12 +2073,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);
>  
> @@ -2046,9 +2129,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);
>  }
>  
> @@ -2066,7 +2149,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);
>  		}
> @@ -2091,17 +2175,32 @@ EXPORT_SYMBOL_GPL(acpi_walk_dep_device_l
>   */
>  int acpi_bus_scan(acpi_handle handle)
>  {
> -	void *device = NULL;
> +	struct acpi_device *device = NULL;
> +
> +	/* Pass 1: Avoid enumerating devices with missing dependencies. */
>  
> -	if (ACPI_SUCCESS(acpi_bus_check_add(handle, 0, NULL, &device)))
> +	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);

Regards,

Hans

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Rafael J. Wysocki]

On Sat, Dec 5, 2020 at 6:03 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/5/20 4:44 PM, Rafael J. Wysocki wrote:
> > On Thursday, December 3, 2020 3:27:27 PM CET Rafael J. Wysocki wrote:
> >> On Thu, Dec 3, 2020 at 10:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > [cut]
> >
> >>> That indeed is not necessary if you take the entire set and always enable the
> >>> new behavior instead of using the module option. I guess we could go that route
> >>> for 5.12, and get it into next as soon as 5.11-rc1 is available for maximum
> >>> testing.
> >
> > I'd prefer to do the whole thing in 5.11-rc and possibly revert something if it
> > causes problems to occur.
>
> Ok, that is you call to make, so that is fine with me.
>
> >>> Do you want me to do a new version of the series which drops the acpi_defer_add_hids[]
> >>> thing and the module option and simply always uses the new behavior?
> >>
> >> No, something else.  Stay tuned.
> >
> > The patch below illustrates what I'd like to do.  It at least doesn't kill my
> > test-bed system, but also it doesn't cause the enumeration ordering to change
> > on that system.
> >
> > It really is three patches in one and (again) I borrowed some code from your
> > patches in the $subject series.
>
> Borrowing some of my code is fine, no worries about that. I'm happy as some
> fix for this gets upstream in some form :)
>
> >  [It is on top of the "ACPI: scan: Add PNP0D80
> > to the _DEP exceptions list" patch I've just posted.]
> >
> >
> > Please let me know what you think.
>
> I think this should work fine. I've 2 small remarks inline below, but nothing
> big / important.
>
> My list of kernel things to look into is longer then my available time
> (something which I assume you are familiar with), so for now I've only taken
> a good look at your proposed solution and not actually tested it.
>
> Let me know if you want me to give this a spin on the device with the _HID
> issue as is, or if you have something closer to a final version ready
> which you want me to test.

I will, thanks!

> > ---
> >  drivers/acpi/scan.c |  141 ++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 120 insertions(+), 21 deletions(-)
> >
> > Index: linux-pm/drivers/acpi/scan.c
> > ===================================================================
> > --- linux-pm.orig/drivers/acpi/scan.c
> > +++ linux-pm/drivers/acpi/scan.c
> > @@ -1646,17 +1646,77 @@ void acpi_device_add_finalize(struct acp
> >       kobject_uevent(&device->dev.kobj, KOBJ_ADD);
> >  }
> >
> > -static int acpi_add_single_object(struct acpi_device **child,
> > -                               acpi_handle handle, int type,
> > -                               unsigned long long sta)
> > +/*
> > + * List of IDs for which we defer enumeration them to the second pass, because
> > + * some of their methods used during addition depend on OpRegions registered by
> > + * the drivers for other ACPI devices.
> > + */
> > +static const char * const acpi_defer_enumeration_ids[] = {
> > +     "BCM2E5B", /* Acer SW3-016 bluetooth HID vs GPIO OpRegs */
> > +     NULL
> > +};
>
> Note that since you defer if there are unmet _DEP-s, this won't be necessary:
>
> This list was purely there as a safer way to select devices which to defer,
> the kernel cmdline option in my patch-set would switch between either using
> this list, or checking _DEP. Since we are going to fully go for using _DEP
> now, this can be dropped.

OK

If that is the case, I'd prefer to check the _DEP list even earlier,
possibly in acpi_bus_check_add(), so as to avoid having to evaluate
_HID or _CID for devices with non-trivial _DEP lists (after taking
acpi_ignore_dep_ids[] into account) in the first pass.

> > +
> > +static bool acpi_should_defer_enumeration(acpi_handle handle,
> > +                                       struct acpi_device_info *info)
> > +{
> > +     struct acpi_handle_list dep_devices;
> > +     acpi_status status;
> > +     int i;
> > +
> > +     if (acpi_info_matches_ids(info, acpi_defer_enumeration_ids))
> > +             return true;
> > +
> > +     /*
> > +      * We check for _HID here to avoid deferring the enumeration of:
> > +      * 1. PCI devices
> > +      * 2. ACPI nodes describing USB ports
> > +      * However, checking for _HID catches more then just these cases ...
> > +      */
> > +     if (!(info->valid & ACPI_VALID_HID))
> > +             return false;
>
> Interesting approach (using _HID), I went with _ADR since _ADR indicates
> (more or less) that the ACPI fwnode is a companion node for a device
> which will be enumerated through other means (such as PCI devices), rather
> then one where the ACPI code will instantiate a platform_device, or
> i2c_client (etc) for it.
>
> Maybe if we want to play it safe check both, and if there either is no
> HID, or there is an ADR do not defer ?  Note just thinking out loud here,
> I'm fine with either approach.

By the spec checking _HID should be equivalent to checking _ADR
(Section 6.1 of ACPI 6.3 says "A device object must contain either an
_HID object or an _ADR,  but should not contain both"), but the
presence of _HID indicates that the device is expected to be
enumerated via ACPI and so _DEP is more likely to really matter IMV.

Re: [PATCH 0/7] ACPI: scan: Split root scanning into 2 steps

[Hans de Goede]

Hi,

On 12/7/20 6:27 PM, Rafael J. Wysocki wrote:
> On Sat, Dec 5, 2020 at 6:03 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 12/5/20 4:44 PM, Rafael J. Wysocki wrote:
>>> On Thursday, December 3, 2020 3:27:27 PM CET Rafael J. Wysocki wrote:
>>>> On Thu, Dec 3, 2020 at 10:53 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> [cut]
>>>
>>>>> That indeed is not necessary if you take the entire set and always enable the
>>>>> new behavior instead of using the module option. I guess we could go that route
>>>>> for 5.12, and get it into next as soon as 5.11-rc1 is available for maximum
>>>>> testing.
>>>
>>> I'd prefer to do the whole thing in 5.11-rc and possibly revert something if it
>>> causes problems to occur.
>>
>> Ok, that is you call to make, so that is fine with me.
>>
>>>>> Do you want me to do a new version of the series which drops the acpi_defer_add_hids[]
>>>>> thing and the module option and simply always uses the new behavior?
>>>>
>>>> No, something else.  Stay tuned.
>>>
>>> The patch below illustrates what I'd like to do.  It at least doesn't kill my
>>> test-bed system, but also it doesn't cause the enumeration ordering to change
>>> on that system.
>>>
>>> It really is three patches in one and (again) I borrowed some code from your
>>> patches in the $subject series.
>>
>> Borrowing some of my code is fine, no worries about that. I'm happy as some
>> fix for this gets upstream in some form :)
>>
>>>  [It is on top of the "ACPI: scan: Add PNP0D80
>>> to the _DEP exceptions list" patch I've just posted.]
>>>
>>>
>>> Please let me know what you think.
>>
>> I think this should work fine. I've 2 small remarks inline below, but nothing
>> big / important.
>>
>> My list of kernel things to look into is longer then my available time
>> (something which I assume you are familiar with), so for now I've only taken
>> a good look at your proposed solution and not actually tested it.
>>
>> Let me know if you want me to give this a spin on the device with the _HID
>> issue as is, or if you have something closer to a final version ready
>> which you want me to test.
> 
> I will, thanks!
> 
>>> ---
>>>  drivers/acpi/scan.c |  141 ++++++++++++++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 120 insertions(+), 21 deletions(-)
>>>
>>> Index: linux-pm/drivers/acpi/scan.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/acpi/scan.c
>>> +++ linux-pm/drivers/acpi/scan.c
>>> @@ -1646,17 +1646,77 @@ void acpi_device_add_finalize(struct acp
>>>       kobject_uevent(&device->dev.kobj, KOBJ_ADD);
>>>  }
>>>
>>> -static int acpi_add_single_object(struct acpi_device **child,
>>> -                               acpi_handle handle, int type,
>>> -                               unsigned long long sta)
>>> +/*
>>> + * List of IDs for which we defer enumeration them to the second pass, because
>>> + * some of their methods used during addition depend on OpRegions registered by
>>> + * the drivers for other ACPI devices.
>>> + */
>>> +static const char * const acpi_defer_enumeration_ids[] = {
>>> +     "BCM2E5B", /* Acer SW3-016 bluetooth HID vs GPIO OpRegs */
>>> +     NULL
>>> +};
>>
>> Note that since you defer if there are unmet _DEP-s, this won't be necessary:
>>
>> This list was purely there as a safer way to select devices which to defer,
>> the kernel cmdline option in my patch-set would switch between either using
>> this list, or checking _DEP. Since we are going to fully go for using _DEP
>> now, this can be dropped.
> 
> OK
> 
> If that is the case, I'd prefer to check the _DEP list even earlier,
> possibly in acpi_bus_check_add(), so as to avoid having to evaluate
> _HID or _CID for devices with non-trivial _DEP lists (after taking
> acpi_ignore_dep_ids[] into account) in the first pass.

That is fine by me.

Note that in my non scientific measurement the slowdown of my patch
(with the cmdline option set to using _DEP as base of the decision
to defer or not) was almost non measurable (seemed to be about 10ms)
on a low-power Cherry Trail system. So I don't think that we need
to worry about optimizing this too much.

We currently do evaluate _HID and _CID of all the deps repeatedly,
typically only a few devices are used as deps for most other
devices. So a possible future optimization would be an acpi_device_info
cache (mapping handle-s to device_info) for devices listed as _DEP.
But again, I don't think we need to worry too much about performance
here.

>>> +
>>> +static bool acpi_should_defer_enumeration(acpi_handle handle,
>>> +                                       struct acpi_device_info *info)
>>> +{
>>> +     struct acpi_handle_list dep_devices;
>>> +     acpi_status status;
>>> +     int i;
>>> +
>>> +     if (acpi_info_matches_ids(info, acpi_defer_enumeration_ids))
>>> +             return true;
>>> +
>>> +     /*
>>> +      * We check for _HID here to avoid deferring the enumeration of:
>>> +      * 1. PCI devices
>>> +      * 2. ACPI nodes describing USB ports
>>> +      * However, checking for _HID catches more then just these cases ...
>>> +      */
>>> +     if (!(info->valid & ACPI_VALID_HID))
>>> +             return false;
>>
>> Interesting approach (using _HID), I went with _ADR since _ADR indicates
>> (more or less) that the ACPI fwnode is a companion node for a device
>> which will be enumerated through other means (such as PCI devices), rather
>> then one where the ACPI code will instantiate a platform_device, or
>> i2c_client (etc) for it.
>>
>> Maybe if we want to play it safe check both, and if there either is no
>> HID, or there is an ADR do not defer ?  Note just thinking out loud here,
>> I'm fine with either approach.
> 
> By the spec checking _HID should be equivalent to checking _ADR
> (Section 6.1 of ACPI 6.3 says "A device object must contain either an
> _HID object or an _ADR,  but should not contain both"), but the
> presence of _HID indicates that the device is expected to be
> enumerated via ACPI and so _DEP is more likely to really matter IMV.

Ah, I see I did not know about the either a HID or an ADR rule. I just
needed something available in acpi_device_info with which I could
skip PCI devices. So I ended up picking ADR, if you prefer HID that
works for me.

Regards,

Hans

Re: [PATCH] ACPI: scan: Defer enumeration of devices with _DEP lists

[youling257]

linux 5.11 rc1 "ACPI: scan: Defer enumeration of devices with _DEP lists" cause my v891w z3735f tablet boot failed, there is only one cursor in the upper left corner.
linux 5.11 rc5 "ACPI: scan: Make acpi_bus_get_device() clear return pointer on error" fixed this problem.