Re: [PATCH v5 10/17] i3c: renesas: Return immediately if there is no transfer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver ignores the return value of wait_for_completion_timeout() during I2C transfers, leading to data races and hardware state corruption.
- [High] The driver unconditionally returns success in I3C transfers regardless of hardware errors or timeouts.
- [High] A timeout during Dynamic Address Assignment (DAA) causes the driver to incorrectly register phantom devices.
- [High] Concurrent I3C and I2C transfers can clobber hardware registers due to missing driver-level serialization.
--

--- Patch [10]: [PATCH v5 10/17] i3c: renesas: Return immediately if there is no transfer ---
commit 10f5d1cd5ccdca9a32d38cbec0d2a0b8846661ac
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Return immediately if there is no transfer

This commit moves the check for zero transfers before allocating the transfer
structure. This avoids an unnecessary memory allocation and cleanup when there
are no transfers to process.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 915090d0ad37e..9a36132200342 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -957,13 +957,13 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>  	u8 start_bit = CNDCTL_STCND;
>  	int i;
>  
> +	if (!i2c_nxfers)
> +		return 0;
> +
>  	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
>  	if (!xfer)
>  		return -ENOMEM;
>  
> -	if (!i2c_nxfers)
> -		return 0;
> -
>  	renesas_i3c_bus_enable(m, false);
>  
>  	init_completion(&xfer->comp);

[Severity: High]
This is a pre-existing issue, but since you are modifying renesas_i3c_i2c_xfers(),
does this function fail to handle timeouts correctly? 

If wait_for_completion_timeout() times out, its return value is discarded, and
cmd->err is likely 0:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
        ...
		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);

		if (cmd->err)
			break;
        ...
}

Will this allow the loop to proceed to the next I2C message and immediately
clobber hardware registers for a new command while the hardware might still be
processing the timed-out command?


[Severity: High]
This is a pre-existing issue, but does renesas_i3c_i2c_xfers() also lack proper
driver-level serialization?

The function enqueues the transfer but immediately starts modifying hardware
registers (like CNDCTL) without waiting for its turn in the queue:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
        ...
		renesas_i3c_enqueue_xfer(i3c, xfer);

	for (i = 0; i < i2c_nxfers; i++) {
		...
		/* Issue Start condition */
		renesas_set_bit(i3c->regs, CNDCTL, start_bit);
        ...
}

Because the I3C core relies on i3c_bus_normaluse_lock() (a read lock), multiple
transfers can be requested concurrently. Can concurrent I2C and I3C transfers
clobber hardware registers?


[Severity: High]
This is a pre-existing issue, but the same lack of serialization appears to
exist in renesas_i3c_i3c_xfers():

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
        ...
		if (!i3c_xfers[i].rnw && i3c_xfers[i].len > 4) {
			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
			if (cmd->len > NTDTBP0_DEPTH * sizeof(u32))
				renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
		}

		renesas_i3c_wait_xfer(i3c, xfer);
        ...
}

Since the function unconditionally writes to the hardware FIFO before calling
enqueue/wait, won't this destroy the active transfer if another thread is
currently transmitting?


[Severity: High]
This is a pre-existing issue, but does renesas_i3c_i3c_xfers() unconditionally
return success?

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
        ...
		renesas_i3c_wait_xfer(i3c, xfer);
	}

	return 0;
}

The xfer->ret value populated by renesas_i3c_wait_xfer() is never checked. Will
this lead to silent failures where hardware errors (like NACK or timeouts) are
ignored?


[Severity: High]
This is a pre-existing issue, but does renesas_i3c_daa() incorrectly register
phantom devices if a timeout occurs during Dynamic Address Assignment?

If the ENTDAA command times out, cmd->rx_count remains 0 and the timeout error
in xfer->ret is ignored:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
        ...
		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);

	newdevs &= ~olddevs;

	for (pos = 0; pos < i3c->maxdevs; pos++) {
		if (newdevs & BIT(pos))
			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos]);
        ...
}

Because cmd->rx_count is 0, GENMASK evaluates to all bits. Will this cause the
driver to loop over this mask and register phantom I3C devices on the bus for
every unused address slot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=10

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