Re: [PATCH v5 2/2] media: i2c: Add driver for AMSOSRMA Mira220

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [High] Dereferencing `mira220->vblank` without checking for NULL in `mira220_init_controls()`.
- [High] Missing `v4l2_subdev_cleanup()` in the driver's remove function causes an active state memory leak.
- [Medium] The uninitialized stack variable `id` is evaluated and logged without checking if the I2C read succeeded.
- [Medium] The active state format code is not updated when `V4L2_CID_HFLIP` or `V4L2_CID_VFLIP` are modified.
- [Medium] Exposure control maximum limit exceeds the 16-bit hardware register width, leading to silent truncation.
- [Medium] Hardware exposure register is not updated when `V4L2_CID_EXPOSURE` is clamped due to a `V4L2_CID_VBLANK` change.
- [Medium] `mira220_power_on()` discards the actual error code when `clk_prepare_enable()` fails.
- [Medium] Missing runtime PM reference hold during probe exposes the device to concurrent suspend.
- [Medium] The error path in probe unconditionally powers off the device without checking its runtime PM state.
- [Medium] V4L2_CID_HBLANK reports an incorrect static value when using a 1-lane MIPI CSI-2 configuration, breaking user-space framerate and exposure calculations.
- [Low] Typo in user-facing control menu item: 'Vertial Gradient' instead of 'Vertical Gradient'.
--

--- Patch [2]: [PATCH v5 2/2] media: i2c: Add driver for AMSOSRMA Mira220 ---
commit fbe78b3a27f19e95a842994664c11c10a075c4aa
Author: Philippe Baetens <philippebaetens@gmail.com>

media: i2c: Add driver for AMSOSRMA Mira220

Add a V4L2 subdev driver for driver for the AMSOSRAM Mira220 image sensor.

diff --git a/drivers/media/i2c/mira220.c b/drivers/media/i2c/mira220.c
index 0000000000000..861f3230375c2
--- /dev/null
+++ b/drivers/media/i2c/mira220.c
@@ -923,10 +923,10 @@ static const cci_reg_sequence mira220_init_reg_list[] = {
[ ... ]
> +static const char *const mira220_test_pattern_menu[] = {
> +	"Disabled",
> +	"Vertial Gradient",
> +};

[Severity: Low]
Is there a typo in the menu item name? It seems like it should be Vertical
Gradient instead of Vertial Gradient.

@@ -1023,12 +1023,12 @@ static int mira220_power_on(struct device *dev)
[ ... ]
> +	ret = clk_prepare_enable(mira220->xclk);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable clock\n", __func__);
> +		goto reg_off;
> +	}
> +
> +	gpiod_set_value_cansleep(mira220->reset_gpio, 0);
> +
> +	return 0;
> +
> +reg_off:
> +	return regulator_bulk_disable(MIRA220_NUM_SUPPLIES, mira220->supplies);
> +}

[Severity: Medium]
Will this discard the actual error code when clk_prepare_enable() fails?

The function returns the result of regulator_bulk_disable(), which typically
returns 0 on success.

This could cause the caller to falsely assume the device was successfully
powered on, potentially leading to confusing I2C read failures later in the
probe process.

@@ -1113,5 +1113,5 @@ static int mira220_write_stop_streaming_regs(struct mira220 *mira220)
[ ... ]
> +static u32 mira220_calc_exposure(struct mira220 *mira220, u32 height, u32 vblank)
> +{
> +	return (height + vblank) -
> +	       (int)(MIRA220_GLOB_NUM_CLK_CYCLES / mira220->row_length);
> +}

[Severity: Medium]
Could this calculation cause the exposure limit to exceed the 16-bit register
capacity? 

With vblank up to 65535, max_exposure can reach roughly 66929. When the value
is written to the hardware register:

ret = cci_write(mira220->regmap, MIRA220_EXP_TIME_REG, exposure, NULL);

it might be silently truncated to 16 bits (e.g., 66929 becomes 1393), resulting
in an incorrectly brief exposure time.

@@ -1155,10 +1155,10 @@ static int mira220_set_ctrl(struct v4l2_ctrl *ctrl)
[ ... ]
> +	if (ctrl->id == V4L2_CID_VBLANK) {
> +		int exposure_max, exposure_def;
> +
> +		/* Update max exposure while meeting expected vblanking */
> +		exposure_max = mira220_calc_exposure(mira220, format->height,
> +						     ctrl->val);
> +		exposure_def = min(exposure_max, MIRA220_DEFAULT_EXPOSURE);
> +		ret = __v4l2_ctrl_modify_range(mira220->exposure,
> +					       mira220->exposure->minimum,
> +					       exposure_max,
> +					       mira220->exposure->step,
> +					       exposure_def);
> +		if (ret)
> +			return ret;
> +	}

[Severity: Medium]
If the current exposure exceeds the new maximum here, the framework clamps it,
but does this code miss updating the hardware exposure register? 

