[PATCH v1 0/2] ACPI: scan: Janitorial changes in acpi_device_add()

[PATCH v1 0/2] ACPI: scan: Janitorial changes in acpi_device_add()

[Rafael J. Wysocki]

Hi,

As per the subject on top of linux-next from today.

Please refer to the patch changelogs for details.

Thanks!

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

[Rafael J. Wysocki]

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;
 		}
 
-		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;
 }

[PATCH v1 2/2] ACPI: scan: Adjust white space in acpi_device_add()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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 <rafael.j.wysocki@intel.com>
---
 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;
 }
 

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

[Hans de Goede]

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;
>  }
> 
> 
> 

Re: [PATCH v1 2/2] ACPI: scan: Adjust white space in acpi_device_add()

[Hans de Goede]

Hi,

On 1/14/21 7:47 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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 <rafael.j.wysocki@intel.com>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

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;
>  }
>  
> 
> 
> 

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

[Rafael J. Wysocki]

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!

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

[Hans de Goede]

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

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

[Andy Shevchenko]

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

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

[Hans de Goede]

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