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@vger.kernel.org # 859c675d84d4: usb: gadget: uvc: consistently use define for headerlen Cc: stable@vger.kernel.org # f262ce66d40c: usb: gadget: uvc: use on returned header len in video_encode_isoc_sg Cc: stable@vger.kernel.org # 61aa709ca58a: usb: gadget: uvc: rework uvcg_queue_next_buffer to uvcg_complete_buffer Cc: stable@vger.kernel.org # 9b969f93bcef: usb: gadget: uvc: giveback vb2 buffer on req complete Cc: stable@vger.kernel.org # aef11279888c: usb: gadget: uvc: improve sg exit condition Cc: stable@vger.kernel.org Signed-off-by: Dan Vacura w36195@motorola.com
--- 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)
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"); + queue->flags |= UVC_QUEUE_MISSED_XFER; + break; + case -ESHUTDOWN: /* disconnect from host. */ uvcg_dbg(&video->uvc->func, "VS request cancelled.\n"); uvcg_queue_cancel(queue, 1);
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@vger.kernel.org # 859c675d84d4: usb: gadget: uvc: consistently use define for headerlen Cc: stable@vger.kernel.org # f262ce66d40c: usb: gadget: uvc: use on returned header len in video_encode_isoc_sg Cc: stable@vger.kernel.org # 61aa709ca58a: usb: gadget: uvc: rework uvcg_queue_next_buffer to uvcg_complete_buffer Cc: stable@vger.kernel.org # 9b969f93bcef: usb: gadget: uvc: giveback vb2 buffer on req complete Cc: stable@vger.kernel.org # 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@vger.kernel.org Signed-off-by: Dan Vacura w36195@motorola.com
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;@@ -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_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR);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);
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)} @@ -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) {@@ -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) {
uvcg_complete_buffer(&video->queue, buf);
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
Hi Greg,
On Tue, Sep 27, 2022 at 10:33:34AM +0200, Greg Kroah-Hartman wrote:
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@vger.kernel.org # 859c675d84d4: usb: gadget: uvc: consistently use define for headerlen Cc: stable@vger.kernel.org # f262ce66d40c: usb: gadget: uvc: use on returned header len in video_encode_isoc_sg Cc: stable@vger.kernel.org # 61aa709ca58a: usb: gadget: uvc: rework uvcg_queue_next_buffer to uvcg_complete_buffer Cc: stable@vger.kernel.org # 9b969f93bcef: usb: gadget: uvc: giveback vb2 buffer on req complete Cc: stable@vger.kernel.org # 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,
It seems we have a regression in 5.15 with some recent, still in development, features for the uvc gadget driver. Compared to the last kernel I worked with, 5.10, I'm seeing stability and functional issues in 5.15, explained in my summary here: https://lore.kernel.org/all/20220926195307.110121-1-w36195@motorola.com/
I think we have few approaches. 1) Work through these issues and get the fixes into mainline and stable versions, this patch starts that effort, but there's still more work to be done. 2) Revert the changes that are causing regressions in 5.15 (two changes from what I can see). 3) Add a configfs ability to allow sg isoc transfers and couple the quarter interrupt logic to only sg xfers.
Approach 2 is my preference, as there are issues still present that need to be figured out. However, I don't know how we can revert to just a stable line. I'm basically looking for feedback and input for the next steps, and if it's just me with these issues on 5.15.
greg k-h
Cc: stable@vger.kernel.org Signed-off-by: Dan Vacura w36195@motorola.com
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;@@ -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_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR);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?
I thought MISSED_XFER was a more explicit name to what is going on, instead of an action. I can change it back.
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);
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)} @@ -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) {@@ -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) {
uvcg_complete_buffer(&video->queue, buf);
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.
Previously this would printout as a dev_warn in a725d0f6dfc5 ("usb: gadget: uvc: call uvc uvcg_warn on completed status instead of uvcg_info"), which I think might be too noisy. Info is helpful to see what's going on if there is video corruption, but I can change to a dbg as well.
thanks,
greg k-h
Appreciate the input,
Dan
On Tue, Sep 27, 2022 at 10:47:55AM -0500, Dan Vacura wrote:
Hi Greg,
On Tue, Sep 27, 2022 at 10:33:34AM +0200, Greg Kroah-Hartman wrote:
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@vger.kernel.org # 859c675d84d4: usb: gadget: uvc: consistently use define for headerlen Cc: stable@vger.kernel.org # f262ce66d40c: usb: gadget: uvc: use on returned header len in video_encode_isoc_sg Cc: stable@vger.kernel.org # 61aa709ca58a: usb: gadget: uvc: rework uvcg_queue_next_buffer to uvcg_complete_buffer Cc: stable@vger.kernel.org # 9b969f93bcef: usb: gadget: uvc: giveback vb2 buffer on req complete Cc: stable@vger.kernel.org # 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,
It seems we have a regression in 5.15 with some recent, still in development, features for the uvc gadget driver. Compared to the last kernel I worked with, 5.10, I'm seeing stability and functional issues in 5.15, explained in my summary here: https://lore.kernel.org/all/20220926195307.110121-1-w36195@motorola.com/
I think we have few approaches.
- Work through these issues and get the fixes into mainline and stable
versions, this patch starts that effort, but there's still more work to be done. 2) Revert the changes that are causing regressions in 5.15 (two changes from what I can see). 3) Add a configfs ability to allow sg isoc transfers and couple the quarter interrupt logic to only sg xfers.
Approach 2 is my preference, as there are issues still present that need to be figured out. However, I don't know how we can revert to just a stable line. I'm basically looking for feedback and input for the next steps, and if it's just me with these issues on 5.15.
Worry about the latest kernel release first. Once we get things fixed there, we can backport any needed changes to older stable/LTS kernels as needed.
We can't go back in time and do new development on older kernels.
For Android devices, you can always use the `android-mainline` AOSP kernel branch as it is up to date with Linus's tree at all times and is known to successfully boot and run at least one real device.
#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?
I thought MISSED_XFER was a more explicit name to what is going on, instead of an action. I can change it back.
That's fine, but you didn't document why you were doing this.
Perhaps this needs to be a patch series, remember, each patch only should do 1 thing.
- 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.
Previously this would printout as a dev_warn in a725d0f6dfc5 ("usb: gadget: uvc: call uvc uvcg_warn on completed status instead of uvcg_info"), which I think might be too noisy. Info is helpful to see what's going on if there is video corruption, but I can change to a dbg as well.
info is never a valid level for an error. dbg is best if a user can not actually do something about the error.
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Dan Vacura -
Greg Kroah-Hartman