Re: [PATCH 2/2] media: i2c: Add onsemi AR0234 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] Incorrect clamping order in crop selection bounds calculation causes unsigned integer underflow, bypassing bounds checks.
- [High] Incorrect validation of `pm_runtime_get_if_active()` allows execution to proceed on negative error codes, leading to a usage counter underflow.
- [High] Missing `v4l2_subdev_cleanup()` causes a memory leak of the subdevice active state.
- [High] Static hblank lower bounds permit dynamic line length reduction below the absolute hardware minimum.
- [High] Potential division by zero during delay calculation if the clock rate is zero.
- [Medium] Unlocked V4L2 control helpers are called during initialization without acquiring the control handler lock, triggering a lockdep splat.
- [Medium] Mixing goto-based error handling with scope-based `__free()` cleanup in the same function violates API guidelines.
- [Medium] Changing the crop height fails to update exposure control limits reliably.
- [Medium] The active format state is modified before verifying PLL configuration, leaving state inconsistent on error.
- [Medium] Unlocked access to shared format state in `ar0234_get_frame_desc`.
- [Low] Using `DIV_ROUND_UP` with a 64-bit numerator causes a link error on 32-bit architectures.
--

--- Patch [2]: [PATCH 2/2] media: i2c: Add onsemi AR0234 image sensor driver ---
commit eb8d4f60c8466169ec68bf0a9f862a5fa52e97fd
Author: Alexander Shiyan <eagle.alexander923@gmail.com>
Subject: media: i2c: Add onsemi AR0234 image sensor driver

This commit adds a new driver for the onsemi AR0234 CMOS image sensor.

diff --git a/drivers/media/i2c/ar0234.c b/drivers/media/i2c/ar0234.c
new file mode 100644
--- /dev/null
+++ b/drivers/media/i2c/ar0234.c
@@ -0,0 +1,1265 @@
[ ... ]
> static void ar0234_set_framing_limits(struct ar0234 *ar0234, u32 width)
> {
> 	int hblank = AR0234_LINE_LENGTH_PCK_MIN * 4 - width;
> 
> 	__v4l2_ctrl_s_ctrl(ar0234->vblank, AR0234_VBLANK_MIN);
> 
> 	__v4l2_ctrl_modify_range(ar0234->hblank, AR0234_HBLANK_MIN,
> 				 AR0234_HBLANK_MAX, 4, hblank);
> }

[Severity: High]
Could this allow the line length to be reduced below the hardware minimum?

If userspace reduces the format width and sets hblank to this static minimum,
the resulting programmed line length might violate the hardware minimum
requirement of 612. Should AR0234_HBLANK_MIN be calculated dynamically based
on the current width?

