[PATCH 0/1] uvc gadget sg performance issues

[PATCH 0/1] uvc gadget sg performance issues

[Dan Vacura]

Hello uvc gadget developers,

I'm working on a 5.15.41 based kernel on a qcom chipset with the dwc3
controller and I'm encountering two problems related to the recent performance
improvement changes:

https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-5-m.grzeschik@pengutronix.de/  and 
https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-6-m.grzeschik@pengutronix.de/

If I revert these two changes, then I have much improved stability and a
transmission problem I'm seeing is gone. Has there been any success from
others on 5.15 with this uvc improvement and any recommendations for my
current problems?  Those being:

1) a smmu panic, snippet here: 

    <3>[  718.314900][  T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!
    <3>[  718.314994][  T803] arm-smmu 15000000.apps-smmu: FAR    = 0x00000000efe60800
    <3>[  718.315023][  T803] arm-smmu 15000000.apps-smmu: PAR    = 0x0000000000000000
    <3>[  718.315048][  T803] arm-smmu 15000000.apps-smmu: FSR    = 0x40000402 [TF R SS ]
    <3>[  718.315074][  T803] arm-smmu 15000000.apps-smmu: FSYNR0    = 0x5f0003
    <3>[  718.315096][  T803] arm-smmu 15000000.apps-smmu: FSYNR1    = 0xaa02
    <3>[  718.315117][  T803] arm-smmu 15000000.apps-smmu: context bank#    = 0x1b
    <3>[  718.315141][  T803] arm-smmu 15000000.apps-smmu: TTBR0  = 0x001b0000c2a92000
    <3>[  718.315165][  T803] arm-smmu 15000000.apps-smmu: TTBR1  = 0x001b000000000000
    <3>[  718.315192][  T803] arm-smmu 15000000.apps-smmu: SCTLR  = 0x0a5f00e7 ACTLR  = 0x00000003
    <3>[  718.315245][  T803] arm-smmu 15000000.apps-smmu: CBAR  = 0x0001f300
    <3>[  718.315274][  T803] arm-smmu 15000000.apps-smmu: MAIR0   = 0xf404ff44 MAIR1   = 0x0000efe4
    <3>[  718.315297][  T803] arm-smmu 15000000.apps-smmu: SID = 0x40
    <3>[  718.315318][  T803] arm-smmu 15000000.apps-smmu: Client info: BID=0x5, PID=0xa, MID=0x2
    <3>[  718.315377][  T803] arm-smmu 15000000.apps-smmu: soft iova-to-phys=0x0000000000000000

    I can reduce this panic with the proposed patch, but it still happens until I
    disable the "req->no_interrupt = 1" logic.
 
2) The frame is not fully transmitted in dwc3 with sg support enabled. 

    There seems to be a mapping limit I'm seeing where only the roughly first
    70% of the total frame is sent. Interestingly, if I allocate a larger
    size for the buffer upfront, in uvc_queue_setup(), like sizes[0] =
    video->imagesize * 3. Then the issue rarely happens. For example, when I
    do YUYV I see green, uninitialized data, at the bottom part of the
    frame. If I do MJPG with smaller filled sizes, the transmission is fine.

    +-------------------------+
    |                         |
    |                         |
    |                         |
    |      Good data          |
    |                         |
    |                         |
    |                         |
    +-------------------------+
    |xxxxxxxxxxxxxxxxxxxxxxxxx|
    |xxxx  Bad data  xxxxxxxxx|
    |xxxxxxxxxxxxxxxxxxxxxxxxx|
    +-------------------------+


Appreciate any thoughts or feedback related to these issues.


Dan Vacura (1):
  usb: gadget: uvc: fix sg handling in error case

 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(-)

-- 
2.20.4

[PATCH] usb: gadget: uvc: fix sg handling in error case

[Dan Vacura]

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);
-- 
2.34.1

Re: [PATCH 0/1] uvc gadget sg performance issues

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 4254 bytes --]

Hi Dan!

On Mon, Sep 26, 2022 at 02:53:06PM -0500, Dan Vacura wrote:
>
>Hello uvc gadget developers,
>
>I'm working on a 5.15.41 based kernel on a qcom chipset with the dwc3
>controller and I'm encountering two problems related to the recent performance
>improvement changes:

