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

[sashiko-bot@kernel.org]

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 <aman.kumarpandey@nxp.com>

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.

> +	i3c_bus_maintenance_unlock(&master->bus);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_send_ccc_cmd);

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