Re: [BUG] uvc_status_stop hangs if called from async_ctrl.work

[Sean Anderson <sean.anderson@linux.dev>]

On 3/10/26 16:56, Ricardo Ribalda wrote:
> Hi Sean
> 
> Thanks for the report.
> 
> I have not been able to repro with qv4l2 on my computer :(.
> 
> Could you try if this patch works for you? Not saying that it is
> beautiful patch, or the way to do it.... but it will let me know if I
> am looking in the right place.
> 
> 
> diff --git a/drivers/media/usb/uvc/uvc_status.c
> b/drivers/media/usb/uvc/uvc_status.c
> index 231cfee8e7c2..cca2aed162c3 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -340,7 +340,9 @@ static void uvc_status_stop(struct uvc_device *dev)
>          * Cancel any pending asynchronous work. If any status event was queued,
>          * process it synchronously.
>          */
> -       if (cancel_work_sync(&w->work))
> +       if (&w->work == current_work())
> +               cancel_work(&w->work);
> +       else if (cancel_work_sync(&w->work))
>                 uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
> 
>         /* Kill the urb. */
> @@ -352,7 +354,7 @@ static void uvc_status_stop(struct uvc_device *dev)
>          * cancelled before returning or it could then race with a future
>          * uvc_status_start() call.
>          */
> -       if (cancel_work_sync(&w->work))
> +       if (&w->work != current_work() && cancel_work_sync(&w->work))
>                 uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
> 
>         /*

I don't think this works since the urb will be rescheduled as flush_status
is set to false again. However, the following patch works for me:

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)
--

The first cancel_work_sync also seems superfluous to me, since we have to cancel
again anyway.

--Sean