Since exposure and vblank are not clustered, mira220_set_ctrl() is not
triggered for exposure, and only the vblank hardware register is updated.

This could leave the hardware exposure register out-of-sync with the clamped
value.

@@ -1177,10 +1177,10 @@ static int mira220_set_ctrl(struct v4l2_ctrl *ctrl)
[ ... ]
> +	case V4L2_CID_HFLIP:
> +		ret = cci_write(mira220->regmap, MIRA220_HFLIP_REG,
> +				mira220->hflip->val, NULL);
> +		break;
> +	case V4L2_CID_VFLIP:
> +		ret = cci_write(mira220->regmap, MIRA220_VFLIP_REG,
> +				mira220->vflip->val, NULL);
> +		break;

[Severity: Medium]
Should the active state format code be updated when the layout-modifying
controls change the underlying format? 

Modifying these controls alters the Bayer format. Without updating the active
state state->pads[0].format.code, subsequent calls to get the format could
return the stale format code without the flip applied.

@@ -1486,8 +1486,8 @@ static int mira220_identify_module(struct mira220 *mira220)
[ ... ]
> +	mira220_otp_read(mira220, 0x3a, 0, &id, &ret);
> +	if (id < 1 || id > 2) {
> +		dev_err(&client->dev,
> +			"Read OTP 0x3a, id must be 1 or 2, but got: 0x%02x\n", id);
> +		ret = -EINVAL;
> +		goto err_power_off;
> +	}

[Severity: Medium]
Does this evaluate the uninitialized id variable before validating the
accumulated ret status? 

mira220_otp_read() is designed to skip execution if ret is already non-zero.
If any previous OTP read failed, or if reading 0x3a itself fails, id is left
uninitialized. 

This block could potentially log the uninitialized value before checking the
error code in ret.

@@ -1837,12 +1837,12 @@ static int mira220_init_controls(struct mira220 *mira220)
[ ... ]
> +	hblank = v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_HBLANK,
> +				   MIRA220_HBLANK_1600x1400_304,
> +				   MIRA220_HBLANK_1600x1400_304, 1,
> +				   MIRA220_HBLANK_1600x1400_304);

[Severity: Medium]
Will this hardcoded hblank value report an incorrect static value when using a
1-lane MIPI CSI-2 configuration? 

It assumes a 2-lane layout where row_length is 304. If the device parses a
1-lane configuration, row_length doubles to 608, which would effectively
double the line time, meaning hblank should be dynamically set to 4480. 

Because the control remains at 1440, user space might be misled into
calculating a falsely high frame rate.

> +	if (hblank)
> +		hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	/* Max exposure is determined by vblank + vsize and Tglob. */
> +	max_exposure = mira220_calc_exposure(mira220,
> +					     MIRA220_PIXEL_ARRAY_HEIGHT,
> +					     mira220->vblank->val);

[Severity: High]
Can mira220->vblank be NULL here? 

The v4l2_ctrl_new_std() call above could fail and return NULL if memory
allocation fails. 

Dereferencing mira220->vblank->val immediately without verifying if the
creation succeeded could cause a kernel panic during device probe.

@@ -1936,6 +1936,6 @@ static int mira220_probe(struct i2c_client *client)
[ ... ]
> +	/* Enable runtime PM and turn off the device */
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	ret = mira220_identify_module(mira220);

[Severity: Medium]
Is it possible for the device to be concurrently suspended while probe
continues? 

The device is marked active with a usage count of 0 after pm_runtime_enable().
If the PM subsystem or userspace triggers a suspend (e.g. by setting
autosuspend_delay_ms=0), the device could be powered down while probe executes
mira220_identify_module().

This could lead to I2C timeouts or bus crashes. Taking a pm_runtime reference
hold could prevent this.

@@ -1985,5 +1985,5 @@ static int mira220_probe(struct i2c_client *client)
[ ... ]
> +error_power_off:
> +	pm_runtime_disable(dev);
> +	mira220_power_off(dev);
> +	pm_runtime_set_suspended(dev);
> +	return ret;
> +}

[Severity: Medium]
Should this error path check the runtime PM state before unconditionally
powering off the device? 

The PM core might have already suspended the device due to the zero usage count.

Calling mira220_power_off() unconditionally could redundantly disable clocks
and regulators that are already disabled, potentially causing double-disable
warnings.

@@ -1992,7 +1992,7 @@ static void mira220_remove(struct i2c_client *client)
[ ... ]
> +static void mira220_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct mira220 *mira220 = to_mira220(sd);
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +
> +	v4l2_ctrl_handler_free(mira220->sd.ctrl_handler);

[Severity: High]
Does this unbind path miss a call to v4l2_subdev_cleanup()? 

The driver initializes the subdev active state via v4l2_subdev_init_finalize()
during probe, but fails to clean it up in mira220_remove(). 

This could permanently leak the memory allocated for the active state and pads
every time the driver is unbound.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-mira220-v5-0-533b0ad1065e@ideasonboard.com?part=2