Re: [PATCH] media: uvcvideo: Fix deadlock if uvc_status_stop is called from async_ctrl.work

[Laurent Pinchart]

On Tue, Mar 10, 2026 at 06:22:59PM -0400, Sean Anderson wrote:
> If a UVC camera has an asynchronous control, uvc_status_stop may be
> called from async_ctrl.work:
> 
> uvc_ctrl_status_event_work()
>     uvc_ctrl_status_event()
>         uvc_ctrl_clear_handle()
> 	    uvc_pm_put()
> 	        uvc_status_put()
> 		    uvc_status_stop()
> 		        cancel_work_sync()
> 
> This will cause a deadlock, since cancel_work_sync will wait for
> uvc_ctrl_status_event_work to complete before returning.
> 
> Fix this by returning early from uvc_status_stop if we are currently in
> the work function. flush_status now remains false until uvc_status_start
> is called again, ensuring that uvc_ctrl_status_event_work won't resubmit
> the URB.
> 
> Fixes: a32d9c41bdb8 ("media: uvcvideo: Make power management granular")
> Closes: https://lore.kernel.org/all/6733bdfb-3e88-479f-8956-ab09c04c433e@xxxxxxxxx/
> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx>
> ---
> 
>  drivers/media/usb/uvc/uvc_status.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index 231cfee8e7c2c..2a23606c7f4c6 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -316,6 +316,14 @@ static int uvc_status_start(struct uvc_device *dev, gfp_t flags)
>  	if (!dev->int_urb)
>  		return 0;
>  
> +	/*
> +	 * If the work called uvc_status_stop it may still be running. Wait for
> +	 * it to finish before we submit the urb.

I don't like this much. The code is becoming really convoluted and hard
to follow. Would there be a way to solve the issue with a broader
refactoring that would simplify the implementation ?

> +	 */
> +	cancel_work_sync(&dev->async_ctrl.work);
> +
> +	/* Clear the flush status if we were previously stopped */

s/stopped/stopped./

> +	smp_store_release(&dev->flush_status, false);

Add a blank line here.

>  	return usb_submit_urb(dev->int_urb, flags);
>  }
>  
> @@ -336,6 +344,14 @@ static void uvc_status_stop(struct uvc_device *dev)
>  	 */
>  	smp_store_release(&dev->flush_status, true);
>  
> +	/*
> +	 * We will deadlock if we are currently in the work function.
> +	 * Fortunately, we know that the URB is already dead and that no

The URB hasn't been killed, at has just completed and not been
resubmitted. I'd write

	/*
	 * If we are called from the event work function, the URB is guaranteed
	 * to not be in flight as it has completed and has not been resubmitted.
	 * There's no need to cancel the work (which would deadlock), or to kill
	 * the URB.
	 */

> +	 * further work can be queued, so there's nothing left for us to do.
> +	 */
> +	if (current_work() == &w->work)
> +		return;
> +
>  	/*
>  	 * Cancel any pending asynchronous work. If any status event was queued,
>  	 * process it synchronously.
> @@ -354,15 +370,6 @@ static void uvc_status_stop(struct uvc_device *dev)
>  	 */
>  	if (cancel_work_sync(&w->work))
>  		uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
> -
> -	/*
> -	 * From this point, there are no events on the queue and the status URB
> -	 * is dead. No events will be queued until uvc_status_start() is called.
> -	 * The barrier is needed to make sure that flush_status is visible to
> -	 * uvc_ctrl_status_event_work() when uvc_status_start() will be called
> -	 * again.
> -	 */
> -	smp_store_release(&dev->flush_status, false);
>  }
>  
>  int uvc_status_resume(struct uvc_device *dev)

-- 
Regards,

Laurent Pinchart