Re: [PATCH 2/2] media: i2c: Add OmniVision OV3660 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pm: incorrect pm_runtime_get_if_active() return check leads to unbalance
- [Low] v4l2: returning void expression in int-returning ov3660_init_controls() breaks build
--

commit 2864676387c5f4fec52017c3465e681222f3113b
Author: Nguyen Minh Tien <tien.nguyenminh@embeddedlinux.blog>
Subject: media: i2c: Add OmniVision OV3660 image sensor driver

This commit adds a new Video4Linux2 driver for the OmniVision OV3660
3 megapixel image sensor with an 8-bit parallel DVP interface. It supports
various resolutions and formats, with features like automatic/manual
exposure and gain, flips, and test patterns.

> diff --git a/drivers/media/i2c/ov3660.c b/drivers/media/i2c/ov3660.c
> new file mode 100644
> index 0000000000000..b8b5452285f40
> --- /dev/null
> +++ b/drivers/media/i2c/ov3660.c
[ ... ]
> +static int ov3660_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct ov3660 *sensor =
> +		container_of_const(ctrl->handler, struct ov3660, ctrls);
> +	u64 val;
> +	int ret;
> +
> +	/* the AEC/AGC results are only there while the sensor is powered */
> +	if (!pm_runtime_get_if_active(sensor->dev))
> +		return 0;

[Severity: High]
Does this error handling logic safely account for negative return codes?

pm_runtime_get_if_active() returns 1 on success, 0 if inactive, and a
negative error code (such as -EINVAL) if Runtime PM is disabled. By checking
!pm_runtime_get_if_active(), the negative error condition evaluates to false,
bypassing the early return.

This allows the execution to proceed with hardware access while the device
might be powered off, and later unconditionally calls
pm_runtime_put_autosuspend() without having incremented the usage counter,
leading to an unbalanced put.

[ ... ]
> +static int ov3660_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	/* written when the stream starts */
> +	if (!pm_runtime_get_if_active(sensor->dev))
> +		return 0;

[Severity: High]
Can this also bypass the early return on negative error codes?

Similar to ov3660_g_volatile_ctrl() above, negative errors from
pm_runtime_get_if_active() will allow execution to fall through, potentially
causing unsafe hardware access and a mismatched pm_runtime_put_autosuspend()
at the end of the function.

[ ... ]
> +static int ov3660_init_controls(struct ov3660 *sensor)
> +{
[ ... ]
> +	sensor->link_freq =
> +		v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> +				       ARRAY_SIZE(ov3660_link_freqs) - 1,
> +				       mode->link_freq_index,
> +				       ov3660_link_freqs);
> +	v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> +
> +	if (hdl->error)
> +		return v4l2_ctrl_handler_free(hdl);

[Severity: Low]
Does returning v4l2_ctrl_handler_free() here cause a build failure?

ov3660_init_controls() is declared to return an int, but
v4l2_ctrl_handler_free() returns void. Returning a void expression inside an
int-returning function is invalid C. It would also fail to propagate the
actual error code from hdl->error to the caller.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008160650.73927-1-tien.nguyenminh@embeddedlinux.blog?part=2