Re: [PATCH 2/3] media: i2c: add imx576 image sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect check of pm_runtime_get_if_active() return value allows usage count underflow and unprotected hardware access.
- [High] The sensor's HOLD register is not released if a control write fails, permanently freezing parameter updates.
- [High] The calculation of `pixel_rate` suffers from a 32-bit unsigned integer overflow, resulting in severely incorrect pixel rate reporting on 32-bit architectures.
- [Low] The dev_pm_ops structure is assigned unconditionally to the driver, producing dead code when CONFIG_PM is disabled.
--

commit 59203a6d78786f12f9149e145b22e86d70893eef
Author: Himanshu Bhavani <himanshu.bhavani@siliconsignals.io>
Date:   Wed May 20 11:56:34 2026 +0000

media: i2c: add imx576 image sensor driver

Add a v4l2 subdevice driver for the Sony imx576 sensor. The driver includes
support for manual exposure, analog gain, and vblank/hblank controls.

> diff --git a/drivers/media/i2c/imx576.c b/drivers/media/i2c/imx576.c
> new file mode 100644
> index 0000000000000..910cbcfb6031f
> --- /dev/null
> +++ b/drivers/media/i2c/imx576.c
[ ... ]
> +static int imx576_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct imx576 *imx576 = container_of_const(ctrl->handler,
> +						   struct imx576, handler);
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_mbus_framefmt *fmt;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_get_locked_active_state(&imx576->sd);
> +	fmt = v4l2_subdev_state_get_format(state, 0);
> +
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		/* Honour the VBLANK limits when setting exposure */
> +		ret = __v4l2_ctrl_modify_range(imx576->exposure,
> +					       IMX576_EXPOSURE_MIN,
> +					       fmt->height + ctrl->val -
> +					       IMX576_EXPOSURE_OFFSET,
> +					       1, IMX576_EXPOSURE_DEFAULT);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (pm_runtime_get_if_active(imx576->dev) == 0)
> +		return 0;

[Severity: High]
Could this lead to an unbalanced usage counter and invalid hardware access if
Runtime PM is disabled? pm_runtime_get_if_active() can return a negative error
code (like -EINVAL). If it returns less than zero, this check evaluates to
false, which would allow the function to continue accessing hardware and
eventually call pm_runtime_put() without a prior successful get. Should this
check be <= 0 instead?

> +
> +	cci_write(imx576->regmap, IMX576_REG_HOLD, 1, &ret);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_VBLANK: {
> +		u64 vmax = fmt->height + ctrl->val;
> +
> +		ret = cci_write(imx576->regmap, IMX576_REG_VTS, vmax, &ret);
> +		break;
> +	}
> +	case V4L2_CID_EXPOSURE:
> +		ret = cci_write(imx576->regmap, IMX576_REG_EXPOSURE, ctrl->val, &ret);
> +		break;
> +	case V4L2_CID_ANALOGUE_GAIN:
> +		ret = cci_write(imx576->regmap, IMX576_REG_ANALOG_GAIN,
> +				ctrl->val, &ret);
> +		break;
> +	default:
> +		dev_err(imx576->dev, "Invalid control %d\n", ctrl->id);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	cci_write(imx576->regmap, IMX576_REG_HOLD, 0, &ret);

[Severity: High]
Does this leave the sensor permanently stuck in HOLD mode if any of the
preceding control writes fail?

Since cci_write() acts as a no-op when the passed error pointer holds a
non-zero value, a failure in updating exposure or gain would cause this final
write to be skipped.

Should this final write use a separate error variable or pass NULL to
guarantee the HOLD register is released?

> +
> +	pm_runtime_put(imx576->dev);
> +
> +	return ret;
> +}
[ ... ]
> +static int imx576_init_controls(struct imx576 *imx576)
> +{
> +	const struct imx576_mode *mode = &supported_modes_10bit[0];
> +	struct v4l2_fwnode_device_properties props;
> +	u64 vblank_def, hblank_def, pixel_rate;
> +	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	u32 lpfr;
> +	int ret;
[ ... ]
> +	imx576->vblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx576_ctrl_ops,
> +					   V4L2_CID_VBLANK, vblank_def,
> +					   IMX576_VBLANK_MAX, 1,
> +					   vblank_def);
> +
> +	/* pixel_rate = link_frequency * 2 * nr_of_lanes / bits_per_sample */
> +	pixel_rate = div_u64(IMX576_LINK_FREQ_600MHZ * 2 * IMX576_NUM_DATA_LANES, 10);

[Severity: High]
Can this multiplication overflow on 32-bit architectures?

IMX576_LINK_FREQ_600MHZ expands to (600 * HZ_PER_MHZ), which evaluates as a
32-bit unsigned long. Multiplying this by 2 and then by 4 (the number of
data lanes) yields 4,800,000,000, which exceeds the maximum 32-bit unsigned
value and will wrap around before it is implicitly promoted to u64 for
div_u64().

Would it be safer to explicitly cast IMX576_LINK_FREQ_600MHZ to u64 before
the multiplication?

> +	v4l2_ctrl_new_std(ctrl_hdlr, &imx576_ctrl_ops, V4L2_CID_PIXEL_RATE,
> +			  pixel_rate, pixel_rate, 1, pixel_rate);
[ ... ]
> +static DEFINE_RUNTIME_DEV_PM_OPS(imx576_pm_ops,
> +				 imx576_power_off, imx576_power_on, NULL);
> +
> +static const struct of_device_id imx576_of_match[] = {
> +	{ .compatible = "sony,imx576" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx576_of_match);
> +
> +static struct i2c_driver imx576_driver = {
> +	.driver = {
> +		.name = "imx576",
> +		.pm = &imx576_pm_ops,

[Severity: Low]
Will this cause dead code to be compiled into the kernel when CONFIG_PM
is disabled? According to the Power Management subsystem guidelines,
the dev_pm_ops structure pointer should be assigned using the pm_ptr()
wrapper to allow the compiler to eliminate the unused structure and its
callbacks.

> +		.of_match_table = imx576_of_match,
> +	},
> +	.probe = imx576_probe,
> +	.remove = imx576_remove,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520115641.11729-1-himanshu.bhavani@siliconsignals.io?part=2