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

[sashiko-bot@kernel.org]

Sashiko AI review found 6 potential issue(s):
- [High] SETAASA devices are broken after a system suspend/resume or hibernation because the SETAASA command is not re-issued.
- [High] SETAASA devices are silently ignored and never discovered if the `assigned-address` property is omitted in the Device Tree.
- [High] Bus initialization aborts entirely if the SETAASA command receives a NACK (returns I3C_ERROR_M2).
- [High] Failure or lack of support for the optional vendor SETHID command incorrectly aborts the mandatory SETAASA command.
- [High] The subsystem registers SETAASA devices as fully initialized even if the SETAASA broadcast command is unsupported by the master controller.
- [Medium] The `assigned-address` slot is leaked for SETAASA devices if it differs from the static address.
--

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

i3c: master: Add support for devices using SETAASA

This commit adds support for discovering I3C devices using SETAASA, such as
SPD5118 and SPD5108 attached to DDR5 memory modules. It implements the
SETAASA CCC command, optionally preceded by SETHID, to assign static
addresses as dynamic addresses during dynamic address assignment.

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

> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
[ ... ]
> @@ -1049,6 +1049,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)
[ ... ]
> +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);
> +	i3c_ccc_cmd_dest_cleanup(&dest);
> +	if (ret)
> +		return ret;

If the master controller driver rejects unsupported vendor CCCs and returns
-EOPNOTSUPP, or the devices NACK the command, this early return will prevent
the mandatory SETAASA command from being broadcast.

Should the SETHID failure be tolerated so the mandatory SETAASA sequence can
still proceed?

> +
> +	/* 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;
> +}
[ ... ]
> @@ -1733,6 +1774,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;
> +	}

In i3c_master_bus_init(), if i3c_master_setaasa_locked() returns -EOPNOTSUPP,
the failure is caught, a warning is logged, and initialization proceeds.

Later, this block blindly assigns the static address as the dynamic address
and returns success, which causes the framework to allocate and register the
I3C device under the assumption it is addressable.

Does this cause subsequent I/O to the device to silently fail or time out
due to the target hardware never actually receiving the SETAASA command?

Additionally, if a SETAASA device is defined in the Device Tree with an
assigned-address that differs from its static_addr, i3c_master_bus_init()
reserves the init_dyn_addr slot by marking it as I3C_ADDR_SLOT_EXT_DESIRED.

Since this code returns early for SETAASA devices, the slot for init_dyn_addr
is never cleared or updated.

Could this permanently leak the slot in the bus's address status table?

[ ... ]
> @@ -2132,6 +2185,16 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  			goto err_bus_cleanup;
>  	}
>  
> +	if (master->addr_method & I3C_ADDR_METHOD_SETAASA) {
> +		ret = i3c_master_setaasa_locked(master);
> +		if (ret) {
> +			if (ret == -EOPNOTSUPP)
> +				dev_warn(&master->dev, "SETAASA is not supported. Some devices may not be detected.\n");
> +			else
> +				goto err_bus_cleanup;
> +		}
> +	}

Broadcast CCC commands return I3C_ERROR_M2 if no I3C devices on the bus ACK
them, which can happen if the SETAASA device is offline or the bus only
contains I2C devices.

Unlike RSTDAA and DISEC, which explicitly tolerate this normal condition by
checking for I3C_ERROR_M2, this error check does not.

Will a NACK on the SETAASA command cause a complete denial of service for
the I3C controller and all its devices by jumping to err_bus_cleanup?

Looking further down in i3c_master_bus_init(), the pre-defined devices
iteration explicitly skips entries where init_dyn_addr is zero:

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

If a SETAASA device is defined in the Device Tree without the
assigned-address property, init_dyn_addr defaults to 0, which means the loop
skips the device and i3c_master_early_i3c_dev_add() is never executed.

Since SETAASA devices do not participate in dynamic address assignment,
does this mean the subsystem will never discover or initialize them? Should
the framework automatically infer init_dyn_addr = static_addr for SETAASA
devices if assigned-address is not specified?

Finally, during a system hibernation restore, the I3C subsystem issues a
bus-wide dynamic address reset via i3c_master_do_daa_ext(master, true),
which broadcasts RSTDAA to reset all dynamic addresses. Following this, it
calls master->ops->do_daa(master) which broadcasts ENTDAA.

However, the resume path does not appear to re-broadcast the SETAASA command.

Because SETAASA devices do not participate in ENTDAA and rely exclusively on
SETAASA to adopt their static address, will they remain without a dynamic
address after resume, rendering them permanently inaccessible?

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