| Message ID | 2999734.9HhbEeWEHR@kreacher |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: scan: Janitorial changes in acpi_device_add() | expand |
Hi, On 1/14/21 7:46 PM, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > The upfront allocation of new_bus_id is done to avoid allocating > memory under acpi_device_lock, but it doesn't really help, > because (1) it leads to many unnecessary memory allocations for > _ADR devices, (2) kstrdup_const() is run under that lock anyway and > (3) it complicates the code. > > Rearrange acpi_device_add() to allocate memory for a new struct > acpi_device_bus_id instance only when necessary, eliminate a redundant > local variable from it and reduce the number of labels in there. > > No intentional functional impact. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/scan.c | 57 +++++++++++++++++++++++----------------------------- > 1 file changed, 26 insertions(+), 31 deletions(-) > > Index: linux-pm/drivers/acpi/scan.c > =================================================================== > --- linux-pm.orig/drivers/acpi/scan.c > +++ linux-pm/drivers/acpi/scan.c > @@ -621,12 +621,23 @@ void acpi_bus_put_acpi_device(struct acp > put_device(&adev->dev); > } > > +static struct acpi_device_bus_id *acpi_device_bus_id_match(const char *dev_id) > +{ > + struct acpi_device_bus_id *acpi_device_bus_id; > + > + /* Find suitable bus_id and instance number in acpi_bus_id_list. */ > + list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) { > + if (!strcmp(acpi_device_bus_id->bus_id, dev_id)) > + return acpi_device_bus_id; > + } > + return NULL; > +} > + > int acpi_device_add(struct acpi_device *device, > void (*release)(struct device *)) > { > + struct acpi_device_bus_id *acpi_device_bus_id; > int result; > - struct acpi_device_bus_id *acpi_device_bus_id, *new_bus_id; > - int found = 0; > > if (device->handle) { > acpi_status status; > @@ -652,38 +663,26 @@ int acpi_device_add(struct acpi_device * > INIT_LIST_HEAD(&device->del_list); > mutex_init(&device->physical_node_lock); > > - new_bus_id = kzalloc(sizeof(struct acpi_device_bus_id), GFP_KERNEL); > - if (!new_bus_id) { > - pr_err(PREFIX "Memory allocation error\n"); > - result = -ENOMEM; > - goto err_detach; > - } > - > mutex_lock(&acpi_device_lock); > - /* > - * Find suitable bus_id and instance number in acpi_bus_id_list > - * If failed, create one and link it into acpi_bus_id_list > - */ > - 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))) { > - acpi_device_bus_id->instance_no++; > - found = 1; > - kfree(new_bus_id); > - break; > + > + acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device)); > + if (acpi_device_bus_id) { > + acpi_device_bus_id->instance_no++; > + } else { > + acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), > + GFP_KERNEL); > + if (!acpi_device_bus_id) { > + result = -ENOMEM; > + goto err_unlock; > } > - } > - if (!found) { > - acpi_device_bus_id = new_bus_id; > acpi_device_bus_id->bus_id = > kstrdup_const(acpi_device_hid(device), GFP_KERNEL); > if (!acpi_device_bus_id->bus_id) { > - pr_err(PREFIX "Memory allocation error for bus id\n"); > + kfree(acpi_device_bus_id); > result = -ENOMEM; > - goto err_free_new_bus_id; > + goto err_unlock; > } When I have cases like this, where 2 mallocs are necessary I typically do it like this: const char *bus_id; ... } else { acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), GFP_KERNEL); bus_id = kstrdup_const(acpi_device_hid(device), GFP_KERNEL); if (!acpi_device_bus_id || !bus_id) { kfree(acpi_device_bus_id); kfree(bus_id); result = -ENOMEM; goto err_unlock; } acpi_device_bus_id->bus_id = bus_id; list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list); } ... So that there is only one if / 1 error-handling path for both mallocs. I personally find this a bit cleaner. Either way, with or without this change, the patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > > - acpi_device_bus_id->instance_no = 0; > 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); > @@ -718,13 +717,9 @@ int acpi_device_add(struct acpi_device * > list_del(&device->node); > list_del(&device->wakeup_list); > > - err_free_new_bus_id: > - if (!found) > - kfree(new_bus_id); > - > + err_unlock: > mutex_unlock(&acpi_device_lock); > > - err_detach: > acpi_detach_data(device->handle, acpi_scan_drop_device); > return result; > } > > >
On Sat, Jan 16, 2021 at 1:37 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 1/14/21 7:46 PM, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > The upfront allocation of new_bus_id is done to avoid allocating > > memory under acpi_device_lock, but it doesn't really help, > > because (1) it leads to many unnecessary memory allocations for > > _ADR devices, (2) kstrdup_const() is run under that lock anyway and > > (3) it complicates the code. > > > > Rearrange acpi_device_add() to allocate memory for a new struct > > acpi_device_bus_id instance only when necessary, eliminate a redundant > > local variable from it and reduce the number of labels in there. > > > > No intentional functional impact. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > --- > > drivers/acpi/scan.c | 57 +++++++++++++++++++++++----------------------------- > > 1 file changed, 26 insertions(+), 31 deletions(-) > > > > Index: linux-pm/drivers/acpi/scan.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/scan.c > > +++ linux-pm/drivers/acpi/scan.c > > @@ -621,12 +621,23 @@ void acpi_bus_put_acpi_device(struct acp > > put_device(&adev->dev); > > } > > > > +static struct acpi_device_bus_id *acpi_device_bus_id_match(const char *dev_id) > > +{ > > + struct acpi_device_bus_id *acpi_device_bus_id; > > + > > + /* Find suitable bus_id and instance number in acpi_bus_id_list. */ > > + list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) { > > + if (!strcmp(acpi_device_bus_id->bus_id, dev_id)) > > + return acpi_device_bus_id; > > + } > > + return NULL; > > +} > > + > > int acpi_device_add(struct acpi_device *device, > > void (*release)(struct device *)) > > { > > + struct acpi_device_bus_id *acpi_device_bus_id; > > int result; > > - struct acpi_device_bus_id *acpi_device_bus_id, *new_bus_id; > > - int found = 0; > > > > if (device->handle) { > > acpi_status status; > > @@ -652,38 +663,26 @@ int acpi_device_add(struct acpi_device * > > INIT_LIST_HEAD(&device->del_list); > > mutex_init(&device->physical_node_lock); > > > > - new_bus_id = kzalloc(sizeof(struct acpi_device_bus_id), GFP_KERNEL); > > - if (!new_bus_id) { > > - pr_err(PREFIX "Memory allocation error\n"); > > - result = -ENOMEM; > > - goto err_detach; > > - } > > - > > mutex_lock(&acpi_device_lock); > > - /* > > - * Find suitable bus_id and instance number in acpi_bus_id_list > > - * If failed, create one and link it into acpi_bus_id_list > > - */ > > - 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))) { > > - acpi_device_bus_id->instance_no++; > > - found = 1; > > - kfree(new_bus_id); > > - break; > > + > > + acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device)); > > + if (acpi_device_bus_id) { > > + acpi_device_bus_id->instance_no++; > > + } else { > > + acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), > > + GFP_KERNEL); > > + if (!acpi_device_bus_id) { > > + result = -ENOMEM; > > + goto err_unlock; > > } > > - } > > - if (!found) { > > - acpi_device_bus_id = new_bus_id; > > acpi_device_bus_id->bus_id = > > kstrdup_const(acpi_device_hid(device), GFP_KERNEL); > > if (!acpi_device_bus_id->bus_id) { > > - pr_err(PREFIX "Memory allocation error for bus id\n"); > > + kfree(acpi_device_bus_id); > > result = -ENOMEM; > > - goto err_free_new_bus_id; > > + goto err_unlock; > > } > > When I have cases like this, where 2 mallocs are necessary I typically do it like this: > > const char *bus_id; > > ... > > } else { > acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), > GFP_KERNEL); > bus_id = kstrdup_const(acpi_device_hid(device), GFP_KERNEL); > if (!acpi_device_bus_id || !bus_id) { > kfree(acpi_device_bus_id); > kfree(bus_id); > result = -ENOMEM; > goto err_unlock; > } > acpi_device_bus_id->bus_id = bus_id; > list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list); > } > > ... > > So that there is only one if / 1 error-handling path for both mallocs. > I personally find this a bit cleaner. Yes, that looks better. Let me do it this way, but I won't resend the patch if you don't mind. > Either way, with or without this change, the patch looks good to me: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> Thanks!
Hi, On 1/18/21 4:16 PM, Rafael J. Wysocki wrote: > On Sat, Jan 16, 2021 at 1:37 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 1/14/21 7:46 PM, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> >>> The upfront allocation of new_bus_id is done to avoid allocating >>> memory under acpi_device_lock, but it doesn't really help, >>> because (1) it leads to many unnecessary memory allocations for >>> _ADR devices, (2) kstrdup_const() is run under that lock anyway and >>> (3) it complicates the code. >>> >>> Rearrange acpi_device_add() to allocate memory for a new struct >>> acpi_device_bus_id instance only when necessary, eliminate a redundant >>> local variable from it and reduce the number of labels in there. >>> >>> No intentional functional impact. >>> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> --- >>> drivers/acpi/scan.c | 57 +++++++++++++++++++++++----------------------------- >>> 1 file changed, 26 insertions(+), 31 deletions(-) >>> >>> Index: linux-pm/drivers/acpi/scan.c >>> =================================================================== >>> --- linux-pm.orig/drivers/acpi/scan.c >>> +++ linux-pm/drivers/acpi/scan.c >>> @@ -621,12 +621,23 @@ void acpi_bus_put_acpi_device(struct acp >>> put_device(&adev->dev); >>> } >>> >>> +static struct acpi_device_bus_id *acpi_device_bus_id_match(const char *dev_id) >>> +{ >>> + struct acpi_device_bus_id *acpi_device_bus_id; >>> + >>> + /* Find suitable bus_id and instance number in acpi_bus_id_list. */ >>> + list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) { >>> + if (!strcmp(acpi_device_bus_id->bus_id, dev_id)) >>> + return acpi_device_bus_id; >>> + } >>> + return NULL; >>> +} >>> + >>> int acpi_device_add(struct acpi_device *device, >>> void (*release)(struct device *)) >>> { >>> + struct acpi_device_bus_id *acpi_device_bus_id; >>> int result; >>> - struct acpi_device_bus_id *acpi_device_bus_id, *new_bus_id; >>> - int found = 0; >>> >>> if (device->handle) { >>> acpi_status status; >>> @@ -652,38 +663,26 @@ int acpi_device_add(struct acpi_device * >>> INIT_LIST_HEAD(&device->del_list); >>> mutex_init(&device->physical_node_lock); >>> >>> - new_bus_id = kzalloc(sizeof(struct acpi_device_bus_id), GFP_KERNEL); >>> - if (!new_bus_id) { >>> - pr_err(PREFIX "Memory allocation error\n"); >>> - result = -ENOMEM; >>> - goto err_detach; >>> - } >>> - >>> mutex_lock(&acpi_device_lock); >>> - /* >>> - * Find suitable bus_id and instance number in acpi_bus_id_list >>> - * If failed, create one and link it into acpi_bus_id_list >>> - */ >>> - 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))) { >>> - acpi_device_bus_id->instance_no++; >>> - found = 1; >>> - kfree(new_bus_id); >>> - break; >>> + >>> + acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device)); >>> + if (acpi_device_bus_id) { >>> + acpi_device_bus_id->instance_no++; >>> + } else { >>> + acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), >>> + GFP_KERNEL); >>> + if (!acpi_device_bus_id) { >>> + result = -ENOMEM; >>> + goto err_unlock; >>> } >>> - } >>> - if (!found) { >>> - acpi_device_bus_id = new_bus_id; >>> acpi_device_bus_id->bus_id = >>> kstrdup_const(acpi_device_hid(device), GFP_KERNEL); >>> if (!acpi_device_bus_id->bus_id) { >>> - pr_err(PREFIX "Memory allocation error for bus id\n"); >>> + kfree(acpi_device_bus_id); >>> result = -ENOMEM; >>> - goto err_free_new_bus_id; >>> + goto err_unlock; >>> } >> >> When I have cases like this, where 2 mallocs are necessary I typically do it like this: >> >> const char *bus_id; >> >> ... >> >> } else { >> acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), >> GFP_KERNEL); >> bus_id = kstrdup_const(acpi_device_hid(device), GFP_KERNEL); >> if (!acpi_device_bus_id || !bus_id) { >> kfree(acpi_device_bus_id); >> kfree(bus_id); >> result = -ENOMEM; >> goto err_unlock; >> } >> acpi_device_bus_id->bus_id = bus_id; >> list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list); >> } >> >> ... >> >> So that there is only one if / 1 error-handling path for both mallocs. >> I personally find this a bit cleaner. > > Yes, that looks better. > > Let me do it this way, but I won't resend the patch if you don't mind. Not resending is fine. Regards, Hans > >> Either way, with or without this change, the patch looks good to me: >> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Thanks! >
On Mon, Jan 18, 2021 at 04:16:16PM +0100, Rafael J. Wysocki wrote: > On Sat, Jan 16, 2021 at 1:37 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 1/14/21 7:46 PM, Rafael J. Wysocki wrote: ... > > When I have cases like this, where 2 mallocs are necessary I typically do it like this: > > > > const char *bus_id; > > > > ... > > > > } else { > > acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), > > GFP_KERNEL); > > bus_id = kstrdup_const(acpi_device_hid(device), GFP_KERNEL); > > if (!acpi_device_bus_id || !bus_id) { > > kfree(acpi_device_bus_id); > > kfree(bus_id); Just to be sure, shouldn't it be kfree_const() ? > > result = -ENOMEM; > > goto err_unlock; > > } > > acpi_device_bus_id->bus_id = bus_id; > > list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list); > > } > > > > ... > > > > So that there is only one if / 1 error-handling path for both mallocs. > > I personally find this a bit cleaner. -- With Best Regards, Andy Shevchenko
Hi, On 1/18/21 4:32 PM, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 04:16:16PM +0100, Rafael J. Wysocki wrote: >> On Sat, Jan 16, 2021 at 1:37 PM Hans de Goede <hdegoede@redhat.com> wrote: >>> On 1/14/21 7:46 PM, Rafael J. Wysocki wrote: > > ... > >>> When I have cases like this, where 2 mallocs are necessary I typically do it like this: >>> >>> const char *bus_id; >>> >>> ... >>> >>> } else { >>> acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), >>> GFP_KERNEL); >>> bus_id = kstrdup_const(acpi_device_hid(device), GFP_KERNEL); >>> if (!acpi_device_bus_id || !bus_id) { >>> kfree(acpi_device_bus_id); > > >>> kfree(bus_id); > > Just to be sure, shouldn't it be kfree_const() ? Yes I beleive it should, my bad. Regards, Hans > >>> result = -ENOMEM; >>> goto err_unlock; >>> } >>> acpi_device_bus_id->bus_id = bus_id; >>> list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list); >>> } >>> >>> ... >>> >>> So that there is only one if / 1 error-handling path for both mallocs. >>> I personally find this a bit cleaner. >
Index: linux-pm/drivers/acpi/scan.c =================================================================== --- linux-pm.orig/drivers/acpi/scan.c +++ linux-pm/drivers/acpi/scan.c @@ -621,12 +621,23 @@ void acpi_bus_put_acpi_device(struct acp put_device(&adev->dev); } +static struct acpi_device_bus_id *acpi_device_bus_id_match(const char *dev_id) +{ + struct acpi_device_bus_id *acpi_device_bus_id; + + /* Find suitable bus_id and instance number in acpi_bus_id_list. */ + list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) { + if (!strcmp(acpi_device_bus_id->bus_id, dev_id)) + return acpi_device_bus_id; + } + return NULL; +} + int acpi_device_add(struct acpi_device *device, void (*release)(struct device *)) { + struct acpi_device_bus_id *acpi_device_bus_id; int result; - struct acpi_device_bus_id *acpi_device_bus_id, *new_bus_id; - int found = 0; if (device->handle) { acpi_status status; @@ -652,38 +663,26 @@ int acpi_device_add(struct acpi_device * INIT_LIST_HEAD(&device->del_list); mutex_init(&device->physical_node_lock); - new_bus_id = kzalloc(sizeof(struct acpi_device_bus_id), GFP_KERNEL); - if (!new_bus_id) { - pr_err(PREFIX "Memory allocation error\n"); - result = -ENOMEM; - goto err_detach; - } - mutex_lock(&acpi_device_lock); - /* - * Find suitable bus_id and instance number in acpi_bus_id_list - * If failed, create one and link it into acpi_bus_id_list - */ - 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))) { - acpi_device_bus_id->instance_no++; - found = 1; - kfree(new_bus_id); - break; + + acpi_device_bus_id = acpi_device_bus_id_match(acpi_device_hid(device)); + if (acpi_device_bus_id) { + acpi_device_bus_id->instance_no++; + } else { + acpi_device_bus_id = kzalloc(sizeof(*acpi_device_bus_id), + GFP_KERNEL); + if (!acpi_device_bus_id) { + result = -ENOMEM; + goto err_unlock; } - } - if (!found) { - acpi_device_bus_id = new_bus_id; acpi_device_bus_id->bus_id = kstrdup_const(acpi_device_hid(device), GFP_KERNEL); if (!acpi_device_bus_id->bus_id) { - pr_err(PREFIX "Memory allocation error for bus id\n"); + kfree(acpi_device_bus_id); result = -ENOMEM; - goto err_free_new_bus_id; + goto err_unlock; } - acpi_device_bus_id->instance_no = 0; 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); @@ -718,13 +717,9 @@ int acpi_device_add(struct acpi_device * list_del(&device->node); list_del(&device->wakeup_list); - err_free_new_bus_id: - if (!found) - kfree(new_bus_id); - + err_unlock: mutex_unlock(&acpi_device_lock); - err_detach: acpi_detach_data(device->handle, acpi_scan_drop_device); return result; }