Re: [PATCH v3 05/13] i3c: master: Add support for devices using SETAASA

[sashiko-bot@kernel.org]

Sashiko AI review found 7 potential issue(s):
- [High] Missing `i3c_master_reattach_i3c_dev` call for SETAASA devices causes hardware state desynchronization and address collisions.
- [High] Unhandled `I3C_ERROR_M2` in `i3c_master_setaasa_locked` aborts entire I3C bus initialization if no devices acknowledge the broadcast.
- [High] SETAASA devices without an explicit `assigned-address` property are silently ignored and never initialized.
- [Medium] Broadcasting SETAASA before issuing targeted SETDASA commands breaks devices intended for SETDASA.
- [Medium] Bypassing `i3c_master_retrieve_dev_info` for SETAASA devices leaves critical device capabilities zeroed.
- [Medium] Broadcasting Vendor Extension CCC (SETHID) without a defining byte violates the MIPI I3C specification.
- [Medium] Mismatch between `assigned-address` and `static_addr` for SETAASA devices permanently leaks address slots.
--

commit 198585b433bb26dea4a874b95e4dc205539e927a
Author: Akhil R <akhilrajeev@nvidia.com>

i3c: master: Add support for devices using SETAASA

This commit adds support for devices using the SETAASA (Set All Addresses to
Static Address) method. It enables the discovery and dynamic address
assignment of these devices without relying on ENTDAA or SETDASA.

Link: https://www.mipi.org/mipi-disco-for-i3c-download

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index ffec97157d4883..de72317c9a5c73 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -1051,6 +1052,47 @@ static int i3c_master_rstdaa_locked(struct i3c_master_controller *master,
>  	return ret;
>  }
>  
> +/**
> + * i3c_master_setaasa_locked() - start a SETAASA procedure (Set All Addresses to Static Address)
> + * @master: I3C master object
> + *
> + * Send a SETAASA CCC command to set all attached I3C devices' dynamic addresses to
> + * their static address.
> + *
> + * This function must be called with the bus lock held in write mode.
> + *
> + * First, the SETHID CCC command is sent, followed by the SETAASA CCC.
> + *
> + * Return: 0 in case of success, a positive I3C error code if the error is
> + * one of the official Mx error codes, and a negative error code otherwise.
> + */
> +static int i3c_master_setaasa_locked(struct i3c_master_controller *master)
> +{
> +	struct i3c_ccc_cmd_dest dest;
> +	struct i3c_ccc_cmd cmd;
> +	int ret;
> +
> +	/*
> +	 * Send SETHID CCC command. Though it is a standard CCC command specified
> +	 * in JESD300-5, we are not defining a separate macro to be explicit that
> +	 * the value falls under the vendor specific range.
> +	 */
> +	i3c_ccc_cmd_dest_init(&dest, I3C_BROADCAST_ADDR, 0);
> +	i3c_ccc_cmd_init(&cmd, false, I3C_CCC_VENDOR(0, true), &dest, 1);

Does this violate the MIPI I3C specification? The command is initialized
with a 0-length payload. The specification mandates that Vendor Extension
CCCs must include a defining byte (Vendor ID) in their payload when
broadcast.

Could broadcasting 0x61 without an identifier cause non-JEDEC devices to
misinterpret the command as their own vendor-specific extension?

> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +	if (ret)
> +		return ret;

Should this code handle the I3C_ERROR_M2 case? If no active devices
acknowledge the frame, the hardware controller returns an I3C_ERROR_M2
error.

Unlike RSTDAA and ENTDAA which explicitly suppress this error, propagating
it back to i3c_master_bus_init() causes the initialization to abort and
tears down all I2C and I3C devices on the bus.

> +
> +	/* Send SETAASA CCC command */
> +	i3c_ccc_cmd_dest_init(&dest, I3C_BROADCAST_ADDR, 0);
> +	i3c_ccc_cmd_init(&cmd, false, I3C_CCC_SETAASA, &dest, 1);
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);
> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +
> +	return ret;
> +}

[ ... ]

> @@ -1739,6 +1781,18 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
>  	if (ret)
>  		goto err_free_dev;
>  
> +	/*
> +	 * For devices using SETAASA instead of ENTDAA, the address is statically
> +	 * assigned. Update the dynamic address to the provided static address.
> +	 * Reattaching the I3C device is not useful. It is also not mandatory
> +	 * for such devices to implement CCC commands like GETPID, GETDCR etc.
> +	 * Hence, we can return here.
> +	 */
> +	if (i3cdev->boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		i3cdev->info.dyn_addr = i3cdev->boardinfo->static_addr;
> +		return 0;
> +	}

By returning early, this bypasses the i3c_master_reattach_i3c_dev() call.
Does this leave the hardware state desynchronized? The dynamic address slot
is never marked as I3C_ADDR_SLOT_I3C_DEV in master->bus.addrslots, which
could lead to bus collisions during ENTDAA or Hot-Join.

Furthermore, the hardware master driver's .reattach_i3c_dev() callback is
skipped, meaning the physical controller's Device Address Table is never
updated and subsequent private transfers might fail.

Does skipping i3c_master_retrieve_dev_info() leave structural bounds like
max_read_len and max_write_len initialized to zero? Even if standard CCCs
are optional for these devices, downstream drivers might expect these fields
to be populated or have safe default values.

Also, if a user provides an assigned-address (init_dyn_addr) that differs
from the static_addr, this early return seems to leak the reserved address
slot. The init_dyn_addr slot is reserved earlier in i3c_master_bus_init(),
but this path never clears or updates it.

> +
>  	ret = i3c_master_setdasa_locked(master, i3cdev->info.static_addr,
>  					i3cdev->boardinfo->init_dyn_addr);
>  	if (ret)

[ ... ]

> @@ -2145,6 +2199,12 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  	if (ret)
>  		goto err_bus_cleanup;
>  
> +	if (master->addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		ret = i3c_master_setaasa_locked(master);
> +		if (ret)
> +			goto err_bus_cleanup;
> +	}

Could broadcasting SETAASA before issuing targeted SETDASA commands break
devices intended for SETDASA?

If a device supports both methods, the initial SETAASA broadcast will cause
it to adopt its static address immediately. The subsequent SETDASA command
meant to assign a specific software-configured dynamic address would then
fail or be ignored.

> +
>  	/*
>  	 * Reserve init_dyn_addr first, and then try to pre-assign dynamic
>  	 * address and retrieve device information if needed.

Since SETAASA forces the dynamic address to equal the static address,
devices using this method might omit the optional assigned-address property.

If assigned-address is omitted, init_dyn_addr is 0. Looking at the loop
immediately following this code:

	list_for_each_entry(i3cboardinfo, &master->boardinfo.i3c, node) {
		if (!i3cboardinfo->init_dyn_addr)
			continue;

Does this cause SETAASA devices without an assigned-address to be completely
skipped and never initialized during the static device setup?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260423085718.70762-1-akhilrajeev@nvidia.com?part=5