Re: [PATCH v2 06/13] media: sony-starvis2: Allow runtime control of link frequency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: sony-starvis2: missing lock and NULL check for link_freq init
--

commit 54243fb94593be25be7c3f168bb8161959b863e5
Author: Dave Stevenson <dave.stevenson@raspberrypi.com>

media: sony-starvis2: Allow runtime control of link frequency

This commit connects V4L2_CID_LINK_FREQ to allow runtime configuration
of the link frequency when the sensor is not streaming.

> diff --git a/drivers/media/i2c/sony-starvis2.c b/drivers/media/i2c/sony-starvis2.c
> index 66c77c5957e32..2bb438d22ada0 100644
> --- a/drivers/media/i2c/sony-starvis2.c
> +++ b/drivers/media/i2c/sony-starvis2.c

[ ... ]

> @@ -1210,25 +1236,23 @@ static int starvis2_init_controls(struct starvis2 *starvis2)
>  
>  	starvis2->vmax = starvis2->variant->vmax_default;
>  
> -	hmax = variant->hmax_min_link_freq[__fls(starvis2->link_freq_bitmap)];
> -	if (starvis2->lane_mode == STARVIS2_LANEMODE_4L)
> -		hmax = max(hmax >> 1, variant->hmax_min_pixel_array);
> -
>  	/* PIXEL_RATE is fixed and read-only */
>  	v4l2_ctrl_new_std(ctrl_hdlr, &starvis2_ctrl_ops, V4L2_CID_PIXEL_RATE,
>  			  starvis2->variant->pixel_rate,
>  			  starvis2->variant->pixel_rate, 1,
>  			  starvis2->variant->pixel_rate);
>  
> -	/* LINK_FREQ is also read only */
> -	link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, &starvis2_ctrl_ops,
> -					   V4L2_CID_LINK_FREQ,
> -					   ARRAY_SIZE(link_freqs) - 1,
> -					   __ffs(starvis2->link_freq_bitmap),
> -					   link_freqs);
> -
> -	if (link_freq)
> -		link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +	starvis2->link_freq =
> +		v4l2_ctrl_new_int_menu(ctrl_hdlr, &starvis2_ctrl_ops,
> +				       V4L2_CID_LINK_FREQ,
> +				       __fls(starvis2->link_freq_bitmap),
> +				       __ffs(starvis2->link_freq_bitmap),
> +				       link_freqs);
> +	__v4l2_ctrl_modify_range(starvis2->link_freq,
> +				 __ffs(starvis2->link_freq_bitmap),
> +				 __fls(starvis2->link_freq_bitmap),
> +				 ~(starvis2->link_freq_bitmap),
> +				 __ffs(starvis2->link_freq_bitmap));

[Severity: High]
Does this code pass starvis2->link_freq to __v4l2_ctrl_modify_range() without
checking if it is NULL? If v4l2_ctrl_new_int_menu() fails to allocate memory,
it returns NULL, which would cause a NULL pointer dereference when
__v4l2_ctrl_modify_range() accesses ctrl->handler.

Also, is it safe to call __v4l2_ctrl_modify_range() in starvis2_init_controls()
without holding the control handler lock? This unlocked variant expects the
handler lock to be held, which will trigger a lockdep assertion failure. Should
this code use the locked variant v4l2_ctrl_modify_range() instead?

>  	vblank = starvis2->vmax - starvis2->variant->active_area.height;
>  	max_vblank = STARVIS2_VMAX_MAX - starvis2->variant->active_area.height;
> @@ -1236,6 +1260,10 @@ static int starvis2_init_controls(struct starvis2 *starvis2)
>  					     V4L2_CID_VBLANK, vblank, max_vblank,
>  					     2, vblank);
>  
> +	hmax = variant->hmax_min_link_freq[starvis2->link_freq->val];

[Severity: High]
Can this cause a NULL pointer dereference if starvis2->link_freq is NULL?
It appears this accesses the val field without a prior check.

> +	if (starvis2->lane_mode == STARVIS2_LANEMODE_4L)
> +		hmax = max(hmax >> 1, variant->hmax_min_pixel_array);
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-media-starvis2-v2-0-3670722d250a@raspberrypi.com?part=6