Re: [PATCH v3 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume

On 6/8/26 23:36, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> 
> New issues:
> - [Critical] `renesas_i3c_group_devs_in_slots()` crashes on free slots and fails to update the `free_pos` bitmask.
> - [Critical] Typo in array swapping logic duplicates I2C devices and corrupts driver state.
> - [High] Call to undefined function `i3c_master_reattach_i3c_dev_locked` and logically invalid usage of the reattach API.
> - [High] Bypassing `i3c_master_add_i3c_dev_locked()` on resume ignores UID arbitration order, breaking device isolation.
> - [High] Redundant and unsafe dereference of `->dev->desc` when retrieving master data.
> - [High] Stale `olddevs` mask corrupts DATBAS registers for relocated I2C devices.
> 
> Pre-existing issues:
> - [High] Integer underflow in `GENMASK` calculation in `renesas_i3c_daa()` causes undefined behavior.
> --
> 
> commit 3b3dbe672a58e6c80b4a3c7aad942c61076f9cd2
> Author: Claudiu Beznea <[email protected]>
> 
> i3c: renesas: Perform Dynamic Address Assignment on resume
> 
> The patch modifies the Renesas I3C driver to run Dynamic Address Assignment
> (DAA) in the controller resume path to restore communication with I3C devices
> that lose power during suspend. It groups I2C and I3C devices during resume
> and re-runs ENTDAA for I3C devices.
> 
>> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
>> --- a/drivers/i3c/master/renesas-i3c.c
>> +++ b/drivers/i3c/master/renesas-i3c.c
> 
> [ ... ]
> 
>> +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];
> 
> [Severity: Critical]
> If i2c_pos points to a free slot because earlier iterations skipped free
> slots without advancing i2c_pos, won't i3c_dev be NULL?

Since, as of my code inspection, the detach could happen only though a 
re-attachment procedure, this should lead to continuous slots allocation in 
free_pos. Thus, at the point this code is executed there should be no gaps in 
free_pos, unless I'm missing something.

> 
> Furthermore, does this function need to update i3c->free_pos when swapping
> slots to ensure the bitmask stays synchronized with the array layout?

Since, as of my code inspection, the detach could happen only though a 
re-attachment procedure, this should lead to continuous slots allocation in 
free_pos. Thus, at the point this code is executed there should be no gaps in 
free_pos, swapping positions in i3c->addrs[] wouldn't need updating free_pos.

> 
>> +		i2c_dev = i2c_addr.i2c_dev;
>> +
>> +		new_i2c_data = i3c_dev_get_master_data(i3c_dev->dev->desc);
> 
> [Severity: High]
> Is the ->dev->desc dereference here safe?

At the point this code is executed the devices should have already been
properly allocated.

> Since i3c_dev is already a struct i3c_dev_desc *, the dev field might
> be NULL if the device was only partially attached or memory allocation failed,
> causing a crash. i3c_dev_get_master_data() can accept i3c_dev directly.
> 
>> +		new_i3c_data = i2c_dev_get_master_data(i2c_dev);
>> +
>> +		i3c->addrs[i2c_pos] = i3c->addrs[pos];
>> +		i3c->addrs[i2c_pos].is_i2c = true;
>> +		i3c->addrs[i2c_pos].i2c_dev = i2c_dev;
>> +
>> +		i3c->addrs[pos] = i2c_addr;
> 
> [Severity: Critical]
> Since i2c_addr was initialized to i3c->addrs[pos] above, doesn't this
> assign the entry to itself?

Indeed, this looks wrong.

> 
> This would duplicate the I2C device into both slots, destroy the original I3C
> device's dynamic address state, and cross-assign private data structures.
> Should i3c_addr be used instead?
> 
>> +		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);
> 
> [ ... ]
> 
>>   static int renesas_i3c_daa(struct i3c_master_controller *m)
>>   {
>>   	struct renesas_i3c *i3c = to_renesas_i3c(m);
>>   	struct renesas_i3c_cmd *cmd;
>>   	u32 olddevs, newdevs;
>>   	u8 last_addr = 0, pos;
>> +	u8 first_i3c_pos = 0;
>>   	int ret;
> 
> [ ... ]
> 
>> @@ -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);
> 
> [Severity: High]
> Since olddevs is computed from i3c->free_pos before calling
> renesas_i3c_group_devs_in_slots(), which shuffles the array but doesn't
> update i3c->free_pos, is it possible for olddevs to be stale?

As of the current code base, the i2c slots in free_pos should be continuous
and this should not happen.

> 
> If an I2C device is relocated into an empty slot where the stale mask is 0,
> the subsequent DAA loop might fall through and overwrite the I2C device's slot
> with a new I3C dynamic address.
> 
>> +
>>   	/* Setting DATBASn registers for target devices. */
>>   	for (pos = 0; pos < i3c->maxdevs; pos++) {
> 
> [ ... ]
> 
>> @@ -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);
> 
> [Severity: High]
> This is a pre-existing issue, but if cmd->rx_count equals i3c->maxdevs
> (e.g., when no devices are assigned), the first argument to GENMASK becomes
> -1.
> 
> Doesn't this invoke undefined behavior via an invalid shift count in the
> GENMASK macro, returning garbage values?
> 
>> +	/* 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->dev->desc;
> 
> [Severity: High]
> Is it safe to dereference i3c_dev->dev->desc here?
> 
> If dev is NULL due to partial attachment or memory allocation failure prior
> to suspend, this will cause a kernel crash.

This should not happen since at this point the devices should be properly 
attached. Moreover, using directly i3c->addrs[pos].i3c_dev doesn't
work for the case here.

> 
>> +			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]
> Does i3c_master_reattach_i3c_dev_locked() exist in the kernel?
> 
> Calling a non-existent function will result in a build failure. Additionally,
> even if this was intended to be i3c_master_reattach_i3c_dev(), that API
> sends a SETNEWDA CCC to the device's old dynamic address. Since ENTDAA just
> assigned a new dynamic address via hardware, won't the device NACK the
> command?

The API is part of the series pointed out in the cover letter of this series.

> 
> Furthermore, by bypassing i3c_master_add_i3c_dev_locked(), does this ignore
> the PID arbitration order? ENTDAA assigns addresses based on PID arbitration,
> so the device that won arbitration might not correspond to the descriptor at
> i3c->addrs[pos], breaking device isolation.
> 
>> +		} else {
>> +			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos].addr);
>> +		}
>>   	}
>>   
>>   	return 0;
> 

-- 
Thank you,
Claudiu


-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c