Re: [PATCH net-next v5 3/7] i2c: mux: Propagate software nodes to channel adapters

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
i2c: mux: Propagate software nodes to channel adapters

This attaches the matching software-node child of the mux device to each
i2c-mux channel adapter, tracks whether the mux owns that attachment for
cleanup, and extends the I2C firmware-node adapter lookup so it also
matches a device's secondary fwnode.

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 3ec04787a7373..f39575ee02444 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1876,12 +1876,22 @@ int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter)
>  }
>  EXPORT_SYMBOL_GPL(devm_i2c_add_adapter);
>  
> -static int i2c_dev_or_parent_fwnode_match(struct device *dev, const void *data)
> +static bool i2c_device_match_fwnode(struct device *dev, const void *data)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +
>  	if (device_match_fwnode(dev, data))
> +		return true;
> +
> +	return !IS_ERR_OR_NULL(fwnode) && fwnode->secondary == data;
> +}

[Severity: Medium]
Is the new fwnode->secondary dereference safe here?  The old code only used
device_match_fwnode(), which compares pointers and never dereferences the
fwnode.

i2c_dev_or_parent_fwnode_match() runs as a bus_find_device() match callback
from i2c_find_adapter_by_fwnode()/i2c_get_adapter_by_fwnode(), so it is
invoked for every device on i2c_bus_type, including clients, with no
device_lock held and nothing pinning dev->fwnode.

Meanwhile i2c_unregister_device() detaches and frees the software node
while the client is still in the bus klist:

drivers/i2c/i2c-core-base.c:i2c_unregister_device() {
	...
	device_remove_software_node(&client->dev);
	device_unregister(&client->dev);
}

For a client created with info->swnode and no primary fwnode,
dev->fwnode == &swnode->fwnode, and device_remove_software_node() drops the
last reference:

drivers/base/swnode.c:device_remove_software_node() {
	...
	set_secondary_fwnode(dev, NULL);
	swnode_put(swnode);
}

Can a lookup then observe fwnode = dev_fwnode(dev) before the free and read
fwnode->secondary after it?  The writers are unsynchronized too:

drivers/base/core.c:set_secondary_fwnode() {
	if (fwnode_is_primary(dev->fwnode))
		dev->fwnode->secondary = fwnode;
	else
		dev->fwnode = fwnode;
}

There is no lock common to the reader and the writer and no WRITE_ONCE, so
this also looks like a plain data race that could make the lookup return an
adapter whose software node has already been detached.

Also, does the extra dereference help in that case at all?  A software node
installed in the primary slot always has secondary == ERR_PTR(-ENODEV), so
it can never match through fwnode->secondary; only the window seems new.

> @@ -1891,8 +1901,9 @@ static int i2c_dev_or_parent_fwnode_match(struct device *dev, const void *data)

[ ... ]

> diff --git a/drivers/i2c/i2c-mux.c b/drivers/i2c/i2c-mux.c
> index 681a201c239ba..3bca123907b3d 100644
> --- a/drivers/i2c/i2c-mux.c
> +++ b/drivers/i2c/i2c-mux.c

[ ... ]

> @@ -364,6 +417,8 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>  
>  		priv->adap.dev.of_node = child;
>  		of_node_put(mux_node);
> +	} else {
> +		channel_node = i2c_mux_get_channel_swnode(muxc, chan_id);
>  	}
>  
>  	/*

[ ... ]

> @@ -408,6 +473,8 @@ int i2c_mux_add_adapter(struct i2c_mux_core *muxc,
>  	return 0;
>  
>  err_free_priv:
> +	if (priv->swnode)
> +		device_remove_software_node(&priv->adap.dev);
>  	kfree(priv);
>  	return ret;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but since err_free_priv is being
extended here, should it also drop the DT channel node reference?

In the DT branch of i2c_mux_add_adapter(), priv->adap.dev.of_node holds a
reference obtained either from the for_each_child_of_node() early break, or
from of_node_get(dev_node)/of_node_get(mux_node).  When
i2c_add_adapter()/i2c_add_numbered_adapter() fails, control reaches
err_free_priv, which only does kfree(priv).

That the mux owns that reference is visible in the success-path teardown,
which saves np = adap->dev.of_node and puts it after i2c_del_adapter():

> @@ -429,7 +496,13 @@ void i2c_mux_del_adapters(struct i2c_mux_core *muxc)
>  		sysfs_remove_link(&priv->adap.dev.kobj, "mux_device");
> +		/*
> +		 * Keep the software node through child removal. The adapter
> +		 * device is cleared on deletion, so release the software-node
> +		 * attachment reference via the saved handle afterwards.
> +		 */
>  		i2c_del_adapter(adap);
> +		fwnode_handle_put(priv->swnode);
>  		of_node_put(np);
>  		kfree(priv);
>  	}

Neither i2c_register_adapter()'s error labels nor the driver core drops a
device's of_node reference when device_add() fails, so does the failure path
leak that struct device_node reference?  Would adding
of_node_put(priv->adap.dev.of_node) at err_free_priv be appropriate while
this label is being touched?