* [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC
@ 2021-06-17 12:02 Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2021-06-17 12:02 UTC (permalink / raw)
To: prabhakar.mahadev-lad.rj; +Cc: linux-renesas-soc, linux-clk
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:204 rzg2l_cpg_pll_clk_register()
warn: passing devm_ allocated variable to kfree. 'pll_clk'
drivers/clk/renesas/renesas-rzg2l-cpg.c
166 static struct clk * __init
167 rzg2l_cpg_pll_clk_register(const struct cpg_core_clk *core,
168 struct clk **clks,
169 void __iomem *base,
170 struct rzg2l_cpg_priv *priv)
171 {
172 struct device *dev = priv->dev;
173 const struct clk *parent;
174 struct clk_init_data init;
175 const char *parent_name;
176 struct pll_clk *pll_clk;
177 struct clk *clk;
178
179 parent = clks[core->parent & 0xffff];
180 if (IS_ERR(parent))
181 return ERR_CAST(parent);
182
183 pll_clk = devm_kzalloc(dev, sizeof(*pll_clk), GFP_KERNEL);
184 if (!pll_clk) {
185 clk = ERR_PTR(-ENOMEM);
186 return NULL;
Obviously, "return PTR_ERR(-ENOMEM); is intended. But I looked at the
caller and it has a check for NULL as a kind of work around for this
bug.
When a function returns only NULL and valid pointers, the NULL is an
error. But when a function returns *BOTH* NULL and error pointers the
NULL means that the feature is deliberately disabled by the user. It's
not an error. The caller should not print an error, but instead the
code needs to have NULL checks to avoid the NULL dereference.
For example, maybe the user has disabled blinking lights.
lights = get_blinking_lights();
If get_blinking_lights() fails then we print an error message and return
to the user. We don't try to continue. The user can fix their problem
and try again.
But if the get_blinking_lights() returns NULL that means we have to add
NULL checks:
if (lights)
lights->blink();
187 }
188
189 parent_name = __clk_get_name(parent);
190 init.name = core->name;
191 init.ops = &rzg2l_cpg_pll_ops;
192 init.flags = 0;
193 init.parent_names = &parent_name;
194 init.num_parents = 1;
195
196 pll_clk->hw.init = &init;
197 pll_clk->conf = core->conf;
198 pll_clk->base = base;
199 pll_clk->priv = priv;
200 pll_clk->type = core->type;
201
202 clk = clk_register(NULL, &pll_clk->hw);
203 if (IS_ERR(clk))
204 kfree(pll_clk);
Delete this line.
205
206 return clk;
207 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC
@ 2021-06-17 13:37 Dan Carpenter
2021-06-17 14:14 ` Prabhakar Mahadev Lad
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-06-17 13:37 UTC (permalink / raw)
To: prabhakar.mahadev-lad.rj; +Cc: linux-renesas-soc, linux-clk
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC
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
2021-06-17 14:43 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Prabhakar Mahadev Lad @ 2021-06-17 14:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-renesas-soc@vger.kernel.org, linux-clk@vger.kernel.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC
2021-06-17 14:14 ` Prabhakar Mahadev Lad
@ 2021-06-17 14:43 ` Dan Carpenter
2021-06-17 14:55 ` Prabhakar Mahadev Lad
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2021-06-17 14:43 UTC (permalink / raw)
To: Prabhakar Mahadev Lad
Cc: linux-renesas-soc@vger.kernel.org, linux-clk@vger.kernel.org
On Thu, Jun 17, 2021 at 02:14:06PM +0000, Prabhakar Mahadev Lad wrote:
> > 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)
The size - 1 format is riskier because there is a potential for
underflow.
Imagine that in the future priv->num_mod_clks is zero.
"priv->num_mod_clks - 1" is now UINT_MAX and any value of "clkidx" is
accepted. In this case, you know that the value of num_mod_clks if 57
but it took me some time to figure that out and ensure that it couldn't
be zero.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC
2021-06-17 14:43 ` Dan Carpenter
@ 2021-06-17 14:55 ` Prabhakar Mahadev Lad
0 siblings, 0 replies; 5+ messages in thread
From: Prabhakar Mahadev Lad @ 2021-06-17 14:55 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-renesas-soc@vger.kernel.org, linux-clk@vger.kernel.org
Hi Dan,
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: 17 June 2021 15:43
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Cc: linux-renesas-soc@vger.kernel.org; linux-clk@vger.kernel.org
> Subject: Re: [bug report] clk: renesas: Add CPG core wrapper for RZ/G2L SoC
>
> On Thu, Jun 17, 2021 at 02:14:06PM +0000, Prabhakar Mahadev Lad wrote:
> > > 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)
>
> The size - 1 format is riskier because there is a potential for underflow.
>
Agreed, >= check should do the trick.
Cheers,
Prabhakar
> Imagine that in the future priv->num_mod_clks is zero.
> "priv->num_mod_clks - 1" is now UINT_MAX and any value of "clkidx" is accepted. In this case, you
> know that the value of num_mod_clks if 57 but it took me some time to figure that out and ensure that
> it couldn't be zero.
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-17 14:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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.