Re: [PATCH v1 1/2] ACPI: scan: Rearrange memory allocation in acpi_device_add()

[Hans de Goede <hdegoede@redhat.com>]

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!
>