Re: [PATCH v4 16/16] i3c: renesas: Add runtime PM support

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

New issues:
- [High] Timeout handler fails to abort hardware transfers because it clears the BCTL_ABT bit instead of setting it.

Pre-existing issues:
- [High] Transfer loops do not break on timeout and unconditionally return success, leading to silent failures, race conditions, and memory corruption.
--

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

i3c: renesas: Add runtime PM support

This commit adds runtime PM support to the Renesas I3C driver, guarding
subsystem operations with pm_runtime APIs. It also drops explicit clock
handling, modifies wait_xfer to return unsigned long, and introduces a
function to clear pending IRQs on transfer aborts.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -443,7 +442,24 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
>  	}
>  }
>  
> -static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> +static void renesas_i3c_irqs_mask_and_clear(struct renesas_i3c *i3c)
> +{
> +	guard(spinlock_irqsave)(&i3c->xferqueue.lock);
> +
> +	/* Disable all the interrupts */
> +	renesas_writel(i3c->regs, BIE, 0);
> +	renesas_writel(i3c->regs, NTIE, 0);
> +
> +	/* Clear normal transfer status flags. */
> +	renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0 | NTST_RSPQFF |
> +					   NTST_TEF | NTST_TABTF);
> +	/* Clear bus status flags. */
> +	renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF | BST_SPCNDDF);
> +	/* Clear error flags. */
> +	renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);
> +}

[Severity: High]
Does this actually abort the hardware transfer? The commit message mentions
disabling interrupts and clearing pending IRQ status bits when a completion
timeout occurs.

However, clearing the BCTL_ABT bit is usually how an abort is acknowledged.
To actually initiate a hardware abort, the host typically must set the
BCTL_ABT bit, or explicitly reset the controller.

By only clearing an already-zero bit, does the controller remain in a busy
or hung state, potentially corrupting subsequent transfers?

[ ... ]

> @@ -777,7 +812,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>  		    NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
>  		    NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
>  
> -	renesas_i3c_wait_xfer(i3c, xfer);
> +	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);

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the timeout handling
here in renesas_i3c_daa().

If a timeout occurs (!time_left), the code proceeds without breaking early
or returning an error. Because cmd->rx_count might be empty on a timeout,
does this end up parsing a full newdevs mask and erroneously attaching
phantom devices for all remaining address slots?

[ ... ]

> @@ -970,9 +1022,14 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
>  				renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
>  		}
>  
> -		renesas_i3c_wait_xfer(i3c, xfer);
> +		time_left = renesas_i3c_wait_xfer(i3c, xfer);
> +		if (!time_left)
> +			xfer_failed = true;
>  	}
>  
> +	if (xfer_failed)
> +		renesas_i3c_irqs_mask_and_clear(i3c);
> +
>  	return 0;
>  }

[Severity: High]
This is also a pre-existing issue, but does this loop in
renesas_i3c_i3c_xfers() need to break when a timeout occurs?

By continuing the loop after xfer_failed is set to true, it seems
subsequent commands are enqueued to a controller that might still be hung
from the timed-out transfer.

Additionally, this function unconditionally returns 0 (success) at the end,
completely ignoring the timeout and any hardware errors recorded in
xfer->ret. Would it be safer to return an error code and break out of the
loop immediately?

[ ... ]

> @@ -1097,7 +1175,9 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>  
>  		renesas_set_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
>  
> -		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
> +		time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
> +		if (!time_left)
> +			xfer_failed = true;
>  
>  		if (cmd->err)
>  			break;

[Severity: High]
This is a pre-existing issue, but similar to the i3c transfers above, the
i2c transfer loop in renesas_i3c_i2c_xfers() fails to break on a timeout.

Because it continues enqueuing commands to a potentially hung controller,
could this cause a direct data race on cmd->i2c_buf and ultimately result in
returning a false success status?

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

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