Re: [PATCH v12 2/2] media: i2c: Add driver for AMS-OSRAM Mira220

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: i2c: mira220: NULL pointer dereference in mira220_set_ctrl
- [Medium] media: i2c: mira220: Missing set_selection implementation breaks windowing
--

commit fd4dc828a5972aefbff92b9b37845b066b6f6659
Author: Philippe Baetens <philippebaetens@gmail.com>
media: i2c: Add driver for AMS-OSRAM Mira220

This commit adds a V4L2 subdev driver for the AMS-OSRAM Mira220 image sensor.
It implements support for mono and RGB 12, 10 and 8 bits formats with a
maximum frame rate up to 90 fps.

> diff --git a/drivers/media/i2c/mira220.c b/drivers/media/i2c/mira220.c
> new file mode 100644
> index 0000000000000..581999b46d953
> --- /dev/null
> +++ b/drivers/media/i2c/mira220.c

[ ... ]

> +static int mira220_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct mira220 *mira220 =
> +		container_of(ctrl->handler, struct mira220, ctrl_handler);
> +	struct i2c_client *client = v4l2_get_subdevdata(&mira220->sd);
> +	struct v4l2_mbus_framefmt *format;
> +	struct v4l2_subdev_state *state;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_get_locked_active_state(&mira220->sd);
> +	format = v4l2_subdev_state_get_format(state, 0);
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		int exposure_max, exposure_def;
> +
> +		/* Update max exposure while meeting expected vblanking */
> +		exposure_max = mira220_calc_exposure(mira220, format->height,
> +						     ctrl->val);

[Severity: High]
Does this code risk a kernel panic if the state lookup fails?

The v4l2_subdev_state_get_format() helper can return NULL, and the returned
format pointer is immediately dereferenced here (format->height) without any
NULL check.

Unlike pad operations, the s_ctrl callback is invoked directly by the control
framework and is not guarded by the v4l2_subdev_call() and check_state()
wrappers that normally ensure the format lookup succeeds before invoking the
driver.

[ ... ]

> +static void mira220_update_pad_format(struct mira220 *mira220,
> +				      struct v4l2_mbus_framefmt *fmt, u32 code)
> +{
> +	/* Bayer order varies with flips */
> +	fmt->code = mira220_get_format_code(mira220, code);
> +	/*
> +	 * The mira220 sensor doesn't support binning/skipping but only
> +	 * cropping to reduce the frame size so the correct API to configure
> +	 * windowing is the .set_selection one, while format is fixed to
> +	 * the full resolution 1600x1400 one.
> +	 */
> +	fmt->width = MIRA220_PIXEL_ARRAY_WIDTH;
> +	fmt->height = MIRA220_PIXEL_ARRAY_HEIGHT;
> +	fmt->field = V4L2_FIELD_NONE;

[ ... ]

> +static const struct v4l2_subdev_pad_ops mira220_pad_ops = {
> +	.enum_mbus_code = mira220_enum_mbus_code,
> +	.get_fmt = v4l2_subdev_get_fmt,
> +	.set_fmt = mira220_set_pad_format,
> +	.get_selection = mira220_get_selection,
> +	.enum_frame_size = mira220_enum_frame_size,
> +	.enable_streams = mira220_enable_streams,
> +	.disable_streams = mira220_disable_streams,
> +};

[Severity: Medium]
Will this break the advertised windowing capability?

The driver explicitly restricts set_fmt to the maximum physical resolution of
1600x1400 and notes that cropping configuration is deferred to the
set_selection API. However, the .set_selection callback is omitted entirely
in the v4l2_subdev_pad_ops structure.

Without it, userspace has no mechanism to configure cropping or adjust the
frame size, permanently locking the sensor to its maximum resolution.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-mira220-v12-0-07061064cd7b@ideasonboard.com?part=2