Re: [RFC PATCH 6/8] media: i2c: ov2312: add Omnivison OV2312 driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] media: ov2312: pm_runtime usage counter underflow on stream disable error
- [High] media: ov2312: hardware clock and power leaked in probe
- [High] media: ov2312: reading stale p_new values from unclustered multi controls
- [High] media: ov2312: incorrect endianness conversions break big-endian platforms
- [High] media: ov2312: valid control value of 0 aborts hardware updates
- [High] media: ov2312: lockless enable_count check causes PM runtime usage leak
- [High] media: ov2312: unprotected I2C hardware access on suspended device via s_ctrl
- [Medium] media: ov2312: set_fmt explicitly rejects valid stream 1 configuration
- [Medium] media: ov2312: set_frame_interval rejects unsupported framerates instead of adjusting
- [Medium] media: ov2312: single-capture V4L2 controls always fail with -EINVAL
- [Medium] media: ov2312: TOCTOU race in set_fmt allows format changes while streaming
- [Low] media: ov2312: dead code and compiler warnings from unconditional PM callbacks
- [Low] media: ov2312: mixing scope-based guard() with goto-based error handling
- [Low] media: ov2312: I2C device ID table uses positional initializers
--

commit 0c2ea846b3d892b1fe5473cabeacb68b8c969d79
Author: Jai Luthra <jai.luthra@linux.dev>

media: i2c: ov2312: add Omnivison OV2312 driver

Omnivision OV2312 is an RGB-IR sensor, i.e. it uses a 4x4 R,G,B,Ir bayer
pattern to capture both visible and near-infrared light. Every alternate
frame, the sensor changes the exposure and IR flash strobe registers to
stream an -
A. IR-dominant frame on CSI-2 virtual channel 0
B. RGB-dominant frame on CSI-2 virtual channel 1

