Make sure that all the async work is finished when we stop the status urb.
To: Yunke Cao <[email protected]>
To: Sergey Senozhatsky <[email protected]>
To: Max Staudt <[email protected]>
To: Laurent Pinchart <[email protected]>
To: Mauro Carvalho Chehab <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Ricardo Ribalda <[email protected]>
---
Changes in v2:
- Add a patch for not kalloc dev->status
- Redo the logic mechanism, so it also works with suspend (Thanks Yunke!)
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Ricardo Ribalda (2):
media: uvcvideo: Fix race condition with usb_kill_urb
media: uvcvideo: Do not alloc dev->status
drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
drivers/media/usb/uvc/uvc_status.c | 15 +++++++--------
drivers/media/usb/uvc/uvcvideo.h | 3 ++-
3 files changed, 12 insertions(+), 9 deletions(-)
---
base-commit: 0ec5a38bf8499f403f81cb81a0e3a60887d1993c
change-id: 20221212-uvc-race-09276ea68bf8
Best regards,
--
Ricardo Ribalda <[email protected]>
UVC_MAX_STATUS_SIZE is 16, simplify the code by inlining dev->status.
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/uvc/uvc_status.c | 9 +--------
drivers/media/usb/uvc/uvcvideo.h | 2 +-
2 files changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 09a5802dc974..52999b3b7c48 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -259,15 +259,9 @@ int uvc_status_init(struct uvc_device *dev)
uvc_input_init(dev);
- dev->status = kzalloc(UVC_MAX_STATUS_SIZE, GFP_KERNEL);
- if (dev->status == NULL)
- return -ENOMEM;
-
dev->int_urb = usb_alloc_urb(0, GFP_KERNEL);
- if (dev->int_urb == NULL) {
- kfree(dev->status);
+ if (!dev->int_urb)
return -ENOMEM;
- }
pipe = usb_rcvintpipe(dev->udev, ep->desc.bEndpointAddress);
@@ -296,7 +290,6 @@ void uvc_status_unregister(struct uvc_device *dev)
void uvc_status_cleanup(struct uvc_device *dev)
{
usb_free_urb(dev->int_urb);
- kfree(dev->status);
}
int uvc_status_start(struct uvc_device *dev, gfp_t flags)
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 6a9b72d6789e..ccc7e3b60bf1 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -559,7 +559,7 @@ struct uvc_device {
/* Status Interrupt Endpoint */
struct usb_host_endpoint *int_ep;
struct urb *int_urb;
- u8 *status;
+ u8 status[UVC_MAX_STATUS_SIZE];
bool flush_status;
struct input_dev *input;
char input_phys[64];
--
2.39.0.rc1.256.g54fd8350bd-goog-b4-0.11.0-dev-696ae
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: [email protected]
Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
drivers/media/usb/uvc/uvc_status.c | 6 ++++++
drivers/media/usb/uvc/uvcvideo.h | 1 +
3 files changed, 10 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index c95a2229f4fa..5160facc8e20 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1442,6 +1442,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
+ if (dev->flush_status)
+ return;
+
/* Resubmit the URB. */
w->urb->interval = dev->int_ep->desc.bInterval;
ret = usb_submit_urb(w->urb, GFP_KERNEL);
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 7518ffce22ed..09a5802dc974 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -304,10 +304,16 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags)
if (dev->int_urb == NULL)
return 0;
+ dev->flush_status = false;
return usb_submit_urb(dev->int_urb, flags);
}
void uvc_status_stop(struct uvc_device *dev)
{
+ struct uvc_ctrl_work *w = &dev->async_ctrl;
+
+ dev->flush_status = true;
usb_kill_urb(dev->int_urb);
+ if (cancel_work_sync(&w->work))
+ uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index df93db259312..6a9b72d6789e 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -560,6 +560,7 @@ struct uvc_device {
struct usb_host_endpoint *int_ep;
struct urb *int_urb;
u8 *status;
+ bool flush_status;
struct input_dev *input;
char input_phys[64];
--
2.39.0.rc1.256.g54fd8350bd-goog-b4-0.11.0-dev-696ae
On (22/12/13 15:35), Ricardo Ribalda wrote:
[..]
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -559,7 +559,7 @@ struct uvc_device {
> /* Status Interrupt Endpoint */
> struct usb_host_endpoint *int_ep;
> struct urb *int_urb;
> - u8 *status;
> + u8 status[UVC_MAX_STATUS_SIZE];
Can we use `struct uvc_control_status status;` instead of open-coding it?
Seems that this is what the code wants anyway:
struct uvc_control_status *status =
(struct uvc_control_status *)dev->status;
And then we can drop casts in uvc_status_complete().
On Tue, Dec 13, 2022 at 11:36 PM Ricardo Ribalda <[email protected]> wrote:
>
> 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: [email protected]
> Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
> Signed-off-by: Ricardo Ribalda <[email protected]>
> ---
> drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
> drivers/media/usb/uvc/uvc_status.c | 6 ++++++
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 3 files changed, 10 insertions(+)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index c95a2229f4fa..5160facc8e20 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1442,6 +1442,9 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
>
> uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
>
> + if (dev->flush_status)
> + return;
> +
> /* Resubmit the URB. */
> w->urb->interval = dev->int_ep->desc.bInterval;
> ret = usb_submit_urb(w->urb, GFP_KERNEL);
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index 7518ffce22ed..09a5802dc974 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -304,10 +304,16 @@ int uvc_status_start(struct uvc_device *dev, gfp_t flags)
> if (dev->int_urb == NULL)
> return 0;
>
> + dev->flush_status = false;
> return usb_submit_urb(dev->int_urb, flags);
> }
>
> void uvc_status_stop(struct uvc_device *dev)
> {
> + struct uvc_ctrl_work *w = &dev->async_ctrl;
> +
> + dev->flush_status = true;
> usb_kill_urb(dev->int_urb);
> + if (cancel_work_sync(&w->work))
> + uvc_ctrl_status_event(w->chain, w->ctrl, w->data);
> }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..6a9b72d6789e 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -560,6 +560,7 @@ struct uvc_device {
> struct usb_host_endpoint *int_ep;
> struct urb *int_urb;
> u8 *status;
> + bool flush_status;
> struct input_dev *input;
> char input_phys[64];
>
>
> --
> 2.39.0.rc1.256.g54fd8350bd-goog-b4-0.11.0-dev-696ae
Reviewed-by: Yunke Cao <[email protected]>
Hi Sergey
Thanks for the review
On Wed, 14 Dec 2022 at 01:40, Sergey Senozhatsky
<[email protected]> wrote:
>
> On (22/12/13 15:35), Ricardo Ribalda wrote:
> [..]
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -559,7 +559,7 @@ struct uvc_device {
> > /* Status Interrupt Endpoint */
> > struct usb_host_endpoint *int_ep;
> > struct urb *int_urb;
> > - u8 *status;
> > + u8 status[UVC_MAX_STATUS_SIZE];
>
> Can we use `struct uvc_control_status status;` instead of open-coding it?
> Seems that this is what the code wants anyway:
It can also be a `struct uvc_streaming_status`
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_status.c#n230
so we always need the casting :(
>
> struct uvc_control_status *status =
> (struct uvc_control_status *)dev->status;
>
> And then we can drop casts in uvc_status_complete().
--
Ricardo Ribalda
On (22/12/14 06:57), Ricardo Ribalda wrote:
> > On (22/12/13 15:35), Ricardo Ribalda wrote:
> > [..]
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -559,7 +559,7 @@ struct uvc_device {
> > > /* Status Interrupt Endpoint */
> > > struct usb_host_endpoint *int_ep;
> > > struct urb *int_urb;
> > > - u8 *status;
> > > + u8 status[UVC_MAX_STATUS_SIZE];
> >
> > Can we use `struct uvc_control_status status;` instead of open-coding it?
> > Seems that this is what the code wants anyway:
>
> It can also be a `struct uvc_streaming_status`
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_status.c#n230
>
> so we always need the casting :(
Then perhaps we can put both of them into anon union in struct uvc_device
as stream_status and control_status?