[PATCH v12 5/7] i3c: hub: Add support for the I3C interface in the I3C hub

[Frank Li]

On Wed, Jun 17, 2026 at 11:18:04AM +0000, sashiko-bot at kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> - [Critical] Deterministic NULL pointer dereference in `i3c_hub_free_ibi()` due to recursive callback chain.
> - [Critical] Memory leak and double-free memory corruption in `i3c_hub_request_ibi()` error paths.
> - [Critical] Critical race condition and parent bus state corruption in `i3c_hub_master_priv_xfers()` due to unlocked address mutation.
> - [Critical] Unimplemented `i3c_hub_recycle_ibi_slot()` permanently leaks hardware IBI slots.
> - [Critical] Architectural bypass in `i3c_hub_do_daa()` attaches dynamically discovered devices to the parent bus instead of the virtual hub bus.
> - [High] Calling `i3c_hub_enable_port()` outside of any locks allows concurrent operations to physically enable multiple downstream ports simultaneously, breaking electrical isolation.
> - [High] Silently dropping broadcast RSTDAA commands prevents resetting of downstream devices.
> - [Medium] Accepting I2C devices while unconditionally rejecting all I2C transfers violates the API contract.
> --
>
> commit 2a201a9e0e9a55b31467d874340085240b34afc3
> Author: Lakshay Piplani <lakshay.piplani at nxp.com>
>
> i3c: hub: Add support for the I3C interface in the I3C hub
>
> This patch adds virtual I3C bus support for the hub and provides an
> 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..e8744b5172340
> > --- /dev/null
> > +++ b/drivers/i3c/hub.c
> > @@ -0,0 +1,483 @@
>
> [ ... ]
>
> > +static int i3c_hub_do_daa(struct i3c_hub *hub,
> > +			  struct i3c_master_controller *controller)
> > +{
> > +	struct i3c_master_controller *parent;
> > +	int ret;
> > +
> > +	if (!hub || !hub->hub_dev)
> > +		return -ENODEV;
> > +
> > +	parent = i3c_dev_get_master(hub->hub_dev->desc);
> > +	if (!parent)
> > +		return -ENODEV;
> > +
> > +	i3c_hub_enable_port(controller);
> > +	ret = i3c_master_do_daa(parent);
>
> [Severity: Critical]
> Since `i3c_master_do_daa()` executes the DAA sequence natively on the parent
> controller, will the newly discovered devices be allocated and attached
> directly to the parent bus instead of this virtual hub's bus?
>
> If they are attached to the parent bus, won't subsequent transfers completely
> bypass `i3c_hub_master_priv_xfers()`, causing communication with these
> dynamically enumerated devices to fail?

I forget some background, does all devices under hub port shared the same
I3C address space? that's means dynmatic address is difference for all
difference ports?

I2C switcher can support the same address at difference port. look like
diffenent port devices should attach to hub's bus. Only temparily attach
to parent during transfer?

Frank

>
> [Severity: High]
> Is there a risk of breaking electrical isolation here? `i3c_hub_enable_port()`
> is called before acquiring any parent bus locks.
>
> If multiple threads operate on different virtual controllers concurrently,
> could they physically enable multiple downstream ports at the same time
> before the bus lock is acquired?
>
> > +	i3c_hub_disable_port(controller);
> > +
> > +	return ret;
> > +}
>
> [ ... ]
>
> > +static int i3c_hub_send_ccc_cmd(struct i3c_hub *hub,
> > +				struct i3c_master_controller *controller,
> > +				struct i3c_ccc_cmd *cmd)
> > +{
> > +	struct i3c_master_controller *parent;
> > +	int ret;
> > +
> > +	if (!hub || !hub->hub_dev)
> > +		return -ENODEV;
> > +
> > +	parent = i3c_dev_get_master(hub->hub_dev->desc);
> > +	if (!parent)
> > +		return -ENODEV;
> > +
> > +	i3c_hub_enable_port(controller);
> > +	ret = i3c_master_send_ccc_cmd(parent, cmd);
>
> [Severity: High]
> Similar to `i3c_hub_do_daa()`, does calling `i3c_hub_enable_port()` without a
> lock risk enabling multiple ports concurrently if multiple CCC commands are
> issued on different ports?
>
> > +	i3c_hub_disable_port(controller);
> > +
> > +	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) {
> > +		down_write(&parent->bus.lock);
> > +		hub_dev->info.dyn_addr = target_addr;
> > +
> > +		ret = i3c_master_reattach_i3c_dev_locked(hub_dev, hub_addr);
> > +		up_write(&parent->bus.lock);
> > +		if (ret) {
> > +			hub_dev->info.dyn_addr = hub_addr;
> > +			goto disable;
> > +		}
> > +	}
> > +
> > +	ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);
>
> [Severity: Critical]
> Does dropping `parent->bus.lock` before calling `i3c_device_do_xfers()` expose
> a race condition?
>
> If multiple downstream ports initiate transfers concurrently, could they
> overwrite the shared `hub_dev->info.dyn_addr` before the transfer completes,
> causing packets to be routed to the wrong physical device?
>
> > +
> > +	if (hub_addr != target_addr) {
> > +		down_write(&parent->bus.lock);
> > +		hub_dev->info.dyn_addr = hub_addr;
> > +
> > +		xfer_ret = i3c_master_reattach_i3c_dev_locked(hub_dev,
> > +							      target_addr);
>
> [Severity: Critical]
> Could passing `target_addr` as the old address corrupt the parent bus state?
>
> `i3c_master_reattach_i3c_dev_locked()` frees the old address slot in the bus
> slot map. Will this erroneously mark `target_addr` as `I3C_ADDR_SLOT_FREE`
> on the parent bus?
>
> > +
> > +		up_write(&parent->bus.lock);
> > +		if (!ret)
> > +			ret = xfer_ret;
> > +	}
> > +disable:
> > +	i3c_hub_disable_port(controller);
> > +	return ret;
> > +}
> > +
> > +static int i3c_hub_attach_i2c_dev(struct i2c_dev_desc *dev)
> > +{
> > +	return 0;
> > +}
>
> [ ... ]
>
> > +static int i3c_hub_i2c_xfers(struct i2c_dev_desc *dev,
> > +			     struct i2c_msg *xfers, int nxfers)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
>
> [Severity: Medium]
> Is it intentional to return `0` from `i3c_hub_attach_i2c_dev()` while rejecting
> all I2C transfers with `-EOPNOTSUPP`?
>
> Would it be safer to return an error from the attach callback to cleanly
> prevent I2C driver binding if the hub does not support I2C forwarding?
>
> [ ... ]
>
> > +static int i3c_hub_master_send_ccc_cmd(struct i3c_master_controller *controller,
> > +				       struct i3c_ccc_cmd *cmd)
> > +{
> > +	struct i3c_hub_controller *hub_controller;
> > +	struct i3c_hub *hub;
> > +
> > +	hub_controller = dev_get_drvdata(&controller->dev);
> > +	if (!hub_controller || !hub_controller->hub)
> > +		return -ENODEV;
> > +
> > +	hub = hub_controller->hub;
> > +
> > +	if (!hub->hub_dev)
> > +		return -ENODEV;
> > +
> > +	if (cmd->id == I3C_CCC_RSTDAA(true))
> > +		return 0;
>
> [Severity: High]
> Does silently dropping the broadcast RSTDAA command prevent downstream devices
> from being correctly reset by the hub?
>
> If the command is dropped instead of being forwarded to the downstream ports,
> will devices retain their old dynamic addresses and fail to re-initialize
> during hot-plug recovery?
>
> > +
> > +	return i3c_hub_send_ccc_cmd(hub, controller, cmd);
> > +}
>
> [ ... ]
>
> > +static int i3c_hub_request_ibi(struct i3c_dev_desc *desc,
> > +			       const struct i3c_ibi_setup *req)
> > +{
>
> [ ... ]
>
> > +	ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);
> > +	if (ret) {
> > +		i3c_hub_update_desc_parent(&desc->common, orig_parent);
> > +		up_write(&parent->bus.lock);
> > +		return ret;
> > +	}
> > +
> > +	kfree(desc->ibi);
>
> [Severity: Critical]
> Does freeing `desc->ibi` here leak `desc->ibi->wq`?
>
> When the core function `i3c_dev_request_ibi_locked()` allocates `ibi`, it also
> allocates `ibi->wq`.
>
> Also, if the inner call to `i3c_dev_request_ibi_locked()` fails, this function
> returns an error to the original core caller. Will the outer caller's error
> path execute `kfree(ibi)` on its local variable, resulting in a double-free
> since it was already freed here?
>
> > +	desc->ibi = NULL;
> > +	ret = i3c_dev_request_ibi_locked(desc, req);
> > +	if (ret)
> > +		i3c_master_direct_detach_i3c_dev_locked(desc);
> > +
> > +	i3c_hub_update_desc_parent(&desc->common, orig_parent);
> > +
> > +	up_write(&parent->bus.lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static void i3c_hub_free_ibi(struct i3c_dev_desc *desc)
> > +{
>
> [ ... ]
>
> > +	i3c_hub_enable_port(controller);
> > +
> > +	down_write(&parent->bus.lock);
> > +	orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> > +	i3c_master_direct_detach_i3c_dev_locked(desc);
> > +	i3c_dev_free_ibi_locked(desc);
>
> [Severity: Critical]
> Can this recursive call to `i3c_dev_free_ibi_locked(desc)` cause a null
> pointer dereference?
>
> When the core initially calls `i3c_dev_free_ibi_locked(desc)`, it invokes
> this hub callback. The inner call here successfully frees `desc->ibi` and sets
> it to null.
>
> When control returns to the outer core function, it unconditionally evaluates:
>
>     if (dev->ibi->wq) {
>
> Will this immediately dereference the null pointer?
>
> > +	i3c_hub_update_desc_parent(&desc->common, orig_parent);
> > +	up_write(&parent->bus.lock);
> > +
> > +	i3c_hub_disable_port(controller);
> > +}
>
> [ ... ]
>
> > +static void i3c_hub_recycle_ibi_slot(struct i3c_dev_desc *desc,
> > +				     struct i3c_ibi_slot *slot)
> > +{
> > +}
>
> [Severity: Critical]
> Does leaving this function empty permanently leak hardware IBI slots?
>
> When the core finishes processing an IBI, it returns the slot to the virtual
> controller. Because this doesn't forward the recycle request to the parent
> hardware controller, will the parent controller eventually exhaust its hardware
> slots and permanently stop processing IBIs?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260617110355.1591844-1-lakshay.piplani@nxp.com?part=5