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

[sashiko-bot@kernel.org]

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 <claudiu.beznea.uj@bp.renesas.com>

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?

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

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

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?

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?

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.

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

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;

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

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