Re: [PATCH v2 2/5] media: i2c: vd55g1: Remove spurious pad format update on init_state()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The subdev state lock and the control handler lock are not shared, leading to lockdep failures and data races.
--

commit e1c6d43eeca25a31bf4c1014650f4e8b8276a063
Author: Benjamin Mugnier <benjamin.mugnier@foss.st.com>

media: i2c: vd55g1: Remove spurious pad format update on init_state()

This patch prevents vd55g1_update_pad_fmt() from being called twice during
initialization by setting the format directly in vd55g1_init_state(). The
actual update is now deferred entirely to vd55g1_set_pad_fmt().

> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index fceb437e19bec..22464fe315624 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c
> @@ -1365,9 +1365,9 @@ static int vd55g1_init_state(struct v4l2_subdev *sd,
>  		code = vd55g1_mbus_formats_mono[0];
>  	else
>  		code = vd55g1_mbus_formats_bayer[0][0];
> -	vd55g1_update_pad_fmt(sensor,
> -			      &vd55g1_supported_modes[VD55G1_MODE_IDX_DEF],
> -			      vd55g1_get_fmt_code(sensor, code), &fmt.format);
> +	fmt.format.code = vd55g1_get_fmt_code(sensor, code);
> +	fmt.format.width = vd55g1_supported_modes[VD55G1_MODE_IDX_DEF].width;
> +	fmt.format.height = vd55g1_supported_modes[VD55G1_MODE_IDX_DEF].height;
>  
>  	return vd55g1_set_pad_fmt(sd, sd_state, &fmt);
>  }

[Severity: High]
This is a pre-existing issue, but does the initialization order in the driver
leave the control handler and subdev state locks unshared, potentially leading
to data races and lockdep warnings?

In vd55g1_subdev_init(), the lock assignment happens before the control handler
is initialized:

drivers/media/i2c/vd55g1.c:vd55g1_subdev_init() {
    ...
    sensor->sd.state_lock = sensor->ctrl_handler.lock;
    ret = v4l2_subdev_init_finalize(&sensor->sd);
    ...
}

Since sensor->ctrl_handler.lock is still NULL here, v4l2_subdev_init_finalize()
falls back to allocating an unshared lock for the active state.

The control handler lock is actually initialized later in vd55g1_init_ctrls():

drivers/media/i2c/vd55g1.c:vd55g1_init_ctrls() {
    ...
    v4l2_ctrl_handler_init(hdl, 16);
    ...
}

This means vd55g1_s_ctrl() runs with ctrl_handler.lock held, but expects the
active state lock to be held:

drivers/media/i2c/vd55g1.c:vd55g1_s_ctrl() {
    ...
    struct v4l2_subdev_state *state =
        v4l2_subdev_get_locked_active_state(&sensor->sd);
    ...
}

Because the locks are distinct, this triggers a lockdep assertion failure inside
v4l2_subdev_get_locked_active_state(). It also seems to expose a data race, as
vd55g1_s_ctrl() can read format and crop while vd55g1_set_pad_fmt() modifies
them concurrently using the active state lock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-vd55g4_and_fixes-v2-0-ea0a7cf3cd66@foss.st.com?part=2