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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The control handler is freed before unregistering the subdevice, causing a use-after-free and mutex corruption.
- [High] Catastrophic precision loss in the pixel rate calculation due to sub-optimal integer division order.
- [High] `MIRA016_NUM_FRAMES_REG` is incorrectly defined as a 32-bit register, causing it to overlap with `MIRA016_TARGET_FRAME_TIME_REG`.
- [High] 64-bit divisions will cause build failures on 32-bit architectures.
- [Medium] `read_poll_timeout` silently swallows I2C read errors by overwriting the error code with 0.
- [Medium] Active state is corrupted when set_fmt is called during streaming.
--

commit 2b80e1eeb468d23eb8cad50e0145a2e47627a080
Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

media: i2c: mira016: Add driver for Mira016

This commit introduces a new V4L2 I2C sensor driver for the ams OSRAM Mira016
camera.

> diff --git a/drivers/media/i2c/mira016.c b/drivers/media/i2c/mira016.c
> new file mode 100644
> index 0000000000000..4ea2d87bd63c8
> --- /dev/null
> +++ b/drivers/media/i2c/mira016.c
> @@ -0,0 +1,2300 @@
[ ... ]
> +#define MIRA016_EXP_TIME_L_REG		CCI_REG32(0x000e)
> +#define MIRA016_EXP_TIME_S_REG		CCI_REG32(0x0012)
> +#define MIRA016_NUM_FRAMES_REG		CCI_REG32(0x0007)
> +#define MIRA016_TARGET_FRAME_TIME_REG	CCI_REG32(0x0008)

[Severity: High]
MIRA016_NUM_FRAMES_REG is defined as a 32-bit register at 0x0007, and
MIRA016_TARGET_FRAME_TIME_REG is at 0x0008. Will a subsequent write to
0x0008 corrupt the higher bytes of the write to 0x0007?

[ ... ]
> +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: High]
Does this sequence cause precision loss? The division by trow_psec happens
before the final multiplication, truncating the result to the nearest
megahertz.

[ ... ]
> +static int mira016_configure_timings(struct mira016 *mira016)
> +{
> +	int ret = 0;
> +	u64 val;
> +
> +	/* Configure the PLL clock tree. */
> +
> +	cci_write(mira016->regmap, MIRA016_BANK_SEL_REG, 0, &ret);
[ ... ]
> +	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: Medium]
If cci_read fails, it returns a negative error code in ret. Does
read_poll_timeout swallow this error by overwriting ret with 0 upon
successfully breaking the loop without a timeout?

[ ... ]
> +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;
> +
> +	crop = v4l2_subdev_state_get_crop(state, 0);
> +	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: Medium]
The active state is modified in-place before checking if the subdevice is
streaming. If it is streaming, the hardware update is aborted but the active
state remains modified. Can this leave the active state out of sync with the
hardware?

[ ... ]
> +static int mira016_write_frame_duration_reg(struct mira016 *mira016,
> +					    struct v4l2_subdev_state *state,
> +					    u32 vblank_lines)
> +{
> +	struct v4l2_rect *crop = v4l2_subdev_state_get_crop(state, 0);
> +	u32 frame_duration;
> +	int ret = 0;
> +
> +	frame_duration = mira016_lines_to_usec(mira016,
> +					       crop->height + vblank_lines);
> +
> +	/* Write Bank 1 context 0 and context 1 */
> +	cci_write(mira016->regmap, MIRA016_RW_CONTEXT_REG, 0, &ret);
> +	cci_write(mira016->regmap, MIRA016_BANK_SEL_REG, 1, &ret);
> +	cci_write(mira016->regmap, MIRA016_NUM_FRAMES_REG, 1, &ret);
> +	cci_write(mira016->regmap, MIRA016_TARGET_FRAME_TIME_REG,
> +		  frame_duration, &ret);

[Severity: High]
As mentioned earlier, since MIRA016_NUM_FRAMES_REG is defined as a 32-bit
register at 0x0007 and MIRA016_TARGET_FRAME_TIME_REG is at 0x0008, won't this
subsequent write corrupt the data written to the higher bytes of 0x0007?

[ ... ]
> +static void mira016_timings_calc(struct mira016 *mira016)
> +{
> +	/* Time base is the number of input clock cycles per microsecond. */
> +	mira016->timings.time_base = mira016->xclk_freq / HZ_PER_MHZ;
> +
> +	/* Seq time base is in pico-seconds, to not lose precision. */
> +	mira016->timings.seq_time_base = 8 * HZ_PER_MHZ
> +				       / mira016->pll.data_rate_mbps;

[Severity: High]
Since mira016->pll.data_rate_mbps is a u64, will this direct 64-bit division
cause a linker error on 32-bit architectures?

[ ... ]
> +static void mira016_pll_calc(struct mira016 *mira016)
> +{
> +	/* Multiply link_freq by 2 to account for D-PHY DDR. */
> +	u64 target_mbps = mira016_link_freqs[__ffs(mira016->link_freq_bitmap)] * 2;
> +	u32 clk_in = mira016->xclk_freq;
> +	u32 best = UINT_MAX;
> +	bool found = false;
> +	u32 n_best = 0;
> +	u32 m_best = 0;
> +	u32 gran_tg;
> +	u32 n;
> +	u32 m;
[ ... ]
> +	mira016->pll.otp_gran = DIV_ROUND_UP_ULL(mira016->pll.data_rate_mbps, 160) - 1;
> +
> +	/*
> +	 * As the division might introduce rounding errors, compute gran_tg
> +	 * in the Kbps domain and then divide by 10^3.
> +	 */
> +	gran_tg = mira016->pll.data_rate_mbps * KHZ_PER_MHZ / 1500;

[Severity: High]
Similarly here, won't this direct 64-bit division cause build failures on
32-bit architectures?

> +	mira016->pll.gran_tg = DIV_ROUND_UP_ULL(gran_tg * 50, KHZ_PER_MHZ);
> +
> +	mira016->pll.lut_del = DIV_ROUND_UP_ULL(7000, mira016->pll.gran_tg) - 140;
> +	mira016->pll.byte_period = DIV_ROUND_DOWN_ULL(8000,
> +						      mira016->pll.data_rate_mbps);

[Severity: High]
DIV_ROUND_DOWN_ULL uses do_div() internally, which throws a compile-time error
when given a 64-bit divisor on 32-bit targets. Should a different macro be used
here?

[ ... ]
> +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);
> +	v4l2_subdev_cleanup(&mira016->sd);
> +	media_entity_cleanup(&sd->entity);

[Severity: High]
The control handler is freed before unregistering the subdevice. Can this
cause a use-after-free and mutex corruption if userspace issues an ioctl
concurrently with module removal?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-mira016-v4-0-1950504c131c@ideasonboard.com?part=2