Re: [PATCH v15 6/8] i3c: hub: Add support for the I3C interface in the I3C hub

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Delegating DAA to the parent controller incorrectly instantiates downstream devices on the parent bus instead of the virtual bus.
- [High] Address reservation incorrectly skips assigned addresses that differ from the static address, risking dynamic address collisions.
- [High] Freeing hub device data while IBI is active leaves dangling pointers in the parent controller.
- [Medium] Non-atomic overwrite of `parent_desc.info` causes data race with asynchronous IBI handlers.
- [Medium] Nested acquisition of identical lock class triggers lockdep recursive locking warning.
--

--- Patch [6]: [PATCH v15 6/8] i3c: hub: Add support for the I3C interface in the I3C hub ---
commit b929a913a5f82c8ea68936fda262a420d33a7ea4
Author: Lakshay Piplani <[email protected]>

i3c: hub: Add support for the I3C interface in the I3C hub

Add virtual I3C bus support for the hub and provide interface to enable
or disable downstream ports.

> diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> new file mode 100644
> index 0000000000000..04726393004b7
> --- /dev/null
> +++ b/drivers/i3c/hub.c
> @@ -0,0 +1,675 @@

[ ... ]

> +static int i3c_hub_attach_i3c_dev(struct i3c_dev_desc *dev)
> +{
[ ... ]
> +	data->parent_desc.common.master = parent;
> +	data->parent_desc.info = dev->info;
> +
> +	i3c_bus_maintenance_lock(&parent->bus);

[Severity: Medium]
Will this trigger a lockdep warning for recursive locking?

The core I3C subsystem already holds the virtual controller's bus lock when
invoking operations like attach_i3c_dev or enable_ibi. Since all I3C bus
locks are initialized with the same lockdep class, acquiring the parent's
bus lock here looks like it might require subclass nesting annotations.

> +	ret = i3c_master_attach_i3c_dev_controller(&data->parent_desc);
> +	i3c_bus_maintenance_unlock(&parent->bus);
> +	if (ret) {

[ ... ]

> +static int i3c_hub_reattach_i3c_dev(struct i3c_dev_desc *dev,
> +				    u8 old_dyn_addr)
> +{
[ ... ]
> +	/* Re-sync device information after the address change. */
> +	data->parent_desc.info = dev->info;

[Severity: Medium]
Is it safe to perform a non-atomic update of this structure without holding
the parent controller's bus lock or the hub routing lock?

If the device has an active IBI, could the physical parent controller's
asynchronous IBI interrupt handler concurrently read this descriptor and
see corrupted or torn state?

> +
> +	i3c_bus_maintenance_lock(&parent->bus);
> +	ret = i3c_master_reattach_i3c_dev_controller(&data->parent_desc,

[ ... ]

> +static void i3c_hub_detach_i3c_dev(struct i3c_dev_desc *dev)
> +{
[ ... ]
> +	/*
> +	 * The generic IBI lifecycle must be released before detaching the
> +	 * physical controller state.
> +	 */
> +	WARN_ON(data->parent_desc.ibi);
> +
> +	if (parent) {
> +		i3c_bus_maintenance_lock(&parent->bus);
> +		i3c_master_detach_i3c_dev_controller(&data->parent_desc);
> +		i3c_bus_maintenance_unlock(&parent->bus);
> +	}
> +
> +	i3c_dev_set_master_data(dev, NULL);
> +	mutex_destroy(&data->parent_desc.ibi_lock);
> +	kfree(data);

[Severity: High]
If data->parent_desc.ibi is non-NULL (triggering the warning), does
proceeding to kfree(data) create a use-after-free?

Because i3c_master_detach_i3c_dev_controller() does not automatically flush
or free the generic IBI workqueue resources, lingering asynchronous IBI work
items queued on the parent controller might dereference data->parent_desc
after it is freed.

Should the active IBI resources be explicitly flushed and released before
freeing this memory?

> +}

[ ... ]

> +static int i3c_hub_do_daa(struct i3c_hub *hub,
> +			  struct i3c_master_controller *controller)
> +{
[ ... ]
> +	/*
> +	 * Downstream devices reachable through hub target-port routes share the
> +	 * parent controller's I3C address space. The hub gates access to a
> +	 * target-port network, but it does not create an independent dynamic
> +	 * address domain per virtual bus.
> +	 *
> +	 * Run DAA on the parent controller so dynamic addresses remain unique
> +	 * across all downstream devices, even when they are behind different
> +	 * target ports.
> +	 */
> +	ret = i3c_master_do_daa(parent);

[Severity: High]
Does calling i3c_master_do_daa() on the parent controller cause newly
discovered downstream devices to be instantiated on the parent bus rather
than the virtual hub bus?

If they are bound to the parent controller, won't subsequent client transfers
go directly to parent->ops->i3c_xfers, entirely bypassing
i3c_hub_master_priv_xfers() and hitting the closed hub port?

> +	i3c_hub_disable_port(controller);
> +	mutex_unlock(&hub->lock);
> +
> +	return ret;
> +}

[ ... ]

> +int i3c_hub_reserve_parent_addrslots_from_dt(struct i3c_hub_controller *hubc,
> +					     struct device_node *node)
> +{
[ ... ]
> +		ret = of_property_read_u32(child, "assigned-address", &assigned_addr);
> +		if (ret)
> +			continue;
> +
> +		static_addr = reg[0];
> +
> +		if (!static_addr || !assigned_addr)
> +			continue;
> +
> +		if (static_addr != assigned_addr)
> +			continue;

[Severity: High]
Why do we skip reserving the assigned address if it differs from the static
address?

If static_addr != assigned_addr (for example, expecting assignment via
SETDASA or SETNEWDA), this leaves the assigned address marked as free on
the parent bus. Could this allow the parent controller to accidentally
allocate this assigned address to another device during ENTDAA, causing a
collision?

> +
> +		i3c_bus_maintenance_lock(&parent->bus);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c