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;
MIME-Version: 1.0
References: <3494203.VBaj39JGmp@kreacher> <2999734.9HhbEeWEHR@kreacher> <8218eff4-6629-ac20-ec3f-a66aad445bb6@redhat.com>
In-Reply-To: <8218eff4-6629-ac20-ec3f-a66aad445bb6@redhat.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Mon, 18 Jan 2021 16:16:16 +0100
Message-ID: <CAJZ5v0hv2FX2wtuwu9Jd1zZiGut9kUzQvCH5vXLMyFpqvvYOkA@mail.gmail.com>
Subject: Re: [PATCH v1 1/2] ACPI: scan: Rearrange memory allocation in acpi_device_add()
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux ACPI <linux-acpi@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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!