Re: [PATCH v10 3/9] 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:
- [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?

> +
> +	/* 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.

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