Re: [PATCH] media: i2c: ov02c10: Check for errors in disable_streams

[Hans de Goede <hansg@kernel.org>]

Hi,

On 24-Jan-26 08:17, Saikiran wrote:
> The ov02c10_disable_streams() function ignores the return value from
> cci_write() when stopping the sensor. If the I2C write fails (e.g.,
> due to CCI timeout, power management race, or device removal), the
> error is silently lost.
> 
> While we still need to return 0 and call pm_runtime_put() regardless
> of hardware state (to prevent PM reference leaks and pipeline lock
> issues), we should at least log when the hardware stop fails.
> 
> This change:
> 1. Captures the cci_write() return value
> 2. Logs an error if the write fails
> 3. Still returns 0 to ensure proper cleanup
> 
> Returning an error from disable_streams would cause the camss driver's
> video_stop_streaming() to exit early without releasing the pipeline
> lock, permanently locking the camera (see commit 044f54e7c).
> 
> Related-to: commit 7673f757858c ("media: i2c: ov02c10: Fix race condition in remove and relax reset timings")
> Signed-off-by: Saikiran <bjsaikiran@gmail.com>
> ---
>  drivers/media/i2c/ov02c10.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c
> index b86cae3d2b74..db191dccff75 100644
> --- a/drivers/media/i2c/ov02c10.c
> +++ b/drivers/media/i2c/ov02c10.c
> @@ -629,10 +629,20 @@ static int ov02c10_disable_streams(struct v4l2_subdev *sd,
>  				   u32 pad, u64 streams_mask)
>  {
>  	struct ov02c10 *ov02c10 = to_ov02c10(sd);
> +	int ret;
> +
> +	ret = cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 0, NULL);
> +	if (ret)
> +		dev_err(ov02c10->dev, "failed to stop streaming: %d\n", ret);
>  
> -	cci_write(ov02c10->regmap, OV02C10_REG_STREAM_CONTROL, 0, NULL);

cci_write() already logs a message on errors itself, so this is
undesirable as it will lead to duplicate log messages.

>  	pm_runtime_put(ov02c10->dev);
>  
> +	/*
> +	 * Return 0 even if cci_write failed. The stream is being stopped,
> +	 * so we must release the PM runtime reference regardless of hardware
> +	 * state. Returning an error here would cause pipeline lock leaks in
> +	 * the camss driver.
> +	 */

And as Brian stated such a big comment is not really necessary. disable_streams()
always returning 0 is normal and at some point we really ought to just make
its return-type "void".

So neither change is necessary here and this patch should be dropped.

Regards,

Hans