All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed

From: Dan Carpenter <dan.carpenter@oracle.com>
To: 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
Date: Thu, 17 Jun 2021 16:37:01 +0300	[thread overview]
Message-ID: <YMtP/QHWljGcv3JX@mwanda> (raw)

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

next             reply	other threads:[~2021-06-17 13:37 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-17 13:37 Dan Carpenter [this message]
2021-06-17 14:14 ` [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC Prabhakar Mahadev Lad
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=YMtP/QHWljGcv3JX@mwanda \
    --to=dan.carpenter@oracle.com \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=prabhakar.mahadev-lad.rj@bp.renesas.com \
    /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

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.