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

On Mon, Aug 17, 2026 at 10:53:39AM +0000, [email protected] wrote:
> 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);

beside check other sashiko report.

need use i3c_master_attach_i3c_dev_controller_locked() if require caller
hold lock

Frank

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