What's about that odd kernel number. UVC is under heavy development, if
you plan to work with this code, you should probably test top of tree.

>https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-5-m.grzeschik@pengutronix.de/  and
>https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-6-m.grzeschik@pengutronix.de/
>
>If I revert these two changes, then I have much improved stability and a
>transmission problem I'm seeing is gone. Has there been any success from
>others on 5.15 with this uvc improvement and any recommendations for my
>current problems?  Those being:
>
>1) a smmu panic, snippet here: 
>
>    <3>[  718.314900][  T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!
>    <3>[  718.314994][  T803] arm-smmu 15000000.apps-smmu: FAR    = 0x00000000efe60800
>    <3>[  718.315023][  T803] arm-smmu 15000000.apps-smmu: PAR    = 0x0000000000000000
>    <3>[  718.315048][  T803] arm-smmu 15000000.apps-smmu: FSR    = 0x40000402 [TF R SS ]
>    <3>[  718.315074][  T803] arm-smmu 15000000.apps-smmu: FSYNR0    = 0x5f0003
>    <3>[  718.315096][  T803] arm-smmu 15000000.apps-smmu: FSYNR1    = 0xaa02
>    <3>[  718.315117][  T803] arm-smmu 15000000.apps-smmu: context bank#    = 0x1b
>    <3>[  718.315141][  T803] arm-smmu 15000000.apps-smmu: TTBR0  = 0x001b0000c2a92000
>    <3>[  718.315165][  T803] arm-smmu 15000000.apps-smmu: TTBR1  = 0x001b000000000000
>    <3>[  718.315192][  T803] arm-smmu 15000000.apps-smmu: SCTLR  = 0x0a5f00e7 ACTLR  = 0x00000003
>    <3>[  718.315245][  T803] arm-smmu 15000000.apps-smmu: CBAR  = 0x0001f300
>    <3>[  718.315274][  T803] arm-smmu 15000000.apps-smmu: MAIR0   = 0xf404ff44 MAIR1   = 0x0000efe4
>    <3>[  718.315297][  T803] arm-smmu 15000000.apps-smmu: SID = 0x40
>    <3>[  718.315318][  T803] arm-smmu 15000000.apps-smmu: Client info: BID=0x5, PID=0xa, MID=0x2
>    <3>[  718.315377][  T803] arm-smmu 15000000.apps-smmu: soft iova-to-phys=0x0000000000000000
>
>    I can reduce this panic with the proposed patch, but it still happens until I
>    disable the "req->no_interrupt = 1" logic.
>
>2) The frame is not fully transmitted in dwc3 with sg support enabled.
>
>    There seems to be a mapping limit I'm seeing where only the roughly first
>    70% of the total frame is sent. Interestingly, if I allocate a larger
>    size for the buffer upfront, in uvc_queue_setup(), like sizes[0] =
>    video->imagesize * 3. Then the issue rarely happens. For example, when I
>    do YUYV I see green, uninitialized data, at the bottom part of the
>    frame. If I do MJPG with smaller filled sizes, the transmission is fine.
>
>    +-------------------------+
>    |                         |
>    |                         |
>    |                         |
>    |      Good data          |
>    |                         |
>    |                         |
>    |                         |
>    +-------------------------+
>    |xxxxxxxxxxxxxxxxxxxxxxxxx|
>    |xxxx  Bad data  xxxxxxxxx|
>    |xxxxxxxxxxxxxxxxxxxxxxxxx|
>    +-------------------------+
>
>
>Appreciate any thoughts or feedback related to these issues.

Anyway, this is probably due to the frames being given back to early to
the frameproducer. We have the following patches mainline now to fix this issue:

aef11279888c00e1841a3533a35d279285af3a51 usb: gadget: uvc: improve sg exit condition
9b969f93bcef9b3d9e92f1810e22bbd6c344a0e5 usb: gadget: uvc: giveback vb2 buffer on req complete

You might need some patches beforehand to get them applied on your
stack.

Regards,
Michael

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/1] uvc gadget sg performance issues

[Dan Vacura]

Hi Michael, thanks for the prompt reply!

