Re: [PATCH v2 5/5] media: i2c: vd55g1: Add support for vd55g4

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unchecked dereference of `device_get_match_data()` result in `vd55g1_detect()` leads to a NULL pointer dereference.

Pre-existing issues:
- [High] The V4L2 subdev state lock and control handler lock are mismatched, causing lockless execution of `__v4l2_ctrl_modify_range()`.
--

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

media: i2c: vd55g1: Add support for vd55g4

vd55g4 is the same device as vd65g4 but outputs in monochrome instead of
RGB. Adapt the driver structure according to this new variant, and add
its support.

> diff --git a/drivers/media/i2c/vd55g1.c b/drivers/media/i2c/vd55g1.c
> index 37d44abd1435c..c4142d771e7c8 100644
> --- a/drivers/media/i2c/vd55g1.c
> +++ b/drivers/media/i2c/vd55g1.c

[ ... ]

> @@ -1658,38 +1693,46 @@ static int vd55g1_init_ctrls(struct vd55g1 *sensor)

[ ... ]

>  static int vd55g1_detect(struct vd55g1 *sensor)
>  {
> -	unsigned int dt_id = (uintptr_t)device_get_match_data(sensor->dev);
> -	u64 rev, id;
> -	int ret;
> +	const struct vd55g1_version *dt_version =
> +		device_get_match_data(sensor->dev);
> +	const struct vd55g1_version *version;
> +	u64 color, id;
> +	int ret = 0;
>  
> -	ret = vd55g1_read(sensor, VD55G1_REG_MODEL_ID, &id, NULL);
> +	vd55g1_read(sensor, VD55G1_REG_MODEL_ID, &id, &ret);
> +	vd55g1_read(sensor, VD55G1_REG_COLOR_VERSION, &color, &ret);
>  	if (ret)
>  		return ret;
>  
> -	if (id != VD55G1_MODEL_ID_VD55G1 && id != VD55G1_MODEL_ID_VD65G4) {
> -		dev_warn(sensor->dev, "Unsupported sensor id 0x%x\n",
> -			 (u32)id);
> +	version = vd55g1_get_version(id, color);
> +	if (!version) {
> +		dev_warn(sensor->dev, "Unsupported sensor version, expected %s\n",
> +			 dt_version->name);

[Severity: High]
Could dt_version be NULL here? If the device is instantiated via software
nodes or forced to bind via sysfs without matching OF compatible properties,
device_get_match_data() returns NULL.

If it returns NULL, this will cause a kernel oops when dereferencing
dt_version->name here, as well as dt_version->id and dt_version->color
below. The previous version of this code safely cast the result to an
integer instead of dereferencing it.

>  		return -ENODEV;
>  	}
> -	if (id != dt_id) {
> -		dev_err(sensor->dev, "Probed sensor %s and device tree definition (%s) mismatch",
> -			VD55G1_MODEL_ID_NAME(id), VD55G1_MODEL_ID_NAME(dt_id));
> +	if (version->id != dt_version->id ||
> +	    version->color != dt_version->color) {
> +		dev_err(sensor->dev, "Probed sensor version %s and device tree definition %s mismatch",
> +			version->name, dt_version->name);
>  		return -ENODEV;
>  	}

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a mismatched lock assignment
in vd55g1_subdev_init()?

It appears sensor->sd.state_lock is assigned to sensor->ctrl_handler.lock
before v4l2_ctrl_handler_init() is called, meaning the lock is still NULL:

vd55g1_subdev_init() {
    ...
    sensor->sd.state_lock = sensor->ctrl_handler.lock;
    ret = v4l2_subdev_init_finalize(&sensor->sd);
    ...
}

Since v4l2_ctrl_handler_init() allocates a different internal mutex later,
the V4L2 core allocates a separate mutex for the state. When V4L2 core
invokes vd55g1_set_pad_fmt(), it calls __v4l2_ctrl_modify_range() while
holding only the subdev state lock. Could this violate V4L2 locking
invariants and cause a data race if a concurrent user-space VIDIOC_S_CTRL
ioctl is issued?

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