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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Hardware configuration validation for link frequencies is silently bypassed because of a missing clock initialization.
- [High] V4L2 format constraints are not enforced when evaluating `V4L2_SUBDEV_FORMAT_TRY`, allowing arbitrary dimensions.
- [Medium] The logic to filter modes for 4-lane configurations relies on brittle array pointer math and implicit array ordering.
- [Low] The local variable `supported_modes` shadows a global file-scope variable.
--

commit d690502fca6d668750580bed748cc5e7a41d5785
Author: Pengyu Luo <mitltlatltl@gmail.com>
Date:   Mon May 11 10:39:25 2026 +0000

    media: hi846: fix modes handling for different lane cases
    
    When using 4-lane, v4l2_find_nearest_size may return an unsupported
    mode, 640x480 mode, and it is set to the default mode. To fix it,
    specifying the supported modes for different lane cases, once the lane
    count is determined.
    
    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 61297ef66a0eb..c77b34a5e1662 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c

[ ... ]

> @@ -1736,8 +1740,8 @@ 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),
> +		v4l2_find_nearest_size(hi846->supported_modes,
> +				       hi846->num_modes,
>  				       width, height, mf->width, mf->height);
>  	dev_dbg(&client->dev, "%s: found mode: %dx%d\n", __func__,
>  		hi846->cur_mode->width, hi846->cur_mode->height);

Is the V4L2_SUBDEV_FORMAT_TRY request handled correctly earlier in this
function?

The API mandates that TRY requests must be validated, clamped, and adjusted
to match the hardware supported sizes identically to active format requests.
By returning early and blindly copying the format like this:

if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
        *v4l2_subdev_state_get_format(sd_state, format->pad) = *mf;
        return 0;
}

Does this allow userspace to inject arbitrary dimensions into the subdev
state?

[ ... ]

> @@ -1821,8 +1825,11 @@ static int hi846_enum_frame_size(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_frame_size_enum *fse)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct hi846 *hi846 = to_hi846(sd);
> +	const struct hi846_mode *supported_modes = hi846->supported_modes;
> +
>  
> -	if (fse->pad || fse->index >= ARRAY_SIZE(supported_modes))
> +	if (fse->pad || fse->index >= hi846->num_modes)
>  		return -EINVAL;

This is not a bug, but does this local variable shadow the global file-scope
supported_modes array? This might trigger compiler warnings and cause
confusion during future refactoring.

[ ... ]

> @@ -1950,12 +1957,12 @@ static int hi846_identify_module(struct hi846 *hi846)
>  static s64 hi846_check_link_freqs(struct hi846 *hi846,
>  				  struct v4l2_fwnode_endpoint *ep)
>  {
> -	int freqs_count = ARRAY_SIZE(supported_modes);
> +	int freqs_count = hi846->num_modes;
>  	u64 link_freq;
>  	int i, j;
>  
>  	for (i = 0; i < freqs_count; i++) {
> -		link_freq = hi846_get_link_freq(hi846, &supported_modes[i]);
> +		link_freq = hi846_get_link_freq(hi846, &hi846->supported_modes[i]);
>  		for (j = 0; j < ep->nr_of_link_frequencies; j++)
>  			if (link_freq == ep->link_frequencies[j])
>  				break;

Does this logic successfully validate the hardware configuration?

Looking at hi846_probe(), it calls hi846_parse_dt() which uses this function
to check frequencies. However, this happens before the hi846 clock is
initialized via devm_v4l2_sensor_clk_get().

Because the clock is zero-initialized, hi846_get_link_freq() will calculate
a frequency of 0. This loop will then fail to find 0 in the device tree
endpoint list and ultimately return 0.

Since the caller checks if (fq), which evaluates to false, does this
silently bypass the hardware configuration validation entirely?

[ ... ]

> @@ -1999,6 +2006,13 @@ static int hi846_parse_dt(struct hi846 *hi846, struct device *dev)
>  
>  	hi846->nr_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
>  
> +	hi846->supported_modes = supported_modes;
> +	hi846->num_modes = NUM_MODES_2LANE;
> +	if (hi846->nr_lanes == 4) {
> +		hi846->supported_modes = supported_modes + 1;
> +		hi846->num_modes = NUM_MODES_4LANE;
> +	}
> +

Is this pointer math relying on implicit array ordering? It appears to
assume that there is exactly one unsupported mode for 4-lane setups and
that it is always located at index 0.

If a new mode is added later, or a 2-lane-only mode is appended to the
end of the array, this offset calculation will fail. Could we use
v4l2_find_nearest_size_conditional() here instead and pass a condition
function to dynamically evaluate if reg_list_4lane.num_of_regs is greater
than zero?

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