From: Hans de Goede <hdegoede@redhat.com>
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>
Subject: Re: [PATCH v1 1/2] ACPI: scan: Rearrange memory allocation in acpi_device_add()
Date: Sat, 16 Jan 2021 13:35:57 +0100 [thread overview]
Message-ID: <8218eff4-6629-ac20-ec3f-a66aad445bb6@redhat.com> (raw)
In-Reply-To: <2999734.9HhbEeWEHR@kreacher>
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;
> }
>
>
>
next prev parent reply other threads:[~2021-01-16 12:37 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-14 18:45 [PATCH v1 0/2] ACPI: scan: Janitorial changes in acpi_device_add() Rafael J. Wysocki
2021-01-14 18:46 ` [PATCH v1 1/2] ACPI: scan: Rearrange memory allocation " Rafael J. Wysocki
2021-01-16 12:35 ` Hans de Goede [this message]
2021-01-18 15:16 ` Rafael J. Wysocki
2021-01-18 15:26 ` Hans de Goede
2021-01-18 15:32 ` Andy Shevchenko
2021-01-18 15:34 ` Hans de Goede
2021-01-14 18:47 ` [PATCH v1 2/2] ACPI: scan: Adjust white space " Rafael J. Wysocki
2021-01-16 12:36 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8218eff4-6629-ac20-ec3f-a66aad445bb6@redhat.com \
--to=hdegoede@redhat.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rjw@rjwysocki.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).