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

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the return value of `wait_for_completion_timeout()` is ignored in `renesas_i3c_i2c_xfers()`, leading to falsely reporting success on timeouts and continuing to issue subsequent messages to a hung bus.
- [High] This problem wasn't introduced by this patch, but transfer completion status (`xfer->ret`) is completely ignored in `renesas_i3c_i3c_xfers()`, causing all I3C private transfers to unconditionally report success even if they fail or time out.
--

commit c9a5059d97052e39bcba94fdb82d462a309a98d2
Author: Claudiu Beznea <[email protected]>

    i3c: renesas: Return immediately if there is no transfer
    
    There is no need to allocate a transfer structure when i2c_nxfers is zero.
    Return immediately instead of unnecessarily allocating memory.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 4a055c9f4ffea..1fd39dd578474 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1066,13 +1066,13 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>  	u8 start_bit = CNDCTL_STCND;
>  	int i;
>  
> +	if (!i2c_nxfers)
> +		return 0;
> +

[Severity: High]
This isn't a bug introduced by this patch, but the return value of
wait_for_completion_timeout() is ignored later in renesas_i3c_i2c_xfers():

		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);

		if (cmd->err)
			break;

When wait_for_completion_timeout() returns 0 (indicating a timeout), there
is no check to set cmd->err. Since cmd->err remains 0, the loop incorrectly
continues to issue start conditions for remaining messages on the timed-out
bus.

Finally, the function returns cmd->err (0), falsely reporting success to
the I2C subsystem. Does this silently hide I2C timeouts from caller drivers?

[Severity: High]
This isn't a bug introduced by this patch, but transfer completion status
is completely ignored in renesas_i3c_i3c_xfers() in the same file:

		renesas_i3c_wait_xfer(i3c, xfer);
	}

	return 0;

The function renesas_i3c_wait_xfer() records completion status (e.g., -EIO,
-ENOSPC, -ETIMEDOUT) in xfer->ret. However, the loop never inspects
xfer->ret and unconditionally returns 0 at the end.

Could this silently hide NACKs, parity errors, and timeouts from I3C target
drivers, breaking the subsystem's error handling and leading to false
assumptions about data validity?

>  	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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c