Re: [PATCH v5 2/4] media: ox05b1s: Add omnivision OX05B1S raw sensor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] media: ox05b1s: NULL pointer dereference via of_device_get_match_data()
- [Medium] media: ox05b1s: vblank control minimum allows VTS below max exposure
- [Medium] media: ox05b1s: unsigned subtraction underflow in nearest size calculation
- [Medium] media: ox05b1s: precision loss in HTS calculation corrupts register write
--

commit b187cd28d540e370c1f99f3b0849d51ecd800efc
Author: Mirela Rabulea <mirela.rabulea@nxp.com>

media: ox05b1s: Add omnivision OX05B1S raw sensor driver

Add a v4l2 subdevice driver for the Omnivision OX05B1S RGB-IR sensor.
The Omnivision OX05B1S is a 1/2.5-Inch CMOS image sensor with an
active array size of 2592 x 1944.

> diff --git a/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c b/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
> new file mode 100644
> index 0000000000000..45d38dc836e6a
> --- /dev/null
> +++ b/drivers/media/i2c/ox05b1s/ox05b1s_mipi.c
[ ... ]
> +static int ox05b1s_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +	case V4L2_CID_HBLANK:
> +		/*
> +		 * Convert pixel-domain hblank back to SA1_CLK cycles for HTS.
> +		 * pixel_rate = link_freq * 2 * num_data_lanes / bpp
> +		 * hts_cycles = (width + hblank) * sa1_clk / pixel_rate
> +		 */
> +		pixel_rate = div_u64((u64)sensor->mode->link_freq * 2 *
> +				     sensor->num_data_lanes,
> +				     sensor->mode->bpp);
> +		hts = (u32)div_u64((u64)(w + ctrl->val) * sensor->mode->sa1_clk,
> +				   pixel_rate);
> +		ret = cci_write(sensor->regmap, OX05B1S_REG_TIMING_HTS,
> +				hts, NULL);

[Severity: Medium]
Will this truncating integer division cause precision loss? When HTS cycles
are converted to pixels for the HBLANK control and back here, rounding
errors can lead to an incorrect, off-by-one timing value being written to
the sensor, which might result in framing errors or frame rate
discrepancies.

> +		break;
[ ... ]
> +static int ox05b1s_update_controls(struct ox05b1s *sensor)
> +{
[ ... ]
> +	/*
> +	 * hblank in pixels = HTS_cycles * pixel_rate / sa1_clk - width
> +	 * Computed as u64 to avoid overflow, then truncated to u32.
> +	 */
> +	hblank = (u32)div_u64((u64)hts * pixel_rate,
> +			      sensor->mode->sa1_clk) - sensor->mode->width;

[Severity: Medium]
Could this truncation compound the precision loss mentioned above when
calculating HTS?

> +
> +	ret = __v4l2_ctrl_modify_range(sensor->ctrls.hblank, hblank, hblank,
> +				       1, hblank);
> +	if (ret) {
> +		dev_err(dev, "Modify range for hblank %u-%u failed\n",
> +			hblank, hblank);
> +		goto out;
> +	}
> +	__v4l2_ctrl_s_ctrl(sensor->ctrls.hblank,
> +			   sensor->ctrls.hblank->default_value);
> +
> +	ret = __v4l2_ctrl_modify_range(sensor->ctrls.vblank, 0, vblank * 4,
> +				       1, vblank);

[Severity: Medium]
Is it safe to hardcode the vblank minimum to 0? The maximum exposure is
statically set based on the default VTS. If vblank is reduced to 0, does
this allow user space to request an exposure time greater than the frame
time, potentially causing sensor lockup?

> +	if (ret) {
[ ... ]
> +static const struct ox05b1s_mode *ox05b1s_nearest_size(const struct ox05b1s_mode *supported_modes,
> +						       struct v4l2_subdev_format *fmt)
> +{
[ ... ]
> +		if (supported_modes->code != fmt->format.code)
> +			continue;
> +
> +		err = abs(w - fmt->format.width) + abs(h - fmt->format.height);

[Severity: Medium]
Since both w and fmt->format.width are unsigned 32-bit integers, won't
this subtraction underflow and wrap around to a large positive value before
abs() evaluates it? Could this result in an incorrect mode selection when
falling back to the nearest size?

> +		if (err > min_error)
> +			continue;
[ ... ]
> +static int ox05b1s_read_chip_id(struct ox05b1s *sensor)
> +{
[ ... ]
> +	switch (chip_id) {
> +	case OX05B1S_CHIP_ID:
> +		camera_name = "ox05b1s";
> +		break;
> +	default:
> +		camera_name = "unknown";
> +		break;
> +	}
> +
> +	if (chip_id == sensor->model->chip_id) {

[Severity: High]
Does this unconditionally dereference sensor->model? If the device was
instantiated without an OF node, sensor->model would be NULL, potentially
leading to a kernel panic here.

> +		dev_dbg(dev, "Camera %s detected, chip_id=%llx\n",
> +			camera_name, chip_id);
> +	} else {
[ ... ]
> +static int ox05b1s_probe(struct i2c_client *client)
> +{
[ ... ]
> +	sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> +	if (IS_ERR(sensor->regmap))
> +		return dev_err_probe(dev, PTR_ERR(sensor->regmap),
> +				     "Failed to allocate sensor register map\n");
> +
> +	sensor->i2c_client = client;
> +
> +	sensor->model = of_device_get_match_data(dev);

[Severity: High]
Can of_device_get_match_data() return NULL here? If the device is
instantiated via sysfs new_device, ACPI PRP0001, or i2c_board_info without
an OF node, this assignment leaves sensor->model as NULL. Should this be
checked, or should device_get_match_data() / i2c_get_match_data() be used
instead?

> +
> +	ox05b1s_get_gpios(sensor);
> +	if (IS_ERR(sensor->rst_gpio))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005175102.2358881-1-mirela.rabulea@nxp.com?part=2