diff mbox series

[v7,01/10] ACPI: scan: Remove the second DSDT traversal

Message ID 20230328101303.1458570-2-sakari.ailus@linux.intel.com (mailing list archive)
State Superseded, archived
Headers show
Series ACPI _CRS CSI-2 and MIPI DisCo for Imaging support | expand

Commit Message

Sakari Ailus March 28, 2023, 10:12 a.m. UTC
Collect the devices with _DEP into a list and continue processing them
after a full traversal, instead of doing a full second traversal of the
tree.

This makes the second DSDT traversal pass unnecessary as we already have
the nodes we're interested in in a linked list.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/scan.c | 125 ++++++++++++++++++++++++++++++++------------
 1 file changed, 92 insertions(+), 33 deletions(-)

Comments

Andy Shevchenko March 28, 2023, 2:45 p.m. UTC | #1
On Tue, Mar 28, 2023 at 01:12:54PM +0300, Sakari Ailus wrote:
> Collect the devices with _DEP into a list and continue processing them
> after a full traversal, instead of doing a full second traversal of the
> tree.
> 
> This makes the second DSDT traversal pass unnecessary as we already have
> the nodes we're interested in in a linked list.

...

> +/**
> + * struct acpi_postponed_handle - A postponed ACPI handle
> + * @handle: The postponed handle
> + * @list: Entry in a postponed list
> + *
> + * One such entry represents an ACPI handle the scanning of which has been
> + * postponed.
> + */
> +struct acpi_postponed_handle {
> +	acpi_handle handle;
> +	struct list_head list;
> +};

If you put the list to be the first member, container_of() against it becomes a
no-op at compile time. Have you checked the code generation if you swap these
members?

> +/**
> + * struct acpi_scan_context - Context for scanning ACPI devices
> + * @device: The first encountered device, typically the root of the scanned tree
> + * @postponed_head: The list head of the postponed ACPI handles
> + */
> +struct acpi_scan_context {
> +	struct acpi_device *device;
> +	struct list_head postponed_head;
> +};

Ditto.

...

> +/**
> + * acpi_bus_handle_postpone - Add an ACPI handle to a given postponed list
> + * @handle: The ACPI handle
> + * @head: Postponed list head
> + *
> + * Add a given ACPI handle to a list of ACPI objects for which the creation
> + * of the device objects is to be postponed.

`kernel-doc -v ...` complains on the absence of Return section. Is it the same
for you?

> + */
Sakari Ailus March 28, 2023, 2:48 p.m. UTC | #2
Hi Andy,

Thank you for the review.

On Tue, Mar 28, 2023 at 05:45:13PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 01:12:54PM +0300, Sakari Ailus wrote:
> > Collect the devices with _DEP into a list and continue processing them
> > after a full traversal, instead of doing a full second traversal of the
> > tree.
> > 
> > This makes the second DSDT traversal pass unnecessary as we already have
> > the nodes we're interested in in a linked list.
> 
> ...
> 
> > +/**
> > + * struct acpi_postponed_handle - A postponed ACPI handle
> > + * @handle: The postponed handle
> > + * @list: Entry in a postponed list
> > + *
> > + * One such entry represents an ACPI handle the scanning of which has been
> > + * postponed.
> > + */
> > +struct acpi_postponed_handle {
> > +	acpi_handle handle;
> > +	struct list_head list;
> > +};
> 
> If you put the list to be the first member, container_of() against it becomes a
> no-op at compile time. Have you checked the code generation if you swap these
> members?

I haven't checked but that would seem like a reasonable thing to do. A
pointer to the handle isn't used, unlike it is for the list.

> 
> > +/**
> > + * struct acpi_scan_context - Context for scanning ACPI devices
> > + * @device: The first encountered device, typically the root of the scanned tree
> > + * @postponed_head: The list head of the postponed ACPI handles
> > + */
> > +struct acpi_scan_context {
> > +	struct acpi_device *device;
> > +	struct list_head postponed_head;
> > +};
> 
> Ditto.
> 
> ...
> 
> > +/**
> > + * acpi_bus_handle_postpone - Add an ACPI handle to a given postponed list
> > + * @handle: The ACPI handle
> > + * @head: Postponed list head
> > + *
> > + * Add a given ACPI handle to a list of ACPI objects for which the creation
> > + * of the device objects is to be postponed.
> 
> `kernel-doc -v ...` complains on the absence of Return section. Is it the same
> for you?

That may well be. These comments wouldn't necessarily even need to be
kernel-doc as the rest isn't so documented either. I can of course fix the
error while leaving these kernel-doc.
Sakari Ailus March 28, 2023, 2:55 p.m. UTC | #3
On Tue, Mar 28, 2023 at 05:48:35PM +0300, Sakari Ailus wrote:
> > > +/**
> > > + * acpi_bus_handle_postpone - Add an ACPI handle to a given postponed list
> > > + * @handle: The ACPI handle
> > > + * @head: Postponed list head
> > > + *
> > > + * Add a given ACPI handle to a list of ACPI objects for which the creation
> > > + * of the device objects is to be postponed.
> > 
> > `kernel-doc -v ...` complains on the absence of Return section. Is it the same
> > for you?
> 
> That may well be. These comments wouldn't necessarily even need to be
> kernel-doc as the rest isn't so documented either. I can of course fix the
> error while leaving these kernel-doc.

No warning is produced here. That would seem like a false positive as the
function returns void.
diff mbox series

Patch

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 0c6f06abe3f4..df97c2babf39 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2029,10 +2029,52 @@  static u32 acpi_scan_check_dep(acpi_handle handle, bool check_dep)
 	return count;
 }
 
-static bool acpi_bus_scan_second_pass;
+/**
+ * struct acpi_postponed_handle - A postponed ACPI handle
+ * @handle: The postponed handle
+ * @list: Entry in a postponed list
+ *
+ * One such entry represents an ACPI handle the scanning of which has been
+ * postponed.
+ */
+struct acpi_postponed_handle {
+	acpi_handle handle;
+	struct list_head list;
+};
+
+/**
+ * struct acpi_scan_context - Context for scanning ACPI devices
+ * @device: The first encountered device, typically the root of the scanned tree
+ * @postponed_head: The list head of the postponed ACPI handles
+ */
+struct acpi_scan_context {
+	struct acpi_device *device;
+	struct list_head postponed_head;
+};
+
+/**
+ * acpi_bus_handle_postpone - Add an ACPI handle to a given postponed list
+ * @handle: The ACPI handle
+ * @head: Postponed list head
+ *
+ * Add a given ACPI handle to a list of ACPI objects for which the creation
+ * of the device objects is to be postponed.
+ */
+static void acpi_bus_handle_postpone(acpi_handle handle,
+				     struct list_head *head)
+{
+	struct acpi_postponed_handle *ph;
+
+	ph = kzalloc(sizeof(*ph), GFP_KERNEL);
+	if (!ph)
+		return;
+
+	ph->handle = handle;
+	list_add(&ph->list, head);
+}
 
 static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
