[PATCH v4 1/6] usb: gadget: uvc: fix dropped frame after missed isoc

[PATCH v4 1/6] usb: gadget: uvc: fix dropped frame after missed isoc

[Dan Vacura]

With the re-use of the previous completion status in 0d1c407b1a749
("usb: dwc3: gadget: Return proper request status") it could be possible
that the next frame would also get dropped if the current frame has a
missed isoc error. Ensure that an interrupt is requested for the start
of a new frame.

Fixes: fc78941d8169 ("usb: gadget: uvc: decrease the interrupt load to a quarter")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V3:
- no change, new patch in series
V3 -> V4:
- no change

 drivers/usb/gadget/function/uvc_video.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index bb037fcc90e6..323977716f5a 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -431,7 +431,8 @@ static void uvcg_video_pump(struct work_struct *work)
 
 		/* Endpoint now owns the request */
 		req = NULL;
-		video->req_int_count++;
+		if (buf->state != UVC_BUF_STATE_DONE)
+			video->req_int_count++;
 	}
 
 	if (!req)
-- 
2.34.1

[PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc

[Dan Vacura]

From: Jeff Vanhoof <qjv001@motorola.com>

arm-smmu related crashes seen after a Missed ISOC interrupt when
no_interrupt=1 is used. This can happen if the hardware is still using
the data associated with a TRB after the usb_request's ->complete call
has been made.  Instead of immediately releasing a request when a Missed
ISOC interrupt has occurred, this change will add logic to cancel the
request instead where it will eventually be released when the
END_TRANSFER command has completed. This logic is similar to some of the
cleanup done in dwc3_gadget_ep_dequeue.

Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
Cc: <stable@vger.kernel.org>
Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
Co-developed-by: Dan Vacura <w36195@motorola.com>
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V3:
- no change, new patch in series
V3 -> V4:
- no change

 drivers/usb/dwc3/core.h   |  1 +
 drivers/usb/dwc3/gadget.c | 38 ++++++++++++++++++++++++++------------
 2 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8f9959ba9fd4..9b005d912241 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -943,6 +943,7 @@ struct dwc3_request {
 #define DWC3_REQUEST_STATUS_DEQUEUED		3
 #define DWC3_REQUEST_STATUS_STALLED		4
 #define DWC3_REQUEST_STATUS_COMPLETED		5
+#define DWC3_REQUEST_STATUS_MISSED_ISOC		6
 #define DWC3_REQUEST_STATUS_UNKNOWN		-1
 
 	u8			epnum;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 079cd333632e..411532c5c378 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2021,6 +2021,9 @@ static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep)
 		case DWC3_REQUEST_STATUS_STALLED:
 			dwc3_gadget_giveback(dep, req, -EPIPE);
 			break;
+		case DWC3_REQUEST_STATUS_MISSED_ISOC:
+			dwc3_gadget_giveback(dep, req, -EXDEV);
+			break;
 		default:
 			dev_err(dwc->dev, "request cancelled with wrong reason:%d\n", req->status);
 			dwc3_gadget_giveback(dep, req, -ECONNRESET);
@@ -3402,21 +3405,32 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
 	struct dwc3		*dwc = dep->dwc;
 	bool			no_started_trb = true;
 
-	dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
+	if (status == -EXDEV) {
+		struct dwc3_request *tmp;
+		struct dwc3_request *req;
 
-	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
-		goto out;
+		if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
+			dwc3_stop_active_transfer(dep, true, true);
 
-	if (!dep->endpoint.desc)
-		return no_started_trb;
+		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
+			dwc3_gadget_move_cancelled_request(req,
+					DWC3_REQUEST_STATUS_MISSED_ISOC);
+	} else {
+		dwc3_gadget_ep_cleanup_completed_requests(dep, event, status);
 
-	if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
-		list_empty(&dep->started_list) &&
-		(list_empty(&dep->pending_list) || status == -EXDEV))
-		dwc3_stop_active_transfer(dep, true, true);
-	else if (dwc3_gadget_ep_should_continue(dep))
-		if (__dwc3_gadget_kick_transfer(dep) == 0)
-			no_started_trb = false;
+		if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
+			goto out;
+
+		if (!dep->endpoint.desc)
+			return no_started_trb;
+
+		if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+			list_empty(&dep->started_list) && list_empty(&dep->pending_list))
+			dwc3_stop_active_transfer(dep, true, true);
+		else if (dwc3_gadget_ep_should_continue(dep))
+			if (__dwc3_gadget_kick_transfer(dep) == 0)
+				no_started_trb = false;
+	}
 
 out:
 	/*
-- 
2.34.1

Re: [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc

[Greg Kroah-Hartman]

On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
> From: Jeff Vanhoof <qjv001@motorola.com>
> 
> arm-smmu related crashes seen after a Missed ISOC interrupt when
> no_interrupt=1 is used. This can happen if the hardware is still using
> the data associated with a TRB after the usb_request's ->complete call
> has been made.  Instead of immediately releasing a request when a Missed
> ISOC interrupt has occurred, this change will add logic to cancel the
> request instead where it will eventually be released when the
> END_TRANSFER command has completed. This logic is similar to some of the
> cleanup done in dwc3_gadget_ep_dequeue.
> 
> Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
> Co-developed-by: Dan Vacura <w36195@motorola.com>
> Signed-off-by: Dan Vacura <w36195@motorola.com>
> ---
> V1 -> V3:
> - no change, new patch in series
> V3 -> V4:
> - no change

I need an ack from the dwc3 maintainer before I can take this one.

thanks,

greg k-h

Re: [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc

[Jeff Vanhoof]

Hi Greg,

On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
> > From: Jeff Vanhoof <qjv001@motorola.com>
> > 
> > arm-smmu related crashes seen after a Missed ISOC interrupt when
> > no_interrupt=1 is used. This can happen if the hardware is still using
> > the data associated with a TRB after the usb_request's ->complete call
> > has been made.  Instead of immediately releasing a request when a Missed
> > ISOC interrupt has occurred, this change will add logic to cancel the
> > request instead where it will eventually be released when the
> > END_TRANSFER command has completed. This logic is similar to some of the
> > cleanup done in dwc3_gadget_ep_dequeue.
> > 
> > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
> > Co-developed-by: Dan Vacura <w36195@motorola.com>
> > Signed-off-by: Dan Vacura <w36195@motorola.com>
> > ---
> > V1 -> V3:
> > - no change, new patch in series
> > V3 -> V4:
> > - no change
> 
> I need an ack from the dwc3 maintainer before I can take this one.
> 
> thanks,
> 
> greg k-h

Thinh has rejected this version of the patch. He has provided an alternative
implementation which has been testing well for us so far. Either Thinh or Dan
will formalize this patch within the next few days.
The latest proposed changes are:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index dfaf9ac24c4f..50287437d6de 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
        if (event->status & DEPEVT_STATUS_SHORT && !chain)
                return 1;
 
+       if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
+               return 1;
+
        if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
            (trb->ctrl & DWC3_TRB_CTRL_LST))
                return 1;
@@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
        struct scatterlist *s;
        unsigned int num_queued = req->num_queued_sgs;
        unsigned int i;
+       bool missed_isoc = false;
        int ret = 0;
 
        for_each_sg(sg, s, num_queued, i) {
@@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
                req->sg = sg_next(s);
                req->num_queued_sgs--;
 
+               if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
+                       missed_isoc = true;
+
                ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
                                trb, event, status, true);
                if (ret)
                        break;
        }
 
+       if (missed_isoc)
+               ret = 1;
+
        return ret;
 }


Thanks,
Jeff

Re: [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc

[Thinh Nguyen]

On Sat, Oct 22, 2022, Jeff Vanhoof wrote:
> Hi Greg,
> 
> On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
> > > From: Jeff Vanhoof <qjv001@motorola.com>
> > > 
> > > arm-smmu related crashes seen after a Missed ISOC interrupt when
> > > no_interrupt=1 is used. This can happen if the hardware is still using
> > > the data associated with a TRB after the usb_request's ->complete call
> > > has been made.  Instead of immediately releasing a request when a Missed
> > > ISOC interrupt has occurred, this change will add logic to cancel the
> > > request instead where it will eventually be released when the
> > > END_TRANSFER command has completed. This logic is similar to some of the
> > > cleanup done in dwc3_gadget_ep_dequeue.
> > > 
> > > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
> > > Co-developed-by: Dan Vacura <w36195@motorola.com>
> > > Signed-off-by: Dan Vacura <w36195@motorola.com>
> > > ---
> > > V1 -> V3:
> > > - no change, new patch in series
> > > V3 -> V4:
> > > - no change
> > 
> > I need an ack from the dwc3 maintainer before I can take this one.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Thinh has rejected this version of the patch. He has provided an alternative
> implementation which has been testing well for us so far. Either Thinh or Dan
> will formalize this patch within the next few days.
> The latest proposed changes are:
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index dfaf9ac24c4f..50287437d6de 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>         if (event->status & DEPEVT_STATUS_SHORT && !chain)
>                 return 1;
>  
> +       if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
> +               return 1;
> +
>         if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
>             (trb->ctrl & DWC3_TRB_CTRL_LST))
>                 return 1;
> @@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>         struct scatterlist *s;
>         unsigned int num_queued = req->num_queued_sgs;
>         unsigned int i;
> +       bool missed_isoc = false;
>         int ret = 0;
>  
>         for_each_sg(sg, s, num_queued, i) {
> @@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>                 req->sg = sg_next(s);
>                 req->num_queued_sgs--;
>  
> +               if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
> +                       missed_isoc = true;
> +
>                 ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>                                 trb, event, status, true);
>                 if (ret)
>                         break;
>         }
>  
> +       if (missed_isoc)
> +               ret = 1;
> +
>         return ret;
>  }
> 
> 

That's just a debug patch. I'll send out proper fix patches.

Thanks,
Thinh

Re: [PATCH v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc

[Michael Grzeschik]

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

Hi Thinh,

On Mon, Oct 24, 2022 at 10:47:53PM +0000, Thinh Nguyen wrote:
>On Sat, Oct 22, 2022, Jeff Vanhoof wrote:
>> Hi Greg,
>>
>> On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote:
>> > On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
>> > > From: Jeff Vanhoof <qjv001@motorola.com>
>> > >
>> > > arm-smmu related crashes seen after a Missed ISOC interrupt when
>> > > no_interrupt=1 is used. This can happen if the hardware is still using
>> > > the data associated with a TRB after the usb_request's ->complete call
>> > > has been made.  Instead of immediately releasing a request when a Missed
>> > > ISOC interrupt has occurred, this change will add logic to cancel the
>> > > request instead where it will eventually be released when the
>> > > END_TRANSFER command has completed. This logic is similar to some of the
>> > > cleanup done in dwc3_gadget_ep_dequeue.
>> > >
>> > > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
>> > > Cc: <stable@vger.kernel.org>
>> > > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
>> > > Co-developed-by: Dan Vacura <w36195@motorola.com>
>> > > Signed-off-by: Dan Vacura <w36195@motorola.com>
>> > > ---
>> > > V1 -> V3:
>> > > - no change, new patch in series
>> > > V3 -> V4:
>> > > - no change
>> >
>> > I need an ack from the dwc3 maintainer before I can take this one.
>> >
>> > thanks,
>> >
>> > greg k-h
>>
>> Thinh has rejected this version of the patch. He has provided an alternative
>> implementation which has been testing well for us so far. Either Thinh or Dan
>> will formalize this patch within the next few days.
>> The latest proposed changes are:
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index dfaf9ac24c4f..50287437d6de 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>         if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>                 return 1;
>>
>> +       if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
>> +               return 1;
>> +
>>         if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
>>             (trb->ctrl & DWC3_TRB_CTRL_LST))
>>                 return 1;
>> @@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>>         struct scatterlist *s;
>>         unsigned int num_queued = req->num_queued_sgs;
>>         unsigned int i;
>> +       bool missed_isoc = false;
>>         int ret = 0;
>>
>>         for_each_sg(sg, s, num_queued, i) {
>> @@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>>                 req->sg = sg_next(s);
>>                 req->num_queued_sgs--;
>>
>> +               if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
>> +                       missed_isoc = true;
>> +
>>                 ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>>                                 trb, event, status, true);
>>                 if (ret)
>>                         break;
>>         }
>>
>> +       if (missed_isoc)
>> +               ret = 1;
>> +
>>         return ret;
>>  }
>>
>>
>
>That's just a debug patch. I'll send out proper fix patches.

Ping!

While digging out this thread, I did not find any followup patch
for this suggestion. Did it hit the mailinglist anywhere?

If not, will you send one?

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 v4 2/6] usb: dwc3: gadget: cancel requests instead of release after missed isoc

[Michael Grzeschik]

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

On Tue, Sep 19, 2023 at 11:10:55AM +0200, Michael Grzeschik wrote:
>On Mon, Oct 24, 2022 at 10:47:53PM +0000, Thinh Nguyen wrote:
>>On Sat, Oct 22, 2022, Jeff Vanhoof wrote:
>>>Hi Greg,
>>>
>>>On Sat, Oct 22, 2022 at 01:31:24PM +0200, Greg Kroah-Hartman wrote:
>>>> On Tue, Oct 18, 2022 at 04:50:38PM -0500, Dan Vacura wrote:
>>>> > From: Jeff Vanhoof <qjv001@motorola.com>
>>>> >
>>>> > arm-smmu related crashes seen after a Missed ISOC interrupt when
>>>> > no_interrupt=1 is used. This can happen if the hardware is still using
>>>> > the data associated with a TRB after the usb_request's ->complete call
>>>> > has been made.  Instead of immediately releasing a request when a Missed
>>>> > ISOC interrupt has occurred, this change will add logic to cancel the
>>>> > request instead where it will eventually be released when the
>>>> > END_TRANSFER command has completed. This logic is similar to some of the
>>>> > cleanup done in dwc3_gadget_ep_dequeue.
>>>> >
>>>> > Fixes: 6d8a019614f3 ("usb: dwc3: gadget: check for Missed Isoc from event status")
>>>> > Cc: <stable@vger.kernel.org>
>>>> > Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
>>>> > Co-developed-by: Dan Vacura <w36195@motorola.com>
>>>> > Signed-off-by: Dan Vacura <w36195@motorola.com>
>>>> > ---
>>>> > V1 -> V3:
>>>> > - no change, new patch in series
>>>> > V3 -> V4:
>>>> > - no change
>>>>
>>>> I need an ack from the dwc3 maintainer before I can take this one.
>>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>>
>>>Thinh has rejected this version of the patch. He has provided an alternative
>>>implementation which has been testing well for us so far. Either Thinh or Dan
>>>will formalize this patch within the next few days.
>>>The latest proposed changes are:
>>>
>>>diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>index dfaf9ac24c4f..50287437d6de 100644
>>>--- a/drivers/usb/dwc3/gadget.c
>>>+++ b/drivers/usb/dwc3/gadget.c
>>>@@ -3195,6 +3195,9 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>>        if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>>                return 1;
>>>
>>>+       if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC && !chain)
>>>+               return 1;
>>>+
>>>        if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
>>>            (trb->ctrl & DWC3_TRB_CTRL_LST))
>>>                return 1;
>>>@@ -3211,6 +3214,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>>>        struct scatterlist *s;
>>>        unsigned int num_queued = req->num_queued_sgs;
>>>        unsigned int i;
>>>+       bool missed_isoc = false;
>>>        int ret = 0;
>>>
>>>        for_each_sg(sg, s, num_queued, i) {
>>>@@ -3219,12 +3223,18 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
>>>                req->sg = sg_next(s);
>>>                req->num_queued_sgs--;
>>>
>>>+               if (DWC3_TRB_SIZE_TRBSTS(trb->size) == DWC3_TRBSTS_MISSED_ISOC)
>>>+                       missed_isoc = true;
>>>+
>>>                ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
>>>                                trb, event, status, true);
>>>                if (ret)
>>>                        break;
>>>        }
>>>
>>>+       if (missed_isoc)
>>>+               ret = 1;
>>>+
>>>        return ret;
>>> }
>>>
>>>
>>
>>That's just a debug patch. I'll send out proper fix patches.
>
>Ping!
>
>While digging out this thread, I did not find any followup patch
>for this suggestion. Did it hit the mailinglist anywhere?
>
>If not, will you send one?

Nevermind, I think I found the hunk in a variated version in this series.

https://lore.kernel.org/linux-usb/cover.1666735451.git.Thinh.Nguyen@synopsys.com/

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

[PATCH v4 3/6] 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). This ensures userspace knows if the
frame was successfully sent.

Fixes: e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V2:
- undo error rename
- change uvcg_info to uvcg_dbg
V2 -> V3:
- no changes
V3 -> V4:
- drop extra cc stable cherry-picks, as a request was made to stable

 drivers/usb/gadget/function/uvc_queue.c |  8 +++++---
 drivers/usb/gadget/function/uvc_video.c | 18 ++++++++++++++----
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
index ec500ee499ee..0aa3d7e1f3cc 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_DROP_INCOMPLETE;
 	} 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_DROP_INCOMPLETE) {
+		queue->flags &= ~UVC_QUEUE_DROP_INCOMPLETE;
+		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_video.c b/drivers/usb/gadget/function/uvc_video.c
index 323977716f5a..5993e083819c 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_DROP_INCOMPLETE ||
 	    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_DROP_INCOMPLETE) {
 		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_DROP_INCOMPLETE) {
 		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_dbg(&video->uvc->func, "VS request missed xfer.\n");
+		queue->flags |= UVC_QUEUE_DROP_INCOMPLETE;
+		break;
+
 	case -ESHUTDOWN:	/* disconnect from host. */
 		uvcg_dbg(&video->uvc->func, "VS request cancelled.\n");
 		uvcg_queue_cancel(queue, 1);
