* [bug report] media: i2c: ov5670: Use common clock framework
@ 2023-02-08 13:37 Dan Carpenter
2023-02-08 14:23 ` Jacopo Mondi
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2023-02-08 13:37 UTC (permalink / raw)
To: jacopo.mondi; +Cc: linux-media
Hello Jacopo Mondi,
The patch 8004c91e2095: "media: i2c: ov5670: Use common clock
framework" from Jan 26, 2023, leads to the following Smatch static
checker warning:
drivers/media/i2c/ov5670.c:2670 ov5670_probe()
warn: passing zero to 'PTR_ERR'
drivers/media/i2c/ov5670.c
2648 static int ov5670_probe(struct i2c_client *client)
2649 {
2650 struct ov5670 *ov5670;
2651 const char *err_msg;
2652 u32 input_clk = 0;
2653 bool full_power;
2654 int ret;
2655
2656 ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL);
2657 if (!ov5670) {
2658 ret = -ENOMEM;
2659 err_msg = "devm_kzalloc() error";
2660 goto error_print;
2661 }
2662
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.
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()).
2672
2673 if (input_clk != OV5670_XVCLK_FREQ) {
2674 dev_err(&client->dev,
2675 "Unsupported clock frequency %u\n", input_clk);
2676 return -EINVAL;
2677 }
2678
2679 /* Initialize subdev */
2680 v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
2681
2682 ret = ov5670_regulators_probe(ov5670);
2683 if (ret) {
2684 err_msg = "Regulators probe failed";
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [bug report] media: i2c: ov5670: Use common clock framework 2023-02-08 13:37 [bug report] media: i2c: ov5670: Use common clock framework Dan Carpenter @ 2023-02-08 14:23 ` Jacopo Mondi 2023-02-08 14:30 ` Jacopo Mondi 2023-02-08 15:02 ` Dan Carpenter 0 siblings, 2 replies; 5+ messages in thread From: Jacopo Mondi @ 2023-02-08 14:23 UTC (permalink / raw) To: Dan Carpenter; +Cc: jacopo.mondi, linux-media Hi Dan, On Wed, Feb 08, 2023 at 04:37:35PM +0300, Dan Carpenter wrote: > Hello Jacopo Mondi, > > The patch 8004c91e2095: "media: i2c: ov5670: Use common clock > framework" from Jan 26, 2023, leads to the following Smatch static > checker warning: > > drivers/media/i2c/ov5670.c:2670 ov5670_probe() > warn: passing zero to 'PTR_ERR' > > drivers/media/i2c/ov5670.c > 2648 static int ov5670_probe(struct i2c_client *client) > 2649 { > 2650 struct ov5670 *ov5670; > 2651 const char *err_msg; > 2652 u32 input_clk = 0; > 2653 bool full_power; > 2654 int ret; > 2655 > 2656 ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL); > 2657 if (!ov5670) { > 2658 ret = -ENOMEM; > 2659 err_msg = "devm_kzalloc() error"; > 2660 goto error_print; > 2661 } > 2662 > 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)) 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 :/ Thanks j > > 2672 > 2673 if (input_clk != OV5670_XVCLK_FREQ) { > 2674 dev_err(&client->dev, > 2675 "Unsupported clock frequency %u\n", input_clk); > 2676 return -EINVAL; > 2677 } > 2678 > 2679 /* Initialize subdev */ > 2680 v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops); > 2681 > 2682 ret = ov5670_regulators_probe(ov5670); > 2683 if (ret) { > 2684 err_msg = "Regulators probe failed"; > > regards, > dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] media: i2c: ov5670: Use common clock framework 2023-02-08 14:23 ` Jacopo Mondi @ 2023-02-08 14:30 ` Jacopo Mondi 2023-02-08 15:02 ` Dan Carpenter 1 sibling, 0 replies; 5+ messages in thread From: Jacopo Mondi @ 2023-02-08 14:30 UTC (permalink / raw) To: Jacopo Mondi; +Cc: Dan Carpenter, linux-media, Sakari Ailus Sakari in cc for real this time On Wed, Feb 08, 2023 at 03:23:40PM +0100, Jacopo Mondi wrote: > Hi Dan, > > On Wed, Feb 08, 2023 at 04:37:35PM +0300, Dan Carpenter wrote: > > Hello Jacopo Mondi, > > > > The patch 8004c91e2095: "media: i2c: ov5670: Use common clock > > framework" from Jan 26, 2023, leads to the following Smatch static > > checker warning: > > > > drivers/media/i2c/ov5670.c:2670 ov5670_probe() > > warn: passing zero to 'PTR_ERR' > > > > drivers/media/i2c/ov5670.c > > 2648 static int ov5670_probe(struct i2c_client *client) > > 2649 { > > 2650 struct ov5670 *ov5670; > > 2651 const char *err_msg; > > 2652 u32 input_clk = 0; > > 2653 bool full_power; > > 2654 int ret; > > 2655 > > 2656 ov5670 = devm_kzalloc(&client->dev, sizeof(*ov5670), GFP_KERNEL); > > 2657 if (!ov5670) { > > 2658 ret = -ENOMEM; > > 2659 err_msg = "devm_kzalloc() error"; > > 2660 goto error_print; > > 2661 } > > 2662 > > 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)) > 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 :/ > > Thanks > j > > > > 2672 > > 2673 if (input_clk != OV5670_XVCLK_FREQ) { > > 2674 dev_err(&client->dev, > > 2675 "Unsupported clock frequency %u\n", input_clk); > > 2676 return -EINVAL; > > 2677 } > > 2678 > > 2679 /* Initialize subdev */ > > 2680 v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops); > > 2681 > > 2682 ret = ov5670_regulators_probe(ov5670); > > 2683 if (ret) { > > 2684 err_msg = "Regulators probe failed"; > > > > regards, > > dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] media: i2c: ov5670: Use common clock framework 2023-02-08 14:23 ` Jacopo Mondi 2023-02-08 14:30 ` Jacopo Mondi @ 2023-02-08 15:02 ` Dan Carpenter 2023-02-08 15:47 ` Jacopo Mondi 1 sibling, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2023-02-08 15:02 UTC (permalink / raw) To: Jacopo Mondi; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] media: i2c: ov5670: Use common clock framework 2023-02-08 15:02 ` Dan Carpenter @ 2023-02-08 15:47 ` Jacopo Mondi 0 siblings, 0 replies; 5+ messages in thread From: Jacopo Mondi @ 2023-02-08 15:47 UTC (permalink / raw) To: Dan Carpenter; +Cc: Jacopo Mondi, linux-media 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-02-08 15:47 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-08 13:37 [bug report] media: i2c: ov5670: Use common clock framework Dan Carpenter 2023-02-08 14:23 ` Jacopo Mondi 2023-02-08 14:30 ` Jacopo Mondi 2023-02-08 15:02 ` Dan Carpenter 2023-02-08 15:47 ` Jacopo Mondi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox