[PATCH v2 1/1] ACPI: scan: Use unique number for instance_no

[PATCH v2 1/1] ACPI: scan: Use unique number for instance_no

[Andy Shevchenko]

Current mechanism of incrementing and decrementing plain integer
to get a next free instance_no when creating an ACPI device is buggy.

The simple integer and operations line increment and decrement
on top of it can't cover the possible gaps during run time. The
arbitrary instantiation and elimination of the devices is racy
and after a couple of iterations with unequal amount of devices
being added and removed we may reproduce a bug:

"sysfs: cannot create duplicate filename '/bus/acpi/devices/PRP0001:02'"

Replace plain integer approach by using IDA framework.

Fixes: e49bd2dd5a50 ("ACPI: use PNPID:instance_no as bus_id of ACPI device")
Fixes: ca9dc8d42b30 ("ACPI / scan: Fix acpi_bus_id_list bookkeeping")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2:
* renamed no -> instance_ida
* added instance_no to device::pnp
* rephrased commit message

 drivers/acpi/internal.h |  4 +++-
 drivers/acpi/scan.c     | 32 +++++++++++++++++++++++++++-----
 include/acpi/acpi_bus.h |  1 +
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index e6a5d997241c..417eb768d710 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -9,6 +9,8 @@
 #ifndef _ACPI_INTERNAL_H_
 #define _ACPI_INTERNAL_H_
 
+#include <linux/idr.h>
+
 #define PREFIX "ACPI: "
 
 int early_acpi_osi_init(void);
@@ -98,7 +100,7 @@ extern struct list_head acpi_bus_id_list;
 
 struct acpi_device_bus_id {
 	const char *bus_id;
-	unsigned int instance_no;
+	struct ida instance_ida;
 	struct list_head node;
 };
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index a184529d8fa4..4b445b169ad4 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -479,9 +479,8 @@ static void acpi_device_del(struct acpi_device *device)
 	list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node)
 		if (!strcmp(acpi_device_bus_id->bus_id,
 			    acpi_device_hid(device))) {
-			if (acpi_device_bus_id->instance_no > 0)
-				acpi_device_bus_id->instance_no--;
-			else {
+			ida_simple_remove(&acpi_device_bus_id->instance_ida, device->pnp.instance_no);
+			if (ida_is_empty(&acpi_device_bus_id->instance_ida)) {
 				list_del(&acpi_device_bus_id->node);
 				kfree_const(acpi_device_bus_id->bus_id);
 				kfree(acpi_device_bus_id);
@@ -631,6 +630,20 @@ static struct acpi_device_bus_id *acpi_device_bus_id_match(const char *dev_id)
 	return NULL;
 }
 
+static int acpi_device_set_name(struct acpi_device *device,
+				struct acpi_device_bus_id *acpi_device_bus_id)
+{
+	int result;
+
+	result = ida_simple_get(&acpi_device_bus_id->instance_ida, 0, 255, GFP_KERNEL);
+	if (result < 0)
+		return result;
+
+	device->pnp.instance_no = result;
+	dev_set_name(&device->dev, "%s:%02x", acpi_device_bus_id->bus_id, result);
+	return 0;
+}
+
 int acpi_device_add(struct acpi_device *device,
 		    void (*release)(struct device *))
 {
@@ -665,7 +678,9 @@ int acpi_device_add(struct acpi_device *device,
 
 	acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device));
 	if (acpi_device_bus_id) {
-		acpi_device_bus_id->instance_no++;
+		result = acpi_device_set_name(device, acpi_device_bus_id);
+		if (result)
+			goto err_unlock;
 	} else {
 		acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id),
 					     GFP_KERNEL);
@@ -681,9 +696,16 @@ int acpi_device_add(struct acpi_device *device,
 			goto err_unlock;
 		}
 
+		ida_init(&acpi_device_bus_id->instance_ida);
+
+		result = acpi_device_set_name(device, acpi_device_bus_id);
+		if (result) {
+			kfree(acpi_device_bus_id);
+			goto err_unlock;
+		}
+
 		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
 	}
-	dev_set_name(&device->dev, "%s:%02x", acpi_device_bus_id->bus_id, acpi_device_bus_id->instance_no);
 
 	if (device->parent)
 		list_add_tail(&device->node, &device->parent->children);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 02a716a0af5d..f28b097c658f 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -233,6 +233,7 @@ struct acpi_pnp_type {
 
 struct acpi_device_pnp {
 	acpi_bus_id bus_id;		/* Object name */
+	int instance_no;		/* Instance number of this object */
 	struct acpi_pnp_type type;	/* ID type */
 	acpi_bus_address bus_address;	/* _ADR */
 	char *unique_id;		/* _UID */
-- 
2.30.2

Re: [PATCH v2 1/1] ACPI: scan: Use unique number for instance_no

[Rafael J. Wysocki]

On Fri, Mar 19, 2021 at 7:33 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Current mechanism of incrementing and decrementing plain integer
> to get a next free instance_no when creating an ACPI device is buggy.
>
> The simple integer and operations line increment and decrement
> on top of it can't cover the possible gaps during run time. The
> arbitrary instantiation and elimination of the devices is racy

But it isn't racy AFAICS.  It always happens under acpi_device_lock().

> and after a couple of iterations with unequal amount of devices
> being added and removed we may reproduce a bug:

Re: [PATCH v2 1/1] ACPI: scan: Use unique number for instance_no

[Andy Shevchenko]

On Fri, Mar 19, 2021 at 07:51:00PM +0100, Rafael J. Wysocki wrote:
> On Fri, Mar 19, 2021 at 7:33 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Current mechanism of incrementing and decrementing plain integer
> > to get a next free instance_no when creating an ACPI device is buggy.
> >
> > The simple integer and operations line increment and decrement
> > on top of it can't cover the possible gaps during run time. The
> > arbitrary instantiation and elimination of the devices is racy
> 
> But it isn't racy AFAICS.  It always happens under acpi_device_lock().

Hmm.. indeed. I sent a v3 with the commit message based on your proposal.


> > and after a couple of iterations with unequal amount of devices
> > being added and removed we may reproduce a bug:

-- 
With Best Regards,
Andy Shevchenko