Re: [PATCH v3 2/2] media: i2c: mira016: Add driver for Mira016

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] Unconditional dereference of v4l2_subdev_state_get_format() result causes NULL pointer dereference when CONFIG_MEDIA_CONTROLLER is disabled.
- [High] Incorrect handling of pm_runtime_get_if_in_use() return value treats negative error codes as success, causing usage counter underflow.
- [High] Use-After-Free due to V4L2 control handler being freed before unregistering the subdevice.
- [High] The read_poll_timeout macro swallows the error code from cci_read and fails silently.
- [High] Horizontal ROI registers are improperly defined as 8-bit, truncating their 16-bit values.
- [High] Active state format is overwritten before checking if the subdevice is currently streaming, corrupting the software state.
- [High] Missing delays after enabling regulators, clock, and deasserting reset.
- [Medium] Severe precision loss in pixel rate calculation due to premature integer division.
- [Medium] Mixing goto-based error handling with __free() scope-based cleanup in the same function.
- [Medium] Inconsistent initialization of row_length creates a mismatch between default control limits and explicit format control limits.
- [Low] Integer division always evaluates to 0 in unused esc_period calculation.
--

commit 9c6378991c811d36f2a5849235c3c88b6af0b9b5
Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

media: i2c: mira016: Add driver for Mira016
    
Add driver for the ams OSRAM Mira016 sensor.

> diff --git a/drivers/media/i2c/mira016.c b/drivers/media/i2c/mira016.c
> new file mode 100644
> index 0000000000000..8286b7e2c0a42
> --- /dev/null
> +++ b/drivers/media/i2c/mira016.c
@@ -0,0 +1,2300 @@
[ ... ]
> +/* X ROI */
> +#define MIRA016_XWIN_LEFT_REG		CCI_REG8(0xe02c)
> +#define MIRA016_XWIN_RIGHT_REG		CCI_REG8(0xe02e)

[Severity: High]
Does this code improperly truncate 16-bit register values?

The horizontal window boundaries can exceed 255 (up to 400), but using
CCI_REG8 limits them to 8 bits. When writing these values later via
cci_write(), they will be masked to 8 bits, completely breaking the
horizontal window boundaries. Should these be defined as 16-bit registers?

