Re: [PATCH v10 2/2] media: i2c: add Himax HM1246 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect validation of `pm_runtime_get_if_active()` return value allows unpowered hardware access and causes an unbalanced `pm_runtime_put()` decrement.
- [High] The driver fails to update the active subdev state format when flip controls change the Bayer layout.
- [High] Integer overflow in 32-bit PLL VCO computation results in erroneously validating broken hardware configurations.
- [Medium] Flawed nested loops in PLL calculation prevent finding valid dividers by breaking prematurely.
- [Medium] Logic error in PLL multiplier calculation skips valid configurations.
- [Medium] Unprotected concurrent access to clustered control values during TRY pad operations.
--

commit a1e846fd762489ac057abc081793eb13dea90d9f
Author: Matthias Fend <matthias.fend@emfend.at>

media: i2c: add Himax HM1246 image sensor driver

This commit adds a V4L2 sub-device driver for the Himax HM1246-AWD image
sensor. It operates the sensor in raw mode and bypasses the internal ISP.

> diff --git a/drivers/media/i2c/hm1246.c b/drivers/media/i2c/hm1246.c
> new file mode 100644
> index 0000000000000..ba120967814f3
> --- /dev/null
> +++ b/drivers/media/i2c/hm1246.c

[ ... ]

> +/* Get the format code of the mode considering current flip setting. */
> +static u32 hm1246_get_format_code(struct hm1246 *hm1246,
> +				  const struct hm1246_mode *hm1246_mode)
> +{
> +	return hm1246_mode->codes[FLIP_FORMAT_INDEX(hm1246->vflip_ctrl->val,
> +						    hm1246->hflip_ctrl->val)];
> +}

[Severity: Medium]
Could this result in unprotected concurrent access to clustered control values?

When invoked during pad operations (like VIDIOC_SUBDEV_ENUM_MBUS_CODE) acting
on the V4L2_SUBDEV_FORMAT_TRY state, the lock held is the file handle's TRY
state lock, not the control handler lock.

Accessing hm1246->vflip_ctrl->val and hm1246->hflip_ctrl->val directly
without holding v4l2_ctrl_lock(hm1246->ctrls.lock) allows a data race with
concurrent VIDIOC_S_CTRL calls, potentially resulting in reading a
torn/inconsistent set of flip values and exposing incorrect Bayer patterns.

[ ... ]

