On Sat, Jan 16, 2021 at 1:37 PM Hans de Goede [off-list ref] wrote:

Hi,

On 1/14/21 7:46 PM, Rafael J. Wysocki wrote:

quoted

From: Rafael J. Wysocki <redacted>

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

Thanks!