Re: [PATCH v3 2/2] media: i2c: Add Sony IMX908 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect error handling of `pm_runtime_get_if_active()` return value allows unpowered hardware access and causes reference count underflow.
- [High] Unsigned integer underflow in VBLANK and HBLANK recalculations causes incorrect clamping and sudden frame rate drops.
- [High] Unbalanced `pm_runtime_disable()` due to explicitly calling it alongside `devm_pm_runtime_enable()`.
- [Medium] `__v4l2_ctrl_modify_range()` is incorrectly passed the current dynamic control value as the new default value.
- [Medium] The error path in `imx908_probe` leaves the Runtime PM status as `RPM_ACTIVE` while physically powering off the device.
--

commit 213f0fc18bc025f4c6b33dde193e86993ba40da6
Author: Lachlan Michael <lachlan.michael@sony.com>

media: i2c: Add Sony IMX908 image sensor driver

Adds a V4L2 sub-device driver for the Sony IMX908 CMOS image sensor,
supporting RAW10 and RAW12 output formats via a MIPI CSI-2 interface. Includes
controls for exposure, gain, flipping, blanking, cropping, and test patterns.

> diff --git a/drivers/media/i2c/imx908.c b/drivers/media/i2c/imx908.c
> new file mode 100644
> index 0000000000000..ca85a5cd058ab
> --- /dev/null
> +++ b/drivers/media/i2c/imx908.c
[ ... ]
> +static u32 imx908_hmax_to_hblank(u16 hmax, u32 width)
> +{
> +	return hmax * IMX908_PIX_PER_CLK - width;
> +}

[Severity: High]
Does this expression silently underflow when called with a new width that
exceeds the old horizontal period? If so, an unsigned underflow would cause
the limits to be drastically miscalculated, effectively maximizing horizontal
blanking and functionally breaking the video stream.

[ ... ]
> +static int imx908_update_hblank_limits(struct imx908 *imx, u32 old_width,
> +				       u32 width, u8 bpp)
> +{
[ ... ]
> +	ret = __v4l2_ctrl_modify_range(imx->ctrls.hblank, min_hblank,
> +				       max_hblank, IMX908_PIX_PER_CLK, hblank);

[Severity: Medium]
Is it intentional to pass hblank (the current computed blanking) as the
default value here? This overwrites the default value of the V4L2 control
with the user's currently selected dynamic value, meaning a reset to default
will track this dynamic value rather than the true factory baseline.

[ ... ]
> +static int imx908_update_vblank_limits(struct imx908 *imx,
> +				       const struct v4l2_rect *crop,
> +				       u32 old_height)
> +{
> +	u32 min_vblank = imx908_calc_min_vblank(crop);
> +	u32 max_vblank = IMX908_VMAX_MAX - crop->height;
> +	/* Preserve frame length across height changes when possible. */
> +	u32 vmax = old_height + imx->ctrls.vblank->val;
> +	u32 vblank = clamp_t(u32, vmax - crop->height,
> +			     min_vblank, max_vblank);

[Severity: High]
Could this expression silently underflow when crop height increases past the
previous frame's vmax via the VIDIOC_SUBDEV_S_SELECTION ioctl? Because vmax
and crop->height are unsigned, if crop->height is larger, the subtraction
will wrap around to a very large positive number, causing clamp_t to cap it
at max_vblank.

> +	int ret;
> +
> +	ret = __v4l2_ctrl_modify_range(imx->ctrls.vblank, min_vblank,
> +				       max_vblank, 1, vblank);

[Severity: Medium]
Similar to the hblank limit update, does passing vblank here mistakenly
overwrite the true factory default with the dynamically calculated vblank?

[ ... ]
> +static int imx908_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +		ret = __v4l2_ctrl_modify_range(imx->ctrls.exposure,
> +					       IMX908_EXPOSURE_MIN,
> +					       max_exposure,
> +					       IMX908_EXPOSURE_STEP,
> +					       current_exposure);

[Severity: Medium]
Is current_exposure erroneously used as the new default value for the exposure
control here?

[ ... ]
> +	/* Hardware writes only when powered; cached ctrls applied on resume */
> +	if (!pm_runtime_get_if_active(imx->dev))
> +		return 0;

[Severity: High]
Does this logic fail to catch negative error codes? If CONFIG_PM is enabled
but runtime PM is disabled for the device, pm_runtime_get_if_active returns
-EINVAL. The check !-22 evaluates to false, so execution would fall through,
allowing unauthorized I2C register writes on a disabled device.

[ ... ]
> +	default:
> +		dev_warn(imx->dev,
> +			 "ctrl(id:0x%x,val:0x%x) is not handled\n",
> +			 ctrl->id, ctrl->val);
> +		break;
> +	}
> +
> +	pm_runtime_put(imx->dev);
> +
> +	return ret;
> +}

[Severity: High]
Following up on the pm_runtime_get_if_active check above, if -EINVAL was
returned and the function proceeds, won't this pm_runtime_put decrement the
usage count without a prior successful increment, triggering an underflow?

[ ... ]
> +static int imx908_probe(struct i2c_client *client)
> +{
[ ... ]
> +err_power_off:
> +	imx908_power_off(imx->dev);
> +
> +	return ret;
> +}

[Severity: Medium]
Can the PM core be left with a state mismatch in this error path? The
hardware is powered down via imx908_power_off, but the PM runtime state is
left as RPM_ACTIVE because pm_runtime_set_suspended is not called to undo
the earlier pm_runtime_set_active.

[ ... ]
> +static void imx908_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct imx908 *imx = to_imx908(sd);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	v4l2_subdev_cleanup(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
> +
> +	pm_runtime_disable(imx->dev);

[Severity: High]
Since devm_pm_runtime_enable is used in imx908_probe, does this explicit call
to pm_runtime_disable conflict with the devm cleanup? This could result in a
double disable on driver removal, leaking the reference and permanently
breaking runtime PM for the device upon subsequent re-binding.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828064843.65047-1-lachlan.michael@sony.com?part=2