Re: [PATCH v4] staging: most: video: prevent probes during component exit

[Dan Carpenter <dan.carpenter@linaro.org>]

On Tue, Dec 02, 2025 at 10:21:43AM -0500, Pavan Kumar Yalagada wrote:
> When comp_exit() runs, comp_probe_channel() could still add new devices
> to video_devices, creating a race and potentially leaving the list in
> an inconsistent state.
> 
> This patch prevents new devices from being added while exiting by:
> 
> - comp_probe_channel() checks comp_exiting before modifying video_devices.
> 
> 	if (unlikely(comp_exiting)) {
> 		spin_unlock_irq(&list_lock);
> 		ret = -BUSY;
> 		goto err_unreg;
> 	}
> 
> This ensures that all partially created resources are properly freed
> and no new channels are allowed while the driver is being unloaded.
> 
> Signed-off-by: Pavan Kumar Yalagada <pavankumaryalagada@gmail.com>
> 
> ---
> 
> v4:
>  - Used unlikely() for the comp_exiting check to optimize branch
>    prediction.

This isn't a fast path.  Only use likely/unlikely() if it's going to
show up in benchmarks.  If we can't measure the speed improvement then
it's just making the code messy for no reason.

>  - Removed err_cleanup and use err_unreg for proper cleanup
>    of partially initialized mdev/v4l2 structures.
>  - Removed redundant spinlock usage around 'comp_exiting = true;'
>    in comp_exit().
>  - Removed explicit kfree(mdev) in comp_exit() loop.
> 
> v3:
>  - comp_exiting flag update and memory cleanup for early exits.
>  - Commit message clarified for reviewers.
>  - Removed WARN/BUG as it becomes unnecessary.
> ---
>  drivers/staging/most/video/video.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/most/video/video.c b/drivers/staging/most/video/video.c
> index 32f71d9a9cf7..21cb94250d93 100644
> --- a/drivers/staging/most/video/video.c
> +++ b/drivers/staging/most/video/video.c
> @@ -60,6 +60,8 @@ static inline struct comp_fh *to_comp_fh(struct file *filp)
>  static LIST_HEAD(video_devices);
>  static DEFINE_SPINLOCK(list_lock);
> 
> +static bool comp_exiting;
> +
>  static inline bool data_ready(struct most_video_dev *mdev)
>  {
>  	return !list_empty(&mdev->pending_mbos);
> @@ -498,6 +500,11 @@ static int comp_probe_channel(struct most_interface *iface, int channel_idx,
>  		goto err_unreg;
> 
>  	spin_lock_irq(&list_lock);
> +	if (unlikely(comp_exiting)) {
> +		spin_unlock_irq(&list_lock);
> +		ret = -EBUSY;
> +		goto err_unreg;

You will need to call comp_unregister_videodev() on this error
path.  See my blog if you want.

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

> +	}
>  	list_add(&mdev->list, &video_devices);
>  	spin_unlock_irq(&list_lock);
>  	return 0;
> @@ -555,6 +562,8 @@ static void __exit comp_exit(void)
>  {
>  	struct most_video_dev *mdev, *tmp;
> 
> +	comp_exiting = true;
> +
>  	/*
>  	 * As the mostcore currently doesn't call disconnect_channel()
>  	 * for linked channels while we call most_deregister_component()
> @@ -569,13 +578,13 @@ static void __exit comp_exit(void)
>  		comp_unregister_videodev(mdev);
>  		v4l2_device_disconnect(&mdev->v4l2_dev);
>  		v4l2_device_put(&mdev->v4l2_dev);
> +

Don't add a blank line here.

Minor stuff.  Otherwise I think this is okay.  Fix that up and resend.

regards,
dan carpenter

>  		spin_lock_irq(&list_lock);
>  	}
>  	spin_unlock_irq(&list_lock);
> 
>  	most_deregister_configfs_subsys(&comp);
>  	most_deregister_component(&comp);
> -	BUG_ON(!list_empty(&video_devices));
>  }
> 
>  module_init(comp_init);
> --
> 2.47.3
>