Re: [PATCH v3 17/17] i3c: renesas: Add runtime PM support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Async Runtime PM put during driver unbind leaves the clock enabled.
- [High] Newly added renesas_i3c_abort_xfer clears the abort bit instead of setting it, leaving hardware stuck.

Pre-existing issues:
- [High] Timeout handling in transfer loops fails to break the loop, ignores the timeout, and incorrectly returns success.
- [High] I3C RX buffer is not advanced during FIFO reads, leaving buffer tails uninitialized.
--

commit 810cc3873a9dc52a932c03376330719f06d17de0
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Add runtime PM support

This commit adds runtime PM support to the Renesas I3C driver, replacing
explicit clock management with runtime PM APIs. It updates controller
operations with PM guards and introduces a transfer abort mechanism to handle
timeouts asynchronously.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index b6c282c53a541..d6121ca869908 100644
> --- 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_abort_xfer(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 clear the abort bit instead of setting it?

Hardware abort bits like BCTL_ABT typically must be written as 1 to trigger
an abort. If we only clear the bit, will the hardware controller be left
running the timed-out transfer in the background and get stuck?

> +}
> +
[ ... ]
> @@ -975,9 +1027,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)
> +			abort_xfer = true;

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of a break here
cause the loop to continue iterating?

If the transfer times out, it sets abort_xfer = true but continues to the
next command, overwriting the shared cmd descriptor.

Also, since renesas_i3c_i3c_xfers() unconditionally returns 0 at the end,
will this hide the timeout from the subsystem and potentially cause it to
read uninitialized RX buffers?

>  	}
>  
> +	if (abort_xfer)
> +		renesas_i3c_abort_xfer(i3c);
> +
>  	return 0;
>  }
[ ... ]
> @@ -1045,8 +1112,12 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
>  	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>  	struct renesas_i3c *i3c = to_renesas_i3c(m);
> +	int ret;
>  
> -	renesas_writel(i3c->regs, DATBAS(data->index), 0);
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (!ret)
> +		renesas_writel(i3c->regs, DATBAS(data->index), 0);
>  
>  	i3c_dev_set_master_data(dev, NULL);
>  	i3c->addrs[data->index].addr = 0;

[Severity: High]
Does this asynchronously put the runtime PM usage counter without a
synchronous suspend before driver teardown completes?

During driver unbind, renesas_i3c_remove() unregisters the I3C master, which
detaches devices. The PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND macro releases
the device asynchronously via pm_runtime_put_autosuspend().

Right after remove returns, devres calls pm_runtime_disable(), which cancels
the pending autosuspend work. Could this leave the hardware permanently in
the RPM_ACTIVE state and leak resources by never disabling the clocks?

[ ... ]
> @@ -1102,7 +1180,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)
> +			abort_xfer = true;
>  
>  		if (cmd->err)
>  			break;

[Severity: High]
This is a pre-existing issue, but similar to I3C transfers above, does this
fail to break the loop when wait_for_completion_timeout() times out?

Since cmd->err remains 0 on timeout, the loop continues and eventually returns
0, hiding the timeout from the subsystem.


Additionally, I noticed a pre-existing issue outside the immediate scope of
this patch:

[Severity: High]
This is a pre-existing issue, but in renesas_i3c_rx_isr() and
renesas_i3c_resp_isr(), do the I3C FIFO reads copy data into cmd->rx_buf
without advancing the buffer pointer?

renesas_i3c_rx_isr() does:

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

Since cmd->rx_buf is not incremented and rx_count is overwritten rather than
accumulated, will multiple interrupts overwrite the beginning of the buffer?
Could this leave the tail of the buffer uninitialized, leading to information
leaks or data corruption when the subsystem reads it?

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

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