Hi Dan On Wed, Feb 08, 2023 at 06:02:12PM +0300, Dan Carpenter wrote: > 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. > So let's please the machine overlords and silence the static analyzer false positives :) > > 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"); > That's defintely better. I can send a patch right away Thanks! > regards, > dan carpenter >
