| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 27 Oct 2023 13:10:21 +0200
From: Michael Grzeschik <mgr@...gutronix.de>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Jayant Chowdhary <jchowdhary@...gle.com>,
Thinh.Nguyen@...opsys.com, arakesh@...gle.com, etalvala@...gle.com,
dan.scally@...asonboard.com, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2] usb:gadget:uvc Do not use worker thread to pump usb
requests
On Fri, Oct 27, 2023 at 10:51:17AM +0300, Laurent Pinchart wrote:
>Thank you for the patch.
>
>On Thu, Oct 26, 2023 at 09:56:35PM +0000, Jayant Chowdhary wrote:
>> This patch is based on top of
>> https://lore.kernel.org/linux-usb/20230930184821.310143-1-arakesh@google.com/T/#t:
>>
>> When we use an async work queue to perform the function of pumping
>> usb requests to the usb controller, it is possible that thread scheduling
>> affects at what cadence we're able to pump requests. This could mean usb
>> requests miss their uframes - resulting in video stream flickers on the host
>> device.
>>
>> In this patch, we move the pumping of usb requests to
>> 1) uvcg_video_complete() complete handler for both isoc + bulk
>> endpoints. We still send 0 length requests when there is no uvc buffer
>> available to encode.
>
>This means you will end up copying large amounts of data in interrupt
>context. The work queue was there to avoid exactly that, as it will
>introduce delays that can affect other parts of the system. I think this
>is a problem.
Regarding Thin's argument about possible scheduling latency that is already
introducing real errors, this seemed like a good solution.
But sure, this potential latency introduced in the interrupt context can
trigger other side effects.
However I think we need some compromise since both arguments are very valid.
Any ideas, how to solve this?
>> 2) uvc_v4l2_qbuf - only for bulk endpoints since it is not legal to send
>> 0 length requests.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
>> Signed-off-by: Jayant Chowdhary <jchowdhary@...gle.com>
>> Suggested-by: Jayant Chowdhary <jchowdhary@...gle.com>
>> Suggested-by: Avichal Rakesh <arakesh@...gle.com>
>> Tested-by: Jayant Chowdhary <jchowdhary@...gle.com>
>> ---
>> v1->v2: Fix code style and add self Signed-off-by
>>
>> drivers/usb/gadget/function/f_uvc.c | 4 --
>> drivers/usb/gadget/function/uvc.h | 4 +-
>> drivers/usb/gadget/function/uvc_v4l2.c | 5 +-
>> drivers/usb/gadget/function/uvc_video.c | 71 ++++++++++++++++---------
>> drivers/usb/gadget/function/uvc_video.h | 2 +
>> 5 files changed, 51 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
>> index ae08341961eb..53cb2539486d 100644
>> --- a/drivers/usb/gadget/function/f_uvc.c
>> +++ b/drivers/usb/gadget/function/f_uvc.c
>> @@ -959,14 +959,10 @@ static void uvc_function_unbind(struct usb_configuration *c,
>> {
>> struct usb_composite_dev *cdev = c->cdev;
>> struct uvc_device *uvc = to_uvc(f);
>> - struct uvc_video *video = &uvc->video;
>> long wait_ret = 1;
>>
>> uvcg_info(f, "%s()\n", __func__);
>>
>> - if (video->async_wq)
>> - destroy_workqueue(video->async_wq);
>> -
>> /*
>> * If we know we're connected via v4l2, then there should be a cleanup
>> * of the device from userspace either via UVC_EVENT_DISCONNECT or
>> diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
>> index be0d012aa244..498f344fda4b 100644
>> --- a/drivers/usb/gadget/function/uvc.h
>> +++ b/drivers/usb/gadget/function/uvc.h
>> @@ -88,9 +88,6 @@ struct uvc_video {
>> struct uvc_device *uvc;
>> struct usb_ep *ep;
>>
>> - struct work_struct pump;
>> - struct workqueue_struct *async_wq;
>> -
>> /* Frame parameters */
>> u8 bpp;
>> u32 fcc;
>> @@ -116,6 +113,7 @@ struct uvc_video {
>> /* Context data used by the completion handler */
>> __u32 payload_size;
>> __u32 max_payload_size;
>> + bool is_bulk;
>
>This should be introduced in a separate patch.
>
>>
>> struct uvc_video_queue queue;
>> unsigned int fid;
>> diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
>> index f4d2e24835d4..678ea6df7b5c 100644
>> --- a/drivers/usb/gadget/function/uvc_v4l2.c
>> +++ b/drivers/usb/gadget/function/uvc_v4l2.c
>> @@ -414,10 +414,7 @@ uvc_v4l2_qbuf(struct file *file, void *fh, struct v4l2_buffer *b)
>> ret = uvcg_queue_buffer(&video->queue, b);
>> if (ret < 0)
>> return ret;
>> -
>> - if (uvc->state == UVC_STATE_STREAMING)
>> - queue_work(video->async_wq, &video->pump);
>> -
>> + uvcg_video_pump_qbuf(video);
>> return ret;
>> }
>>
>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> index ab3f02054e85..0fcd8e5edbac 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -24,6 +24,8 @@
>> * Video codecs
>> */
>>
>> +static void uvcg_video_pump(struct uvc_video *video);
>> +
>> static int
>> uvc_video_encode_header(struct uvc_video *video, struct uvc_buffer *buf,
>> u8 *data, int len)
>> @@ -329,7 +331,9 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>> */
>> if (video->is_enabled) {
>> list_add_tail(&req->list, &video->req_free);
>> - queue_work(video->async_wq, &video->pump);
>> + spin_unlock_irqrestore(&video->req_lock, flags);
>> + uvcg_video_pump(video);
>> + return;
>> } else {
>> uvc_video_free_request(ureq, ep);
>> }
>> @@ -409,20 +413,31 @@ uvc_video_alloc_requests(struct uvc_video *video)
>> * Video streaming
>> */
>>
>> +void uvcg_video_pump_qbuf(struct uvc_video *video)
>> +{
>> + /*
>> + * Only call uvcg_video_pump() from qbuf, for bulk eps since
>> + * for isoc, the complete handler will call uvcg_video_pump()
>> + * consistently. Calling it for isoc eps, while correct
>> + * will increase contention for video->req_lock since the
>> + * complete handler will be called more often.
>> + */
>> + if (video->is_bulk)
>> + uvcg_video_pump(video);
>
>Am I the only one to see the *major* race condition that this patch
>introduces ?
Possible that you are. Please elaborate.
>> +}
>> +
>> /*
>> * uvcg_video_pump - Pump video data into the USB requests
>> *
>> * This function fills the available USB requests (listed in req_free) with
>> * video data from the queued buffers.
>> */
>> -static void uvcg_video_pump(struct work_struct *work)
>> +static void uvcg_video_pump(struct uvc_video *video)
>> {
>> - struct uvc_video *video = container_of(work, struct uvc_video, pump);
>> 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 usb_request *req = NULL;
>> - struct uvc_buffer *buf;
>> + struct uvc_request *ureq = NULL;
>> + struct uvc_buffer *buf = NULL, *last_buf = NULL;
>> unsigned long flags;
>> bool buf_done;
>> int ret;
>> @@ -455,7 +470,8 @@ static void uvcg_video_pump(struct work_struct *work)
>> if (buf != NULL) {
>> video->encode(req, video, buf);
>> buf_done = buf->state == UVC_BUF_STATE_DONE;
>> - } else if (!(queue->flags & UVC_QUEUE_DISCONNECTED) && !is_bulk) {
>> + } else if (!(queue->flags & UVC_QUEUE_DISCONNECTED) &&
>> + !video->is_bulk) {
>> /*
>> * No video buffer available; the queue is still connected and
>> * we're transferring over ISOC. Queue a 0 length request to
>> @@ -500,18 +516,30 @@ static void uvcg_video_pump(struct work_struct *work)
>> req->no_interrupt = 1;
>> }
>>
>> - /* Queue the USB request */
>> - ret = uvcg_video_ep_queue(video, req);
>> spin_unlock_irqrestore(&queue->irqlock, flags);
>> -
>> + spin_lock_irqsave(&video->req_lock, flags);
>> + if (video->is_enabled) {
>> + /* Queue the USB request */
>> + ret = uvcg_video_ep_queue(video, req);
>> + /* Endpoint now owns the request */
>> + req = NULL;
>> + video->req_int_count++;
>> + } else {
>> + ret = -ENODEV;
>> + ureq = req->context;
>> + last_buf = ureq->last_buf;
>> + ureq->last_buf = NULL;
>> + }
>> + spin_unlock_irqrestore(&video->req_lock, flags);
>> if (ret < 0) {
>> + if (last_buf != NULL) {
>> + // Return the buffer to the queue in the case the
>> + // request was not queued to the ep.
>
>Wrong comment style.
>
>> + uvcg_complete_buffer(&video->queue, last_buf);
>> + }
>> uvcg_queue_cancel(queue, 0);
>> break;
>> }
>> -
>> - /* Endpoint now owns the request */
>> - req = NULL;
>> - video->req_int_count++;
>> }
>>
>> if (!req)
>> @@ -556,7 +584,6 @@ uvcg_video_disable(struct uvc_video *video)
>> }
>> spin_unlock_irqrestore(&video->req_lock, flags);
>>
>> - cancel_work_sync(&video->pump);
>> uvcg_queue_cancel(&video->queue, 0);
>>
>> spin_lock_irqsave(&video->req_lock, flags);
>> @@ -626,14 +653,16 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
>> if (video->max_payload_size) {
>> video->encode = uvc_video_encode_bulk;
>> video->payload_size = 0;
>> - } else
>> + video->is_bulk = true;
>> + } else {
>> video->encode = video->queue.use_sg ?
>> uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
>> + video->is_bulk = false;
>> + }
>>
>> video->req_int_count = 0;
>>
>> - queue_work(video->async_wq, &video->pump);
>> -
>> + uvcg_video_pump(video);
>> return ret;
>> }
>>
>> @@ -646,12 +675,6 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
>> INIT_LIST_HEAD(&video->ureqs);
>> INIT_LIST_HEAD(&video->req_free);
>> spin_lock_init(&video->req_lock);
>> - INIT_WORK(&video->pump, uvcg_video_pump);
>> -
>> - /* Allocate a work queue for asynchronous video pump handler. */
>> - video->async_wq = alloc_workqueue("uvcgadget", WQ_UNBOUND | WQ_HIGHPRI, 0);
>> - if (!video->async_wq)
>> - return -EINVAL;
>>
>> video->uvc = uvc;
>> video->fcc = V4L2_PIX_FMT_YUYV;
>> diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h
>> index 03adeefa343b..29c6b9a2e9c3 100644
>> --- a/drivers/usb/gadget/function/uvc_video.h
>> +++ b/drivers/usb/gadget/function/uvc_video.h
>> @@ -18,4 +18,6 @@ int uvcg_video_enable(struct uvc_video *video, int enable);
>>
>> int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc);
>>
>> +void uvcg_video_pump_qbuf(struct uvc_video *video);
>> +
>> #endif /* __UVC_VIDEO_H__ */
>
>--
>Regards,
>
>Laurent Pinchart
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists