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

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

New issues:
- [High] Removal of DATBASn save/restore logic permanently bricks the I3C bus after system suspend/resume.
- [Medium] Pending asynchronous suspend is cancelled during driver removal, leaking power.

Pre-existing issues:
- [High] Transfer timeouts are ignored and loops are not aborted, falsely returning success and risking heap buffer overflows on hung hardware.
--

--- Patch [17]: [PATCH v5 17/17] i3c: renesas: Add runtime PM support ---
commit fd9b8f0562f27e4a6338d1856950821776c595b5
Author: Claudiu Beznea <[email protected]>

i3c: renesas: Add runtime PM support

On the SoCs where the Renesas I3C driver is enabled (RZ/G3S and RZ/G3E),
the clocks of the IP are managed through a clock PM domain. To keep the
I3C code simpler, the explicit clock handling was dropped along with the
addition of runtime PM support, in favor of the runtime PM APIs.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 378baac71aefa..1975c599ca438 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -491,6 +499,12 @@ static void renesas_i3c_bus_enable(struct i3c_master_controller *m, bool i3c_mod
>  static int renesas_i3c_reset(struct renesas_i3c *i3c)
>  {
>  	u32 val;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Will this leak power when the driver is unbound or the module is unloaded?

During driver removal, the I3C core calls renesas_i3c_bus_cleanup() which in
turn calls renesas_i3c_reset(). This acquires the PM synchronously but releases
it asynchronously with an autosuspend delay.

Because devm_pm_runtime_enable() was used in probe, the devres cleanup
immediately executes pm_runtime_disable(). This cancels any pending
asynchronous autosuspend work, leaving the device and its PM domain clocks
permanently enabled.

[ ... ]

> @@ -872,9 +914,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;
>  	}

[Severity: High]
This is a pre-existing issue, but does this missing timeout handling risk a
heap buffer overflow?

If a hardware transfer is delayed and the timeout expires, the loop continues
to the next message and reuses the shared cmd descriptor for the next transfer.

If the delayed hardware eventually completes the first transfer while the
second is set up, renesas_i3c_rx_isr() will read the data:

    read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);

Since it reads based on the hardware FIFO length without validating against
cmd->len, won't this overwrite the new, potentially smaller, buffer?

Also, won't this incorrectly return 0 (success) on a timeout since the loop is
not aborted?

[ ... ]

> @@ -1479,13 +1555,13 @@ static int renesas_i3c_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
> +	ret = renesas_i3c_reset(i3c);
>  	if (ret)
>  		goto err_resets_asserted;

[Severity: High]
Does dropping the DATBASn register save/restore logic break the I3C bus after
suspend/resume?

The renesas_i3c_reset() function clears all hardware registers, including
DATBAS, to 0. Later in the resume process, i3c_master_do_daa_ext() calls
renesas_i3c_daa() to rediscover devices.

However, renesas_i3c_daa() explicitly skips DATBAS programming for
already-attached devices:

    for (pos = 0; pos < i3c->maxdevs; pos++) {
        if (olddevs & BIT(pos))
            continue;
        ...
        renesas_writel(i3c->regs, DATBAS(pos), datbas_dvdyad_with_parity(ret));
    }

Because the previously attached devices never dropped their dynamic addresses,
they will not participate in DAA. This leaves their DATBAS registers at 0.

Won't all subsequent communication fail since the controller targets address 0?

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

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