| Message ID | 20210319192109.40041-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/1] ACPI: scan: Use unique number for instance_no | expand |
On Fri, Mar 19, 2021 at 8:21 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > 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. > > Replace above mentioned 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> > --- > v3: rewrote commit message once again as proposed by Rafael in v1 > 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); This is ida_alloc_range(ida, start, (end) - 1, gfp), so I think it should be 256 above, instead of 255. While at it, though, there can be more than 256 CPU devices easily on contemporary systems, so I would use a greater number here. Maybe 4096 and define a symbol for it?
On Mon, Mar 22, 2021 at 4:57 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Mar 19, 2021 at 8:21 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > 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. > > > > Replace above mentioned approach by using IDA framework. ... > > + result = ida_simple_get(&acpi_device_bus_id->instance_ida, 0, 255, GFP_KERNEL); > > This is ida_alloc_range(ida, start, (end) - 1, gfp), so I think it > should be 256 above, instead of 255. Ah, good catch! > While at it, though, there can be more than 256 CPU devices easily on > contemporary systems, so I would use a greater number here. Maybe > 4096 and define a symbol for it? I was thinking about it, but there is a problem with the device name, since it will break a lot of code, And taking into account that currently we don't change the behaviour it is good enough per se as a fix. That said, we may extend by an additional patch with a logic like this: res = ida_get(4096) if (res < 0) return res; if (res >= 256) use %04x else use %02x Would it make sense to you? -- With Best Regards, Andy Shevchenko
On Mon, Mar 22, 2021 at 4:02 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Mar 22, 2021 at 4:57 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Fri, Mar 19, 2021 at 8:21 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > 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. > > > > > > Replace above mentioned approach by using IDA framework. > > ... > > > > + result = ida_simple_get(&acpi_device_bus_id->instance_ida, 0, 255, GFP_KERNEL); > > > > This is ida_alloc_range(ida, start, (end) - 1, gfp), so I think it > > should be 256 above, instead of 255. > > Ah, good catch! > > > > While at it, though, there can be more than 256 CPU devices easily on > > contemporary systems, so I would use a greater number here. Maybe > > 4096 and define a symbol for it? > > I was thinking about it, but there is a problem with the device name, > since it will break a lot of code, What problem is there? > And taking into account that currently we don't change the behaviour > it is good enough per se as a fix. > > That said, we may extend by an additional patch with a logic like this: > > res = ida_get(4096) > if (res < 0) > return res; > if (res >= 256) > use %04x > else > use %02x > > Would it make sense to you? I'm not sure why not to always use %02x ? It doesn't truncate numbers longer than 2 digits AFAICS.
On Mon, Mar 22, 2021 at 5:42 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > On Mon, Mar 22, 2021 at 4:02 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Mar 22, 2021 at 4:57 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > On Fri, Mar 19, 2021 at 8:21 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > 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. > > > > > > > > Replace above mentioned approach by using IDA framework. > > > > ... > > > > > > + result = ida_simple_get(&acpi_device_bus_id->instance_ida, 0, 255, GFP_KERNEL); > > > > > > This is ida_alloc_range(ida, start, (end) - 1, gfp), so I think it > > > should be 256 above, instead of 255. > > > > Ah, good catch! > > > > > > > While at it, though, there can be more than 256 CPU devices easily on > > > contemporary systems, so I would use a greater number here. Maybe > > > 4096 and define a symbol for it? > > > > I was thinking about it, but there is a problem with the device name, > > since it will break a lot of code, > > What problem is there? If we have only 2 digits, but you are right, we have _at least_ two digits. > > And taking into account that currently we don't change the behaviour > > it is good enough per se as a fix. > > > > That said, we may extend by an additional patch with a logic like this: > > > > res = ida_get(4096) > > if (res < 0) > > return res; > > if (res >= 256) > > use %04x > > else > > use %02x > > > > Would it make sense to you? > > I'm not sure why not to always use %02x ? It doesn't truncate numbers > longer than 2 digits AFAICS. Yeah, should work. Thanks for review, I'll send a new version soon. -- With Best Regards, Andy Shevchenko
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 */
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. Replace above mentioned 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> --- v3: rewrote commit message once again as proposed by Rafael in v1 drivers/acpi/internal.h | 4 +++- drivers/acpi/scan.c | 32 +++++++++++++++++++++++++++----- include/acpi/acpi_bus.h | 1 + 3 files changed, 31 insertions(+), 6 deletions(-)