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

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

On 3/10/26 18:08, Ricardo Ribalda wrote:
> Hi Sean
> 
> On Tue, 10 Mar 2026 at 22:23, Sean Anderson <sean.anderson@linux.dev> wrote:
>>
>> 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:
> 
> It does not work, in the sense that the urb is re submited... but if
> would have confirmed the rootcause of the lockdep. But you have
> already proven that with your patch, which I think is correct :).
> Thanks for that
> 
> Can you resend it as a proper patch?
> 
> You probably want to add:
> Fixes: a32d9c41bdb8 ("media: uvcvideo: Make power management granular")

Done.

> I will try to review with extra care and a big cup of tea tomorrow morning.
> 
> Thanks!
> 
>>
>> 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.
> 
> It has been a while since we did this, but I believe that since we did
> not use locks, the only way to guarantee the event queue was flushed
> and the URB was killed was to have the double cancel_work() in that
> order.

Oh, I see because we could have something like

CPU A                             CPU B
================================= =======================
uvc_ctrl_status_event_work
    smp_load_acqure(flush_status)
                                  uvc_status_stop()
                                      smp_store_release()
                                      usb_kill_urb()
                                      cancel_work_sync()
    usb_submit_urb()

--Sean