[PATCH] media: i2c: ov5670: fix double runtime-suspend on remove

[PATCH] media: i2c: ov5670: fix double runtime-suspend on remove

[Cong Nguyen]

ov5670_remove() calls ov5670_runtime_suspend() unconditionally, and
that function has no internal "already off" guard -- it always
asserts the reset/powerdown gpios, disables the regulators, and
disables/unprepares the clock.

probe() ends with pm_runtime_idle(), which drives an immediate
synchronous self-suspend (already calling ov5670_runtime_suspend()
once) whenever nothing is streaming. That's the common case, so
remove()'s unconditional second call clk_disable_unprepare()s an
already-unprepared clock -- hitting the same underflow WARN() fixed
elsewhere in this tree -- and disables already-disabled regulators,
on every normal removal, not just an edge case.

Guard it with pm_runtime_status_suspended(), same as sibling
imx412.c's remove().

Fixes: 62ab1e325978 ("media: i2c: ov5670: Add runtime_pm operations")
Assisted-by: LLM
Signed-off-by: Cong Nguyen <congnt264@gmail.com>
---
 drivers/media/i2c/ov5670.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 01fa892de079..a08bb4e9bfca 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2784,7 +2784,8 @@ static void ov5670_remove(struct i2c_client *client)
 	mutex_destroy(&ov5670->mutex);
 
 	pm_runtime_disable(ov5670->dev);
-	ov5670_runtime_suspend(ov5670->dev);
+	if (!pm_runtime_status_suspended(ov5670->dev))
+		ov5670_runtime_suspend(ov5670->dev);
 
 	v4l2_fwnode_endpoint_free(&ov5670->endpoint);
 }
-- 
2.25.1

Re: [PATCH] media: i2c: ov5670: fix double runtime-suspend on remove

[Laurent Pinchart]

On Mon, Sep 28, 2026 at 09:10:45PM +0700, Cong Nguyen wrote:
> ov5670_remove() calls ov5670_runtime_suspend() unconditionally, and
> that function has no internal "already off" guard -- it always
> asserts the reset/powerdown gpios, disables the regulators, and
> disables/unprepares the clock.
> 
> probe() ends with pm_runtime_idle(), which drives an immediate
> synchronous self-suspend (already calling ov5670_runtime_suspend()
> once) whenever nothing is streaming. That's the common case, so
> remove()'s unconditional second call clk_disable_unprepare()s an
> already-unprepared clock -- hitting the same underflow WARN() fixed
> elsewhere in this tree -- and disables already-disabled regulators,
> on every normal removal, not just an edge case.
> 
> Guard it with pm_runtime_status_suspended(), same as sibling
> imx412.c's remove().
> 
> Fixes: 62ab1e325978 ("media: i2c: ov5670: Add runtime_pm operations")
> Assisted-by: LLM
> Signed-off-by: Cong Nguyen <congnt264@gmail.com>

How have you tested this ?

> ---
>  drivers/media/i2c/ov5670.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 01fa892de079..a08bb4e9bfca 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2784,7 +2784,8 @@ static void ov5670_remove(struct i2c_client *client)
>  	mutex_destroy(&ov5670->mutex);
>  
>  	pm_runtime_disable(ov5670->dev);
> -	ov5670_runtime_suspend(ov5670->dev);
> +	if (!pm_runtime_status_suspended(ov5670->dev))
> +		ov5670_runtime_suspend(ov5670->dev);
>  
>  	v4l2_fwnode_endpoint_free(&ov5670->endpoint);
>  }

-- 
Regards,

Laurent Pinchart