Re: [PATCH v5 06/10] media: imx219: Fix vertical blanking and exposure for analogue binning

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unscaled `exposure_max` allows userspace to request exposure times that exceed the physical frame length during analog binning.
--

commit 5bb0595912a9dcd70c3d849b873e9a213a70ec05
Author: Sakari Ailus <sakari.ailus@linux.intel.com>

media: imx219: Fix vertical blanking and exposure for analogue binning

This commit removes a workaround that doubled the pixel rate, frame length
in lines, and exposure time for analog binning. It updates the minimum, maximum,
and step values for the control.

> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 3aebcbaa3fcd6..3cee31758b7e9 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
[ ... ]
> @@ -879,23 +868,25 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2;
>  
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
[ ... ]
>  		/* Update limits and set FPS to default */
> +		int vblank_min = ((int)mode->height * (1 - bin_hv) / bin_hv) +
> +			IMX219_VBLANK_MIN;
>  		ret = __v4l2_ctrl_modify_range(imx219->vblank,
> -					       IMX219_VBLANK_MIN * rate_factor,
> -					       (IMX219_FLL_MAX - mode->height) *
> -					       rate_factor, rate_factor,
> -					       mode->fll_def - mode->height);
> +					       vblank_min,
> +					       IMX219_FLL_MAX - mode->height, 1,
> +					       (int)(mode->fll_def / bin_hv) -
> +					       (int)mode->height);
>  		if (ret)
>  			return ret;
>  
>  		ret = __v4l2_ctrl_s_ctrl(imx219->vblank,
> -					 mode->fll_def - mode->height);
> +					 (int)(mode->fll_def / bin_hv) -
> +					 (int)mode->height);
>  		if (ret)
>  			return ret;

[Severity: High]
Does this code allow userspace to request an exposure time that exceeds the
physical frame length?

When vblank is initialized via __v4l2_ctrl_s_ctrl() above, it dynamically
computes and sets the correct upper limit for the exposure control under
the hood.

However, immediately following this in imx219_set_pad_format(), the code uses
the unscaled mode->fll_def to modify the exposure limits:

		/* Update max exposure while meeting expected vblanking */
		exposure_max = mode->fll_def - IMX219_EXPOSURE_OFFSET;
		exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ?
				exposure_max : IMX219_EXPOSURE_DEFAULT;
		ret = __v4l2_ctrl_modify_range(imx219->exposure,
					       imx219->exposure->minimum,
					       exposure_max,
					       imx219->exposure->step,
					       exposure_def);

Since this patch removes the rate_factor division when writing V4L2_CID_EXPOSURE
to the sensor hardware in imx219_set_ctrl(), and exposure_max is not scaled
by bin_hv here, could this overwrite the safely computed hardware bound with an
unscaled maximum?

If so, userspace could successfully request an exposure value (e.g., 1703) while
the physical frame length is only 853, which might violate the fundamental
sensor constraint that exposure time must be less than the frame length.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607215356.842932-1-sakari.ailus@linux.intel.com?part=6