> diff --git a/drivers/media/i2c/ov2312.c b/drivers/media/i2c/ov2312.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/i2c/ov2312.c
> @@ -0,0 +1,939 @@
[ ... ]
> +static int ov2312_read(struct ov2312 *ov2312, u16 addr, u32 *val, size_t nbytes)
> +{
> +	int ret;
> +	__le32 val_le = 0;
> +
> +	ret = regmap_bulk_read(ov2312->regmap, addr, &val_le, nbytes);
> +	if (ret < 0) {
> +		dev_err(ov2312->dev, "%s: failed to read reg 0x%04x: %d\n",
> +			__func__, addr, ret);
> +		return ret;
> +	}
> +
> +	*val = le32_to_cpu(val_le);

[Severity: High]
Does this endianness conversion work correctly on big-endian architectures?
Since regmap_bulk_read reads an array of bytes into a little-endian 32-bit
variable, le32_to_cpu will swap the bytes on big-endian platforms, which
might corrupt the read value.

> +	return 0;
> +}
[ ... ]
> +static int ov2312_set_fmt(struct v4l2_subdev *sd,
> +			  const struct v4l2_subdev_client_info *ci,
> +			  struct v4l2_subdev_state *state,
> +			  struct v4l2_subdev_format *fmt)
> +{
> +	struct ov2312 *ov2312 = to_ov2312(sd);
> +	struct v4l2_mbus_framefmt *format;
> +	const struct v4l2_area *fsize;
> +	u32 code;
> +	int ret = 0;
> +
> +	if (fmt->pad != 0)
> +		return -EINVAL;
> +
> +	if (fmt->stream != 0)
> +		return -EINVAL;

[Severity: Medium]
Will this break standard V4L2 tooling that attempts to configure the format
for stream 1? Userspace will receive -EINVAL when trying to configure the
RGB stream on virtual channel 1.

> +
> +	/* Sensor only supports a single format. */
> +	code = ov2312_mbus_formats[0];
> +
> +	/* Find the nearest supported frame size. */
> +	fsize = v4l2_find_nearest_size(ov2312_framesizes,
> +				       ARRAY_SIZE(ov2312_framesizes), width,
> +				       height, fmt->format.width,
> +				       fmt->format.height);
> +
> +	v4l2_subdev_lock_state(state);
> +
> +	/* Update the stored format and return it. */
> +	format = v4l2_subdev_state_get_format(state, fmt->pad, fmt->stream);
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && ov2312->enable_count) {

[Severity: Medium]
Is it safe to read ov2312->enable_count here without holding the lock
(ov2312->lock)? If a concurrent VIDIOC_STREAMON starts the stream right
after this check, the active format might be overwritten while the device
is streaming, causing a pipeline state mismatch.

> +		ret = -EBUSY;
> +		goto done;
> +	}
[ ... ]
> +static int ov2312_set_frame_interval(struct v4l2_subdev *sd,
> +					 struct v4l2_subdev_state *state,
> +				     struct v4l2_subdev_frame_interval *fi)
> +{
> +	struct ov2312 *ov2312 = to_ov2312(sd);
> +
> +	if (!fi->interval.numerator)
> +		return -EINVAL;
> +
> +	dev_dbg(ov2312->dev, "%s: Set framerate %dfps\n", __func__,
> +		fi->interval.denominator / fi->interval.numerator);
> +
> +	if ((fi->interval.denominator / fi->interval.numerator) != ov2312->fps / 2) {
> +		dev_err(ov2312->dev, "%s: Framerate can only be %dfps\n",
> +			__func__, ov2312->fps / 2);
> +		return -EINVAL;

[Severity: Medium]
Is returning -EINVAL here correct for V4L2 negotiation? Standard behavior
usually involves adjusting or clipping unsupported frame intervals to the
closest supported one, rather than rejecting them with an error.

> +	}
> +
> +	return 0;
> +}
> +
> +static int ov2312_detect(struct ov2312 *ov2312)
> +{
> +	int ret;
> +	u32 id;
> +
> +	ret = ov2312_read(ov2312, OV2312_SC_CHIP_ID_HI, &id, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	id = cpu_to_be16(id);

[Severity: High]
Does this patch the endianness flaw correctly? Since id is a 32-bit variable,
cpu_to_be16 will only convert the lower 16 bits. On big-endian platforms,
this combined with the previous le32_to_cpu conversion might result in
the wrong chip ID being detected, causing probe failure.

> +
> +	if (id != OV2312_CHIP_ID) {
[ ... ]
> +static int ov2312_set_AB_mode(struct ov2312 *ov2312)
> +{
> +	bool ir_ready  = ov2312->exposure_multi->p_new.p_u32[1] &&
> +			 ov2312->again_multi->p_new.p_u32[1] &&
> +			 ov2312->dgain_multi->p_new.p_u32[1];
> +	bool rgb_ready = ov2312->exposure_multi->p_new.p_u32[0] &&
> +			 ov2312->again_multi->p_new.p_u32[0] &&
> +			 ov2312->dgain_multi->p_new.p_u32[0];

[Severity: High]
Can these p_new values be stale? Because these controls are not bound
together via v4l2_ctrl_cluster(), setting one of them won't automatically
synchronize the p_new values of the others. This could lead to unpredictable
hardware register states when updating the controls.

[Severity: High]
Also, does a control value of 0 incorrectly abort hardware updates here?
Since analog and digital gains can legitimately be set to 0, using the
logical AND (&&) might evaluate to false, meaning valid configuration
requests could be silently ignored.

> +	int ret;
> +
> +	if (ir_ready) {
[ ... ]
> +static int ov2312_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct ov2312 *ov2312 = container_of(ctrl->handler,
> +					     struct ov2312, ctrls);
> +	int ret;
> +
> +	/*
> +	 * If the device is not powered up by the host driver do
> +	 * not apply any controls to H/W at this time. Instead
> +	 * the controls will be restored right after power-up.
> +	 */
> +	if (pm_runtime_suspended(ov2312->dev))
> +		return 0;

[Severity: High]
Is it possible for the device to suspend asynchronously right after this
check? Accessing the I2C bus locklessly without pinning the power state
with a reference (e.g., pm_runtime_get_if_in_use) could lead to hanging
the bus or crashing when writing to powered-off hardware.

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE_MULTI:
> +	case V4L2_CID_AGAIN_MULTI:
> +	case V4L2_CID_DGAIN_MULTI:
> +		dev_dbg(ov2312->dev, "debug: %s: %s = [%u, %u]\n", __func__,
> +				ctrl->name, ctrl->p_new.p_u32[0], ctrl->p_new.p_u32[1]);
> +
> +		ret = ov2312_set_AB_mode(ov2312);
> +		break;
> +
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:
> +		ret = ov2312_set_orientation(ov2312);
> +		break;
> +
> +	default:
> +		ret = -EINVAL;

[Severity: Medium]
Will this return -EINVAL for the single-capture backward-compatibility
controls like V4L2_CID_EXPOSURE? The driver registers them, but they
are not handled in this switch statement, which will cause standard userspace
apps to fail when setting them.

> +	}
> +
> +	return ret;
> +}
[ ... ]
> +static int ov2312_sd_enable_streams(struct v4l2_subdev *sd,
> +				       struct v4l2_subdev_state *state,
> +				       u32 pad, u64 streams_mask)
> +{
> +	struct ov2312 *ov2312 = to_ov2312(sd);
> +	int ret;
> +
> +	guard(mutex)(&ov2312->lock);
> +
> +	if (!ov2312->enable_count) {
> +		ret = pm_runtime_resume_and_get(ov2312->dev);
> +		if (ret < 0)
> +			goto err;

[Severity: Low]
Is it intended to mix scope-based guard() with goto-based error handling
here? Subsystem guidelines recommend avoiding this combination within the
same function, as it creates confusing ownership and cleanup semantics.

> +
> +		ret = ov2312_start_stream(ov2312);
> +		if (ret < 0) {
> +			pm_runtime_put(ov2312->dev);
> +			goto err;
> +		}
> +	}
> +
> +	ov2312->enable_count++;
> +
> +	return 0;
> +
> +err:
> +	dev_err(ov2312->dev,
> +		"%s: failed to enable streams %d\n", __func__, ret);
> +	return ret;
> +}
> +
> +static int ov2312_sd_disable_streams(struct v4l2_subdev *sd,
> +					struct v4l2_subdev_state *state,
> +					u32 pad, u64 streams_mask)
> +{
> +	struct ov2312 *ov2312 = to_ov2312(sd);
> +	int ret;
> +
> +	mutex_lock(&ov2312->lock);
> +
> +	if (ov2312->enable_count == 1) {
> +		ret = ov2312_stop_stream(ov2312);
> +		if (ret < 0)
> +			goto err_runtime_put;
> +	}
> +
> +	ov2312->enable_count--;
> +	mutex_unlock(&ov2312->lock);
> +
> +	if (!ov2312->enable_count)

[Severity: High]
Could checking enable_count without the lock here cause a PM runtime
usage leak? If a concurrent stream enable happens right after unlocking,
the condition evaluates to false and the pm_runtime_put() call is skipped,
preventing the device from ever suspending.

> +		pm_runtime_put(ov2312->dev);
> +
> +	return 0;
> +
> +err_runtime_put:
> +	mutex_unlock(&ov2312->lock);
> +	pm_runtime_put(ov2312->dev);

[Severity: High]
Should ov2312->enable_count be decremented on this error path? Dropping
the PM runtime reference without reducing the enable count desynchronizes
the power state, which could lead to PM underflows or hardware access
on a powered-off device later.

> +	return ret;
> +}
[ ... ]
> +static int ov2312_probe(struct i2c_client *client)
> +{
[ ... ]
> +	/* PM Runtime */
> +	pm_runtime_enable(ov2312->dev);
> +	pm_runtime_set_suspended(ov2312->dev);

[Severity: High]
Is it correct to set the PM runtime state to suspended here? The hardware
was turned on earlier in the probe with ov2312_power_on(), and
ov2312_power_off() hasn't been called.

> +
> +	ret = v4l2_subdev_init_finalize(sd);
> +	if (ret < 0)
> +		goto err_pm_disable;
> +
> +	/* Finally, register the subdev. */
> +	ret = v4l2_async_register_subdev(sd);
> +	if (ret < 0) {
> +		ret = dev_err_probe(ov2312->dev, ret,
> +				    "v4l2 subdev register failed\n");
> +		goto err_subdev_cleanup;
> +	}
> +
> +	dev_info(ov2312->dev, "ov2312 probed\n");
> +	return 0;

[Severity: High]
Does returning success here leak the hardware clock and power? The PM core
believes the device is suspended, but it is actually powered on, leading
to a permanent power drain and potential double-enable on the next resume.

> +
> +err_subdev_cleanup:
> +	v4l2_subdev_cleanup(&ov2312->sd);
[ ... ]
> +static const struct i2c_device_id ov2312_id[] = {
> +	{ "ov2312", 0 },

[Severity: Low]
Does this violate the I2C subsystem rules? Initialized arrays of type
struct i2c_device_id are required to use named initializers instead of
positional ones.

> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i2c, ov2312_id);
[ ... ]
> +static struct i2c_driver ov2312_i2c_driver = {
> +	.driver = {
> +		.name	= "ov2312",
> +		.pm	= &ov2312_pm_ops,

[Severity: Low]
Should this use the pm_ptr() wrapper? Assigning the PM operations structure
unconditionally will cause compiler warnings about unused static functions
when CONFIG_PM is disabled.

> +		.of_match_table = of_match_ptr(ov2312_of_match),
> +	},
> +	.probe		= ov2312_probe,
> +	.remove		= ov2312_remove,
> +	.id_table	= ov2312_id,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260925133001.2780868-1-r-donadkar@ti.com?part=6