Re: [PATCH v3 19/20] media: imx355: Switch to using the subdev state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `__v4l2_ctrl_grab()` is called without holding the required control handler lock, leading to lockdep assertions and data races.
- [Medium] V4L2 control values (`vflip` and `hflip`) are accessed locklessly during TRY state pad operations, introducing data races.
- [Medium] The error path for `v4l2_subdev_init_finalize()` bypasses `media_entity_cleanup()`.
--

--- Patch [19]: [PATCH v3 19/20] media: imx355: Switch to using the subdev state ---
commit 71948b5f6b6de08ab0164654fbf608971cc7cfe3
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: imx355: Switch to using the subdev state

The subdev state is now preferred rather than handling crop and format within
the driver state, so switch the driver to using it.

> 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

[ ... ]

> @@ -705,9 +675,7 @@ static int imx355_enum_mbus_code(struct v4l2_subdev *sd,
>  	if (code->index > 0)
>  		return -EINVAL;
>  
> -	mutex_lock(&imx355->mutex);
>  	code->code = imx355_get_format_code(imx355);

[Severity: Medium]
Will this lead to unprotected reads of the vflip and hflip controls?
 
When performing TRY operations, the V4L2 framework only holds the file handle
lock, not the subdev active state lock (which aliases to the control handler
lock).
 
Because the explicit driver mutex was removed here, it seems concurrent
operations could modify the control values while imx355_get_format_code()
reads them. Should the control handler lock be acquired before reading
these values?

> -	mutex_unlock(&imx355->mutex);
>  
>  	return 0;
>  }

[ ... ]

> @@ -992,14 +916,10 @@ static int imx355_set_stream(struct v4l2_subdev *sd, int enable)
>  	__v4l2_ctrl_grab(imx355->vflip, enable);
>  	__v4l2_ctrl_grab(imx355->hflip, enable);

[Severity: High]
Does this execute __v4l2_ctrl_grab() without holding the required lock?
 
Looking at __v4l2_ctrl_grab(), it asserts that the control handler lock is
held. Since the explicit driver mutex was removed and the subdev active
state lock is released before reaching this point (or not acquired at all
on the disable path), will this trigger a lockdep splat and introduce a
data race in the control flag modification?

> -	mutex_unlock(&imx355->mutex);
> -
>  	return ret;
>  
>  err_rpm_put:
>  	pm_runtime_put_autosuspend(imx355->dev);
> -err_unlock:
> -	mutex_unlock(&imx355->mutex);
>  
>  	return ret;
>  }

[ ... ]

> @@ -1351,6 +1267,13 @@ static int imx355_probe(struct i2c_client *client)
>  		goto error_handler_free;
>  	}
>  
> +	imx355->sd.state_lock = imx355->ctrl_handler.lock;
> +	ret = v4l2_subdev_init_finalize(&imx355->sd);
> +	if (ret < 0) {
> +		dev_err_probe(imx355->dev, ret, "subdev init error\n");
> +		goto error_handler_free;

[Severity: Medium]
Does this error path leak the media entity pads?
 
Earlier in imx355_probe(), media_entity_pads_init() is called. If
v4l2_subdev_init_finalize() fails, the code jumps to error_handler_free,
which bypasses the media_entity_cleanup() call located under the
error_media_entity_runtime_pm label. Should this jump to a label that
cleans up the media entity instead?

> +	}
> +
>  	/*
>  	 * Device is already turned on by i2c-core with ACPI domain PM.
>  	 * Enable runtime PM and turn off the device.

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