-				      struct acpi_device **adev_p)
+				      struct acpi_scan_context *ctx)
 {
 	struct acpi_device *device = acpi_fetch_acpi_dev(handle);
 	acpi_object_type acpi_type;
@@ -2051,7 +2093,7 @@  static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
 
 		/* Bail out if there are dependencies. */
 		if (acpi_scan_check_dep(handle, check_dep) > 0) {
-			acpi_bus_scan_second_pass = true;
+			acpi_bus_handle_postpone(handle, &ctx->postponed_head);
 			return AE_CTRL_DEPTH;
 		}
 
@@ -2086,22 +2128,22 @@  static acpi_status acpi_bus_check_add(acpi_handle handle, bool check_dep,
 	acpi_scan_init_hotplug(device);
 
 out:
-	if (!*adev_p)
-		*adev_p = device;
+	if (!ctx->device)
+		ctx->device = 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)
+					void *ctx, void **unused)
 {
-	return acpi_bus_check_add(handle, true, (struct acpi_device **)ret_p);
+	return acpi_bus_check_add(handle, true, (struct acpi_scan_context *)ctx);
 }
 
 static acpi_status acpi_bus_check_add_2(acpi_handle handle, u32 lvl_not_used,
-					void *not_used, void **ret_p)
+					void *ctx, void **device)
 {
-	return acpi_bus_check_add(handle, false, (struct acpi_device **)ret_p);
+	return acpi_bus_check_add(handle, false, (struct acpi_scan_context *)ctx);
 }
 
 static void acpi_default_enumeration(struct acpi_device *device)
@@ -2422,37 +2464,54 @@  EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev);
  */
 int acpi_bus_scan(acpi_handle handle)
 {
-	struct acpi_device *device = NULL;
-
-	acpi_bus_scan_second_pass = false;
-
-	/* Pass 1: Avoid enumerating devices with missing dependencies. */
+	struct acpi_scan_context ctx = {
+		.postponed_head = LIST_HEAD_INIT(ctx.postponed_head),
+	};
+	struct acpi_postponed_handle *ph, *tmp_ph;
+	int ret = 0;
 
-	if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &device)))
+	if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &ctx)))
 		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
-				    acpi_bus_check_add_1, NULL, NULL,
-				    (void **)&device);
-
-	if (!device)
-		return -ENODEV;
-
-	acpi_bus_attach(device, (void *)true);
+				    acpi_bus_check_add_1, NULL, (void *)&ctx,
+				    NULL);
 
-	if (!acpi_bus_scan_second_pass)
-		return 0;
-
-	/* Pass 2: Enumerate all of the remaining devices. */
+	if (!ctx.device) {
+		ret = -ENODEV;
+		goto out_release;
+	}
 
-	device = NULL;
+	acpi_bus_attach(ctx.device, (void *)true);
 
-	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);
+	/*
+	 * Proceed to register ACPI devices that were postponed due to _DEP
+	 * objects during the namespace walk.
+	 */
+	list_for_each_entry_safe(ph, tmp_ph, &ctx.postponed_head, list) {
+		list_del(&ph->list);
+		/* Set device NULL here to obtain the root for this sub-tree. */
+		ctx.device = NULL;
+		/*
+		 * Do this manually, as the namespace walk will only include
+		 * sub-nodes, not the node itself. ctx.device is set to the
+		 * ACPI device corresponding ph->handle.
+		 */
+		acpi_bus_check_add_2(ph->handle, 0, &ctx, NULL);
+		/* Walk the rest of the sub-namespace. */
+		acpi_walk_namespace(ACPI_TYPE_ANY, ph->handle, ACPI_UINT32_MAX,
+				    acpi_bus_check_add_2, NULL, (void *)&ctx,
+				    NULL);
+		if (ctx.device)
+			acpi_bus_attach(ctx.device, NULL);
+		kfree(ph);
+	}
 
-	acpi_bus_attach(device, NULL);
+out_release:
+	list_for_each_entry_safe(ph, tmp_ph, &ctx.postponed_head, list) {
+		list_del(&ph->list);
+		kfree(ph);
+	}
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(acpi_bus_scan);