Re: [PATCH v3 01/17] i3c: renesas: Check that the transfer is valid before accessing it

[sashiko-bot@kernel.org]

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

New issues:
- [High] Returning early from ISRs on the `!xfer` path without disabling the hardware interrupt enables or correctly resolving the interrupt conditions causes an interrupt storm (hard lockup).

Pre-existing issues:
- [High] Missing loop breaks on transfer timeouts lead to hardware state machine corruption, data races, and silent failures.
- [High] Read-Modify-Write (RMW) operations on status registers unintentionally clear active hardware interrupts, leading to lost interrupts and hangs.
--

commit 5e4d8b30b6f49a760746b92be5b09e02f9393a8d
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Check that the transfer is valid before accessing it

The patch checks if the transfer is valid before accessing it in the interrupt
handlers and clears status flags to avoid triggering the same interrupts again.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca2..6e7ece2e0b4ee 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1014,6 +1014,12 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
>  
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			/* Clear the Transmit Buffer Empty status flag. */
> +			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0);

[Severity: High]
Does clearing the status bit without satisfying the hardware condition or
disabling the interrupt mask leave the hardware condition active?

For level-sensitive interrupts like an empty TX FIFO (NTST_TDBEF0), clearing
the status bit without filling the FIFO or disabling the interrupt mask
(NTIE_TDBEIE0) might leave the hardware condition active.

Could the hardware immediately re-trigger the ISR upon return, causing an
infinite interrupt loop and CPU lockup?

[Severity: High]
This is a pre-existing issue, but does using renesas_clear_bit() to clear
hardware interrupts risk losing interrupts?

The renesas_clear_bit() helper performs a read-modify-write operation on
status registers. If a new hardware interrupt asserts between the read and
the write, writing back the read value of 0 could unintentionally clear the
newly asserted interrupt.

Could this cause the driver to lose interrupts and hang while waiting for
events that were accidentally cleared?

> +			return IRQ_HANDLED;
> +		}

[ ... ]

> @@ -1235,6 +1262,11 @@ static irqreturn_t renesas_i3c_stop_isr(int irq, void *data)
>  
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			/* Clear the RX/TX Data Buffer Full status flag. */
> +			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0);
> +			return IRQ_HANDLED;

[Severity: High]
Does this early return miss clearing the actual Stop Condition flag
(BST_SPCNDDF)?

If the Stop Condition flag is not cleared, could the interrupt remain
asserted and lead to an interrupt storm?

> +		}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the loop in renesas_i3c_i3c_xfers()
fail to break on transfer timeouts?

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
        ...
		time_left = renesas_i3c_wait_xfer(i3c, xfer);
		if (!time_left)
			abort_xfer = true;
	}

	if (abort_xfer)
		renesas_i3c_abort_xfer(i3c);

	return 0;
}

When renesas_i3c_wait_xfer() times out, abort_xfer is set to true but the
loop continues. The code proceeds to modify xfer->cmds for the next message
and re-enqueues the xfer structure while the hardware controller is still
actively processing the timed-out transfer.

Could this cause a data race on cmd and corrupt the hardware state machine?

Additionally, renesas_i3c_i3c_xfers() unconditionally returns 0, which seems
to falsely report success to the subsystem even if all transfers timed out.

A similar issue appears to exist in renesas_i3c_i2c_xfers():

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
        ...
		time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
		if (!time_left)
			abort_xfer = true;

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

Because the timeout does not set cmd->err, the loop fails to break and
continues to process subsequent messages.

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

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