[ ... ]
> static int ar0234_set_pad_format(struct v4l2_subdev *sd,
> 				 struct v4l2_subdev_state *state,
> 				 struct v4l2_subdev_format *fmt)
> {
[ ... ]
> 	*v4l2_subdev_state_get_format(state, fmt->pad) = fmt->format;
> 
> 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> 		int ret = ar0234_calculate_pll(ar0234, mode);
> 
> 		if (ret) {
> 			dev_err(sd->dev, "PLL recalculation failed: %d\n", ret);
> 			return -EINVAL;
> 		}

[Severity: Medium]
Is it safe to write to the active format state before verifying that the PLL
configuration is structurally possible?

If ar0234_calculate_pll() fails, it returns -EINVAL, but the active state
has already been irreversibly modified and left inconsistent.

[ ... ]
> static int ar0234_set_selection(struct v4l2_subdev *sd,
> 				struct v4l2_subdev_state *state,
> 				struct v4l2_subdev_selection *sel)
> {
[ ... ]
> 	/* Clamp the top-left corner so that the whole rectangle stays */
> 	/* inside the active pixel array */
> 	rect.left =
> 		clamp_t(u32, rect.left, AR0234_PIXEL_ARRAY_LEFT,
> 			AR0234_PIXEL_ARRAY_LEFT + AR0234_PIXEL_ARRAY_WIDTH -
> 			rect.width);

[Severity: Critical]
Can this bounds check underflow if a maliciously large rect.width is provided?

Subtracting an unvalidated width from the pixel array dimensions could result
in a massive upper limit due to 32-bit unsigned arithmetic underflow, defeating
the clamp for rect.left and causing subsequent clamps to fail as well.

[ ... ]
> 	*crop = rect;
> 	sel->r = rect;
> 
> 	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> 		ar0234_set_framing_limits(ar0234, crop->width);
> 
> 	return 0;
> }

[Severity: Medium]
Does changing the crop height require updating the exposure control limits?

ar0234_set_framing_limits() forces vblank to AR0234_VBLANK_MIN, but if vblank
is already at its minimum, the V4L2 core skips the control setter. As a
result, the exposure limit is never recalculated for the smaller crop height.

[ ... ]
> static int ar0234_set_ctrl(struct v4l2_ctrl *ctrl)
> {
[ ... ]
> 	if (!pm_runtime_get_if_active(sd->dev))
> 		return 0;

[Severity: High]
Does this properly handle negative error codes from pm_runtime_get_if_active?

If runtime PM is disabled, it returns a negative error code (e.g., -EINVAL).
Because it is non-zero, this check bypasses the early return, proceeds to
access the hardware, and then calls pm_runtime_put_autosuspend(), which will
underflow the usage counter since get never incremented it.

[ ... ]
> static int ar0234_ctrls_init(struct ar0234 *ar0234)
> {
[ ... ]
> 	ar0234->sd.ctrl_handler = &ar0234->ctrls;
> 
> 	ar0234_set_link_limits(ar0234, &ar0234_modes[0]);
> 	ar0234_set_framing_limits(ar0234, AR0234_PIXEL_ARRAY_WIDTH);
> 
> 	return 0;
> }

[Severity: Medium]
Could these calls trigger a lockdep warning during boot?

ar0234_set_link_limits() and ar0234_set_framing_limits() invoke unlocked
internal control API functions which assert that the control handler lock
is held. Since this happens during probe before the lock is explicitly
acquired, this might cause a diagnostic splat.

[ ... ]
> static int ar0234_parse_hw_config(struct ar0234 *ar0234)
> {
> 	struct v4l2_fwnode_endpoint *ep_cfg = &ar0234->ep_cfg;
> 	struct fwnode_handle *ep __free(fwnode_handle) = NULL;
[ ... ]
> 	switch (ep_cfg->bus.mipi_csi2.num_data_lanes) {
> 	case 2:
> 	case 4:
> 		break;
> 	default:
> 		ret = dev_err_probe(ar0234->sd.dev, -EINVAL,
> 				    "Invalid number of CSI2 data lanes %d\n",
> 				    ep_cfg->bus.mipi_csi2.num_data_lanes);
> 		goto done_endpoint_free;

[Severity: Medium]
Is it acceptable to mix goto-based error labels with scope-based __free()
cleanup in the same function?

According to the cleanup.h API guidelines, mixing these heavily obscures
ownership semantics and makes the function susceptible to memory leaks and
double-free bugs during future maintenance.

[ ... ]
> static int ar0234_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> 				 struct v4l2_mbus_frame_desc *fd)
> {
[ ... ]
> 	state = v4l2_subdev_lock_and_get_active_state(&ar0234->sd);
> 	fmt = v4l2_subdev_state_get_format(state, pad);
> 	v4l2_subdev_unlock_state(state);
> 
> 	mode = ar0234_mode_from_code(ar0234, fmt->code, true);

[Severity: Medium]
Is it safe to access fmt->code after releasing the active state lock?

The lock is dropped, so a concurrent set_fmt ioctl could potentially overwrite
the format while it is being read here, leading to inconsistent reads.

[ ... ]
> static int ar0234_power_on(struct device *dev)
> {
[ ... ]
> 	gpiod_set_value_cansleep(ar0234->reset, 0);
> 
> 	/* ~160000 EXTCLKs */
> 	fsleep(DIV_ROUND_UP(160000ULL * 1000000, clk_get_rate(ar0234->clk)));
> 
> 	return 0;
> }

[Severity: Low]
Can this cause a build failure on 32-bit architectures? 

DIV_ROUND_UP with a 64-bit numerator expands to a 64-bit by 32-bit division
using the / operator, which is prohibited in the kernel. Should this use
DIV_ROUND_UP_ULL instead?

[Severity: High]
Also, what happens if clk_get_rate(ar0234->clk) returns 0?

Since the clock rate is used as a divisor without prior validation, a
misconfigured clock could cause a kernel panic (division by zero) during
driver probe.

[ ... ]
> static void ar0234_subdev_cleanup(struct ar0234 *ar0234)
> {
> 	media_entity_cleanup(&ar0234->sd.entity);
> 	v4l2_ctrl_handler_free(&ar0234->ctrls);
> }

[Severity: High]
Does this leak the active subdevice state?

The V4L2 active state is allocated by v4l2_subdev_init_finalize() during
probe, but it appears v4l2_subdev_cleanup() is never called to free it during
teardown, leading to a permanent leak on every unbind or failed probe.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731073505.2278769-1-eagle.alexander923@gmail.com?part=2