Re: [PATCH v3 04/17] i3c: renesas: Reconfigure the DATBAS register on re-attach

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The reattach_i3c_dev function unconditionally moves devices to a different free index on every call, causing endless index bouncing, array fragmentation, and unnecessary DATBAS register rewrites, which likely breaks Dynamic Address Assignment (DAA).
--

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

i3c: renesas: Reconfigure the DATBAS register on re-attach

This commit reconfigures the DATBAS register during device re-attach when a
device changes its position in the i3c->addrs[] array. It also updates the
software caches.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 4c86e72578043..76a4831098c94 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -892,10 +892,26 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>  	struct renesas_i3c *i3c = to_renesas_i3c(m);
>  	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
> +	int pos;
> +
> +	pos = renesas_i3c_get_free_pos(i3c);
> +
> +	if (data->index != pos && pos >= 0) {

[Severity: High]
Does this logic unconditionally move devices to a different index on every
reattach call?

Because the device currently occupies data->index, that index's bit is cleared
in i3c->free_pos (meaning it is not free). Therefore, the lowest free slot
(pos) returned by renesas_i3c_get_free_pos() can never equal the currently
occupied slot (data->index).

As a result, this condition unconditionally evaluates to true on every
reattach call as long as a free slot exists. This forces the device to vacate
its current hardware index, move to the new position, and rewrite its DATBAS
registers, resulting in perpetual ping-ponging of slots instead of compacting
the array.

Furthermore, this fragmentation leaves lower indices free while upper ones are
occupied. During renesas_i3c_daa(), the controller calculates the number of
expected DAA devices as maxdevs - ret (where ret is the lowest free index).
If there are fragmented active devices at higher indices, will the hardware's
sequential DAA assignment overwrite their DATBAS configuration and lead to
functional failures?

> +		renesas_writel(i3c->regs, DATBAS(data->index), 0);
> +		i3c->addrs[data->index] = 0;
> +		i3c->free_pos |= BIT(data->index);
> +
> +		data->index = pos;
> +		i3c->free_pos &= ~BIT(data->index);
> +	}

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

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