On Mon, Sep 26, 2022 at 10:15:41PM +0200, Michael Grzeschik wrote:
> Hi Dan!
> 
> On Mon, Sep 26, 2022 at 02:53:06PM -0500, Dan Vacura wrote:
> > 
> > Hello uvc gadget developers,
> > 
> > I'm working on a 5.15.41 based kernel on a qcom chipset with the dwc3
> > controller and I'm encountering two problems related to the recent performance
> > improvement changes:
> 
> What's about that odd kernel number. UVC is under heavy development, if
> you plan to work with this code, you should probably test top of tree.

Yes, it's a bit behind and it looks like some of the initial work you
did for scatter/gather got pulled into the 5.15 tree, but subsequent
changes didn't. I don't have much control over the kernel versioning as
we're part of the GKI Android initiative:
https://source.android.com/docs/core/architecture/kernel/generic-kernel-image
and we can only work off of what is provided, like this release line:
https://android.googlesource.com/kernel/common/+log/refs/heads/android13-5.15

Perhaps we can revert these changes for the 5.15 kernel (and other
versions) they were not intended for?

> 
> > https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-5-m.grzeschik@pengutronix.de/  and
> > https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-6-m.grzeschik@pengutronix.de/
> > 
> > If I revert these two changes, then I have much improved stability and a
> > transmission problem I'm seeing is gone. Has there been any success from
> > others on 5.15 with this uvc improvement and any recommendations for my
> > current problems?  Those being:
> > 
> > 1) a smmu panic, snippet here: 
> > 
> >    <3>[  718.314900][  T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!
> >    <3>[  718.314994][  T803] arm-smmu 15000000.apps-smmu: FAR    = 0x00000000efe60800
> >    <3>[  718.315023][  T803] arm-smmu 15000000.apps-smmu: PAR    = 0x0000000000000000
> >    <3>[  718.315048][  T803] arm-smmu 15000000.apps-smmu: FSR    = 0x40000402 [TF R SS ]
> >    <3>[  718.315074][  T803] arm-smmu 15000000.apps-smmu: FSYNR0    = 0x5f0003
> >    <3>[  718.315096][  T803] arm-smmu 15000000.apps-smmu: FSYNR1    = 0xaa02
> >    <3>[  718.315117][  T803] arm-smmu 15000000.apps-smmu: context bank#    = 0x1b
> >    <3>[  718.315141][  T803] arm-smmu 15000000.apps-smmu: TTBR0  = 0x001b0000c2a92000
> >    <3>[  718.315165][  T803] arm-smmu 15000000.apps-smmu: TTBR1  = 0x001b000000000000
> >    <3>[  718.315192][  T803] arm-smmu 15000000.apps-smmu: SCTLR  = 0x0a5f00e7 ACTLR  = 0x00000003
> >    <3>[  718.315245][  T803] arm-smmu 15000000.apps-smmu: CBAR  = 0x0001f300
> >    <3>[  718.315274][  T803] arm-smmu 15000000.apps-smmu: MAIR0   = 0xf404ff44 MAIR1   = 0x0000efe4
> >    <3>[  718.315297][  T803] arm-smmu 15000000.apps-smmu: SID = 0x40
> >    <3>[  718.315318][  T803] arm-smmu 15000000.apps-smmu: Client info: BID=0x5, PID=0xa, MID=0x2
> >    <3>[  718.315377][  T803] arm-smmu 15000000.apps-smmu: soft iova-to-phys=0x0000000000000000
> > 
> >    I can reduce this panic with the proposed patch, but it still happens until I
> >    disable the "req->no_interrupt = 1" logic.
> > 
> > 2) The frame is not fully transmitted in dwc3 with sg support enabled.
> > 
> >    There seems to be a mapping limit I'm seeing where only the roughly first
> >    70% of the total frame is sent. Interestingly, if I allocate a larger
> >    size for the buffer upfront, in uvc_queue_setup(), like sizes[0] =
> >    video->imagesize * 3. Then the issue rarely happens. For example, when I
> >    do YUYV I see green, uninitialized data, at the bottom part of the
> >    frame. If I do MJPG with smaller filled sizes, the transmission is fine.
> > 
> >    +-------------------------+
> >    |                         |
> >    |                         |
> >    |                         |
> >    |      Good data          |
> >    |                         |
> >    |                         |
> >    |                         |
> >    +-------------------------+
> >    |xxxxxxxxxxxxxxxxxxxxxxxxx|
> >    |xxxx  Bad data  xxxxxxxxx|
> >    |xxxxxxxxxxxxxxxxxxxxxxxxx|
> >    +-------------------------+
> > 
> > 
> > Appreciate any thoughts or feedback related to these issues.
> 
> Anyway, this is probably due to the frames being given back to early to
> the frameproducer. We have the following patches mainline now to fix this issue:
> 
> aef11279888c00e1841a3533a35d279285af3a51 usb: gadget: uvc: improve sg exit condition
> 9b969f93bcef9b3d9e92f1810e22bbd6c344a0e5 usb: gadget: uvc: giveback vb2 buffer on req complete

