Hi Dan,
Thank you for the review.
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Sent: 17 June 2021 14:37
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx
> Subject: [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC
>
> Hello Lad Prabhakar,
>
> The patch ef3c613ccd68: "clk: renesas: Add CPG core wrapper for
> RZ/G2L SoC" from Jun 9, 2021, leads to the following static checker
> warning:
>
> drivers/clk/renesas/renesas-rzg2l-cpg.c:226 rzg2l_cpg_clk_src_twocell_get()
> warn: array off by one? 'priv->clks[clkidx]'
>
> drivers/clk/renesas/renesas-rzg2l-cpg.c
> 209 static struct clk
> 210 *rzg2l_cpg_clk_src_twocell_get(struct of_phandle_args *clkspec,
> 211 void *data)
> 212 {
> 213 unsigned int clkidx = clkspec->args[1];
> 214 struct rzg2l_cpg_priv *priv = data;
> 215 struct device *dev = priv->dev;
> 216 const char *type;
> 217 struct clk *clk;
> 218
> 219 switch (clkspec->args[0]) {
> 220 case CPG_CORE:
> 221 type = "core";
> 222 if (clkidx > priv->last_dt_core_clk) {
>
> The ->last_dt_core_clk value comes from the device tree and I hate that
> we have to trust it. I haven't looked at the device tree and I only
> look at the code but based on the name "last_", I assume that
> in the device tree data this is set to either:
>
> last_dt_core_clk = priv->num_core_clks + priv->num_mod_clks - 1;
>
> Or maybe it's set so that:
>
> last_dt_core_clk = priv->num_core_clks - 1;
>
> So I think that it is not off by one (based on the naming scheme). But
> I would prefer that this code just used:
>
> if (clkidx >= priv->num_core_clks)
>
> Or:
> if (clkidx >= priv->num_core_clks + priv->num_mod_clks)
>
last_dt_core_clk comes from header include/dt-bindings/clock/r9a07g044-cpg.h and used in the SoC specific file in case of RZ/G2L its
assigned to " LAST_DT_CORE_CLK = R9A07G044_OSCCLK," in drivers/clk/renesas/r9a07g044-cpg.c index of the core clocks start from zero so this check will hold good here.
> 223 dev_err(dev, "Invalid %s clock index %u\n", type, clkidx);
> 224 return ERR_PTR(-EINVAL);
> 225 }
> 226 clk = priv->clks[clkidx];
> 227 break;
> 228
> 229 case CPG_MOD:
> 230 type = "module";
> 231 if (clkidx > priv->num_mod_clks) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Smatch did not catch it, but this condition is definitely off by one. ;)
>
Good catch this definitely needs to be if (clkidx > (priv->num_mod_clks - 1)
Cheers,
Prabhakar
