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

[PATCH v1 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 fragile.

In case of hot plug event or namespace removal of the device instances
with the low numbers the plain integer counter can't cover the gaps
and become desynchronized with real state of affairs. If during next
hot plug event or namespace injection the new instances of
the devices need to be instantiated, the counter may mistakenly point
to the existing instance_no and kernel will complain:
"sysfs: cannot create duplicate filename '/bus/acpi/devices/XXXX1234: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>
---
 drivers/acpi/internal.h |  4 ++-
 drivers/acpi/scan.c     | 55 +++++++++++++++++++++++++++++++++++++----
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index e6a5d997241c..6fee4f71ba1c 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 no;
 	struct list_head node;
 };
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index a184529d8fa4..a118a58f7dad 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -468,9 +468,27 @@ static void acpi_device_release(struct device *dev)
 	kfree(acpi_dev);
 }
 
+static int acpi_device_get_instance_no(struct acpi_device *device)
+{
+	const char *p;
+	int result;
+	int error;
+
+	p = strrchr(dev_name(&device->dev), ':');
+	if (!p)
+		return -ENODATA;
+
+	error = kstrtoint(p + 1, 16, &result);
+	if (error)
+		return error;
+
+	return result;
+}
+
 static void acpi_device_del(struct acpi_device *device)
 {
 	struct acpi_device_bus_id *acpi_device_bus_id;
+	int result;
 
 	mutex_lock(&acpi_device_lock);
 	if (device->parent)
@@ -479,9 +497,13 @@ 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 {
+			result = acpi_device_get_instance_no(device);
+			if (result < 0)
+				dev_warn(&device->dev, "Can't get instance no\n");
+			else
+				ida_simple_remove(&acpi_device_bus_id->no, result);
+
+			if (ida_is_empty(&acpi_device_bus_id->no)) {
 				list_del(&acpi_device_bus_id->node);
 				kfree_const(acpi_device_bus_id->bus_id);
 				kfree(acpi_device_bus_id);
@@ -631,6 +653,19 @@ 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->no, 0, 255, GFP_KERNEL);
+	if (result < 0)
+		return 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 +700,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 < 0)
+			goto err_unlock;
 	} else {
 		acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id),
 					     GFP_KERNEL);
@@ -681,9 +718,17 @@ int acpi_device_add(struct acpi_device *device,
 			goto err_unlock;
 		}
 
+		ida_init(&acpi_device_bus_id->no);
+
+		result = acpi_device_set_name(device, acpi_device_bus_id);
+		if (result < 0) {
+			ida_destroy(&acpi_device_bus_id->no);
+			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);
-- 
2.30.1

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

[Rafael J. Wysocki]

On Fri, Mar 12, 2021 at 5:02 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 fragile.
>
> In case of hot plug event or namespace removal of the device instances
> with the low numbers the plain integer counter can't cover the gaps
> and become desynchronized with real state of affairs. If during next
> hot plug event or namespace injection the new instances of
> the devices need to be instantiated, the counter may mistakenly point
> to the existing instance_no and kernel will complain:
> "sysfs: cannot create duplicate filename '/bus/acpi/devices/XXXX1234:02'"

This is a slightly convoluted way of stating that there is a bug in
acpi_device_del().

Yes, there is one, the instance_no decrementation is clearly incorrect.

> Replace plain integer approach by using IDA framework.

Also the general idea of using IDA for the instance numbering is a good one IMO.

> 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>
> ---
>  drivers/acpi/internal.h |  4 ++-
>  drivers/acpi/scan.c     | 55 +++++++++++++++++++++++++++++++++++++----
>  2 files changed, 53 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index e6a5d997241c..6fee4f71ba1c 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 no;

struct ida instance_ida; ?

>         struct list_head node;
>  };
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index a184529d8fa4..a118a58f7dad 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -468,9 +468,27 @@ static void acpi_device_release(struct device *dev)
>         kfree(acpi_dev);
>  }
>
> +static int acpi_device_get_instance_no(struct acpi_device *device)
> +{
> +       const char *p;
> +       int result;
> +       int error;
> +
> +       p = strrchr(dev_name(&device->dev), ':');
> +       if (!p)
> +               return -ENODATA;
> +
> +       error = kstrtoint(p + 1, 16, &result);
> +       if (error)
> +               return error;
> +
> +       return result;
> +}

I don't like the above at all.

I would just store the instance number in struct acpi_device_pnp (say).

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

[Andy Shevchenko]

On Fri, Mar 19, 2021 at 06:00:38PM +0100, Rafael J. Wysocki wrote:
> On Fri, Mar 12, 2021 at 5:02 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 fragile.
> >
> > In case of hot plug event or namespace removal of the device instances
> > with the low numbers the plain integer counter can't cover the gaps
> > and become desynchronized with real state of affairs. If during next
> > hot plug event or namespace injection the new instances of
> > the devices need to be instantiated, the counter may mistakenly point
> > to the existing instance_no and kernel will complain:
> > "sysfs: cannot create duplicate filename '/bus/acpi/devices/XXXX1234:02'"
> 
> This is a slightly convoluted way of stating that there is a bug in
> acpi_device_del().

Any suggestion how to massage the above?
But in the dry end, yes, decrementing is a bug.

> Yes, there is one, the instance_no decrementation is clearly incorrect.
> 
> > Replace plain integer approach by using IDA framework.
> 
> Also the general idea of using IDA for the instance numbering is a good one IMO.

...

> > -       unsigned int instance_no;
> > +       struct ida no;
> 
> struct ida instance_ida; ?

Will rename.

...

> > +       p = strrchr(dev_name(&device->dev), ':');
> > +       if (!p)
> > +               return -ENODATA;
> > +
> > +       error = kstrtoint(p + 1, 16, &result);
> > +       if (error)
> > +               return error;
> > +
> > +       return result;
> 
> I don't like the above at all.
> 
> I would just store the instance number in struct acpi_device_pnp (say).

TBH, I simply didn't know which struct to touch and left this one and I also
don't like it. Lemme see if acpi_device_pnp is good enough for that.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Mar 19, 2021 at 08:06:18PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 19, 2021 at 06:00:38PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Mar 12, 2021 at 5:02 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:

> > This is a slightly convoluted way of stating that there is a bug in
> > acpi_device_del().
> 
> Any suggestion how to massage the above?
> But in the dry end, yes, decrementing is a bug.

Okay, v2 has been sent. I tried to simplify the message.


-- 
With Best Regards,
Andy Shevchenko

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

[Rafael J. Wysocki]

On Fri, Mar 19, 2021 at 7:06 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Mar 19, 2021 at 06:00:38PM +0100, Rafael J. Wysocki wrote:
> > On Fri, Mar 12, 2021 at 5:02 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 fragile.
> > >
> > > In case of hot plug event or namespace removal of the device instances
> > > with the low numbers the plain integer counter can't cover the gaps
> > > and become desynchronized with real state of affairs. If during next
> > > hot plug event or namespace injection the new instances of
> > > the devices need to be instantiated, the counter may mistakenly point
> > > to the existing instance_no and kernel will complain:
> > > "sysfs: cannot create duplicate filename '/bus/acpi/devices/XXXX1234:02'"
> >
> > This is a slightly convoluted way of stating that there is a bug in
> > acpi_device_del().
>
> Any suggestion how to massage the above?

Why don't you simply say something like "The decrementation of
acpi_device_bus_id->instance_no in acpi_device_del() is incorrect,
because it may cause a duplicate instance number to be allocated next
time a device with the same acpi_device_bus_id is added."