linux-kernel - Re: [PATCH v2 2/3] media: i2c: ov5647: Switch to using the sub-device state lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aVLOlkqSHn-N5Wi0@kekkonen.localdomain>
Date: Mon, 29 Dec 2025 20:55:18 +0200
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Xiaolei Wang <xiaolei.wang@...driver.com>
Cc: laurent.pinchart@...asonboard.com, dave.stevenson@...pberrypi.com,
	jacopo@...ndi.org, mchehab@...nel.org,
	prabhakar.mahadev-lad.rj@...renesas.com, hverkuil+cisco@...nel.org,
	johannes.goede@....qualcomm.com, hverkuil-cisco@...all.nl,
	jai.luthra@...asonboard.com, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] media: i2c: ov5647: Switch to using the
 sub-device state lock

Hi Xiaolei,

On Mon, Dec 29, 2025 at 10:30:17AM +0800, Xiaolei Wang wrote:
> Switch to using the sub-device state lock and properly call
> v4l2_subdev_init_finalize() / v4l2_subdev_cleanup() on probe() /
> remove().
> 
> Signed-off-by: Xiaolei Wang <xiaolei.wang@...driver.com>
> ---
>  drivers/media/i2c/ov5647.c | 40 +++++++++++++-------------------------
>  1 file changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index fd69f1616794..f0ca8cc14794 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -91,7 +91,6 @@ struct ov5647 {
>  	struct v4l2_subdev		sd;
>  	struct regmap                   *regmap;
>  	struct media_pad		pad;
> -	struct mutex			lock;
>  	struct clk			*xclk;
>  	struct gpio_desc		*pwdn;
>  	bool				clock_ncont;
> @@ -652,7 +651,7 @@ static int ov5647_stream_on(struct v4l2_subdev *sd)
>  	}
>  
>  	/* Apply customized values from user when stream starts. */
> -	ret =  __v4l2_ctrl_handler_setup(sd->ctrl_handler);
> +	ret =  v4l2_ctrl_handler_setup(sd->ctrl_handler);
>  	if (ret)
>  		return ret;
>  
> @@ -807,15 +806,12 @@ __ov5647_get_pad_crop(struct ov5647 *ov5647,
>  static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	struct ov5647 *sensor = to_sensor(sd);
>  	int ret;
>  
> -	mutex_lock(&sensor->lock);

Note that you shouldn't remove mutex_lock() here quite yet -- s_stream()
callback won't involve sub-device state and thus the caller won't take the
state lock either. In other words, the end result is fine after the third
patch so you should explicitly lock the active state and remove that in the
third patch (see e.g. v4l2_subdev_lock_and_get_active_state() in
drivers/media/i2c/imx290.c).

> -
>  	if (enable) {
>  		ret = pm_runtime_resume_and_get(&client->dev);
>  		if (ret < 0)
> -			goto error_unlock;
> +			return ret;
>  
>  		ret = ov5647_stream_on(sd);
>  		if (ret < 0) {
> @@ -831,14 +827,10 @@ static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
>  		pm_runtime_put(&client->dev);
>  	}
>  
> -	mutex_unlock(&sensor->lock);
> -
>  	return 0;
>  
>  error_pm:
>  	pm_runtime_put(&client->dev);
> -error_unlock:
> -	mutex_unlock(&sensor->lock);
>  
>  	return ret;
>  }

-- 
Kind regards,

Sakari Ailus