[ ... ]
> +static inline u32 mira016_calc_prate(struct mira016 *mira016, u32 h_tot)
> +{
> +	/*
> +	 * Pixel rate is calculate as the row duration divided by the total line
> +	 * length.
> +	 *
> +	 * pixel time (psec) = t_row(psec) / h_tot
> +	 * pixel rate (mbps) = 10^12 / pixel_time
> +	 *		     = 10^12 * h_tot / t_row
> +	 *		     = h_tot * 10^6 / t_row * 10^6 to avoid overflows
> +	 */
> +	u32 trow_psec = mira016_trow_psec(mira016);
> +
> +	return h_tot * HZ_PER_MHZ / trow_psec * HZ_PER_MHZ;

[Severity: Medium]
Will this evaluate left-to-right and cause severe precision loss?

The integer division by trow_psec happens before the final multiplication
by HZ_PER_MHZ, acting as a floor function and truncating all sub-MHz
precision. Could this be reworked to retain precision?

[ ... ]
> +static int mira016_set_pad_format(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_state *state,
> +				  struct v4l2_subdev_format *fmt)
> +{
> +	struct mira016 *mira016 = to_mira016(sd);
> +	struct v4l2_rect *crop;
> +	u32 pixel_rate;
> +	u32 min_vblank;
> +	u32 gain_max;
> +	int ret;
> +
> +	mira016_update_pad_format(mira016, &fmt->format, fmt->format.code);
> +	*v4l2_subdev_state_get_format(state, 0) = fmt->format;

[Severity: Critical]
Is it possible for v4l2_subdev_state_get_format() to return a NULL pointer?

When CONFIG_MEDIA_CONTROLLER is disabled, the state parameter can be NULL,
which causes v4l2_subdev_state_get_format() to safely return NULL.
Dereferencing it directly here would cause a kernel panic. Should this
result be checked?

> +
> +	crop = v4l2_subdev_state_get_crop(state, 0);

[Severity: Critical]
Similar to the format pointer above, could v4l2_subdev_state_get_crop()
return NULL here and cause a kernel panic if CONFIG_MEDIA_CONTROLLER is
disabled?

> +	crop->width = fmt->format.width;
> +	crop->height = fmt->format.height;
> +	crop->left = MIRA016_PIXEL_ARRAY_LEFT;
> +	crop->top = MIRA016_PIXEL_ARRAY_TOP;
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> +		return 0;
> +
> +	/*
> +	 * Update the row length: changing the image format implies changing the
> +	 * row_length parameter, which changes the line duration and the pixel
> +	 * rate consequentially. Also, changing the image format changes the
> +	 * analogue gain limits.
> +	 *
> +	 * Do not allow to change image format while the subdevice is streaming.
> +	 *
> +	 * TODO: row length depends on binning, update it also in the
> +	 * implementation of set_selection.
> +	 */
> +	if (v4l2_subdev_is_streaming(sd))
> +		return -EBUSY;

[Severity: High]
Does this code corrupt the active state if streaming is already active?

The active software state is overwritten before checking if the subdevice
is streaming. If this check returns -EBUSY, the rejected format remains
in the software state rather than what's actually active on hardware.

[ ... ]
> +static int mira016_get_selection(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *state,
> +				 struct v4l2_subdev_selection *sel)
> +{
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP:
> +		sel->r = *v4l2_subdev_state_get_crop(state, 0);

[Severity: Critical]
Can this dereference a NULL pointer if CONFIG_MEDIA_CONTROLLER is disabled?

The v4l2_subdev_state_get_crop() helper may return NULL, which would cause
a kernel panic when dereferenced here.

[ ... ]
> +static int mira016_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
[ ... ]
> +
> +	if (!pm_runtime_get_if_in_use(&client->dev))
> +		return 0;

[Severity: High]
Does this treat negative error codes as success?

If runtime PM is disabled, pm_runtime_get_if_in_use() returns -EINVAL.
In C, !(-EINVAL) is false, which bypasses the early return. This allows the
driver to proceed accessing hardware that may be unpowered, and later call
pm_runtime_put_autosuspend(), causing an unbalanced usage counter underflow.

[ ... ]
> +static int mira016_power_on(struct device *dev)
> +{
[ ... ]
> +	ret = clk_prepare_enable(mira016->xclk);
> +	if (ret) {
> +		dev_err(&client->dev, "%s: failed to enable clock\n", __func__);
> +		regulator_bulk_disable(ARRAY_SIZE(mira016_supplies),
> +				       mira016->supplies);
> +		return ret;
> +	}
> +
> +	gpiod_set_value_cansleep(mira016->reset_gpio, 0);
> +
> +	return 0;

[Severity: High]
Are stabilization delays missing after enabling regulators, clock, and
deasserting reset?

Hardware sensors typically require stabilization time before their I2C
interface becomes responsive. Returning immediately here could cause the
first I2C read in mira016_identify_module() to fail with a NACK.

[ ... ]
> +static int mira016_configure_timings(struct mira016 *mira016)
> +{
[ ... ]
> +	cci_write(mira016->regmap, MIRA016_PLL_PD_REG, 0, &ret);
> +	if (ret)
> +		return ret;
> +
> +	ret = read_poll_timeout(cci_read, ret,
> +				((ret < 0 || (val & MIRA016_PLL_LOCKED))),
> +				1000, 1000, false, mira016->regmap,
> +				MIRA016_PLL_LOCK_REG, &val, NULL);
> +	if (ret < 0)
> +		return ret;

[Severity: High]
Does this condition swallow the error code from cci_read?

If cci_read fails and returns a negative error (e.g., -EIO), the condition
(ret < 0 || (val & MIRA016_PLL_LOCKED)) evaluates to true. The macro will
successfully exit the loop and return 0, masking the critical hardware read
failure and bypassing the subsequent error check.

[ ... ]
> +static int mira016_configure_horizontal_roi(struct mira016 *mira016,
> +					    struct v4l2_subdev_state *state)
> +{
[ ... ]
> +	u16 xwin_right = crop->left + crop->width - 1;
> +	u16 fifo_threshold;
> +	u16 hsync_length;
> +	int ret = 0;
> +
> +	cci_write(mira016->regmap, MIRA016_XWIN_LEFT_REG, crop->left, &ret);
> +	cci_write(mira016->regmap, MIRA016_XWIN_RIGHT_REG, xwin_right, &ret);

[Severity: High]
Can xwin_right be truncated here?

Since MIRA016_XWIN_RIGHT_REG is defined as CCI_REG8, the 16-bit xwin_right
value will be masked to 8 bits by cci_write(), corrupting the horizontal
window boundaries.

[ ... ]
> +static int mira016_parse_endpoint(struct device *dev, struct mira016 *mira016)
> +{
> +	struct fwnode_handle *endpoint __free(fwnode_handle) = NULL;
> +	struct v4l2_fwnode_endpoint ep_cfg = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> +	};
> +	int ret;
[ ... ]
> +	ret = v4l2_link_freq_to_bitmap(dev, ep_cfg.link_frequencies,
> +				       ep_cfg.nr_of_link_frequencies,
> +				       mira016_link_freqs,
> +				       ARRAY_SIZE(mira016_link_freqs),
> +				       &mira016->link_freq_bitmap);
> +	if (ret)
> +		goto err_free_ep;
> +
> +	/* TODO: Implement D-PHY configuration to support continuous clock. */
> +	if (!(ep_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)) {
> +		dev_err(dev, "Continuous clock is not supported\n");
> +		ret = -EINVAL;
> +		goto err_free_ep;
> +	}
> +
> +	mira016->bus_config = ep_cfg.bus.mipi_csi2.flags;
> +
> +err_free_ep:
> +	v4l2_fwnode_endpoint_free(&ep_cfg);
> +
> +	return ret;

[Severity: Medium]
Is it safe to mix goto-based error handling with scope-based cleanup here?

The core kernel cleanup subsystem rules forbid mixing goto-based error
handling with __free() scope-based cleanup helpers in the same function,
as it creates confusing ownership semantics.

[ ... ]
> +static int mira016_validate_xclk_freq(struct mira016 *mira016)
> +{
[ ... ]
> +	mira016->pll.esc_l = mira016->xclk_freq < MHZ(20) ? 0 :
> +			     mira016->xclk_freq < MHZ(60) ? 1 : 2;
> +	mira016->pll.esc_period = mira016->xclk_freq < MHZ(20) ?
> +				  DIV_ROUND_CLOSEST(1000, mira016->xclk_freq)

[Severity: Low]
Does this calculation always evaluate to 0?

Since xclk_freq is constrained to be greater than 12MHz, dividing 1000 by
xclk_freq will always yield 0. Also, this variable appears to be unused
after being assigned.

[ ... ]
> +static int mira016_probe(struct i2c_client *client)
> +{
[ ... ]
> +	/*
> +	 * Calculate the PLL configuration based on the link frequency
> +	 * selected by .dts and compute the sensor timing bases.
> +	 *
> +	 * Initialize row_length to a value matching the default format for
> +	 * exposure and frame time limits calculations.
> +	 */
> +	mira016_pll_calc(mira016);
> +	mira016_timings_calc(mira016);
> +	mira016->timings.row_length = 1262;

[Severity: Medium]
Does this hardcoded row_length create a mismatch with dynamic calculations?

This establishes initial frame rate limits. However, if userspace applies
the exact same default format later, mira016_calc_row_length() might
calculate a different value (e.g., 1062), unexpectedly altering the limits
even though the format didn't logically change.

[ ... ]
> +static void mira016_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct mira016 *mira016 = to_mira016(sd);
> +
> +	v4l2_ctrl_handler_free(mira016->sd.ctrl_handler);
> +
> +	v4l2_async_unregister_subdev(sd);

[Severity: High]
Does this teardown order create a use-after-free window?

By freeing the control handler before unregistering the subdevice, the V4L2
device node remains accessible to userspace. If a control ioctl is issued
during this race window, the kernel will attempt to dereference the freed
control handler or lock the destroyed mutex. Should these calls be swapped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-mira016-v3-0-b078a6793d65@ideasonboard.com?part=2