[bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

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)

   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. ;)

   232                          dev_err(dev, "Invalid %s clock index %u\n", type,
   233                                  clkidx);
   234                          return ERR_PTR(-EINVAL);
   235                  }
   236                  clk = priv->clks[priv->num_core_clks + clkidx];
   237                  break;
   238  
   239          default:
   240                  dev_err(dev, "Invalid CPG clock type %u\n", clkspec->args[0]);
   241                  return ERR_PTR(-EINVAL);
   242          }
   243  
   244          if (IS_ERR(clk))
   245                  dev_err(dev, "Cannot get %s clock %u: %ld", type, clkidx,
   246                          PTR_ERR(clk));
   247          else
   248                  dev_dbg(dev, "clock (%u, %u) is %pC at %lu Hz\n",
   249                          clkspec->args[0], clkspec->args[1], clk,
   250                          clk_get_rate(clk));
   251          return clk;
   252  }

regards,
dan carpenter