-- 
2.34.1

[PATCH v4 4/6] usb: gadget: uvc: fix sg handling during video encode

[Dan Vacura]

From: Jeff Vanhoof <qjv001@motorola.com>

In uvc_video_encode_isoc_sg, the uvc_request's sg list is
incorrectly being populated leading to corrupt video being
received by the remote end. When building the sg list the
usage of buf->sg's 'dma_length' field is not correct and
instead its 'length' field should be used.

Fixes: e81e7f9a0eb9 ("usb: gadget: uvc: add scatter gather support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Jeff Vanhoof <qjv001@motorola.com>
Signed-off-by: Dan Vacura <w36195@motorola.com>
---
V1 -> V3:
- no change, new patch in series
V3 -> V4:
- no change

 drivers/usb/gadget/function/uvc_video.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 5993e083819c..dd1c6b2ca7c6 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -157,10 +157,10 @@ uvc_video_encode_isoc_sg(struct usb_request *req, struct uvc_video *video,
 	sg = sg_next(sg);
 
 	for_each_sg(sg, iter, ureq->sgt.nents - 1, i) {
-		if (!len || !buf->sg || !sg_dma_len(buf->sg))
+		if (!len || !buf->sg || !buf->sg->length)
 			break;
 
-		sg_left = sg_dma_len(buf->sg) - buf->offset;
+		sg_left = buf->sg->length - buf->offset;
 		part = min_t(unsigned int, len, sg_left);
 
 		sg_set_page(iter, sg_page(buf->sg), part, buf->offset);
-- 
2.34.1