Re: [bug report] media: i2c: ov5670: Use common clock framework

[Dan Carpenter <error27@gmail.com>]

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