Re: [PATCH v10 3/9] i3c: master: Add APIs for I3C hub support

[Frank Li <Frank.li@nxp.com>]

On Mon, May 25, 2026 at 08:00:44AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Incorrect logic and API contract violation in `i3c_master_supports_ccc_cmd()`.
> - [Medium] `i3c_master_direct_attach_i3c_dev_locked()` incorrectly returns `-EINVAL` for devices without addresses.
> --
>
> commit e1382a24562500ebcc8529e9a5fff3016b719a36
> Author: Aman Kumar Pandey <aman.kumarpandey@nxp.com>
>
> i3c: master: Add APIs for I3C hub support
>
> This commit adds helpers for attaching and detaching I3C devices, as well as
> helpers to check CCC support and send CCC commands. These additions prepare the
> subsystem for I3C hub support.
>
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index ec3547f75469e..4f0565faa182f 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
>
> [ ... ]
>
> > +/**
> > + * i3c_master_direct_attach_i3c_dev_locked() - attach an I3C device to a master
> > + * @master: I3C master controller to attach the device to
> > + * @dev: I3C device descriptor representing the device
> > + *
> > + * This function attaches an I3C device to its master controller once the
> > + * device has a valid address on the bus. Devices without an assigned address
> > + * are ignored. The master device itself is never attached through this bus.
> > + *
> > + * Context: Caller must hold master->bus.lock in write mode.
> > + *
> > + * Return: 0 on success, or a negative error code if the attach operation
> > + *         fails in the master controller driver.
> > + */
> > +int i3c_master_direct_attach_i3c_dev_locked(struct i3c_master_controller *master,
> > +					    struct i3c_dev_desc *dev)
> > +{
> > +	int ret = 0;
> > +
> > +	/*
> > +	 * We don't attach devices to the controller until they are
> > +	 * addressable on the bus.
> > +	 */
> > +
> > +	if (!dev->info.static_addr && !dev->info.dyn_addr)
> > +		return -EINVAL;
>
> [Severity: Medium]
> The documentation for this function states that "Devices without an assigned
> address are ignored." However, this code returns -EINVAL instead of 0.
>
> Looking at the internal i3c_master_attach_i3c_dev(), it returns 0 to safely
> ignore unaddressed devices until they acquire an address. Should this new
> helper also return 0 to match both its documentation and the subsystem's
> existing convention?

Suppose AI is correct, please check it.

>
> > +
> > +	/* Do not attach the master device itself. */
> > +	if (master->this != dev && master->ops->attach_i3c_dev)
> > +		ret = master->ops->attach_i3c_dev(dev);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(i3c_master_direct_attach_i3c_dev_locked);
>
> [ ... ]
>
> > +/**
> > + * i3c_master_supports_ccc_cmd() - check CCC command support
> > + * @master: I3C master controller
> > + * @cmd: CCC command to verify
> > + *
> > + * This function verifies whether the given I3C master controller supports
> > + * the specified Common Command Code (CCC).
> > + *
> > + * Return: 0 if the CCC command is supported and executed successfully,
> > + *         -EINVAL if arguments are invalid,
> > + *         -EOPNOTSUPP if the master does not support CCC commands,
> > + *         or another negative error code from the master's operation.
> > + */
> > +int i3c_master_supports_ccc_cmd(struct i3c_master_controller *master,
> > +				const struct i3c_ccc_cmd *cmd)
> > +{
> > +	if (!cmd || !master)
> > +		return -EINVAL;
> > +
> > +	if (!master->ops->supports_ccc_cmd)
> > +		return -EOPNOTSUPP;
> > +
> > +	return master->ops->supports_ccc_cmd(master, cmd);
> > +}
>
> [Severity: Medium]
> Does this function's logic and return value match its documentation and the I3C
> subsystem semantics?
>
> The docstring states it returns 0 if a command is supported, but it actually
> returns the boolean result of the supports_ccc_cmd callback (1 for supported,
> 0 for unsupported). A caller checking for 0 as success would incorrectly treat
> supported commands as unsupported.

AI said correct, please fix it.

Frank
>
> Additionally, if the callback is absent, this returns -EOPNOTSUPP. However, in
> other places like i3c_master_send_ccc_cmd_locked(), the core I3C logic assumes
> all commands are supported if the callback is NULL. Could returning -EOPNOTSUPP
> here falsely indicate no commands are supported?


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