[PATCH v1 0/4] ACPI: glue: Three fixes and optimization

[PATCH v1 0/4] ACPI: glue: Three fixes and optimization

[Rafael J. Wysocki]

Hi All,

This series fixes three issues in the ACPI code responsible for binding
physical devices to their ACPI companions and adds an optimization to
it.

Please see the changelogs of the patches for details.

Thanks!

[PATCH v1 1/4] ACPI: glue: Carry out companion lookup under bus_type_sem

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

When acpi_device_notify() looks up an ACPI companion for the given
device, it invokes callbacks from struct acpi_bus_type() without
synchronization which may lead to a use-after-free if the driver
module containing those callbacks is unloaded at the same time.

Address this by holding bus_type_sem throughout the entire ACPI
companion lookup and the execution of the .setup() callback in
struct acpi_bus_type (if present) instead of dropping the semaphore
(prematurely) after finding a matching struct acpi_bus_type.

For this purpose, rename acpi_get_bus_type() to acpi_companion_lookup(),
make it return a struct acpi_device pointer, and move the relevant code
from acpi_device_notify() to it.

Also notice that the only case in which the .bind() callback from an
ACPI scan handler may need to be invoked is when the given device is
a platform one, so adjust acpi_device_notify() accordingly and drop
the "done" label that is not used any more from it.

Fixes: 2ef5236660b6 ("ACPI: glue: Look for ACPI bus type only if ACPI companion is not known")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Applies on top of

https://lore.kernel.org/linux-acpi/12989369.O9o76ZdvQC@rafael.j.wysocki/

which is in linux-pm.git/linux-next now.

Thanks!

---
 drivers/acpi/glue.c | 61 +++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index a47cccc4efd3..40e6513a9942 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -59,19 +59,34 @@ int unregister_acpi_bus_type(struct acpi_bus_type *type)
 }
 EXPORT_SYMBOL_GPL(unregister_acpi_bus_type);
 
-static struct acpi_bus_type *acpi_get_bus_type(struct device *dev)
+static struct acpi_device *acpi_companion_lookup(struct device *dev)
 {
-	struct acpi_bus_type *tmp, *ret = NULL;
+	struct acpi_bus_type *type;
 
-	down_read(&bus_type_sem);
-	list_for_each_entry(tmp, &bus_type_list, list) {
-		if (tmp->match(dev)) {
-			ret = tmp;
-			break;
+	guard(rwsem_read)(&bus_type_sem);
+
+	list_for_each_entry(type, &bus_type_list, list) {
+		struct acpi_device *adev;
+
+		if (!type->match(dev))
+			continue;
+
+		adev = type->find_companion(dev);
+		if (!adev) {
+			dev_dbg(dev, "ACPI companion not found\n");
+			return NULL;
 		}
+		if (acpi_bind_one(dev, adev)) {
+			dev_dbg(dev, "Binding to ACPI companion failed\n");
+			return NULL;
+		}
+		if (type->setup)
+			type->setup(dev);
+
+		return adev;
 	}
-	up_read(&bus_type_sem);
-	return ret;
+
+	return NULL;
 }
 
 #define FIND_CHILD_MIN_SCORE	1
@@ -360,40 +375,22 @@ void acpi_device_notify(struct device *dev)
 
 	ret = acpi_bind_one(dev, NULL);
 	if (ret) {
-		struct acpi_bus_type *type = acpi_get_bus_type(dev);
-
-		if (!type)
+		adev = acpi_companion_lookup(dev);
+		if (!adev)
 			return;
-
-		adev = type->find_companion(dev);
-		if (!adev) {
-			dev_dbg(dev, "ACPI companion not found\n");
-			return;
-		}
-		ret = acpi_bind_one(dev, adev);
-		if (ret) {
-			dev_dbg(dev, "Binding to ACPI companion failed\n");
-			return;
-		}
-		if (type->setup) {
-			type->setup(dev);
-			goto done;
-		}
 	} else {
 		adev = ACPI_COMPANION(dev);
 
 		if (dev_is_pci(dev)) {
 			pci_acpi_setup(dev, adev);
-			goto done;
 		} else if (dev_is_platform(dev)) {
 			acpi_configure_pmsi_domain(dev);
+
+			if (adev->handler && adev->handler->bind)
+				adev->handler->bind(dev);
 		}
 	}
 
-	if (adev->handler && adev->handler->bind)
-		adev->handler->bind(dev);
-
-done:
 	dev_dbg(dev, "Bound to ACPI device %s\n", acpi_dev_name(adev));
 }
 
-- 
2.51.0

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

[Rafael J. Wysocki]

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

[PATCH v1 3/4] ACPI: glue: Fix up and adjust acpi_unbind_one()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Since none of the acpi_unbind_one() callers check its return value and
it always returns 0 anyway, make it void.

Also notice that unlocking physical_node_lock for the given ACPI device
should be carried out before dropping the reference to it in case that
reference is the last one (highly unlikely), so rearrange the code to
make that happen.

Fixes: 3e3327837c18 ("ACPI: Use list_for_each_entry() in acpi_unbind_one()")
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/glue.c     | 10 ++++++----
 include/acpi/acpi_bus.h |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 89336a5fa78b..1981ebfb5ce0 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -322,13 +322,13 @@ int acpi_bind_one(struct device *dev, struct acpi_device *acpi_dev)
 }
 EXPORT_SYMBOL_GPL(acpi_bind_one);
 
