Re: [PATCH v3 20/20] media: imx355: Remove storing cur_mode in the state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `__v4l2_ctrl_grab()` function is called without holding the required control handler lock, triggering a lockdep BUG and causing a data race.
--

--- Patch [20]: [PATCH v3 20/20] media: imx355: Remove storing cur_mode in the state ---
commit 0f6829a36bfe297cb0fe5da42f9da61884297d27
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: imx355: Remove storing cur_mode in the state

This commit removes the cur_mode pointer from the imx355 structure. It
changes the driver to retrieve the mode information directly from the subdev
state instead.

> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -892,19 +892,23 @@ static int imx355_stop_streaming(struct imx355 *imx355)
>  static int imx355_set_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct imx355 *imx355 = to_imx355(sd);
> +	struct v4l2_subdev_state *state;
>  	int ret = 0;
>  
>  	if (enable) {
>  		ret = pm_runtime_resume_and_get(imx355->dev);
>  		if (ret < 0)
>  			return ret;
>  
>  		/*
>  		 * Apply default & customized values
>  		 * and then start streaming.
>  		 */
> +		state = v4l2_subdev_lock_and_get_active_state(sd);
>  		ret = imx355_start_streaming(imx355);
> +		v4l2_subdev_unlock_state(state);
>  		if (ret)
>  			goto err_rpm_put;
>  	} else {
>  		imx355_stop_streaming(imx355);
>  		pm_runtime_put_autosuspend(imx355->dev);
>  	}
>  
>  	/* vflip and hflip cannot change during streaming */
>  	__v4l2_ctrl_grab(imx355->vflip, enable);
>  	__v4l2_ctrl_grab(imx355->hflip, enable);

[Severity: High]
Is it safe to call __v4l2_ctrl_grab() without holding the control handler lock
here?

This patch adds v4l2_subdev_lock_and_get_active_state() around
imx355_start_streaming(), but releases the lock immediately with
v4l2_subdev_unlock_state().

It then calls __v4l2_ctrl_grab() completely unprotected. In the enable == 0
path, the lock is never taken at all.

Since __v4l2_ctrl_grab() asserts lockdep_assert_held(ctrl->handler->lock),
could this cause a lockdep splat or a data race on the control state? Should
the driver use the locked v4l2_ctrl_grab() variant here instead?

>  
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-media-imx355-v3-0-9df386a623d7@raspberrypi.com?part=20