mirror of
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git
synced 2024-09-12 13:55:32 +00:00
media: uvcvideo: Fix race condition with usb_kill_urb
usb_kill_urb warranties that all the handlers are finished when it
returns, but does not protect against threads that might be handling
asynchronously the urb.
For UVC, the function uvc_ctrl_status_event_async() takes care of
control changes asynchronously.
If the code is executed in the following order:
CPU 0 CPU 1
===== =====
uvc_status_complete()
uvc_status_stop()
uvc_ctrl_status_event_work()
uvc_status_start() -> FAIL
Then uvc_status_start will keep failing and this error will be shown:
<4>[ 5.540139] URB 0000000000000000 submitted while active
drivers/usb/core/urb.c:378 usb_submit_urb+0x4c3/0x528
Let's improve the current situation, by not re-submiting the urb if
we are stopping the status event. Also process the queued work
(if any) during stop.
CPU 0 CPU 1
===== =====
uvc_status_complete()
uvc_status_stop()
uvc_status_start()
uvc_ctrl_status_event_work() -> FAIL
Hopefully, with the usb layer protection this should be enough to cover
all the cases.
Cc: stable@vger.kernel.org
Fixes: e5225c820c
("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
Reviewed-by: Yunke Cao <yunkec@chromium.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
This commit is contained in:
parent
716c330433
commit
619d9b710c
3 changed files with 43 additions and 0 deletions
|
@ -6,6 +6,7 @@
|
||||||
* Laurent Pinchart (laurent.pinchart@ideasonboard.com)
|
* Laurent Pinchart (laurent.pinchart@ideasonboard.com)
|
||||||
*/
|
*/
|
||||||
|
|
||||||
|
#include <asm/barrier.h>
|
||||||
#include <linux/bitops.h>
|
#include <linux/bitops.h>
|
||||||
#include <linux/kernel.h>
|
#include <linux/kernel.h>
|
||||||
#include <linux/list.h>
|
#include <linux/list.h>
|
||||||
|
@ -1571,6 +1572,10 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
|
||||||
|
|
||||||
uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
|
uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
|
||||||
|
|
||||||
|
/* The barrier is needed to synchronize with uvc_status_stop(). */
|
||||||
|
if (smp_load_acquire(&dev->flush_status))
|
||||||
|
return;
|
||||||
|
|
||||||
/* Resubmit the URB. */
|
/* Resubmit the URB. */
|
||||||
w->urb->interval = dev->int_ep->desc.bInterval;
|
w->urb->interval = dev->int_ep->desc.bInterval;
|
||||||
ret = usb_submit_urb(w->urb, GFP_KERNEL);
|
ret = usb_submit_urb(w->urb, GFP_KERNEL);
|
||||||
|
|
|
@ -6,6 +6,7 @@
|
||||||
* Laurent Pinchart (laurent.pinchart@ideasonboard.com)
|
* Laurent Pinchart (laurent.pinchart@ideasonboard.com)
|
||||||
*/
|
*/
|
||||||
|
|
||||||
|
#include <asm/barrier.h>
|
||||||
#include <linux/kernel.h>
|
#include <linux/kernel.h>
|
||||||
#include <linux/input.h>
|
#include <linux/input.h>
|
||||||
#include <linux/slab.h>
|
#include <linux/slab.h>
|
||||||
|
@ -311,5 +312,41 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags)
|
||||||
|
|
||||||
void uvc_status_stop(struct uvc_device *dev)
|
void uvc_status_stop(struct uvc_device *dev)
|
||||||
{
|
{
|
||||||
|
struct uvc_ctrl_work *w = &dev->async_ctrl;
|
||||||
|
|
||||||
|
/*
|
||||||
|
* Prevent the asynchronous control handler from requeing the URB. The
|
||||||
|
* barrier is needed so the flush_status change is visible to other
|
||||||
|
* CPUs running the asynchronous handler before usb_kill_urb() is
|
||||||
|
* called below.
|
||||||
|
*/
|
||||||
|
smp_store_release(&dev->flush_status, true);
|
||||||
|
|
||||||
|
/*
|
||||||
|
* Cancel any pending asynchronous work. If any status event was queued,
|
||||||
|
* process it synchronously.
|
||||||
|
*/
|
||||||
|
if (cancel_work_sync(&w->work))
|
||||||
|
uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
|
||||||
|
|
||||||
|
/* Kill the urb. */
|
||||||
usb_kill_urb(dev->int_urb);
|
usb_kill_urb(dev->int_urb);
|
||||||
|
|
||||||
|
/*
|
||||||
|
* The URB completion handler may have queued asynchronous work. This
|
||||||
|
* won't resubmit the URB as flush_status is set, but it needs to be
|
||||||
|
* cancelled before returning or it could then race with a future
|
||||||
|
* uvc_status_start() call.
|
||||||
|
*/
|
||||||
|
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);
|
||||||
}
|
}
|
||||||
|
|
|
@ -577,6 +577,7 @@ struct uvc_device {
|
||||||
struct usb_host_endpoint *int_ep;
|
struct usb_host_endpoint *int_ep;
|
||||||
struct urb *int_urb;
|
struct urb *int_urb;
|
||||||
struct uvc_status *status;
|
struct uvc_status *status;
|
||||||
|
bool flush_status;
|
||||||
|
|
||||||
struct input_dev *input;
|
struct input_dev *input;
|
||||||
char input_phys[64];
|
char input_phys[64];
|
||||||
|
|
Loading…
Reference in a new issue