Re: [PATCH v6 2/5] media: hi846: Fix link frequency handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete filtering of unsupported link frequencies during probe causes state corruption, leading to a kernel crash (NULL pointer dereference) in the final series.
--

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

media: hi846: Fix link frequency handling

This patch modifies link frequency handling to calculate supported values
dynamically based on PLL configuration, lane count, and MCLK rate. It replaces
hardcoded frequencies and uses v4l2_link_freq_to_bitmap() to find the
intersection with device tree values.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 7f069aca0fce5..2f8624f9bdf3a 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
[ ... ]
> @@ -1749,7 +1774,14 @@ static int hi846_set_format(struct v4l2_subdev *sd,
>  	mf->code = HI846_MEDIA_BUS_FORMAT;
>  	mf->field = V4L2_FIELD_NONE;
>  
> -	__v4l2_ctrl_s_ctrl(hi846->link_freq, hi846_get_link_freq_index(hi846));
> +	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;
> +	}
> +
> +	__v4l2_ctrl_s_ctrl(hi846->link_freq, idx);

[Severity: High]
If an unsupported mode was selected, could this regression violate the V4L2
API by returning -EINVAL here and leaving hi846->cur_mode corrupted?

In patch 3 of this series, when filter_modes() correctly rejects the
lane-incompatible Mode 0, does this result in 0 valid modes?

If 0 valid modes remain, would v4l2_find_nearest_size_conditional() return
NULL, causing hi846_set_format() to dereference a NULL hi846->cur_mode and
crash the kernel?

Also, in probe, does the default mode initialization blindly assign
hi846->cur_mode = &supported_modes[0], leaving the driver in an inconsistent
state if Mode 0 is unsupported?

[ ... ]
> @@ -1947,20 +1979,33 @@ static int hi846_identify_module(struct hi846 *hi846)
>  	return 0;
>  }
>  
> -static s64 hi846_check_link_freqs(struct hi846 *hi846,
> -				  struct v4l2_fwnode_endpoint *ep)
> +static int hi846_add_link_freqs(struct hi846 *hi846, struct device *dev,
> +				struct v4l2_fwnode_endpoint *ep)
>  {
> -	const s64 *freqs = hi846_link_freqs;
> -	int freqs_count = ARRAY_SIZE(hi846_link_freqs);
> -	int i, j;
> -
> -	for (i = 0; i < freqs_count; i++) {
> -		for (j = 0; j < ep->nr_of_link_frequencies; j++)
> -			if (freqs[i] == ep->link_frequencies[j])
> -				break;
> -		if (j == ep->nr_of_link_frequencies)
> -			return freqs[i];
> -	}
> +	s64 hi846_link_freqs[ARRAY_SIZE(supported_modes)];
> +	unsigned long freq_bitmap;
> +	int ret, i;
> +
> +	/*
> +	 * Since the MCLK freq varies between platforms, calculating driver
> +	 * supported link freqs here.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(supported_modes); i++)
> +		hi846_link_freqs[i] = hi846_get_link_freq(hi846, &supported_modes[i]);
> +
> +	ret = v4l2_link_freq_to_bitmap(dev, ep->link_frequencies,
> +				       ep->nr_of_link_frequencies,
> +				       hi846_link_freqs,
> +				       ARRAY_SIZE(hi846_link_freqs),
> +				       &freq_bitmap);

[Severity: High]
Does this filtering loop contain a regression by failing to check if the
corresponding modes actually support the configured lane count?

If hi846_add_link_freqs() evaluates hi846_get_link_freq() for all modes
regardless of whether the mode supports the physical lane count
(hi846->nr_lanes), could this allow incompatible configurations to pass probe?

For instance, supported_modes[0] (640x480) lacks 4-lane registers, but this
patch adds a dummy mipi_clk_div_4lane = 8. If hi846->nr_lanes == 4 and the
device tree specifies only this dummy link frequency, would
v4l2_link_freq_to_bitmap() incorrectly match it, causing probe to succeed
instead of failing?

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