Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v1 1/2] ACPI: scan: Rearrange memory allocation in
 acpi_device_add()
To:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux ACPI <linux-acpi@vger.kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
References: <3494203.VBaj39JGmp@kreacher> <2999734.9HhbEeWEHR@kreacher>
From:   Hans de Goede <hdegoede@redhat.com>
Message-ID: <8218eff4-6629-ac20-ec3f-a66aad445bb6@redhat.com>
Date:   Sat, 16 Jan 2021 13:35:57 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: <2999734.9HhbEeWEHR@kreacher>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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