Re: [PATCH v1 2/4] ACPI: glue: Rearrange acpi_bind_one() to avoid breakage

[Rafael J. Wysocki (Intel)]

On Fri, Sep 11, 2026 at 9:40 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Sep 10, 2026 at 07:56:10PM +0200, Rafael J. Wysocki wrote:
>
> > Rearange the code in acpi_bind_one() to avoid situations in which the
> > existing ACPI companion of the given device would be replaced with NULL
> > due to a memory allocation error or because somebody tries to bind a
> > physical device with an ACPI companion to a different ACPI device
> > erroneously.
>
> ...
>
> >  int acpi_bind_one(struct device *dev, struct acpi_device *acpi_dev)
> >  {
> >       struct acpi_device_physical_node *physical_node, *pn;
> > +     struct acpi_device *comp_dev = ACPI_COMPANION(dev);
> >       char physical_node_name[PHYSICAL_NODE_NAME_SIZE];
> >       struct list_head *physnode_list;
> >       unsigned int node_id;
> >       int retval = -EINVAL;
> >
> > -     if (has_acpi_companion(dev)) {
> > -             if (acpi_dev) {
> > -                     dev_warn(dev, "ACPI companion already set\n");
> > +     if (!acpi_dev) {
> > +             if (!comp_dev)
> >                       return -EINVAL;
> > -             } else {
> > -                     acpi_dev = ACPI_COMPANION(dev);
> > -             }
> > -     }
> > -     if (!acpi_dev)
> > -             return -EINVAL;
> >
> > -     acpi_dev_get(acpi_dev);
> > -     get_device(dev);
> > -     physical_node = kzalloc_obj(*physical_node);
> > -     if (!physical_node) {
> > -             retval = -ENOMEM;
> > -             goto err;
> > +             /* If the companion has been set upfront, pick it up. */
> > +             acpi_dev = comp_dev;
> > +     }
> > +     if (comp_dev && comp_dev != acpi_dev) {
> > +             dev_warn(dev, "ACPI companion already set to %s which is not %s\n",
> > +                      acpi_dev_name(comp_dev), acpi_dev_name(acpi_dev));
> > +             return -EEXIST;
> >       }
>
> I would rewrite the above to look as following (if I got the logic right)
>
> int acpi_bind_one(struct device *dev, struct acpi_device *acpi_dev)
> {
>         struct acpi_device_physical_node *physical_node, *pn;
>         char physical_node_name[PHYSICAL_NODE_NAME_SIZE];
>         struct list_head *physnode_list;
>         struct acpi_device *comp_dev;
>         unsigned int node_id;
>         int retval = -EINVAL;
>
>         comp_dev = ACPI_COMPANION(dev);
>         if (comp_dev) {
>                 if (acpi_dev) {
>                         if (comp_dev != acpi_dev) {
>                                 dev_warn(dev, "ACPI companion already set to %s which is not %s\n",
>                                          acpi_dev_name(comp_dev), acpi_dev_name(acpi_dev));
>                                 return -EEXIST;
>                 } else {
>                         /* If the companion has been set upfront, pick it up. */
>                         acpi_dev = comp_dev;
>                 }
>         } else if (!acpi_dev) {
>                 return -EINVAL;
>         }
>
> The rationale is to avoid assignment and known-to-be-false test later on. Also
> split assignment that is going to be validated and moved it closer to the user
> (this helps with maintenance in a long term). Unfortunately double test of comp_dev
> against NULL just replaced with a double check of acpi_dev against NULL, no gain
> here.
>
> TL;DR: original and proposed pieces have their pros and cons.

There are reasons why I prefer the original changes.

There's fewer indentation levels and fewer lines of code there and
!acpi_dev is actually the most common case because acpi_bind_one() is
called with acpi_dev == NULL for every device, so it is better to deal
with it upfront.

However, it can be observed that the condition in the second "if ()"
(in the original $subject patch) is obviously false if acpi_dev is
NULL, so it can be moved to an "else" branch in the first "if ()":

if (!acpi_dev) {
    if (!comp_dev)
        return -EINVAL;

    /* If the companion has been set upfront, pick it up. */
    acpi_dev = comp_dev;
} else if (acpi_dev != comp_dev && comp_dev) {
    dev_warn(dev, "ACPI companion already set to %s which is not %s\n",
             acpi_dev_name(comp_dev), acpi_dev_name(acpi_dev));
    return -EEXIST;
}

and that avoids a redundant check when acpi_dev is NULL to start with
(and is one code line less even).

I'll make this change, but I'd rather not send a whole v2 of the
series for this, so I'll do it when applying the patch.

Thanks!