On Mon, Sep 26, 2022 at 02:53:07PM -0500, Dan Vacura wrote:
> If there is a transmission error the buffer will be returned too early,
> causing a memory fault as subsequent requests for that buffer are still
> queued up to be sent. Refactor the error handling to wait for the final
> request to come in before reporting back the buffer to userspace for all
> transfer types (bulk/isoc/isoc_sg) to ensure userspace knows if the
> frame was successfully sent.
>
> Fixes: e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support")
> Cc: <stable@xxxxxxxxxxxxxxx> # 859c675d84d4: usb: gadget: uvc: consistently use define for headerlen
> Cc: <stable@xxxxxxxxxxxxxxx> # f262ce66d40c: usb: gadget: uvc: use on returned header len in video_encode_isoc_sg
> Cc: <stable@xxxxxxxxxxxxxxx> # 61aa709ca58a: usb: gadget: uvc: rework uvcg_queue_next_buffer to uvcg_complete_buffer
> Cc: <stable@xxxxxxxxxxxxxxx> # 9b969f93bcef: usb: gadget: uvc: giveback vb2 buffer on req complete
> Cc: <stable@xxxxxxxxxxxxxxx> # aef11279888c: usb: gadget: uvc: improve sg exit condition
I don't understand, why we backport all of these commits to 5.15.y if
the original problem isn't in 5.15.y?
Or is it?
I'm confused,
greg k-h
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Dan Vacura <w36195@xxxxxxxxxxxx>
>
> ---
> drivers/usb/gadget/function/uvc_queue.c | 8 +++++---
> drivers/usb/gadget/function/uvc_queue.h | 2 +-
> drivers/usb/gadget/function/uvc_video.c | 18 ++++++++++++++----
> 3 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index ec500ee499ee..72e7ffd9a021 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -304,6 +304,7 @@ int uvcg_queue_enable(struct uvc_video_queue *queue, int enable)
>
> queue->sequence = 0;
> queue->buf_used = 0;
> + queue->flags &= ~UVC_QUEUE_MISSED_XFER;
> } else {
> ret = vb2_streamoff(&queue->queue, queue->queue.type);
> if (ret < 0)
> @@ -329,10 +330,11 @@ int uvcg_queue_enable(struct uvc_video_queue *queue, int enable)
> void uvcg_complete_buffer(struct uvc_video_queue *queue,
> struct uvc_buffer *buf)
> {
> - if ((queue->flags & UVC_QUEUE_DROP_INCOMPLETE) &&
> - buf->length != buf->bytesused) {
> - buf->state = UVC_BUF_STATE_QUEUED;
> + if ((queue->flags & UVC_QUEUE_MISSED_XFER)) {
> + queue->flags &= ~UVC_QUEUE_MISSED_XFER;
> + buf->state = UVC_BUF_STATE_ERROR;
> vb2_set_plane_payload(&buf->buf.vb2_buf, 0, 0);
> + vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR);
> return;
> }
>
> diff --git a/drivers/usb/gadget/function/uvc_queue.h b/drivers/usb/gadget/function/uvc_queue.h
> index 41f87b917f6b..741ec58ae9bb 100644
> --- a/drivers/usb/gadget/function/uvc_queue.h
> +++ b/drivers/usb/gadget/function/uvc_queue.h
> @@ -42,7 +42,7 @@ struct uvc_buffer {
> };
>
> #define UVC_QUEUE_DISCONNECTED (1 << 0)
> -#define UVC_QUEUE_DROP_INCOMPLETE (1 << 1)
> +#define UVC_QUEUE_MISSED_XFER (1 << 1)
Why change the name of the error?
>
> struct uvc_video_queue {
> struct vb2_queue queue;
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index bb037fcc90e6..e46591b067a8 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -88,6 +88,7 @@ uvc_video_encode_bulk(struct usb_request *req, struct uvc_video *video,
> struct uvc_buffer *buf)
> {
> void *mem = req->buf;
> + struct uvc_request *ureq = req->context;
> int len = video->req_size;
> int ret;
>
> @@ -113,13 +114,14 @@ uvc_video_encode_bulk(struct usb_request *req, struct uvc_video *video,
> video->queue.buf_used = 0;
> buf->state = UVC_BUF_STATE_DONE;
> list_del(&buf->queue);
> - uvcg_complete_buffer(&video->queue, buf);
> video->fid ^= UVC_STREAM_FID;
> + ureq->last_buf = buf;
>
> video->payload_size = 0;
> }
>
> if (video->payload_size == video->max_payload_size ||
> + video->queue.flags & UVC_QUEUE_MISSED_XFER ||
> buf->bytesused == video->queue.buf_used)
> video->payload_size = 0;
> }
> @@ -180,7 +182,8 @@ uvc_video_encode_isoc_sg(struct usb_request *req, struct uvc_video *video,
> req->length -= len;
> video->queue.buf_used += req->length - header_len;
>
> - if (buf->bytesused == video->queue.buf_used || !buf->sg) {
> + if (buf->bytesused == video->queue.buf_used || !buf->sg ||
> + video->queue.flags & UVC_QUEUE_MISSED_XFER) {
> video->queue.buf_used = 0;
> buf->state = UVC_BUF_STATE_DONE;
> buf->offset = 0;
> @@ -195,6 +198,7 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
> struct uvc_buffer *buf)
> {
> void *mem = req->buf;
> + struct uvc_request *ureq = req->context;
> int len = video->req_size;
> int ret;
>
> @@ -209,12 +213,13 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video,
>
> req->length = video->req_size - len;
>
> - if (buf->bytesused == video->queue.buf_used) {
> + if (buf->bytesused == video->queue.buf_used ||
> + video->queue.flags & UVC_QUEUE_MISSED_XFER) {
> video->queue.buf_used = 0;
> buf->state = UVC_BUF_STATE_DONE;
> list_del(&buf->queue);
> - uvcg_complete_buffer(&video->queue, buf);
> video->fid ^= UVC_STREAM_FID;
> + ureq->last_buf = buf;
> }
> }
>
> @@ -255,6 +260,11 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
> case 0:
> break;
>
> + case -EXDEV:
> + uvcg_info(&video->uvc->func, "VS request missed xfer.\n");
Why are you spamming the kernel logs at the info level for a USB
transmission problem? That could get very noisy, please change this to
be at the debug level.
thanks,
greg k-h
