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

On Tue, 21 Jul 2026 04:30:58 +0000, Sashiko AI review <[email protected]> wrote:
> commit e158b576a9f733695d868ee16f34bacdb43285b3
> Author: Akhil R <[email protected]>
>
> 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;
> >  }
> >  
> > +static int i3c_master_setaasa_locked(struct i3c_master_controller *master)
> > +{
> > +	struct i3c_ccc_cmd_dest dest;
> > +	struct i3c_ccc_cmd cmd;
> > +	int ret;
> > +
> > +	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?

SETAASA/SETHID are only issued when a device opts in to the SETAASA method
(i3c_master_setaasa_locked() is called solely when addr_method has
I3C_ADDR_METHOD_SETAASA, and SETHID is sent from within it). Existing
drivers are unaffected because SETAASA is a new capability they do not yet
advertise; they can whitelist these CCCs when they add SETAASA support,
which will have to be updated in the individual drivers. This patchset
does that for the DesignWare contoller driver.

> [ ... ]
>
> > @@ -1878,6 +1924,22 @@ static int i3c_master_early_i3c_dev_add(struct i3c_master_controller *master,
> >  	if (ret)
> >  		goto err_free_dev;
> >  
> > +	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.

Address conflicts between statically-addressed devices are rejected up
front in i3c_master_get_i3c_addrs(), so it should not cause any address
collision with other devices on the bus unless the same address is being
assigned through ENTDAA. If this is a concern, we can avoid freeing the
static address for SETAASA devices even if software attachment fails.

Let me know if this needs to be done.

> [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?

Pre-existing; not introduced by this patch. Better addressed separately.

> > +
> > +		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,
> > +	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.

Static addressing requires each device to have a distinct static address on
the bus, and PID-less devices are named from that unique static address
("%d-%02x"). The bus address-slot management also prevents two live devices
from occupying the same address. So a collision would require a malformed
description with duplicate static addresses, which is not a valid bus.

Best Regards,
Akhil