Re: [PATCH v4 06/16] i3c: renesas: Perform Dynamic Address Assignment on resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `renesas_i3c_group_devs_in_slots()` crashes due to NULL pointer dereference when swapping an I2C device into a previously empty slot.
- [High] Slot state corruption and hardware configuration overwrite during device grouping.
- [High] Build failure and state corruption due to calling undefined function `i3c_master_reattach_i3c_dev_locked()` and bypassing PID matching.
--

commit e2b35e6a6eb1e4e88dd575a2f0b9fe6f7bf5f7c5
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Perform Dynamic Address Assignment on resume

The Renesas RZ/G3S SoC supports a power saving mode where power to most
SoC components, including I3C, is turned off. Running DAA in the controller
resume path restores communication for devices that also lose power.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 67b9a31300d09..a63edddbc9fe0 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -637,12 +646,80 @@ static void renesas_i3c_bus_cleanup(struct i3c_master_controller *m)
>  	renesas_i3c_reset(i3c);
>  }
>  
> +static u8 renesas_i3c_group_devs_in_slots(struct renesas_i3c *i3c)
> +{
> +	struct renesas_i3c_i2c_dev_data *new_i2c_data, *new_i3c_data;
> +	struct renesas_i3c_addr i2c_addr, i3c_addr;
> +	struct i3c_dev_desc *i3c_dev;
> +	struct i2c_dev_desc *i2c_dev;
> +	u8 i2c_pos = 0;

[ ... ]

> +	for (u8 pos = 0; pos < i3c->maxdevs; pos++) {
> +		if (i3c->free_pos & BIT(pos))
> +			continue;
> +
> +		if (!i3c->addrs[pos].is_i2c)
> +			continue;
> +
> +		if (pos == i2c_pos) {
> +			i2c_pos++;
> +			continue;
> +		}
> +
> +		/*
> +		 * Swap the I3C device on i2c_pos slot with the I2C dev
> +		 * on pos slot.
> +		 */
> +		i3c_addr = i3c->addrs[i2c_pos];
> +		i3c_dev = i3c_addr.i3c_dev;
> +
> +		i2c_addr = i3c->addrs[pos];
> +		i2c_dev = i2c_addr.i2c_dev;
> +
> +		new_i2c_data = i3c_dev_get_master_data(i3c_dev);

[Severity: High]
Does this crash when swapping an I2C device into a previously empty slot?

When i2c_pos does not advance over empty slots, i3c_dev will be NULL if
the destination slot is empty. Since i3c_dev_get_master_data() unconditionally
dereferences the pointer (dev->common.master_priv), it seems this would cause
a kernel panic when swapping into an empty slot.

> +		new_i3c_data = i2c_dev_get_master_data(i2c_dev);
> +
> +		i3c->addrs[i2c_pos] = i2c_addr;
> +		i3c->addrs[i2c_pos].is_i2c = true;
> +		i3c->addrs[i2c_pos].i2c_dev = i2c_dev;
> +
> +		i3c->addrs[pos] = i3c_addr;
> +		i3c->addrs[pos].is_i2c = false;
> +		i3c->addrs[pos].i3c_dev = i3c_dev;
> +
> +		new_i2c_data->index = i2c_pos++;
> +		new_i3c_data->index = pos;
> +
> +		i2c_dev_set_master_data(i2c_dev, new_i2c_data);
> +		i3c_dev_set_master_data(i3c_dev, new_i3c_data);
> +	}

[ ... ]

> @@ -655,16 +732,25 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>  	olddevs = ~(i3c->free_pos);
>  	i3c->internal_state = I3C_INTERNAL_STATE_CONTROLLER_ENTDAA;
>  
> +	if (i3c->resuming)
> +		first_i3c_pos = renesas_i3c_group_devs_in_slots(i3c);
> +
>  	/* Setting DATBASn registers for target devices. */
>  	for (pos = 0; pos < i3c->maxdevs; pos++) {
> -		if (olddevs & BIT(pos))
> -			continue;
> +		if (olddevs & BIT(pos)) {

[Severity: High]
Does this clobber the configuration of I2C devices swapped into empty slots?

Since renesas_i3c_group_devs_in_slots() does not update i3c->free_pos during
a swap, the olddevs mask calculated before the grouping is now stale.

If an I2C device was moved to a previously empty slot, olddevs & BIT(pos) will
evaluate to false. This bypasses the skip logic, treats the newly moved I2C
device as an unassigned slot, and generates a new dynamic address for it,
blindly overwriting its DATBAS hardware configuration.

[ ... ]

> @@ -689,12 +780,38 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>  
>  	renesas_i3c_wait_xfer(i3c, xfer);
>  
> -	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> -	newdevs &= ~olddevs;
> +	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
> +	/* Re-attach all the I3C devices on resume. */
> +	if (!i3c->resuming)
> +		newdevs &= ~olddevs;
>  
>  	for (pos = 0; pos < i3c->maxdevs; pos++) {
> -		if (newdevs & BIT(pos))
> -			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos]);
> +		if (!(newdevs & BIT(pos)))
> +			continue;
> +
> +		/*
> +		 * If the bus was fully occupied before suspend, re-attach the
> +		 * devices and update the addresses tracked by the subsystem.
> +		 * Without this, i3c_master_add_i3c_dev_locked() returns errors
> +		 * due to a lack of free slots (as reported by
> +		 * renesas_i3c_get_free_pos()), causing the attachment to fail.
> +		 *
> +		 * Otherwise, follow the normal
> +		 * i3c_master_add_i3c_dev_locked() path, which will re-attach the
> +		 * devices, keep the subsystem state and driver addresses in sync,
> +		 * and perform all the required internal bookkeeping.
> +		 */
> +		if (!i3c->free_pos && i3c->resuming) {
> +			struct i3c_dev_desc *dev = i3c->addrs[pos].i3c_dev;
> +			u8 old_dyn_addr;
> +
> +			old_dyn_addr = dev->info.dyn_addr;
> +			dev->info.dyn_addr = i3c->addrs[pos].addr;
> +
> +			i3c_master_reattach_i3c_dev_locked(dev, old_dyn_addr);

[Severity: High]
This calls i3c_master_reattach_i3c_dev_locked() which does not appear to exist
in the kernel, causing a build failure.

Also, does this logic cross-wire physical devices with the wrong software
structures?

It blindly assigns newly generated dynamic addresses to the pre-suspend
device structures based solely on their slot index (pos). However, ENTDAA
assigns addresses based on Provisioned ID (PID) arbitration order, not the
previous slot indices. If the bus topology changes or devices arbitrate in a
different order, commands might be sent to the incorrect targets, potentially
causing data corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612160458.3102106-1-claudiu.beznea@kernel.org?part=6

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c