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

[Ricardo Ribalda]

Hi Sean

Thanks for the patch. In your original report you mentioned that you
could repro with qv4l2 and changing a control.
May I assume that it was while the camera was not streaming and the
control was a "slow" controp (zoom, focus)... Can you give more some
more details?

I have tested your change with 3 threads running:

1 # while true; do yavta --capture=3 /dev/video0; sleep 1;done
2 #  while true; do yavta -w "0x00980900 64" /dev/video0; yavta -w
"0x00980900 0" /dev/video0; done
3 /sys/bus/usb/devices/3-6 # while true; do echo 1 > authorized; sleep
3; echo 0 > authorized; sleep 3 ; done

And I have not seen any freeze. So that is good :), But I also could
not repro without your patch :P.

Anyway I agree with the lockdep report that we introduced a bug when
uvc_status_stop can be called from the async work, So we must fix it.


On Tue, 10 Mar 2026 at 23:23, Sean Anderson <sean.anderson@xxxxxxxxx> 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.
>
Tested-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
Acked-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>

Your patch is very similar to what I sent some time ago (I did not
have the cancel_work_sync() in uvc_status_start())
You can see the old discussion:
https://lore.kernel.org/all/Y6sAO7URJpSIulye@xxxxxxxxxxxxxxxxxxxxxxxxxx/

For now, I am only ack the patch because I want to ensure the locking
is working as expected and need to re-read the old threads.
It would be great if Hans or Laurent also take a look at this.

Thanks again


> 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.
> +        */
> +       cancel_work_sync(&dev->async_ctrl.work);
> +
> +       /* Clear the flush status if we were previously stopped */
> +       smp_store_release(&dev->flush_status, false);
>         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
> +        * 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)
> --
> 2.35.1.1320.gc452695387.dirty
>


-- 
Ricardo Ribalda