This series is cleaning the complete and pump handler to improve
the code quality. It got really hard to follow the locking and escape
cases since the recently introduced patches.
Signed-off-by: Michael Grzeschik <[email protected]>
---
Michael Grzeschik (3):
usb: gadget: uvc: drop unnecessary check for always set req
usb: gadget: uvc: refactor the check for a valid buffer in the pump worker
usb: gadget: uvc: rework complete handler
drivers/usb/gadget/function/uvc_video.c | 109 +++++++++++++++-----------------
1 file changed, 51 insertions(+), 58 deletions(-)
---
base-commit: 88bae831f3810e02c9c951233c7ee662aa13dc2c
change-id: 20240214-uvc-gadget-cleanup-e8484f675c2a
Best regards,
--
Michael Grzeschik <[email protected]>
By toggling the condition check for a valid buffer, the else path
can be completely avoided.
Signed-off-by: Michael Grzeschik <[email protected]>
---
drivers/usb/gadget/function/uvc_video.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index b17c61c932652..b4f3b3c784218 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -594,10 +594,7 @@ static void uvcg_video_pump(struct work_struct *work)
*/
spin_lock_irqsave(&queue->irqlock, flags);
buf = uvcg_queue_head(queue);
-
- if (buf != NULL) {
- video->encode(req, video, buf);
- } else {
+ if (!buf) {
/*
* Either the queue has been disconnected or no video buffer
* available for bulk transfer. Either way, stop processing
@@ -607,6 +604,8 @@ static void uvcg_video_pump(struct work_struct *work)
break;
}
+ video->encode(req, video, buf);
+
spin_unlock_irqrestore(&queue->irqlock, flags);
spin_lock_irqsave(&video->req_lock, flags);
--
2.39.2
The pump function is running in an while(1) loop. The only case this
loop will be escaped is the two breaks. In both cases the req is valid.
Therefor the check for an not set req can be dropped and setting the req
to NULL does also has never any effect.
Signed-off-by: Michael Grzeschik <[email protected]>
---
drivers/usb/gadget/function/uvc_video.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index dd3241fc6939d..b17c61c932652 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -623,14 +623,7 @@ static void uvcg_video_pump(struct work_struct *work)
uvcg_queue_cancel(queue, 0);
break;
}
-
- /* The request is owned by the endpoint / ready list. */
- req = NULL;
}
-
- if (!req)
- return;
-
spin_lock_irqsave(&video->req_lock, flags);
if (video->is_enabled)
list_add_tail(&req->list, &video->req_free);
--
2.39.2
We refactor the complete handler since the return path with the
locking are really difficult to follow. Just simplify the function by
switching the logic return it on an disabled endpoint early. This way
the second level of indentation can be removed.
Signed-off-by: Michael Grzeschik <[email protected]>
---
drivers/usb/gadget/function/uvc_video.c | 95 +++++++++++++++++----------------
1 file changed, 48 insertions(+), 47 deletions(-)
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index b4f3b3c784218..ff7c1fa5c48f3 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -370,6 +370,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
struct uvc_video *video = ureq->video;
struct uvc_video_queue *queue = &video->queue;
struct uvc_buffer *last_buf;
+ struct usb_request *to_queue = req;
unsigned long flags;
bool is_bulk = video->max_payload_size;
int ret = 0;
@@ -425,59 +426,59 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
* we're still streaming before queueing the usb_request
* back to req_free
*/
- if (video->is_enabled) {
+ if (!video->is_enabled) {
+ uvc_video_free_request(ureq, ep);
+ spin_unlock_irqrestore(&video->req_lock, flags);
+ uvcg_queue_cancel(queue, 0);
+
+ return;
+ }
+
+ /*
+ * Here we check whether any request is available in the ready
+ * list. If it is, queue it to the ep and add the current
+ * usb_request to the req_free list - for video_pump to fill in.
+ * Otherwise, just use the current usb_request to queue a 0
+ * length request to the ep. Since we always add to the req_free
+ * list if we dequeue from the ready list, there will never
+ * be a situation where the req_free list is completely out of
+ * requests and cannot recover.
+ */
+ to_queue->length = 0;
+ if (!list_empty(&video->req_ready)) {
+ to_queue = list_first_entry(&video->req_ready,
+ struct usb_request, list);
+ list_del(&to_queue->list);
+ list_add_tail(&req->list, &video->req_free);
/*
- * Here we check whether any request is available in the ready
- * list. If it is, queue it to the ep and add the current
- * usb_request to the req_free list - for video_pump to fill in.
- * Otherwise, just use the current usb_request to queue a 0
- * length request to the ep. Since we always add to the req_free
- * list if we dequeue from the ready list, there will never
- * be a situation where the req_free list is completely out of
- * requests and cannot recover.
+ * Queue work to the wq as well since it is possible that a
+ * buffer may not have been completely encoded with the set of
+ * in-flight usb requests for whih the complete callbacks are
+ * firing.
+ * In that case, if we do not queue work to the worker thread,
+ * the buffer will never be marked as complete - and therefore
+ * not be returned to userpsace. As a result,
+ * dequeue -> queue -> dequeue flow of uvc buffers will not
+ * happen.
*/
- struct usb_request *to_queue = req;
-
- to_queue->length = 0;
- if (!list_empty(&video->req_ready)) {
- to_queue = list_first_entry(&video->req_ready,
- struct usb_request, list);
- list_del(&to_queue->list);
- list_add_tail(&req->list, &video->req_free);
- /*
- * Queue work to the wq as well since it is possible that a
- * buffer may not have been completely encoded with the set of
- * in-flight usb requests for whih the complete callbacks are
- * firing.
- * In that case, if we do not queue work to the worker thread,
- * the buffer will never be marked as complete - and therefore
- * not be returned to userpsace. As a result,
- * dequeue -> queue -> dequeue flow of uvc buffers will not
- * happen.
- */
- queue_work(video->async_wq, &video->pump);
- }
+ queue_work(video->async_wq, &video->pump);
+ }
+ /*
+ * Queue to the endpoint. The actual queueing to ep will
+ * only happen on one thread - the async_wq for bulk endpoints
+ * and this thread for isoc endpoints.
+ */
+ ret = uvcg_video_usb_req_queue(video, to_queue, !is_bulk);
+ if (ret < 0) {
/*
- * Queue to the endpoint. The actual queueing to ep will
- * only happen on one thread - the async_wq for bulk endpoints
- * and this thread for isoc endpoints.
+ * Endpoint error, but the stream is still enabled.
+ * Put request back in req_free for it to be cleaned
+ * up later.
*/
- ret = uvcg_video_usb_req_queue(video, to_queue, !is_bulk);
- if (ret < 0) {
- /*
- * Endpoint error, but the stream is still enabled.
- * Put request back in req_free for it to be cleaned
- * up later.
- */
- list_add_tail(&to_queue->list, &video->req_free);
- }
- } else {
- uvc_video_free_request(ureq, ep);
- ret = 0;
+ list_add_tail(&to_queue->list, &video->req_free);
}
+
spin_unlock_irqrestore(&video->req_lock, flags);
- if (ret < 0)
- uvcg_queue_cancel(queue, 0);
}
static int
--
2.39.2