Re: [PATCH v4 12/16] 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] Division by zero crash if `i2c-scl-hz` property is omitted in pure mode.
- [Medium] Potential division by zero crash and precision loss in open-drain timing calculations.
--

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

i3c: renesas: Update HW registers after SW computations are done

The patch defers hardware register writes until after software calculations
and cache updates are complete in renesas_i3c_bus_init(), aiming to
support future runtime PM enhancements.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index d5247e841a838..96d4fed32bf95 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -560,10 +560,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 and was not introduced by this patch, but can
this cause a division by zero crash?

If the i2c-scl-hz device tree property is not present, of_populate_i3c_bus()
leaves master->bus.scl_rate.i2c as 0. In pure mode (I3C_BUS_MODE_PURE),
i3c_bus_set_mode() explicitly does not initialize scl_rate.i2c, which
would leave it as 0 here.

[ ... ]

>  	ret = i3c_master_get_free_addr(m, 0);
>  	if (ret < 0)
>  		return ret;
>  
> +	info.dyn_addr = ret;
>  	i3c->dyn_addr = ret;
> -	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(ret) | MSDVAD_MDYADV);
> +	i3c->refclk_div = cks;
> +
> +	ret = renesas_i3c_reset(i3c);
> +	if (ret)
> +		return ret;

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but does
the timing calculation in this function risk a division by zero or precision
loss?

In renesas_i3c_bus_init(), there are calculations similar to:

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

If the input clock rate i3c->rate exceeds 1 GHz (NSEC_PER_SEC), the inner
division evaluates to 0, which would cause a divide-by-zero kernel panic.

Even for rates under 1 GHz, this pattern causes severe precision loss for
non-integer multiples of 1 GHz.

Would using a pattern like DIV_ROUND_UP_ULL((u64)ns * rate, NSEC_PER_SEC)
be safer here to prevent both the crash and precision loss?

> +	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
> +	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
> +	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
> +	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(i3c->dyn_addr) | MSDVAD_MDYADV);
> +
> +	/* I3C hw init*/
> +	renesas_i3c_hw_init(i3c);
>  
> -	info.dyn_addr = ret;
>  	return i3c_master_set_info(&i3c->base, &info);
>  }

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

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