Hi,
As per the subject on top of linux-next from today.
Please refer to the patch changelogs for details.
Thanks!
From: Rafael J. Wysocki <[email protected]>
Add empty lines in some places in acpi_device_add() to help
readability and drop leading spaces before the labels in there.
No functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/acpi/scan.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -692,10 +692,12 @@ int acpi_device_add(struct acpi_device *
if (device->wakeup.flags.valid)
list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
+
mutex_unlock(&acpi_device_lock);
if (device->parent)
device->dev.parent = &device->parent->dev;
+
device->dev.bus = &acpi_bus_type;
device->dev.release = release;
result = device_add(&device->dev);
@@ -711,16 +713,19 @@ int acpi_device_add(struct acpi_device *
return 0;
- err:
+err:
mutex_lock(&acpi_device_lock);
+
if (device->parent)
list_del(&device->node);
+
list_del(&device->wakeup_list);
- err_unlock:
+err_unlock:
mutex_unlock(&acpi_device_lock);
acpi_detach_data(device->handle, acpi_scan_drop_device);
+
return result;
}
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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;
}
- 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;
}
Hi,
On 1/14/21 7:46 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
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;
> }
>
>
>
Hi,
On 1/14/21 7:47 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Add empty lines in some places in acpi_device_add() to help
> readability and drop leading spaces before the labels in there.
>
> No functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Patch looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
> ---
> drivers/acpi/scan.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -692,10 +692,12 @@ int acpi_device_add(struct acpi_device *
>
> if (device->wakeup.flags.valid)
> list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
> +
> mutex_unlock(&acpi_device_lock);
>
> if (device->parent)
> device->dev.parent = &device->parent->dev;
> +
> device->dev.bus = &acpi_bus_type;
> device->dev.release = release;
> result = device_add(&device->dev);
> @@ -711,16 +713,19 @@ int acpi_device_add(struct acpi_device *
>
> return 0;
>
> - err:
> +err:
> mutex_lock(&acpi_device_lock);
> +
> if (device->parent)
> list_del(&device->node);
> +
> list_del(&device->wakeup_list);
>
> - err_unlock:
> +err_unlock:
> mutex_unlock(&acpi_device_lock);
>
> acpi_detach_data(device->handle, acpi_scan_drop_device);
> +
> return result;
> }
>
>
>
>
Hi,
On 1/18/21 4:16 PM, Rafael J. Wysocki wrote:
> On Sat, Jan 16, 2021 at 1:37 PM Hans de Goede <[email protected]> wrote:
>>
>> Hi,
>>
>> On 1/14/21 7:46 PM, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]> 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.
>
On Sat, Jan 16, 2021 at 1:37 PM Hans de Goede <[email protected]> wrote:
>
> Hi,
>
> On 1/14/21 7:46 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks!