Re: [PATCH v3 15/20] media: imx355: Define the exposure offset, and use that define

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The check `!pm_runtime_get_if_in_use(imx355->dev)` incorrectly treats `-EINVAL` as success, causing the driver to write to hardware that is likely powered off and perform an unbalanced `pm_runtime_put()`.
- [High] When switching frame formats, the maximum exposure limit might fail to update to the new mode's geometry if the new format's `vblank_def` matches the current `vblank` value.
--

--- Patch [15]: [PATCH v3 15/20] media: imx355: Define the exposure offset, and use that define ---
commit 271b8bad9a7b25d1cb2eae62528be12497b83823
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: imx355: Define the exposure offset, and use that define

This commit introduces a new define, IMX355_EXPOSURE_OFFSET, to represent
the 10-line difference between frame length and maximum exposure time. It
replaces the bare value of 10 in the driver's control calculations with
this new define.

> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index 2bf8da7469fe5..3b86bd2e2e5e8 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
[ ... ]
> @@ -635,7 +636,7 @@ static int imx355_set_ctrl(struct v4l2_ctrl *ctrl)
>  	switch (ctrl->id) {
>  	case V4L2_CID_VBLANK:
>  		/* Update max exposure while meeting expected vblanking */
> -		max = imx355->cur_mode->height + ctrl->val - 10;
> +		max = imx355->cur_mode->height + ctrl->val - IMX355_EXPOSURE_OFFSET;
>  		__v4l2_ctrl_modify_range(imx355->exposure,
>  					 imx355->exposure->minimum,
>  					 max, imx355->exposure->step, max);
>  		break;
>  	}
>  
>  	/*
>  	 * Applying V4L2 control value only happens
>  	 * when power is up for streaming
>  	 */
>  	if (!pm_runtime_get_if_in_use(imx355->dev))
>  		return 0;

[Severity: High]
This is a pre-existing issue, but does this correctly handle negative return
values? 

If pm_runtime_get_if_in_use() returns -EINVAL (which occurs when PM runtime
is disabled or during driver unbind), the driver evaluates !(-EINVAL) as
false (0).

This bypasses the early return, leading to I2C writes to potentially
powered-off hardware, followed by a pm_runtime_put() that drops the usage
counter without a corresponding get. This unbalanced put can cause the
device usage count to prematurely reach 0 while other components hold
references.

[Severity: High]
This isn't a bug introduced by this patch, but does the exposure limit
correctly update when changing frame formats?

drivers/media/i2c/imx355.c:imx355_set_pad_format() {
    ...
		__v4l2_ctrl_modify_range(imx355->vblank, IMX355_VBLANK_MIN,
					 height, 1, vblank_def);
		__v4l2_ctrl_s_ctrl(imx355->vblank, vblank_def);
    ...
}

The driver relies on __v4l2_ctrl_s_ctrl() to trigger the vblank s_ctrl
callback (imx355_set_ctrl) to update the exposure maximum limit based on
the new mode's geometry. 

However, if the new format's default vblank_def happens to equal the
currently set vblank value, the V4L2 framework considers the control
unchanged and short-circuits the callback. As a result, the exposure
maximum limit is not updated, potentially allowing users to set an exposure
time that exceeds the frame length.

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