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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Temporary hub address swapping in `i3c_hub_master_priv_xfers` permanently frees downstream address slots.
- [High] Detaching the downstream device in IBI request/free paths unconditionally frees its parent bus address slots.
- [High] Forwarding DAA directly to the parent controller attaches downstream devices to the wrong bus topology.
- [High] The no-op `recycle_ibi_slot` callback permanently leaks hardware IBI slots allocated by the parent controller.
- [High] `i3c_hub_request_ibi()` fails with `-EBUSY` due to conflicting address reservations or corrupts parent DAT.
- [High] Recursive lock acquisition of `hub->lock` causes deadlocks with IBI workqueue handlers.
- [High] Modifying the device descriptor's master pointer under a shared read-lock causes data races, misrouting private transfers.
- [High] Forwarding direct CCC commands unmodified through the hub causes a NULL pointer dereference in the parent driver.
--

--- Patch [6]: [PATCH v14 6/8] i3c: hub: Add support for the I3C interface in the I3C hub ---
commit 02e794653daf2b554f0c962c355d0bb7abb26374
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
Subject: i3c: hub: Add support for the I3C interface in the I3C hub

This commit adds virtual I3C bus support for the hub, creating virtual
master controllers for downstream ports and forwarding operations through
the parent controller.

> diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c
> index 0000000000000..6105bb710be69
> --- /dev/null
> +++ b/drivers/i3c/hub.c
[ ... ]
> +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 forwarding DAA directly to the parent controller attach dynamically
discovered downstream devices directly to the parent bus instead of the
virtual hub controller?

If these devices are bound to the physical parent, subsequent transfers 
might bypass i3c_hub_master_priv_xfers() entirely, leaving the hub port 
closed and causing communication to fail.

[ ... ]
> +static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
> +				struct i3c_master_controller *controller,
> +				struct i3c_ccc_cmd *cmd)
> +{
[ ... ]
> +	mutex_lock(&hub->lock);
> +	i3c_hub_enable_port(controller);
> +	ret = i3c_master_send_ccc_cmd(parent, cmd);

[Severity: High]
When forwarding direct CCC commands to the parent controller, the command 
structure still contains virtual bus descriptors in its dests array. 

Since virtual devices do not allocate a physical master_priv context, can 
the physical parent controller driver dereference a NULL pointer here when 
it assumes these devices are natively attached?

> +	i3c_hub_disable_port(controller);
> +	mutex_unlock(&hub->lock);
> +
> +	return ret;
> +}
[ ... ]
> +static int i3c_hub_master_priv_xfers(struct i3c_dev_desc *dev,
> +				     struct i3c_xfer *xfers,
> +				     int nxfers,
> +				     enum i3c_xfer_mode mode)
> +{
[ ... ]
> +	if (hub_addr != target_addr) {
> +		hub_dev->info.dyn_addr = target_addr;
> +
> +		ret = i3c_master_reattach_i3c_dev_locked(hub_dev, hub_addr);

[Severity: High]
When the hub address is temporarily swapped here, does 
i3c_master_reattach_i3c_dev_locked() unconditionally mark the original 
address (old_dyn_addr) as free?

This appears to permanently free downstream address slots and could lead 
to address collisions on the parent bus.

> +		if (ret) {
> +			hub_dev->info.dyn_addr = hub_addr;
> +			goto out_unlock_parent;
> +		}
> +	}
[ ... ]
> +static int i3c_hub_request_ibi(struct i3c_dev_desc *desc,
> +			       const struct i3c_ibi_setup *req)
> +{
[ ... ]
> +	i3c_bus_maintenance_lock(&parent->bus);
> +
> +	orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);

[Severity: High]
Is it safe to modify desc->master here while only holding a read lock 
on the bus?

A concurrent private transfer might read desc->master and resolve it to 
the physical parent controller, misrouting the transfer and bypassing 
hub routing entirely.

> +
> +	/*
> +	 * Downstream devices are exposed through the virtual hub controller.
> +	 * Private transfers only use the hub device temporarily with the
> +	 * downstream device's dynamic address; they do not attach the downstream
> +	 * device to the parent controller.
> +	 *
> +	 * Attach the device to the parent controller before requesting IBI so
> +	 * the parent can allocate any controller-private IBI state.
> +	 */
> +	ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);

[Severity: High]
Could attaching the downstream device directly to the parent controller 
fail with -EBUSY if the address was already reserved? 

Additionally, if it succeeds, might this leave duplicate DAT entries in 
the parent hardware when the hub later masquerades as this target address?

