Re: [PATCH v13 1/7] i3c: master: Add APIs for I3C hub support

On Wed, Jul 01, 2026 at 07:22:30AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Exported APIs interact with controller hardware without taking a Runtime PM reference.
> - [Medium] `i3c_master_supports_ccc_cmd` incorrectly returns true for masters that do not implement CCC command sending.
> --
>
> commit b5579dc49beffe04e3498fcfb290372e74bd88f0
> Author: Aman Kumar Pandey <[email protected]>
>
> i3c: master: Add APIs for I3C hub support
>
> This commit adds helper functions for attaching and detaching I3C devices,
> checking CCC support, sending CCC commands, and managing address slot states.
> These additions prepare the subsystem for I3C hub support.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index f1be38a640ca1..3e7b7f7f97cfb 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> [ ... ]
> > @@ -1775,6 +1790,83 @@ static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
> >  	return 0;
> >  }
> >
> > +/**
> > + * i3c_master_direct_attach_i3c_dev_locked() - Attach an I3C device to a master
> [ ... ]
> > +int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
> > +					    struct i3c_dev_desc *dev)
> > +{
> > +	int ret;
> > +
> > +	if (WARN_ON(i3c_dev_get_master(dev) != master))
> > +		return -EINVAL;
> > +
> > +	ret = i3c_master_get_i3c_addrs(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Do not attach the master device itself. */
> > +	if (master->this != dev && master->ops->attach_i3c_dev) {
> > +		ret = master->ops->attach_i3c_dev(dev);
>
> [Severity: High]
> Should this function call i3c_master_rpm_get() to take a runtime PM
> reference before invoking the controller's attach_i3c_dev callback?
>
> Since the I3C subsystem sets pm_suspend_ignore_children(&master->dev, true),
> maintaining an active runtime PM reference on the hub device does not
> automatically resume the parent controller. If an I3C hub calls this on a
> parent master that is runtime suspended, accessing the hardware registers
> could lead to bus faults or hangs.
>
> > +		if (ret) {
> > +			i3c_master_put_i3c_addrs(dev);
> > +			return ret;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);
> > +
> > +/**
> > + * i3c_master_direct_detach_i3c_dev_locked() - Detach an I3C device from a master
> [ ... ]
> > +void i3c_master_direct_detach_i3c_dev_locked(struct i3c_dev_desc *dev)
> > +{
> > +	struct i3c_master_controller *master = i3c_dev_get_master(dev);
> > +
> > +	/*
> > +	 * We don't attach devices to the controller until they are
> > +	 * addressable on the bus, so skip detach for unaddressed devices.
> > +	 */
> > +	if (!dev->info.static_addr && !dev->info.dyn_addr)
> > +		return;
> > +
> > +	/* Do not detach the master device itself. */
> > +	if (master->this != dev && master->ops->detach_i3c_dev)
> > +		master->ops->detach_i3c_dev(dev);
>
> [Severity: High]
> Similar to the attach path, does this detach callback require the parent
> master to be resumed?
>
> If the parent controller is runtime suspended, invoking detach_i3c_dev
> without first taking a runtime PM reference might access suspended hardware.
>
> > +
> > +	dev->common.master_priv = NULL;
> > +	i3c_master_put_i3c_addrs(dev);
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_direct_detach_i3c_dev_locked);
> [ ... ]
> > @@ -2548,6 +2640,50 @@ static void i3c_master_reconcile_dyn_addrs(struct i3c_master_controller *master)
> >  	}
> >  }
> >
> > +/**
> > + * i3c_master_supports_ccc_cmd() - check CCC command support
> [ ... ]
> > +bool i3c_master_supports_ccc_cmd(struct i3c_master_controller *master,
> > +				 const struct i3c_ccc_cmd *cmd)
> > +{
> > +	if (!master || !cmd)
> > +		return false;
> > +
> > +	if (!master->ops->supports_ccc_cmd)
> > +		return true;
>
> [Severity: Medium]
> Is it safe to default to true without verifying that the controller
> actually implements master->ops->send_ccc_cmd?
>
> If a controller lacks send_ccc_cmd, it does not support sending any CCC
> commands. The core will return -EOPNOTSUPP in i3c_master_send_ccc_cmd_locked(),
> but returning true here might cause a hub controller to falsely advertise
> CCC support for its downstream devices.
>
> > +
> > +	return master->ops->supports_ccc_cmd(master, cmd);
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_supports_ccc_cmd);
> > +
> > +/**
> > + * i3c_master_send_ccc_cmd() - send a CCC command
> [ ... ]
> > +int i3c_master_send_ccc_cmd(struct i3c_master_controller *master,
> > +			    struct i3c_ccc_cmd *cmd)
> > +{
> > +	int ret;
> > +
> > +	i3c_bus_maintenance_lock(&master->bus);
> > +	ret = i3c_master_send_ccc_cmd_locked(master, cmd);
>
> [Severity: High]
> Does this function need to take a runtime PM reference before calling
> i3c_master_send_ccc_cmd_locked()?
>
> Like the attach/detach helpers, if an I3C hub calls this on a parent master
> that is suspended, this could lead to hardware access while suspended.

This one need fixed. i3c_master_rpm_get() need be called before
i3c_bus_maintenance_lock();

Frank

>
> > +	i3c_bus_maintenance_unlock(&master->bus);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_send_ccc_cmd);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

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