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

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

Hi Ricardo,

On 3/11/26 12:06, Ricardo Ribalda wrote:
> 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?

Yes. A minimal reproducer is

$ v4l2-ctl -c focus_absolute=500
$ v4l2-ctl -c focus_absolute=500
(hangs)

I believe the reason guvcview does not have this issue is because it
continuously displays camera output, keeping the refcount above one,
whereas qv4l2ctrl 

> 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@linux.dev> 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@chromium.org>
> Acked-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> Your patch is very similar to what I sent some time ago (I did not
> have the cancel_work_sync() in uvc_status_start())

This could probably be downgraded to flush_work() (along with the first
cancel_work_sync in uvc_status_stop).

> You can see the old discussion:
> https://lore.kernel.org/all/Y6sAO7URJpSIulye@pendragon.ideasonboard.com/
> 
> 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@linux.dev/
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>>  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
>>
> 
>