Re: [PATCH v6 3/5] media: hi846: Fix modes handling for different lane cases

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] v4l2_find_nearest_size_conditional() can return NULL if all modes are filtered out, leading to a NULL pointer dereference.
- [Medium] Missing mutex_unlock(&hi846->mutex) on the error path in hi846_set_format().
--

commit 76f8fe21d198e6d98886ca4cb671239b0ef7030d
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: Fix modes handling for different lane cases

When using 4-lane, v4l2_find_nearest_size may return an unsupported
mode. This commit uses v4l2_find_nearest_size_conditional() to filter
it out.

Link: https://lore.kernel.org/linux-media/OmTXoHZJTSGePymL9I-1Cw@puri.sm

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 2f8624f9bdf3a..26b4290148b61 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
[ ... ]
> @@ -1736,20 +1751,6 @@ static int hi846_set_format(struct v4l2_subdev *sd,
>  		return 0;
>  	}
>  
> -	if (hi846->nr_lanes == 2) {
> -		if (!hi846->cur_mode->reg_list_2lane.num_of_regs) {
> -			dev_err(&client->dev,
> -				"this mode is not supported for 2 lanes\n");
> -			return -EINVAL;
> -		}
> -	} else {
> -		if (!hi846->cur_mode->reg_list_4lane.num_of_regs) {
> -			dev_err(&client->dev,
> -				"this mode is not supported for 4 lanes\n");
> -			return -EINVAL;
> -		}
> -	}
> -
>  	mutex_lock(&hi846->mutex);
>  
>  	if (hi846->streaming) {
> @@ -1760,9 +1761,12 @@ static int hi846_set_format(struct v4l2_subdev *sd,
>  	hi846->fmt = fmt;
>  
>  	hi846->cur_mode =
> -		v4l2_find_nearest_size(supported_modes,
> -				       ARRAY_SIZE(supported_modes),
> -				       width, height, mf->width, mf->height);
> +		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]
Does v4l2_find_nearest_size_conditional() have the potential to return NULL
here?

If the device tree's link-frequencies property does not match any mode for the
configured number of lanes, filter_modes() could filter out all modes. If this
happens, v4l2_find_nearest_size_conditional() would return NULL, which would
then be dereferenced in the dev_dbg() call immediately after.

Could we add a NULL check for hi846->cur_mode before dereferencing it?

[Severity: Medium]
In the error path slightly below this diff, if hi846_get_link_freq_index()
returns a negative value, it appears we return without unlocking the mutex:

	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;
	}

Since hi846->mutex is acquired earlier in the function, does this return path
need a mutex_unlock(&hi846->mutex)?

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