Yes, I did grab those in addition to some other necessary changes, noted
in my patch here:
https://patchwork.kernel.org/project/linux-usb/patch/20220926195307.110121-2-w36195@motorola.com/ 
I also pulled a lot of patches from the dwc3 to be almost at parity with
top of tree for the core.c/h, ep0.c/h, and gadget.c/h files, but these
issues persisted.

Out of curiosity, have you tested these changes with dwc3 and if so,
have you tried "usb: gadget: uvc: decrease the interrupt load to a
quarter" with scatter/gather disabled? For me the crash occurs more
often.

Thanks,

Dan

> 
> You might need some patches beforehand to get them applied on your
> stack.
> 
> Regards,
> Michael
> 
> -- 
> 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 |

Re: [PATCH 0/1] uvc gadget sg performance issues

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 6568 bytes --]

On Mon, Sep 26, 2022 at 03:51:31PM -0500, Dan Vacura wrote:
>Hi Michael, thanks for the prompt reply!
>
>On Mon, Sep 26, 2022 at 10:15:41PM +0200, Michael Grzeschik wrote:
>> Hi Dan!
>>
>> On Mon, Sep 26, 2022 at 02:53:06PM -0500, Dan Vacura wrote:
>> >
>> > Hello uvc gadget developers,
>> >
>> > I'm working on a 5.15.41 based kernel on a qcom chipset with the dwc3
>> > controller and I'm encountering two problems related to the recent performance
>> > improvement changes:
>>
>> What's about that odd kernel number. UVC is under heavy development, if
>> you plan to work with this code, you should probably test top of tree.
>
>Yes, it's a bit behind and it looks like some of the initial work you
>did for scatter/gather got pulled into the 5.15 tree, but subsequent
>changes didn't. I don't have much control over the kernel versioning as
>we're part of the GKI Android initiative:
>https://source.android.com/docs/core/architecture/kernel/generic-kernel-image
>and we can only work off of what is provided, like this release line:
>https://android.googlesource.com/kernel/common/+log/refs/heads/android13-5.15
>
>Perhaps we can revert these changes for the 5.15 kernel (and other
>versions) they were not intended for?


This, or we can find out which other patches are intendet to be pulled
into stable, so we can overall improve.

Anyway if you can filter out, which patches are hurting, feel free
to suggest those for revert. I am not very interested in v5.15.

