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?