-int acpi_unbind_one(struct device *dev)
+void acpi_unbind_one(struct device *dev)
 {
 	struct acpi_device *acpi_dev = ACPI_COMPANION(dev);
 	struct acpi_device_physical_node *entry;
 
 	if (!acpi_dev)
-		return 0;
+		return;
 
 	mutex_lock(&acpi_dev->physical_node_lock);
 
@@ -343,15 +343,17 @@ int acpi_unbind_one(struct device *dev)
 			sysfs_remove_link(&acpi_dev->dev.kobj, physnode_name);
 			sysfs_remove_link(&dev->kobj, "firmware_node");
 			ACPI_COMPANION_SET(dev, NULL);
+
+			mutex_unlock(&acpi_dev->physical_node_lock);
+
 			/* Drop references taken by acpi_bind_one(). */
 			put_device(dev);
 			acpi_dev_put(acpi_dev);
 			kfree(entry);
-			break;
+			return;
 		}
 
 	mutex_unlock(&acpi_dev->physical_node_lock);
-	return 0;
 }
 EXPORT_SYMBOL_GPL(acpi_unbind_one);
 
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a10a591c18b2..93b00635ba3d 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -665,7 +665,7 @@ struct acpi_bus_type {
 int register_acpi_bus_type(struct acpi_bus_type *);
 int unregister_acpi_bus_type(struct acpi_bus_type *);
 int acpi_bind_one(struct device *dev, struct acpi_device *adev);
-int acpi_unbind_one(struct device *dev);
+void acpi_unbind_one(struct device *dev);
 
 enum acpi_bridge_type {
 	ACPI_BRIDGE_TYPE_PCIE = 1,
-- 
2.51.0

[PATCH v1 4/4] ACPI: glue: Skip devices with no type in acpi_device_notify()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Since all of the users of struct acpi_bus_type use the device type
for matching, devices without a type will not be matched by any of
them, so they can be skipped early in acpi_device_notify().

Update the code accordingly.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/glue.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 1981ebfb5ce0..c53f3d86254a 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -63,6 +63,9 @@ static struct acpi_device *acpi_companion_lookup(struct device *dev)
 {
 	struct acpi_bus_type *type;
 
+	if (!dev->type)
+		return NULL;
+
 	guard(rwsem_read)(&bus_type_sem);
 
 	list_for_each_entry(type, &bus_type_list, list) {
-- 
2.51.0

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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@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!

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