>> > https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-5-m.grzeschik@pengutronix.de/  and
>> > https://patchwork.kernel.org/project/linux-usb/patch/20210628155311.16762-6-m.grzeschik@pengutronix.de/
>> >
>> > If I revert these two changes, then I have much improved stability and a
>> > transmission problem I'm seeing is gone. Has there been any success from
>> > others on 5.15 with this uvc improvement and any recommendations for my
>> > current problems?  Those being:
>> >
>> > 1) a smmu panic, snippet here: 
>> >
>> >    <3>[  718.314900][  T803] arm-smmu 15000000.apps-smmu: Unhandled arm-smmu context fault from a600000.dwc3!
>> >    <3>[  718.314994][  T803] arm-smmu 15000000.apps-smmu: FAR    = 0x00000000efe60800
>> >    <3>[  718.315023][  T803] arm-smmu 15000000.apps-smmu: PAR    = 0x0000000000000000
>> >    <3>[  718.315048][  T803] arm-smmu 15000000.apps-smmu: FSR    = 0x40000402 [TF R SS ]
>> >    <3>[  718.315074][  T803] arm-smmu 15000000.apps-smmu: FSYNR0    = 0x5f0003
>> >    <3>[  718.315096][  T803] arm-smmu 15000000.apps-smmu: FSYNR1    = 0xaa02
>> >    <3>[  718.315117][  T803] arm-smmu 15000000.apps-smmu: context bank#    = 0x1b
>> >    <3>[  718.315141][  T803] arm-smmu 15000000.apps-smmu: TTBR0  = 0x001b0000c2a92000
>> >    <3>[  718.315165][  T803] arm-smmu 15000000.apps-smmu: TTBR1  = 0x001b000000000000
>> >    <3>[  718.315192][  T803] arm-smmu 15000000.apps-smmu: SCTLR  = 0x0a5f00e7 ACTLR  = 0x00000003
>> >    <3>[  718.315245][  T803] arm-smmu 15000000.apps-smmu: CBAR  = 0x0001f300
>> >    <3>[  718.315274][  T803] arm-smmu 15000000.apps-smmu: MAIR0   = 0xf404ff44 MAIR1   = 0x0000efe4
>> >    <3>[  718.315297][  T803] arm-smmu 15000000.apps-smmu: SID = 0x40
>> >    <3>[  718.315318][  T803] arm-smmu 15000000.apps-smmu: Client info: BID=0x5, PID=0xa, MID=0x2
>> >    <3>[  718.315377][  T803] arm-smmu 15000000.apps-smmu: soft iova-to-phys=0x0000000000000000
>> >
>> >    I can reduce this panic with the proposed patch, but it still happens until I
>> >    disable the "req->no_interrupt = 1" logic.
>> >
>> > 2) The frame is not fully transmitted in dwc3 with sg support enabled.
>> >
>> >    There seems to be a mapping limit I'm seeing where only the roughly first
>> >    70% of the total frame is sent. Interestingly, if I allocate a larger
>> >    size for the buffer upfront, in uvc_queue_setup(), like sizes[0] =
>> >    video->imagesize * 3. Then the issue rarely happens. For example, when I
>> >    do YUYV I see green, uninitialized data, at the bottom part of the
>> >    frame. If I do MJPG with smaller filled sizes, the transmission is fine.
>> >
>> >    +-------------------------+
>> >    |                         |
>> >    |                         |
>> >    |                         |
>> >    |      Good data          |
>> >    |                         |
>> >    |                         |
>> >    |                         |
>> >    +-------------------------+
>> >    |xxxxxxxxxxxxxxxxxxxxxxxxx|
>> >    |xxxx  Bad data  xxxxxxxxx|
>> >    |xxxxxxxxxxxxxxxxxxxxxxxxx|
>> >    +-------------------------+
>> >
>> >
>> > Appreciate any thoughts or feedback related to these issues.
>>
>> Anyway, this is probably due to the frames being given back to early to
>> the frameproducer. We have the following patches mainline now to fix this issue:
>>
>> aef11279888c00e1841a3533a35d279285af3a51 usb: gadget: uvc: improve sg exit condition
>> 9b969f93bcef9b3d9e92f1810e22bbd6c344a0e5 usb: gadget: uvc: giveback vb2 buffer on req complete
>
>Yes, I did grab those in addition to some other necessary changes, noted
>in my patch here:
>https://patchwork.kernel.org/project/linux-usb/patch/20220926195307.110121-2-w36195@motorola.com/
>I also pulled a lot of patches from the dwc3 to be almost at parity with
>top of tree for the core.c/h, ep0.c/h, and gadget.c/h files, but these
>issues persisted.
>
>Out of curiosity, have you tested these changes with dwc3 and if so,
>have you tried "usb: gadget: uvc: decrease the interrupt load to a
>quarter" with scatter/gather disabled? For me the crash occurs more
>often.

I remember seeing some issues when I switched from sg to memcopy
recently. This happened somehow inbetween working on those sg fixes and
was unexpected and happened without having some obvious code to break
things, since that already worked, when I implemented my first sg
series. But that memcopy issue somehow fell from my table. I will look
into that again for sure. I bet the dwc3 gadget driver got some late
patches that could have broken things here. So it is probably the best
to look into those dwc3 patches and bisect them.

Regards,
Michael

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] usb: gadget: uvc: fix sg handling in error case

[Greg Kroah-Hartman]

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;
>  	} 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

Re: [PATCH] usb: gadget: uvc: fix sg handling in error case

[Dan Vacura]

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;
> >  	} 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?

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);
> >  		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.

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

Re: [PATCH] usb: gadget: uvc: fix sg handling in error case

[Greg Kroah-Hartman]

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.
> 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.

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