Re: [PATCH v6 4/5] media: hi846: Add 6MP and 8MP modes support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The crop rectangles for the newly added 6MP (3264x1836) and 8MP (3264x2448) modes overflow the native crop bounds of the sensor.

Pre-existing issues:
- [High] Missing mutex_unlock() on the error path in hi846_set_format(), leading to a deadlock.
- [High] Unchecked return value from v4l2_find_nearest_size_conditional() leads to a NULL pointer dereference in hi846_set_format().
--

commit e8c04d7885133a51b470ee7348db670b6ed848dd
Author: Pengyu Luo <mitltlatltl@gmail.com>
Date:   Sun Aug 30 16:00:24 2026 +0000

    media: hi846: Add 6MP and 8MP modes support
    
    Hi846 is an 8MP sensor, but the upstream driver has only supported 2MP
    mode for years. This patch adds 6MP and 8MP modes to maximize sensor
    utilization.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 26b4290148b61..7ad691364cdc1 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
[ ... ]
> @@ -1136,7 +1236,59 @@ static const struct hi846_mode supported_modes[] = {
>  			.width = 1632 * 2,
>  			.height = 1224 * 2,
>  		},
> -	}
> +	},
> +	{
> +		.width = 3264,
> +		.height = 1836,
[ ... ]
> +		.crop = {
> +			.left = 0x46,
> +			.top = 0x172,
> +			.width = 3264,
> +			.height = 1836,
> +		},

[Severity: Medium]
Will this crop configuration violate the V4L2 API contract?

The crop rectangle extends beyond the native crop bounds of the sensor.
The hardcoded absolute crop bounds in hi846_get_selection() are:

	case V4L2_SEL_TGT_CROP_BOUNDS:
	case V4L2_SEL_TGT_NATIVE_SIZE:
		sel->r.top = 0;
		sel->r.left = 0;
		sel->r.width = 3264;
		sel->r.height = 2448;

For this new 3264x1836 mode, an offset of (70, 370) with a width of 3264 means
the right boundary extends to 3334, which strictly exceeds the advertised
3264 maximum limit. Strict userspace frameworks like libcamera or
v4l2-compliance might reject or clip this, breaking functionality.

> +	},
> +	{
> +		.width = 3264,
> +		.height = 2448,
[ ... ]
> +		.crop = {
> +			.left = 0x46,
> +			.top = 0x40,
> +			.width = 3264,
> +			.height = 2448,
> +		},

[Severity: Medium]
Similarly, does this crop configuration overflow the native width and height
bounds?

With an offset of (70, 64) and size 3264x2448, the boundaries are pushed
to 3334x2512, exceeding the 3264x2448 limits from hi846_get_selection().

Should the native bounds be expanded to accommodate these crop offsets, or
should the offsets be adjusted to 0?

While reviewing this patch, I also noticed a couple of pre-existing issues in
the file introduced earlier in this series:

> static int hi846_set_format(struct v4l2_subdev *sd,
> 			    struct v4l2_subdev_state *sd_state,
> 			    struct v4l2_subdev_format *fmt)
> {
[ ... ]
> 	hi846->cur_mode =
> 		v4l2_find_nearest_size_conditional(supported_modes,
> 						   ARRAY_SIZE(supported_modes),
> 						   width, height,
> 						   mf->width, mf->height,
> 						   filter_modes, hi846);
> 
> 	dev_dbg(&client->dev, "%s: found mode: %dx%d\n", __func__,
> 		hi846->cur_mode->width, hi846->cur_mode->height);

[Severity: High]
This isn't a bug introduced by this patch, but does
v4l2_find_nearest_size_conditional() return NULL if no matching mode is found?

If the device tree configuration provides data-lanes = <4> but the
link-frequencies property only matches modes that have 0 registers for 4-lane
configurations, filter_modes() will evaluate to false for every mode. 

This would cause v4l2_find_nearest_size_conditional() to return NULL, leading
to a kernel crash when hi846->cur_mode->width is unconditionally dereferenced.

[ ... ]
> 	idx = hi846_get_link_freq_index(hi846, hi846->cur_mode);
> 	if (idx < 0) {
> 		dev_err(&client->dev,
> 			"failed to get link freq index: %d\n", idx);
> 		return -EINVAL;
> 	}

[Severity: High]
This is a pre-existing issue, but does this error path leave hi846->mutex
locked?

The function returns without calling mutex_unlock(&hi846->mutex), causing a
deadlock on subsequent calls to driver functions that acquire this mutex.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830160025.211384-1-mitltlatltl@gmail.com?part=4