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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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