From: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: "linux-renesas-soc@vger.kernel.org"
<linux-renesas-soc@vger.kernel.org>,
"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>
Subject: RE: [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC
Date: Thu, 17 Jun 2021 14:14:06 +0000 [thread overview]
Message-ID: <OSZPR01MB7019B7DD71AB6E3FE04AE51CAA0E9@OSZPR01MB7019.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <YMtP/QHWljGcv3JX@mwanda>
Hi Dan,
Thank you for the review.
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: 17 June 2021 14:37
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org
> 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
next prev parent reply other threads:[~2021-06-17 14:14 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-17 13:37 [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC Dan Carpenter
2021-06-17 14:14 ` Prabhakar Mahadev Lad [this message]
2021-06-17 14:43 ` Dan Carpenter
2021-06-17 14:55 ` Prabhakar Mahadev Lad
-- strict thread matches above, loose matches on Subject: below --
2021-06-17 12:02 Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=OSZPR01MB7019B7DD71AB6E3FE04AE51CAA0E9@OSZPR01MB7019.jpnprd01.prod.outlook.com \
--to=prabhakar.mahadev-lad.rj@bp.renesas.com \
--cc=dan.carpenter@oracle.com \
--cc=linux-clk@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.