> +	if (ret) {
> +		i3c_hub_update_desc_parent(&desc->common, orig_parent);
> +		i3c_bus_maintenance_unlock(&parent->bus);
> +		return ret;
> +	}
> +
> +	if (desc->ibi) {
> +		if (desc->ibi->wq) {
> +			destroy_workqueue(desc->ibi->wq);
> +			desc->ibi->wq = NULL;
> +		}
> +
> +		kfree(desc->ibi);
> +		desc->ibi = NULL;
> +	}
> +	ret = i3c_dev_request_ibi_locked(desc, req);
> +	if (ret)
> +		i3c_master_direct_detach_i3c_dev_locked(desc);

[Severity: High]
Does detaching the downstream device here unconditionally free its static 
and dynamic addresses on the parent bus? 

The device is still physically present and using those addresses, so freeing 
them could lead to address collisions.

> +
> +	i3c_hub_update_desc_parent(&desc->common, orig_parent);
> +
> +	i3c_bus_maintenance_unlock(&parent->bus);
> +
> +	return ret;
> +}
> +
> +static void i3c_hub_free_ibi(struct i3c_dev_desc *desc)
> +{
[ ... ]
> +	mutex_lock(&hub->lock);
> +	i3c_hub_enable_port(controller);
> +
> +	i3c_bus_maintenance_lock(&parent->bus);
> +	orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> +	i3c_dev_free_ibi_locked(desc);

[Severity: High]
Can holding hub->lock here cause a deadlock if there are pending IBI work 
handlers? 

Calling i3c_dev_free_ibi_locked() blocks while destroying the workqueue. 
If an active IBI handler attempts to read the IBI payload, it will invoke 
i3c_hub_master_priv_xfers(), which attempts to acquire this exact same lock.

> +
> +	/*
> +	 * Drop the temporary parent attachment created for IBI setup.
> +	 * The downstream device remains exposed through the virtual hub
> +	 * controller.
> +	 */
> +	i3c_master_direct_detach_i3c_dev_locked(desc);

[Severity: High]
Does detaching the device on this path also incorrectly free the 
downstream device's addresses on the parent bus?

> +	i3c_hub_update_desc_parent(&desc->common, orig_parent);
> +	i3c_bus_maintenance_unlock(&parent->bus);
> +
> +	i3c_hub_disable_port(controller);
> +	mutex_unlock(&hub->lock);
> +}
[ ... ]
> +static int i3c_hub_disable_ibi(struct i3c_dev_desc *desc)
> +{
[ ... ]
> +	mutex_lock(&hub->lock);
> +	i3c_hub_enable_port(controller);
> +
> +	orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> +
> +	i3c_bus_maintenance_lock(&parent->bus);
> +	ret = i3c_dev_disable_ibi_locked(desc);

[Severity: High]
Could holding hub->lock while calling i3c_dev_disable_ibi_locked() also 
lead to the same ABBA deadlock with pending IBI workers trying to acquire 
hub->lock via i3c_hub_master_priv_xfers()?

> +	i3c_bus_maintenance_unlock(&parent->bus);
> +
> +	i3c_hub_update_desc_parent(&desc->common, orig_parent);
> +
> +	i3c_hub_disable_port(controller);
> +	mutex_unlock(&hub->lock);
> +
> +	return ret;
> +}
> +
> +static void i3c_hub_recycle_ibi_slot(struct i3c_dev_desc *desc,
> +				     struct i3c_ibi_slot *slot)
> +{
> +	/*
> +	 * TODO: Forward IBI slot recycling to the parent controller when
> +	 * downstream-device IBI support is fully enabled. Current users only
> +	 * rely on the hub device IBI path for SMBus-agent events, so no parent
> +	 * hardware slot is recycled through this virtual callback currently.
> +	 */
> +}

[Severity: High]
Does this empty implementation permanently leak hardware IBI slots 
allocated by the parent controller? 

Over time, failing to return these slots could exhaust the pool and prevent 
further IBI reception.

> +
> +static const struct i3c_master_controller_ops i3c_hub_master_ops_data = {
> +	.bus_init = i3c_hub_master_bus_init,
[ ... ]
> diff --git a/include/linux/i3c/hub.h b/include/linux/i3c/hub.h
> index 0000000000000..8ad2cf3c9d5b8
> --- /dev/null
> +++ b/include/linux/i3c/hub.h
> @@ -0,0 +1,102 @@
[ ... ]
> +static inline struct i3c_master_controller *
> +i3c_hub_update_desc_parent(struct i3c_i2c_dev_desc *desc,
> +			   struct i3c_master_controller *parent)
> +{
> +	struct i3c_master_controller *orig_parent = desc->master;
> +
> +	desc->master = parent;

[Severity: High]
Does modifying desc->master here create a data race with concurrent 
transfers using the I3C core transfer functions?

> +	return orig_parent;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714092053.2461482-1-lakshay.piplani@nxp.com?part=6