Re: [PATCH v5 13/17] i3c: renesas: Update HW registers after SW computations are done

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

Pre-existing issues:
- [High] Potential division by zero when calculating timing adjustments if i3c->rate > 1 GHz.
- [High] Kernel panic due to division by zero if the I3C bus operates in pure mode and the device tree lacks the 'i2c-scl-hz' property.
--

--- Patch [13]: [PATCH v5 13/17] i3c: renesas: Update HW registers after SW computations are done ---
commit c4f913d9658906f3cc459cae7e1d70cd5318dc3c
Author: Claudiu Beznea <[email protected]>
Subject: i3c: renesas: Update HW registers after SW computations are done

This patch modifies renesas_i3c_bus_init() to perform software computations
and cache updates before writing to hardware registers. This prepares the
driver for future runtime PM support by minimizing the time the controller
must remain powered.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 52f09d9666512..2cff7c6ae3692 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -565,10 +565,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
>  	if (!i3c->rate)
>  		return -EINVAL;
>  
> -	ret = renesas_i3c_reset(i3c);
> -	if (ret)
> -		return ret;
> -
>  	i2c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i2c);

[Severity: High]
This is a pre-existing issue, but does this code cause a divide by zero if
the bus operates in pure mode without an explicit I2C rate limit?

If the I3C bus has no I2C children (falling back to I3C_BUS_MODE_PURE) and
the device tree lacks the 'i2c-scl-hz' property, bus->scl_rate.i2c is never
populated and remains 0. 

When DIV_ROUND_UP() executes here in renesas_i3c_bus_init(), will it
trigger a kernel panic?

>  	i3c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i3c);
>  

[ ... ]

> @@ -619,27 +615,31 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
>  			STDBR_SBRHO(double_SBR, od_high_ticks) |
>  			STDBR_SBRLP(pp_low_ticks) |
>  			STDBR_SBRHP(pp_high_ticks);

[Severity: High]
This isn't a bug introduced by this patch, but can we hit a divide by zero
exception during these timing calculations in renesas_i3c_bus_init() and
renesas_i3c_hw_init() if the I3C controller clock (tclk) exceeds 1 GHz?

Looking at the code just before this hunk in renesas_i3c_bus_init():

    od_low_ticks -= t.scl_fall_ns / (NSEC_PER_SEC / i3c->rate) + 1;
    od_high_ticks -= t.scl_rise_ns / (NSEC_PER_SEC / i3c->rate) + 1;

And similarly in renesas_i3c_hw_init() in drivers/i3c/master/renesas-i3c.c:

    val = DIV_ROUND_UP(I3C_BUS_TBUF_MIXED_FM_MIN_NS,
                       NSEC_PER_SEC / i3c->rate);

If i3c->rate is greater than 1,000,000,000, integer division
(NSEC_PER_SEC / i3c->rate) evaluates to 0. 

Will this truncated zero value subsequently be used as a divisor, causing a
kernel panic during the driver's probe sequence?

> -	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
>  
>  	/* Extended Bit Rate setting */
>  	i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
>  		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);

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

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