Re: [PATCH v3 13/20] media: imx355: Convert to new CCI register access helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing lock when calling `__v4l2_ctrl_grab` in `imx355_set_stream` leading to data race
--

--- Patch [13]: [PATCH v3 13/20] media: imx355: Convert to new CCI register access helpers ---
commit fe9b8fcf594a4a8149bc0b6b2e99148353f94fe2
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: imx355: Convert to new CCI register access helpers

This commit replaces the private register access helpers in the imx355 driver
with the new common CCI register access helpers.

> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index f01a1a5ae7c9a..b27329143e481 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
[ ... ]
> @@ -1096,14 +981,14 @@ static int imx355_set_stream(struct v4l2_subdev *sd, int enable)

[Severity: High]
Does this code now lack a lock when calling __v4l2_ctrl_grab() in
imx355_set_stream()?

Since the outer mutex_lock(&imx355->mutex) was removed in this series in
favor of localized locking via v4l2_subdev_lock_and_get_active_state(sd),
the __v4l2_ctrl_grab() calls appear to be left unprotected:

    if (enable) {
        ...
        state = v4l2_subdev_lock_and_get_active_state(sd);
        ret = imx355_start_streaming(imx355);
        v4l2_subdev_unlock_state(state);
        ...
    } else {
        imx355_stop_streaming(imx355);
        ...
    }

    /* vflip and hflip cannot change during streaming */
    __v4l2_ctrl_grab(imx355->vflip, enable);
    __v4l2_ctrl_grab(imx355->hflip, enable);

__v4l2_ctrl_grab() modifies control flags and invokes send_event(), which
iterates over the event subscription list (ctrl->ev_subs).

If userspace concurrently calls VIDIOC_SUBSCRIBE_EVENT or
VIDIOC_UNSUBSCRIBE_EVENT on the flip controls while another thread starts or
stops streaming (VIDIOC_STREAMON / VIDIOC_STREAMOFF), could this cause list
corruption and a kernel panic?

Additionally, __v4l2_ctrl_grab() asserts lockdep_assert_held(), which would
trigger a lockdep splat if the control handler's lock is not held.

>  static int imx355_identify_module(struct imx355 *imx355)
>  {
>  	int ret;
> -	u32 val;
> +	u64 val;
>  
> -	ret = imx355_read_reg(imx355, IMX355_REG_CHIP_ID, 2, &val);
> +	ret = cci_read(imx355->regmap, IMX355_REG_CHIP_ID, &val, NULL);
>  	if (ret)
>  		return ret;

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