drivers/acpi/glue.c | 59 +++++++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 34 deletions(-)
From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
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.
Fixes: 7b1998116bbb ("ACPI / driver core: Store an ACPI device pointer in struct acpi_dev_node")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/acpi/glue.c | 59 +++++++++++++++++++--------------------------
1 file changed, 25 insertions(+), 34 deletions(-)
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 40e6513a9942..89336a5fa78b 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -243,31 +243,26 @@ static void acpi_physnode_link_name(char *buf, unsigned int node_id)
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;
}
- mutex_lock(&acpi_dev->physical_node_lock);
+ guard(mutex)(&acpi_dev->physical_node_lock);
/*
* Keep the list sorted by node_id so that the IDs of removed nodes can
@@ -278,15 +273,12 @@ int acpi_bind_one(struct device *dev, struct acpi_device *acpi_dev)
list_for_each_entry(pn, &acpi_dev->physical_node_list, node) {
/* Sanity check. */
if (pn->dev == dev) {
- mutex_unlock(&acpi_dev->physical_node_lock);
-
- dev_warn(dev, "Already associated with ACPI node\n");
- kfree(physical_node);
- if (ACPI_COMPANION(dev) != acpi_dev)
- goto err;
-
- put_device(dev);
- acpi_dev_put(acpi_dev);
+ if (!comp_dev) {
+ /* Really unexpected. */
+ ACPI_COMPANION_SET(dev, acpi_dev);
+ dev_warn(&acpi_dev->dev,
+ "Physical device list corruption fixed up\n");
+ }
return 0;
}
if (pn->node_id == node_id) {
@@ -295,12 +287,19 @@ int acpi_bind_one(struct device *dev, struct acpi_device *acpi_dev)
}
}
+ physical_node = kzalloc_obj(*physical_node);
+ if (!physical_node)
+ return -ENOMEM;
+
+ acpi_dev_get(acpi_dev);
+ get_device(dev);
+
physical_node->node_id = node_id;
physical_node->dev = dev;
list_add(&physical_node->node, physnode_list);
acpi_dev->physical_node_count++;
- if (!has_acpi_companion(dev))
+ if (!comp_dev)
ACPI_COMPANION_SET(dev, acpi_dev);
acpi_physnode_link_name(physical_node_name, node_id);
@@ -316,18 +315,10 @@ int acpi_bind_one(struct device *dev, struct acpi_device *acpi_dev)
dev_err(dev, "Failed to create link firmware_node (%d)\n",
retval);
- mutex_unlock(&acpi_dev->physical_node_lock);
-
if (acpi_dev->wakeup.flags.valid)
device_set_wakeup_capable(dev, true);
return 0;
-
- err:
- ACPI_COMPANION_SET(dev, NULL);
- put_device(dev);
- acpi_dev_put(acpi_dev);
- return retval;
}
EXPORT_SYMBOL_GPL(acpi_bind_one);
--
2.51.0On 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.
--
With Best Regards,
Andy Shevchenko
On Fri, Sep 11, 2026 at 9:40 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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!
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@linux.intel.com> 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
© 2016 - 2026 Red Hat, Inc.