ov7670_power_on() ignores clk_prepare_enable() and proceeds to toggle
GPIOs and set info->on.  A failed clock enable can therefore be reported
as a powered sensor and later be paired with an unbalanced clock
disable.

Return the clock error before changing GPIO or software state, and
propagate it from the subdevice power operation and probe.

The issue was identified via static analysis and manually reviewed.

Fixes: 030f9f682e66 ("media: ov7670: control clock along with power")

Assisted-by: LLM
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/media/i2c/ov7670.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index 4d040e9feeac..9c5bb391202d 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -1647,14 +1647,17 @@ static int ov7670_s_register(struct v4l2_subdev *sd, const struct v4l2_dbg_regis
 }
 #endif
 
-static void ov7670_power_on(struct v4l2_subdev *sd)
+static int ov7670_power_on(struct v4l2_subdev *sd)
 {
 	struct ov7670_info *info = to_state(sd);
+	int ret;
 
 	if (info->on)
-		return;
+		return 0;
 
-	clk_prepare_enable(info->clk);
+	ret = clk_prepare_enable(info->clk);
+	if (ret)
+		return ret;
 
 	if (info->pwdn_gpio)
 		gpiod_set_value(info->pwdn_gpio, 0);
@@ -1667,6 +1670,8 @@ static void ov7670_power_on(struct v4l2_subdev *sd)
 		usleep_range(3000, 5000);
 
 	info->on = true;
+
+	return 0;
 }
 
 static void ov7670_power_off(struct v4l2_subdev *sd)
@@ -1687,12 +1692,15 @@ static void ov7670_power_off(struct v4l2_subdev *sd)
 static int ov7670_s_power(struct v4l2_subdev *sd, int on)
 {
 	struct ov7670_info *info = to_state(sd);
+	int ret;
 
 	if (info->on == on)
 		return 0;
 
 	if (on) {
-		ov7670_power_on(sd);
+		ret = ov7670_power_on(sd);
+		if (ret)
+			return ret;
 		ov7670_init(sd, 0);
 		ov7670_apply_fmt(sd);
 		ov7675_apply_framerate(sd);
@@ -1872,7 +1880,9 @@ static int ov7670_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	ov7670_power_on(sd);
+	ret = ov7670_power_on(sd);
+	if (ret)
+		return ret;
 
 	if (info->clk) {
 		info->clock_speed = clk_get_rate(info->clk) / 1000000;
-- 
2.43.0

Hi Pengpeng,

Thanks for the patch.

On Fri, Aug 28, 2026 at 05:15:06PM +0800, Pengpeng Hou wrote:
> ov7670_power_on() ignores clk_prepare_enable() and proceeds to toggle
> GPIOs and set info->on.  A failed clock enable can therefore be reported
> as a powered sensor and later be paired with an unbalanced clock
> disable.
> 
> Return the clock error before changing GPIO or software state, and
> propagate it from the subdevice power operation and probe.
> 
> The issue was identified via static analysis and manually reviewed.
> 
> Fixes: 030f9f682e66 ("media: ov7670: control clock along with power")
> 

Please remove extra newline here.

> Assisted-by: LLM

Which one?

> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
>  drivers/media/i2c/ov7670.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index 4d040e9feeac..9c5bb391202d 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -1647,14 +1647,17 @@ static int ov7670_s_register(struct v4l2_subdev *sd, const struct v4l2_dbg_regis
>  }
>  #endif
>  
> -static void ov7670_power_on(struct v4l2_subdev *sd)
> +static int ov7670_power_on(struct v4l2_subdev *sd)
>  {
>  	struct ov7670_info *info = to_state(sd);
> +	int ret;
>  
>  	if (info->on)

That's also unnecessary. No need to change this for a bugfix though.

It'd be also nice to convert the driver to use Runtime PM but that is
obviously out of scope of this patch.

> -		return;
> +		return 0;
>  
> -	clk_prepare_enable(info->clk);
> +	ret = clk_prepare_enable(info->clk);
> +	if (ret)
> +		return ret;
>  
>  	if (info->pwdn_gpio)
>  		gpiod_set_value(info->pwdn_gpio, 0);
> @@ -1667,6 +1670,8 @@ static void ov7670_power_on(struct v4l2_subdev *sd)
>  		usleep_range(3000, 5000);
>  
>  	info->on = true;
> +
> +	return 0;
>  }
>  
>  static void ov7670_power_off(struct v4l2_subdev *sd)
> @@ -1687,12 +1692,15 @@ static void ov7670_power_off(struct v4l2_subdev *sd)
>  static int ov7670_s_power(struct v4l2_subdev *sd, int on)
>  {
>  	struct ov7670_info *info = to_state(sd);
> +	int ret;
>  
>  	if (info->on == on)
>  		return 0;
>  
>  	if (on) {
> -		ov7670_power_on(sd);

You can also declare ret here.

> +		ret = ov7670_power_on(sd);
> +		if (ret)
> +			return ret;
>  		ov7670_init(sd, 0);
>  		ov7670_apply_fmt(sd);
>  		ov7675_apply_framerate(sd);
> @@ -1872,7 +1880,9 @@ static int ov7670_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> -	ov7670_power_on(sd);
> +	ret = ov7670_power_on(sd);
> +	if (ret)
> +		return ret;
>  
>  	if (info->clk) {
>  		info->clock_speed = clk_get_rate(info->clk) / 1000000;

-- 
Regards,

Sakari Ailus