From c1013ff7a5472db637c56bb6237f8343398c03a7 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Thu, 14 Jan 2021 19:46:47 +0100 Subject: [PATCH 1/3] ACPI: scan: Rearrange memory allocation in acpi_device_add() 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 Reviewed-by: Hans de Goede --- drivers/acpi/scan.c | 57 +++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 1db063b02f63..0fb1811772b5 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -623,12 +623,23 @@ void acpi_bus_put_acpi_device(struct acpi_device *adev) 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; @@ -654,38 +665,26 @@ int acpi_device_add(struct acpi_device *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); @@ -720,13 +719,9 @@ int acpi_device_add(struct acpi_device *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; } From 5e73c5187cf4f40a5e02b6c8e4dd0fcf9686c006 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Thu, 14 Jan 2021 19:47:37 +0100 Subject: [PATCH 2/3] ACPI: scan: Adjust white space in acpi_device_add() 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 Reviewed-by: Hans de Goede --- drivers/acpi/scan.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 0fb1811772b5..1510afa7094d 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -694,10 +694,12 @@ int acpi_device_add(struct acpi_device *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); @@ -713,16 +715,19 @@ int acpi_device_add(struct acpi_device *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 83e2c8fc7ab89458b805e96ab37bccadf84f932b Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Mon, 18 Jan 2021 20:25:37 +0100 Subject: [PATCH 3/3] ACPI: scan: Rearrange code related to acpi_get_device_data() There are two callers of acpi_get_device_data(), acpi_bus_get_device() and acpi_bus_get_acpi_device(), but only one of them takes the int return value into account. Moreover, the latter knows that it passes a valid return pointer to acpi_get_device_data() and it properly clears that pointer upfront, so it doesn't need acpi_get_device_data() to do that. For this reason, rearrange acpi_get_device_data() to return a strct acpi_device pointer instead of an int and adapt its callers to that. While at it, rename acpi_get_device_data() to handle_to_device(), because the old name does not really reflect the functionality provided by that function. No intentional functional impact. Signed-off-by: Rafael J. Wysocki Reviewed-by: Hans de Goede --- drivers/acpi/scan.c | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 1510afa7094d..bcbf0fc215c8 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -578,29 +578,31 @@ static void acpi_scan_drop_device(acpi_handle handle, void *context) mutex_unlock(&acpi_device_del_lock); } -static int acpi_get_device_data(acpi_handle handle, struct acpi_device **device, - void (*callback)(void *)) +static struct acpi_device *handle_to_device(acpi_handle handle, + void (*callback)(void *)) { + struct acpi_device *adev = NULL; acpi_status status; - if (!device) - return -EINVAL; - - *device = NULL; - status = acpi_get_data_full(handle, acpi_scan_drop_device, - (void **)device, callback); - if (ACPI_FAILURE(status) || !*device) { - ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No context for object [%p]\n", - handle)); - return -ENODEV; + (void **)&adev, callback); + if (ACPI_FAILURE(status) || !adev) { + acpi_handle_debug(handle, "No context!\n"); + return NULL; } - return 0; + return adev; } int acpi_bus_get_device(acpi_handle handle, struct acpi_device **device) { - return acpi_get_device_data(handle, device, NULL); + if (!device) + return -EINVAL; + + *device = handle_to_device(handle, NULL); + if (!*device) + return -ENODEV; + + return 0; } EXPORT_SYMBOL(acpi_bus_get_device); @@ -612,10 +614,7 @@ static void get_acpi_device(void *dev) struct acpi_device *acpi_bus_get_acpi_device(acpi_handle handle) { - struct acpi_device *adev = NULL; - - acpi_get_device_data(handle, &adev, get_acpi_device); - return adev; + return handle_to_device(handle, get_acpi_device); } void acpi_bus_put_acpi_device(struct acpi_device *adev)