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

[Andy Shevchenko]

On Fri, Sep 11, 2026 at 12:24:16PM +0200, Rafael J. Wysocki (Intel) wrote:
> 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:

...

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

Thanks for clarification. Yes, your variant (as put below) sounds good enough
to me.

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

-- 
With Best Regards,
Andy Shevchenko