> +	/* Find suitable PCLK and SYSCLK dividers. */
> +	for (pclk_div_index = 0; pclk_div_index < ARRAY_SIZE(pclk_div_table);
> +	     pclk_div_index++) {
> +		for (sysclk_div_index = 0;
> +		     sysclk_div_index < ARRAY_SIZE(sysclk_div_table);
> +		     sysclk_div_index++) {
> +			if (sysclk_div_table[sysclk_div_index] *
> +				    sysclk_pclk_ratio ==
> +			    pclk_div_table[pclk_div_index]) {
> +				sysclk_pclk_ratio_found = true;
> +				break;
> +			}
> +		}
> +		if (sysclk_pclk_ratio_found)
> +			break;

[Severity: Medium]
Will this break prematurely without checking other valid combinations?

Breaking immediately upon finding the first valid sysclk_pclk_ratio locks
pclk_div to the first match. If this specific divider cannot satisfy the
subsequent VCO constraint limits (vco_out between 360M and 680M), the
function returns -EINVAL instead of trying other valid pclk_div combinations.

> +	}
> +
> +	if (!sysclk_pclk_ratio_found)
> +		return -EINVAL;
> +
> +	/* Determine an appropriate post divider. */
> +	for (post_div_index = 0; post_div_index < ARRAY_SIZE(post_div_table);
> +	     post_div_index++) {
> +		vco_out = pclk * pclk_div_table[pclk_div_index] *
> +			  post_div_table[post_div_index];

[Severity: High]
Can this overflow the 32-bit vco_out variable?

For higher link frequencies (e.g., link_freq = 96M giving pclk = 192M),
combined with a pclk_div of 6 and a post_div of 8, the mathematical product
is 9,216,000,000. This exceeds the 32-bit limit and overflows to 626,065,408.

Because 626M happens to fall within the valid VCO constraints [360M, 680M],
the driver improperly accepts this completely invalid setup.

> +
> +		if (vco_out >= HM1246_PLL_VCO_MIN &&
> +		    vco_out <= HM1246_PLL_VCO_MAX)
> +			break;
> +	}
> +	if (post_div_index >= ARRAY_SIZE(post_div_table))
> +		return -EINVAL;
> +
> +	/* Find pre-divider and multiplier values. */
> +	for (u32 div = DIV_ROUND_UP(xclk, HM1246_PLL_INCLK_MAX);
> +	     div <= xclk / HM1246_PLL_INCLK_MIN; div++) {
> +		u32 multi, multi_h, multi_l, vco;
> +
> +		multi = DIV_ROUND_CLOSEST_ULL((u64)vco_out * div, xclk);
> +		if (multi < HM1246_PLL_MULTI_MIN ||
> +		    multi > HM1246_PLL_MULTI_MAX)
> +			continue;
> +
> +		multi_h = multi / (HM1246_PLL_MULTI_H_MIN *
> +				   HM1246_PLL_MULTI_L_MAX) +
> +			  2;
> +		multi_l = multi / multi_h;

[Severity: Medium]
Does this logic mistakenly skip valid configurations when multi is exactly 512?

If the required overall multiplier multi is exactly 512, this formula
calculates multi_h as 512 / 512 + 2 = 3. It then sets multi_l = 512 / 3 = 170.
The product is 3 * 170 = 510, which triggers the if (vco != vco_out)
rejection.

However, a multiplier of exactly 512 is valid and achievable with multi_h = 2
and multi_l = 256.

[ ... ]

> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		s64 exposure_max;
> +
> +		exposure_max =
> +			format->height + ctrl->val - HM1246_COARSE_INTG_MARGIN;
> +		ret = __v4l2_ctrl_modify_range(hm1246->exposure_ctrl,
> +					       hm1246->exposure_ctrl->minimum,
> +					       exposure_max,
> +					       hm1246->exposure_ctrl->step,
> +					       exposure_max);
> +
> +		if (ret) {
> +			dev_err(hm1246->dev, "exposure ctrl range update failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (!pm_runtime_get_if_active(hm1246->dev))
> +		return 0;

[Severity: High]
Does this check allow execution to continue on negative error codes?

pm_runtime_get_if_active() returns 1 if the device is active, 0 if it is not
active, and a negative error code (e.g., -EINVAL) if Runtime PM is disabled.

Because of the logical NOT, negative error codes evaluate to false, allowing
execution to bypass the return 0 check. This allows the driver to proceed
with hardware accesses while the device may be unpowered, and leads to an
unmatched pm_runtime_put() at the end of the function.

> +
> +	ret = 0;
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE:
> +		cci_write(hm1246->regmap, HM1246_COARSE_INTG_REG, ctrl->val,
> +			  &ret);
> +		break;
> +
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		cci_write(hm1246->regmap, HM1246_ANALOG_GLOBAL_GAIN_REG,
> +			  ctrl->val, &ret);
> +		break;
> +
> +	case V4L2_CID_VBLANK:
> +		val = format->height + ctrl->val;
> +		cci_write(hm1246->regmap, HM1246_FRAME_LENGTH_LINES_REG, val,
> +			  &ret);
> +		break;
> +
> +	case V4L2_CID_HFLIP:
> +	case V4L2_CID_VFLIP:

[Severity: High]
Does this omission break format negotiation and video pipeline link validation?

The driver registers the V4L2_CID_HFLIP and V4L2_CID_VFLIP controls with the
V4L2_CTRL_FLAG_MODIFY_LAYOUT flag. While the hardware orientation registers
are updated, the format code in the subdevice's active state is not updated
to reflect the new Bayer pattern.

Subsequent calls to the VIDIOC_SUBDEV_G_FMT ioctl will return the stale
Bayer format preserved in the state.

> +		val = 0;
> +		if (hm1246->hflip_ctrl->val)
> +			val |= HM1246_IMAGE_ORIENTATION_HFLIP;
> +		if (hm1246->vflip_ctrl->val)
> +			val |= HM1246_IMAGE_ORIENTATION_VFLIP;
> +
> +		cci_write(hm1246->regmap, HM1246_IMAGE_ORIENTATION_REG, val,
> +			  &ret);
> +		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260619-hm1246-v10-0-d88e431a6c11@emfend.at?part=2