On Wed, Feb 08, 2023 at 03:23:40PM +0100, Jacopo Mondi wrote:
> > 2663 ov5670->xvclk = devm_clk_get(&client->dev, NULL);
> > 2664 if (!IS_ERR_OR_NULL(ov5670->xvclk))
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Imagine CONFIG_HAVE_CLK is not enabled so now devm_clk_get() returns
> > NULL.
> >
> > 2665 input_clk = clk_get_rate(ov5670->xvclk);
> > 2666 else if (PTR_ERR(ov5670->xvclk) == -ENOENT)
> > 2667 device_property_read_u32(&client->dev, "clock-frequency",
> > 2668 &input_clk);
> > 2669 else
> > --> 2670 return dev_err_probe(&client->dev, PTR_ERR(ov5670->xvclk),
> > 2671 "error getting clock\n");
> >
> > A NULL is zero and zero is success.
> >
>
> Ouch! Quite a subtle bug!
>
> > That means this code returns success without doing anything. Perhaps
> > the right thing is to use use Kconfig to ensure that this cannot be
> > build without CONFIG_HAVE_CLK. The other solution is to write the
> > driver with a bunch of NULL checks so that it still runs without a clk.
> >
> > The IS_ERR_OR_NULL() check should be changed to if (IS_ERR()).
>
> >From a very quick lookup at how that symbol is used it seems it is
> selected both by COMMON_CLOCK and HAVE_LEGACY_CLOCK, however I'm not
> sure I know enough to consider safe depending on that symbol.
>
> When it comes to sensor-driver specific issues, I see CCS (the
> reference i2c camera sensor driver) depending on it, so I guess it's
> fine (Sakari in cc), but no other sensor driver does that (actually no
> driver in drivers/linux/media/ does that, not just i2c sensors!)
>
> When it comes to adding NULL checks, the common clock frameworks
> already protects against that, turning the usual
> clock_prepare_enable() and clock_disable_unprepare() calls into nop,
> so if we can't depend on CONFIG_HAVE_CLK I guess we can get away
> with some ugly:
>
> #if defined(CONFIG_HAVE_CLK)
> ov5670->xvclk = devm_clk_get(&client->dev, NULL);
> #else
> ov5670->xvclk = ERR_PTR(-ENOENT);
> #endif
> if (!IS_ERR_OR_NULL(ov5670->xvclk))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The static checker would still complain that we're passing NULL to
PTR_ERR() because of the IS_ERR_OR_NULL() check. It should just be
IS_ERR().
I wouldn't be surprised if the Kconfig ensures that a NULL return is
impossible in the original code. However in the proposed code, then
it's definitely impossible.
> input_clk = clk_get_rate(ov5670->xvclk);
> else if (PTR_ERR(ov5670->xvclk) == -ENOENT)
> device_property_read_u32(&client->dev, "clock-frequency",
> &input_clk);
> else
> return dev_err_probe(&client->dev, PTR_ERR(ov5670->xvclk),
> "error getting clock\n");
>
> Not super nice though :/
Why not just add the NULL path to the check for -ENOENT?
ov5670->xvclk = devm_clk_get(&client->dev, NULL);
if (!IS_ERR_OR_NULL(ov5670->xvclk))
input_clk = clk_get_rate(ov5670->xvclk);
else if (!ov5670->xvclk || PTR_ERR(ov5670->xvclk) == -ENOENT)
device_property_read_u32(&client->dev, "clock-frequency",
&input_clk);
else
return dev_err_probe(&client->dev, PTR_ERR(ov5670->xvclk),
"error getting clock\n");
regards,
dan carpenter
