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

[Lakshay Piplani]

> -----Original Message-----
> From: Frank Li (OSS) <frank.li at oss.nxp.com>
> Sent: Thursday, July 2, 2026 1:12 AM
> To: sashiko-reviews at lists.linux.dev
> Cc: Lakshay Piplani <lakshay.piplani at nxp.com>; linux-i3c at lists.infradead.org;
> robh at kernel.org; Alexandre Belloni <alexandre.belloni at bootlin.com>;
> devicetree at vger.kernel.org; conor+dt at kernel.org; Frank.Li at kernel.org
> Subject: Re: [PATCH v13 5/7] i3c: hub: Add support for the I3C interface in the
> I3C hub
> 
> On Wed, Jul 01, 2026 at 07:23:18AM +0000, sashiko-bot at kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 7 potential issue(s)
> to consider:
> > - [Critical] `i3c_hub_request_ibi()` intentionally frees `desc->ibi` out from
> under its caller without cleaning up its workqueue, causing a memory leak and
> a double-free on failure.
> > - [High] `i3c_hub_free_ibi()` causes a guaranteed NULL pointer dereference in
> the core IBI free path by recursively clearing `dev->ibi`.
> > - [High] Address spoofing in `i3c_hub_master_priv_xfers` corrupts parent bus
> address reservations and suffers from an unprotected race window.
> > - [High] Missing `xfer_lock` in several hub operations allows concurrent port
> enabling, breaking virtual bus isolation.
> > - [High] `i3c_hub_free_ibi()` permanently destroys downstream device
> address reservations on the parent bus.
> > - [High] `i3c_hub_do_daa` delegates DAA entirely to the parent controller,
> breaking boardinfo matching and DT topology for downstream devices.
> > - [Medium] Acquiring `parent->bus.lock` while holding `child->bus.lock`
> triggers a lockdep splat due to unannotated same-class lock nesting.
> > --
> >
> > commit 22c2a31197952a434777498d93a1ed925009d6de
> > Author: Lakshay Piplani <lakshay.piplani at nxp.com>
> > Subject: 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. Virtual master
> > controllers are created for the downstream ports, and bus operations
> > are forwarded through the hub device.
> >
> > > diff --git a/drivers/i3c/hub.c b/drivers/i3c/hub.c new file mode
> > > 100644 index 0000000000000..d29ed40b6f735
> > > --- /dev/null
> > > +++ b/drivers/i3c/hub.c
> > > @@ -0,0 +1,566 @@
> >
> > [ ... ]
> >
> > > +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: High]
> > Does delegating DAA directly to the parent controller bypass the
> > virtual hub bus entirely?
> >
> > By calling i3c_master_do_daa(parent), it appears the parent discovers
> > and attaches downstream devices to the physical bus instead of the
> > virtual controller. Does this cause boardinfo matching to fail since
> > the devices are defined under the hub's device tree node?
> 
> 
> Add comments here: like
> 
> /*
> 	All target devices which connect active ports of hub are tread as
> connect to parent bus at same time. So keep simple, all devices dynamatic
> address should be in the same address space, that means it is differences for all
> target devices even it is at difference ports. So far, address (128, need double
> check) is enough now.
> */
> 
> Please rephrase it.
> 
> >
> > > +	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]
> > Could the omission of hub->xfer_lock in this function allow concurrent
> > port enabling?
> 
> look like correct, you acquire it at i3c_hub_master_priv_xfers(), should be
> similar here.
> 
> >
> > The lock is used to serialize private transfers, but callbacks like
> > i3c_hub_send_ccc_cmd(), i3c_hub_do_daa(), and i3c_hub_enable_ibi() do
> > not acquire it. If a private transfer runs concurrently with a CCC
> > command, could multiple downstream ports be enabled simultaneously and
> > cause the hub to broadcast traffic incorrectly?
> >
> > > +	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)
> > > +{
> > > +	struct i3c_master_controller *controller = i3c_dev_get_master(dev);
> > > +	struct i3c_hub_controller *hub_controller;
> > > +	struct i3c_dev_desc *hub_dev;
> > > +	struct i3c_master_controller *parent;
> > > +	u8 hub_addr, target_addr;
> > > +	struct i3c_hub *hub;
> > > +	int ret, xfer_ret;
> >
> > [ ... ]
> >
> > > +	if (hub_addr != target_addr) {
> > > +		down_write(&parent->bus.lock);
> 
> call i3c_bus_maintenance_lock(), move it to API.
> 
> >
> > [Severity: Medium]
> > Will acquiring parent->bus.lock here trigger a lockdep warning?
> >
> > Since virtual hub operations are invoked by the I3C core with the
> > child bus's lock already held, acquiring the parent bus's lock (which
> > belongs to the same lock class) without nesting annotations might
> > produce a false positive lockdep splat. This also happens in
> > i3c_hub_enable_ibi() and i3c_hub_disable_ibi().
> >
> > > +		hub_dev->info.dyn_addr = target_addr;
> > > +
> > > +		ret = i3c_master_reattach_i3c_dev_locked(hub_dev,
> hub_addr);
> > > +		if (ret) {
> > > +			up_write(&parent->bus.lock);
> > > +			hub_dev->info.dyn_addr = hub_addr;
> > > +			goto disable;
> > > +		}
> > > +		up_write(&parent->bus.lock);
> > > +	}
> > > +
> > > +	ret = i3c_device_do_xfers(hub->hub_dev, xfers, nxfers, mode);
> > > +
> > > +	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: High]
> > Does this sequence permanently destroy device tree static address
> > reservations?
> >
> > When the hub's dynamic address is altered via
> > i3c_master_reattach_i3c_dev_locked(), the hub's original address slot
> > is set to free on the parent bus, leaving it vulnerable to DAA theft
> > since the parent write-lock is dropped during the transfer. When the
> > address is restored later by passing target_addr to
> > i3c_master_reattach_i3c_dev_locked,
> > this marks the downstream device's address as free, which seems to
> > wipe out its reservation.
> 
> It makes sense
> 
> 	i3c_bus_maintenance_lock()
> 	i3c_master_reattach_i3c_dev_locked()
> 	i3c_device_do_xfers_locked()
> 	i3c_master_reattach_i3c_dev_locked()
> 	i3c_bus_maintenance_unlock()
> 
> Another problem is previous lockdep().
> 
> int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_xfer *xfers,
>                         int nxfers, enum i3c_xfer_mode mode) {
> 	...
>         i3c_bus_normaluse_lock(dev->bus);
>         ret = i3c_dev_do_xfers_locked(dev->desc, xfers, nxfers, mode);
>         i3c_bus_normaluse_unlock(dev->bus);
>         ...
> 
>         return ret;
> }
> 
> simple method is promote i3c_bus_normaluse_lock() to
> i3c_bus_maintenance_lock() when dev attached to hub devices.
> 
> and also lock parent master's i3c_bus_maintenance_lock(),
> 
> So here needn't lock in hub.c
> 
> if parent master use write lock, hub->xfer_lock needn't
> 
> >
> > > +
> > > +		up_write(&parent->bus.lock);
> > > +		if (!ret)
> > > +			ret = xfer_ret;
> > > +	}
> > > +disable:
> > > +	i3c_hub_disable_port(controller);
> > > +	mutex_unlock(&hub->xfer_lock);
> > > +	return ret;
> > > +}
> >
> > [ ... ]
> >
> > > +static int i3c_hub_request_ibi(struct i3c_dev_desc *desc,
> > > +			       const struct i3c_ibi_setup *req) {
> > > +	struct i3c_master_controller *controller = i3c_dev_get_master(desc);
> > > +	struct i3c_hub_controller *hub_controller;
> > > +	struct i3c_master_controller *orig_parent;
> > > +	struct i3c_master_controller *parent;
> > > +	struct i3c_hub *hub;
> > > +	int ret;
> >
> > [ ... ]
> >
> > > +	orig_parent = i3c_hub_update_desc_parent(&desc->common, parent);
> > > +
> > > +	ret = i3c_master_direct_attach_i3c_dev_locked(parent, desc);
> 
> All IBI cap devices should be already attached to master bus when DAA? need
> attach again here?
> Actived port's devices logically attached to master bus?
> 
> So needn't switch port.
> 

No. During DAA, downstream devices are enumerated on the virtual controller, but they are not attached to the parent controller. 
The virtual controller's attach_i3c_dev() callback is intentionally a no-op.
For private transfers, we temporarily update the hub's dynamic address to the target device's address, perform the transfer through the
parent controller and then restore the hub's original address. This does not establish any parent controller-specific attachment state.

Therefore, i3c_master_direct_attach_i3c_dev_locked() is still required here so the parent controller can establish its controller-private state before requesting IBI.

Lakshay

> > > +	if (ret) {
> > > +		i3c_hub_update_desc_parent(&desc->common, orig_parent);
> > > +		up_write(&parent->bus.lock);
> > > +		return ret;
> > > +	}
> > > +
> > > +	kfree(desc->ibi);
> > > +	desc->ibi = NULL;
> > > +	ret = i3c_dev_request_ibi_locked(desc, req);
> >
> > [Severity: Critical]
> > Can freeing desc->ibi here cause a memory leak and a double-free?
> >
> > If a downstream device requests an IBI, the core
> > i3c_dev_request_ibi_locked() allocates ibi and ibi->wq before calling
> > this hub operation. By calling
> > kfree(desc->ibi) here, the outer workqueue is leaked.
> >
> > Furthermore, if the recursive call to i3c_dev_request_ibi_locked()
> > fails, the outer caller will execute:
> >
> >     if (ret) {
> >             kfree(ibi);
> >             dev->ibi = NULL;
> >     }
> >
> > Could this result in a double-free on the original allocation?
> 
> look like make sense
> 
> Frank
> >
> > > +	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;
> > > +}
> > > +