v1 -> v2 : Rebased to ToT, and fixed deadlock reported in
https://lore.kernel.org/all/ZRv2UnKztgyqk2pt@xxxxxxxxxxxxxx/
v2 -> v3 : Fix email threading goof-up
v3 -> v4 : re-rebase to ToT & moved to a uvc_video level lock
as discussed in
https://lore.kernel.org/b14b296f-2e08-4edf-aeea-1c5b621e2d0c@xxxxxxxxxx/
v4 -> v5 : Address review comments. Add Reviewed-by & Tested-by.
v5 -> v6 : Added another patch before this one to make uvcg_video_disable
easier to review.
v6 -> v7 : Fix warning reported in
https://lore.kernel.org/202310200457.GwPPFuHX-lkp@xxxxxxxxx/
v7 -> v8 : No change. Getting back in review queue
v8 -> v9 : No change.
v9 -> v10 : Address review comments. Rebase to ToT (usb-next)
v10 -> v11 : Address review comments
drivers/usb/gadget/function/uvc.h | 1 +
drivers/usb/gadget/function/uvc_v4l2.c | 10 +-
drivers/usb/gadget/function/uvc_video.c | 130 ++++++++++++++++++++----
3 files changed, 112 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 993694da0bbc..be0d012aa244 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -102,6 +102,7 @@ struct uvc_video {
unsigned int uvc_num_requests;
/* Requests */
+ bool is_enabled; /* tracks whether video stream is enabled */
unsigned int req_size;
struct list_head ureqs; /* all uvc_requests allocated by uvc_video */
struct list_head req_free;
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 904dd283cbf7..c7e5fa4f29e0 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -468,11 +468,11 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
if (type != video->queue.queue.type)
return -EINVAL;
- uvc->state = UVC_STATE_CONNECTED;
ret = uvcg_video_disable(video);
if (ret < 0)
return ret;
+ uvc->state = UVC_STATE_CONNECTED;
uvc_function_setup_continue(uvc, 1);
return 0;
}
@@ -507,14 +507,6 @@ uvc_v4l2_subscribe_event(struct v4l2_fh *fh,
static void uvc_v4l2_disable(struct uvc_device *uvc)
{
uvc_function_disconnect(uvc);
- /*
- * Drop uvc->state to CONNECTED if it was streaming before.
- * This ensures that the usb_requests are no longer queued
- * to the controller.
- */
- if (uvc->state == UVC_STATE_STREAMING)
- uvc->state = UVC_STATE_CONNECTED;
-
uvcg_video_disable(&uvc->video);
uvcg_free_buffers(&uvc->video.queue);
uvc->func_connected = false;
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index c3e8c48f46a9..164bdeb7f2a9 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -227,6 +227,10 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
* Request handling
*/
+/*
+ * Callers must take care to hold req_lock when this function may be called
+ * from multiple threads. For example, when frames are streaming to the host.
+ */
static void
uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep)
{
@@ -271,9 +275,26 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
struct uvc_request *ureq = req->context;
struct uvc_video *video = ureq->video;
struct uvc_video_queue *queue = &video->queue;
- struct uvc_device *uvc = video->uvc;
+ struct uvc_buffer *last_buf;
unsigned long flags;
+ spin_lock_irqsave(&video->req_lock, flags);
+ if (!video->is_enabled) {
+ /*
+ * When is_enabled is false, uvcg_video_disable() ensures
+ * that in-flight uvc_buffers are returned, so we can
+ * safely call free_request without worrying about
+ * last_buf.
+ */
+ uvc_video_free_request(ureq, ep);
+ spin_unlock_irqrestore(&video->req_lock, flags);
+ return;
+ }
+
+ last_buf = ureq->last_buf;
+ ureq->last_buf = NULL;
+ spin_unlock_irqrestore(&video->req_lock, flags);
+
switch (req->status) {
case 0:
break;
@@ -295,17 +316,26 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
uvcg_queue_cancel(queue, 0);
}
- if (ureq->last_buf) {
- uvcg_complete_buffer(&video->queue, ureq->last_buf);
- ureq->last_buf = NULL;
+ if (last_buf) {
+ spin_lock_irqsave(&queue->irqlock, flags);
+ uvcg_complete_buffer(queue, last_buf);
+ spin_unlock_irqrestore(&queue->irqlock, flags);
}
spin_lock_irqsave(&video->req_lock, flags);
- list_add_tail(&req->list, &video->req_free);
- spin_unlock_irqrestore(&video->req_lock, flags);
-
- if (uvc->state == UVC_STATE_STREAMING)
+ /*
+ * Video stream might have been disabled while we were
+ * processing the current usb_request. So make sure
+ * we're still streaming before queueing the usb_request
+ * back to req_free
+ */
+ if (video->is_enabled) {
+ list_add_tail(&req->list, &video->req_free);
queue_work(video->async_wq, &video->pump);
+ } else {
+ uvc_video_free_request(ureq, ep);
+ }
+ spin_unlock_irqrestore(&video->req_lock, flags);
}
static int
@@ -392,20 +422,22 @@ static void uvcg_video_pump(struct work_struct *work)
struct uvc_video_queue *queue = &video->queue;
/* video->max_payload_size is only set when using bulk transfer */
bool is_bulk = video->max_payload_size;
- struct uvc_device *uvc = video->uvc;
struct usb_request *req = NULL;
struct uvc_buffer *buf;
unsigned long flags;
bool buf_done;
int ret;
- while (uvc->state == UVC_STATE_STREAMING && video->ep->enabled) {
+ while (true) {
+ if (!video->ep->enabled)
+ return;
+
/*
- * Retrieve the first available USB request, protected by the
- * request lock.
+ * Check is_enabled and retrieve the first available USB
+ * request, protected by the request lock.
*/
spin_lock_irqsave(&video->req_lock, flags);
- if (list_empty(&video->req_free)) {
+ if (!video->is_enabled || list_empty(&video->req_free)) {
spin_unlock_irqrestore(&video->req_lock, flags);
return;
}
@@ -487,9 +519,11 @@ static void uvcg_video_pump(struct work_struct *work)
return;
spin_lock_irqsave(&video->req_lock, flags);
- list_add_tail(&req->list, &video->req_free);
+ if (video->is_enabled)
+ list_add_tail(&req->list, &video->req_free);
+ else
+ uvc_video_free_request(req->context, video->ep);
spin_unlock_irqrestore(&video->req_lock, flags);
- return;
}
/*
@@ -498,7 +532,11 @@ static void uvcg_video_pump(struct work_struct *work)
int
uvcg_video_disable(struct uvc_video *video)
{
- struct uvc_request *ureq;
+ unsigned long flags;
+ struct list_head inflight_bufs;
+ struct usb_request *req, *temp;
+ struct uvc_buffer *buf, *btemp;
+ struct uvc_request *ureq, *utemp;
if (video->ep == NULL) {
uvcg_info(&video->uvc->func,
@@ -506,15 +544,58 @@ uvcg_video_disable(struct uvc_video *video)
return -ENODEV;
}
+ INIT_LIST_HEAD(&inflight_bufs);
+ spin_lock_irqsave(&video->req_lock, flags);
+ video->is_enabled = false;
+
+ /*
+ * Remove any in-flight buffers from the uvc_requests
+ * because we want to return them before cancelling the
+ * queue. This ensures that we aren't stuck waiting for
+ * all complete callbacks to come through before disabling
+ * vb2 queue.
+ */
+ list_for_each_entry(ureq, &video->ureqs, list) {
+ if (ureq->last_buf) {
+ list_add_tail(&ureq->last_buf->queue, &inflight_bufs);
+ ureq->last_buf = NULL;
+ }
+ }
+ spin_unlock_irqrestore(&video->req_lock, flags);
+
cancel_work_sync(&video->pump);
uvcg_queue_cancel(&video->queue, 0);
- list_for_each_entry(ureq, &video->ureqs, list) {
- if (ureq->req)
- usb_ep_dequeue(video->ep, ureq->req);
+ spin_lock_irqsave(&video->req_lock, flags);
+ /*
+ * Remove all uvc_requests from ureqs with list_del_init
+ * This lets uvc_video_free_request correctly identify
+ * if the uvc_request is attached to a list or not when freeing
+ * memory.
+ */
+ list_for_each_entry_safe(ureq, utemp, &video->ureqs, list)
+ list_del_init(&ureq->list);
+
+ list_for_each_entry_safe(req, temp, &video->req_free, list) {
+ list_del(&req->list);
+ uvc_video_free_request(req->context, video->ep);
}
- uvc_video_free_requests(video);
+ INIT_LIST_HEAD(&video->ureqs);
+ INIT_LIST_HEAD(&video->req_free);
+ video->req_size = 0;
+ spin_unlock_irqrestore(&video->req_lock, flags);
+
+ /*
+ * Return all the video buffers before disabling the queue.
+ */
+ spin_lock_irqsave(&video->queue.irqlock, flags);
+ list_for_each_entry_safe(buf, btemp, &inflight_bufs, queue) {
+ list_del(&buf->queue);
+ uvcg_complete_buffer(&video->queue, buf);
+ }
+ spin_unlock_irqrestore(&video->queue.irqlock, flags);
+
uvcg_queue_enable(&video->queue, 0);
return 0;
}
@@ -532,6 +613,14 @@ int uvcg_video_enable(struct uvc_video *video)
return -ENODEV;
}
+ /*
+ * Safe to access request related fields without req_lock because
+ * this is the only thread currently active, and no other
+ * request handling thread will become active until this function
+ * returns.
+ */
+ video->is_enabled = true;
+
if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0)
return ret;
@@ -557,6 +646,7 @@ int uvcg_video_enable(struct uvc_video *video)
*/
int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
{
+ video->is_enabled = false;
INIT_LIST_HEAD(&video->ureqs);
INIT_LIST_HEAD(&video->req_free);
spin_lock_init(&video->req_lock);
--
2.42.0.869.gea05f2083d-goog
