Re: [PATCH v6 04/12] i3c: master: Add support for devices using SETAASA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] State inconsistency on error path: a SETAASA device failing software attachment will still claim its static address when the SETAASA broadcast is sent, leading to an address collision.
- [High] Multiple SETAASA devices without specified PIDs will cause a kobject name collision during `device_register()`, preventing registration of subsequent devices.
- [High] The newly introduced SETAASA and SETHID (vendor) CCC commands are not whitelisted in the majority of I3C master controller drivers, causing the entire bus initialization to fail.

Pre-existing issues:
- [High] Preexisting SETDASA Hardware/Software state mismatch on failure due to premature slot freeing before hardware reset.
--

--- Patch [4]: [PATCH v6 04/12] i3c: master: Add support for devices using SETAASA ---
commit e158b576a9f733695d868ee16f34bacdb43285b3
Author: Akhil R <akhilrajeev@nvidia.com>

i3c: master: Add support for devices using SETAASA

This commit adds support for discovering I3C devices using the SETAASA
procedure, such as SPD5118 and SPD5108 attached to DDR5 memory modules. It
adds the SETAASA and SETHID CCC commands and updates the discovery logic.

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

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 623c6b2247d9f..b18dda89c4737 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c

[ ... ]

> @@ -1102,6 +1103,51 @@ 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)

[ ... ]

> +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);
> +	ret = i3c_master_send_ccc_cmd_locked(master, &cmd);

[Severity: High]
Will this fail on most existing I3C master controllers?

Looking at i3c_master_send_ccc_cmd_locked(), it calls
master->ops->supports_ccc_cmd() before sending. Many core controller drivers
(like dw-i3c-master.c) have strict switch-statement whitelists for supported
CCC IDs that do not include these new SETAASA (0x29) and SETHID (0x61)
commands.

Would this result in returning -EOPNOTSUPP and aborting the entire bus
initialization on systems with these controllers?

> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +	if (ret && cmd.err == I3C_ERROR_M2)
> +		ret = 0;
> +	if (ret)
> +		return ret;

[ ... ]

> @@ -1878,6 +1924,22 @@ 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.
> +	 * Reattach the I3C device after updating the dynamic address with the same
> +	 * static address. It is not mandatory for such devices to implement CCC
> +	 * commands like GETPID, GETDCR etc. Hence, we can return after reattaching.
> +	 */
> +	if (i3cdev->boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		i3cdev->info.dyn_addr = i3cdev->boardinfo->static_addr;
> +		ret = i3c_master_reattach_i3c_dev_locked(i3cdev, 0);
> +		if (ret)
> +			goto err_rstdaa;

[Severity: High]
If i3c_master_reattach_i3c_dev_locked() fails here, we jump to err_rstdaa
and eventually free the device structure. However, i3c_master_bus_init()
later unconditionally broadcasts SETAASA to the bus.

Won't the physical device claim this static address during the broadcast,
even though the controller software no longer tracks it? This could lead to
a bus collision when the software later tries to dynamically assign this
"free" address slot to a different device.

[Severity: High]
This is a pre-existing issue, but does jumping to err_rstdaa correctly reset
the hardware if reattachment fails during SETDASA or SETAASA?

If i3c_master_reattach_i3c_dev_locked() fails, its internal error path calls
i3c_master_put_i3c_addrs(), which marks the slot as I3C_ADDR_SLOT_FREE.

When we reach the err_rstdaa path:

err_rstdaa:
    i3c_master_rstdaa_locked(master, i3cdev->boardinfo->init_dyn_addr);

i3c_master_rstdaa_locked() checks i3c_bus_get_addr_slot_status(). Because the
slot is already marked FREE, it returns -EINVAL and silently aborts without
sending the RSTDAA command to the hardware.

Does this leave the physical device holding the address while the software
considers it free?

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

[ ... ]

> @@ -2741,7 +2816,19 @@ i3c_master_add_i3c_boardinfo(struct i3c_master_controller *master,
>  
>  	boardinfo->static_addr = reg[0];
>  
> +	if (!fwnode_property_read_u32(fwnode, "mipi-i3c-static-method", &static_addr_method))
> +		boardinfo->static_addr_method = static_addr_method &
> +					(I3C_ADDR_METHOD_SETDASA | I3C_ADDR_METHOD_SETAASA);
> +

[ ... ]

> +	if (boardinfo->static_addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		/* For SETAASA, static address is taken as the dynamic address. */
> +		init_dyn_addr = boardinfo->static_addr;
> +	}
> +
> +	/* Update the address methods required for device discovery */
> +	master->addr_method |= boardinfo->static_addr_method;
> +
>  	boardinfo->pid = ((u64)reg[1] << 32) | reg[2];
>  
>  	if ((boardinfo->pid & GENMASK_ULL(63, 48)) ||

[Severity: High]
By relaxing the PID validation for SETAASA devices in this function, pid can
default to 0. If a system configures multiple SETAASA devices (like DDR5
SPDs) without PIDs, will this cause a kobject name collision?

During registration, i3c_master_register_new_i3c_devs() names the device
using its bus ID and PID:

dev_set_name(&desc->dev->dev, "%d-%llx", master->bus.id, desc->info.pid);

If multiple devices have pid=0, they would all be named "0-0", causing
device_register() to fail with -EEXIST for subsequent devices and preventing
them from functioning.

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