Re: [PATCH v5 11/17] i3c: renesas: Follow a unified pattern for transfer and command initialization

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `renesas_i3c_i3c_xfers` function completely ignores the return status of individual I3C transfers, unconditionally returning success even on hardware failures or timeouts.
- [High] The `renesas_i3c_i2c_xfers` function ignores timeouts from `wait_for_completion_timeout()`, leading to hardware desynchronization, silent failures, and a race condition.
--

--- Patch [11]: [PATCH v5 11/17] i3c: renesas: Follow a unified pattern for transfer and command initialization ---
commit 0fa956e2c2bc3ba154fb16507f16f40d18d2d2db
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

    i3c: renesas: Follow a unified pattern for transfer and command initialization
    
    Follow a unified pattern for transfer and command initialization across
    the driver. This keeps the code cleaner and easier to follow. Also, in
    some cases the I3C device was enabled before the transfer data structure
    was even allocated.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 9a36132200342..c1396c49f45e5 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -835,15 +835,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 does this function properly
propagate hardware errors?

Looking further down in renesas_i3c_i3c_xfers(), the transfer status is
captured when calling renesas_i3c_wait_xfer():

    renesas_i3c_wait_xfer(i3c, xfer);

However, xfer->ret is completely ignored, and the function unconditionally
returns 0. Hardware I3C bus errors (like NACK, parity errors, CRC failures,
or timeouts) appear to be completely masked from the I3C subsystem and client
drivers.

Can this lead to upper layers assuming write commands succeeded when they
failed, or read commands returning valid data when the buffer actually
contains uninitialized memory?

[ ... ]
> @@ -964,12 +964,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 are timeouts handled safely here?

Further down in renesas_i3c_i2c_xfers(), the function waits for completion:

    wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
    
    if (cmd->err)
        break;

The return value of wait_for_completion_timeout() (which is 0 on timeout) is
not checked, and cmd->err is typically 0 on a timeout. This causes the driver
to proceed with the next transfer erroneously.

If the driver immediately starts the next transfer by modifying the shared cmd
structure in-place while the previous transfer is still enqueued, could a
delayed interrupt from the timed-out transfer access the repurposed cmd
structure?

This seems like it could cause memory corruption or an unpredictable hardware
state.

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

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