Re: [PATCH v4 10/16] i3c: renesas: Follow a unified pattern for transfer and command initialization

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The `renesas_i3c_i3c_xfers` function silently ignores all transfer errors and timeouts, always returning success.
- [High] The driver does not abort or reset the underlying hardware controller when an I2C or I3C transfer times out. This leaves the hardware active in the background, which will corrupt subsequent transfers.
- [High] In `renesas_i3c_i2c_xfers`, the return value of `wait_for_completion_timeout` is ignored, leading to erroneous state progression and reporting success when transfers actually timed out.
- [High] The driver unconditionally terminates every I3C private transfer with a STOP condition, failing to support atomic transactions that require Repeated Start (Sr).
- [High] I3C write transfers larger than 64 bytes will overflow the hardware FIFO and repeatedly corrupt data.
- [High] I3C read transfers larger than 64 bytes will overwrite the beginning of the receive buffer.
- [High] Dynamic Address Assignment (DAA) ignores transfer failures and timeouts.
--

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

i3c: renesas: Follow a unified pattern for transfer and command initialization

This patch unifies the pattern for transfer and command initialization
across the renesas i3c driver. It ensures the transfer data structure is
allocated, completion initialized, and command properties set before enabling
the I3C bus.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index fa9c15381ed6a..883be4326e430 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -726,6 +726,10 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>  	if (!xfer)
>  		return -ENOMEM;
>  
> +	init_completion(&xfer->comp);
> +	cmd = xfer->cmds;
> +	cmd->rx_count = 0;
> +
>  	/* Enable I3C bus. */
>  	renesas_i3c_bus_enable(m, true);

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
renesas_i3c_daa():

    time_left = renesas_i3c_wait_xfer(i3c, xfer);
    if (!time_left)
        renesas_i3c_irqs_mask_and_clear(i3c);

    newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);

The result of the ENTDAA transfer is unverified before trusting
cmd->rx_count. If the assignment command fails or times out, does the
driver blindly proceed to calculate device masks using a stale or zero
rx_count? Could this lead to incorrect device attachment logic and
state desynchronization?

[ ... ]

> @@ -934,15 +934,15 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
>  	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
>  	int i;
>  
> -	/* Enable I3C bus. */
> -	renesas_i3c_bus_enable(m, true);
> -
>  	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
>  	if (!xfer)
>  		return -ENOMEM;
>  
>  	init_completion(&xfer->comp);
>  
> +	/* Enable I3C bus. */
> +	renesas_i3c_bus_enable(m, true);
> +
>  	for (i = 0; i < i3c_nxfers; i++) {
>  		struct renesas_i3c_cmd *cmd = xfer->cmds;

[Severity: High]
This isn't a bug introduced by this patch, but looking at the loop in
renesas_i3c_i3c_xfers() where it sets up the command descriptor:

    cmd->cmd0 = NCMDQP_DEV_INDEX(data->index) | NCMDQP_MODE(0) |
                NCMDQP_RNW(cmd->rnw) | NCMDQP_ROC | NCMDQP_TOC;

Does setting NCMDQP_TOC unconditionally inside the loop break support for
Repeated Start (Sr) sequences? By forcing a STOP condition after each
message, doesn't this prevent the driver from executing atomic
transactions?

[Severity: Critical]
This is also a pre-existing issue, but further down in
renesas_i3c_i3c_xfers(), the function ignores the transfer result:

    if (xfer_failed)
        renesas_i3c_irqs_mask_and_clear(i3c);

    return 0;

Is it intentional to silently ignore all transfer errors and timeouts here?
Because the function always returns 0, upper layers will assume writes
succeeded and might process uninitialized or corrupted read buffers.

[Severity: High]
Another pre-existing issue in this area: does this function properly handle
transfers larger than 64 bytes?

Looking at the write path, the code pushes cmd->len bytes regardless of the
NTDTBP0_DEPTH limit (16 words). Additionally, in renesas_i3c_tx_isr(), the
TX interrupt repeatedly writes the entire cmd->len buffer from the base
address instead of advancing the pointer:

    i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);

Will this reliably overflow the hardware FIFO and repeatedly corrupt data?

[Severity: High]
Similarly for the read path, this is a pre-existing issue, but does the
driver overwrite the beginning of the receive buffer for large reads?

In renesas_i3c_rx_isr() and renesas_i3c_resp_isr(), data is read using:

    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
    cmd->rx_count = read_bytes;

Because cmd->rx_buf is not offset by the bytes already read, and
cmd->rx_count is overwritten instead of accumulated, won't subsequent
interrupt reads write into the very beginning of the user's buffer?

[ ... ]

> @@ -1068,12 +1068,12 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>  	if (!xfer)
>  		return -ENOMEM;
>  
> -	renesas_i3c_bus_enable(m, false);
> -
>  	init_completion(&xfer->comp);
>  	xfer->is_i2c_xfer = true;
>  	cmd = xfer->cmds;
>  
> +	renesas_i3c_bus_enable(m, false);
> +
>  	if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
>  		cmd->err = -EBUSY;
>  		return cmd->err;

[Severity: High]
This is a pre-existing issue, but looking at the timeout handling in
renesas_i3c_i2c_xfers():

    time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
    if (!time_left)
        xfer_failed = true;

    if (cmd->err)
        break;

If the transfer times out, time_left is 0 but cmd->err remains 0. Does this
cause the loop to continue and overwrite the cmd state while the hardware
might still be stuck, eventually returning success (0)?

[Severity: High]
This is another pre-existing issue related to timeouts. When
wait_for_completion_timeout() times out here, the software removes the xfer
object from the queue and masks interrupts in
renesas_i3c_irqs_mask_and_clear():

    renesas_writel(i3c->regs, BIE, 0);
    renesas_writel(i3c->regs, NTIE, 0);

However, it does not send an abort command or reset the hardware.
If a new transfer is initiated, the hardware continues operation in the
background. When the slow hardware eventually completes the original
timed-out transfer, it triggers an interrupt. Will renesas_i3c_resp_isr()
then fetch the new i3c->xferqueue.cur and blindly apply the old transfer's
completion status and read data into the new transfer's buffers, causing
cross-transfer memory corruption?

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

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