2020-01-27 19:32:01

by John Stultz

[permalink] [raw]
Subject: [PATCH v2] usb: dwc3: gadget: Check for IOC/LST bit in TRB->ctrl fields

From: Anurag Kumar Vulisha <[email protected]>

The current code in dwc3_gadget_ep_reclaim_completed_trb() will
check for IOC/LST bit in the event->status and returns if
IOC/LST bit is set. This logic doesn't work if multiple TRBs
are queued per request and the IOC/LST bit is set on the last
TRB of that request.

Consider an example where a queued request has multiple queued
TRBs and IOC/LST bit is set only for the last TRB. In this case,
the core generates XferComplete/XferInProgress events only for
the last TRB (since IOC/LST are set only for the last TRB). As
per the logic in dwc3_gadget_ep_reclaim_completed_trb()
event->status is checked for IOC/LST bit and returns on the
first TRB. This leaves the remaining TRBs left unhandled.

Similarly, if the gadget function enqueues an unaligned request
with sglist already in it, it should fail the same way, since we
will append another TRB to something that already uses more than
one TRB.

To aviod this, this patch changes the code to check for IOC/LST
bits in TRB->ctrl instead.

At a practical level, this patch resolves USB transfer stalls seen
with adb on dwc3 based HiKey960 after functionfs gadget added
scatter-gather support around v4.20.

Cc: Felipe Balbi <[email protected]>
Cc: Yang Fei <[email protected]>
Cc: Thinh Nguyen <[email protected]>
Cc: Tejas Joglekar <[email protected]>
Cc: Andrzej Pietrasiewicz <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: Todd Kjos <[email protected]>
Cc: Greg KH <[email protected]>
Cc: Linux USB List <[email protected]>
Cc: stable <[email protected]>
Tested-by: Tejas Joglekar <[email protected]>
Reviewed-by: Thinh Nguyen <[email protected]>
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
[jstultz: forward ported to mainline, reworded commit log, reworked
to only check trb->ctrl as suggested by Felipe]
Signed-off-by: John Stultz <[email protected]>
---
v2:
* Rework to only check trb->ctrl as suggested by Felipe
* Reword the commit message to include more of Felipe's assessment
---
drivers/usb/dwc3/gadget.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 154f3f3e8cff..9a085eee1ae3 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2420,7 +2420,8 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
if (event->status & DEPEVT_STATUS_SHORT && !chain)
return 1;

- if (event->status & DEPEVT_STATUS_IOC)
+ if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
+ (trb->ctrl & DWC3_TRB_CTRL_LST))
return 1;

return 0;
--
2.17.1


2020-01-28 13:24:13

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH v2] usb: dwc3: gadget: Check for IOC/LST bit in TRB->ctrl fields


Hi,

John Stultz <[email protected]> writes:

> From: Anurag Kumar Vulisha <[email protected]>
>
> The current code in dwc3_gadget_ep_reclaim_completed_trb() will
> check for IOC/LST bit in the event->status and returns if
> IOC/LST bit is set. This logic doesn't work if multiple TRBs
> are queued per request and the IOC/LST bit is set on the last
> TRB of that request.
>
> Consider an example where a queued request has multiple queued
> TRBs and IOC/LST bit is set only for the last TRB. In this case,
> the core generates XferComplete/XferInProgress events only for
> the last TRB (since IOC/LST are set only for the last TRB). As
> per the logic in dwc3_gadget_ep_reclaim_completed_trb()
> event->status is checked for IOC/LST bit and returns on the
> first TRB. This leaves the remaining TRBs left unhandled.
>
> Similarly, if the gadget function enqueues an unaligned request
> with sglist already in it, it should fail the same way, since we
> will append another TRB to something that already uses more than
> one TRB.
>
> To aviod this, this patch changes the code to check for IOC/LST
> bits in TRB->ctrl instead.
>
> At a practical level, this patch resolves USB transfer stalls seen
> with adb on dwc3 based HiKey960 after functionfs gadget added
> scatter-gather support around v4.20.
>
> Cc: Felipe Balbi <[email protected]>
> Cc: Yang Fei <[email protected]>
> Cc: Thinh Nguyen <[email protected]>
> Cc: Tejas Joglekar <[email protected]>
> Cc: Andrzej Pietrasiewicz <[email protected]>
> Cc: Jack Pham <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Greg KH <[email protected]>
> Cc: Linux USB List <[email protected]>
> Cc: stable <[email protected]>
> Tested-by: Tejas Joglekar <[email protected]>
> Reviewed-by: Thinh Nguyen <[email protected]>
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> [jstultz: forward ported to mainline, reworded commit log, reworked
> to only check trb->ctrl as suggested by Felipe]
> Signed-off-by: John Stultz <[email protected]>
> ---
> v2:
> * Rework to only check trb->ctrl as suggested by Felipe
> * Reword the commit message to include more of Felipe's assessment
> ---
> drivers/usb/dwc3/gadget.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 154f3f3e8cff..9a085eee1ae3 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2420,7 +2420,8 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> if (event->status & DEPEVT_STATUS_SHORT && !chain)
> return 1;
>
> - if (event->status & DEPEVT_STATUS_IOC)
> + if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
> + (trb->ctrl & DWC3_TRB_CTRL_LST))

why the LST bit here? It wasn't there before. In fact, we never set LST
in dwc3 anymore :-)

--
balbi


Attachments:
signature.asc (847.00 B)

2020-01-28 17:57:12

by John Stultz

[permalink] [raw]
Subject: Re: [PATCH v2] usb: dwc3: gadget: Check for IOC/LST bit in TRB->ctrl fields

On Tue, Jan 28, 2020 at 5:23 AM Felipe Balbi <[email protected]> wrote:
>
>
> Hi,
>
> John Stultz <[email protected]> writes:
>
> > From: Anurag Kumar Vulisha <[email protected]>
> >
> > The current code in dwc3_gadget_ep_reclaim_completed_trb() will
> > check for IOC/LST bit in the event->status and returns if
> > IOC/LST bit is set. This logic doesn't work if multiple TRBs
> > are queued per request and the IOC/LST bit is set on the last
> > TRB of that request.
> >
> > Consider an example where a queued request has multiple queued
> > TRBs and IOC/LST bit is set only for the last TRB. In this case,
> > the core generates XferComplete/XferInProgress events only for
> > the last TRB (since IOC/LST are set only for the last TRB). As
> > per the logic in dwc3_gadget_ep_reclaim_completed_trb()
> > event->status is checked for IOC/LST bit and returns on the
> > first TRB. This leaves the remaining TRBs left unhandled.
> >
> > Similarly, if the gadget function enqueues an unaligned request
> > with sglist already in it, it should fail the same way, since we
> > will append another TRB to something that already uses more than
> > one TRB.
> >
> > To aviod this, this patch changes the code to check for IOC/LST
> > bits in TRB->ctrl instead.
> >
> > At a practical level, this patch resolves USB transfer stalls seen
> > with adb on dwc3 based HiKey960 after functionfs gadget added
> > scatter-gather support around v4.20.
> >
> > Cc: Felipe Balbi <[email protected]>
> > Cc: Yang Fei <[email protected]>
> > Cc: Thinh Nguyen <[email protected]>
> > Cc: Tejas Joglekar <[email protected]>
> > Cc: Andrzej Pietrasiewicz <[email protected]>
> > Cc: Jack Pham <[email protected]>
> > Cc: Todd Kjos <[email protected]>
> > Cc: Greg KH <[email protected]>
> > Cc: Linux USB List <[email protected]>
> > Cc: stable <[email protected]>
> > Tested-by: Tejas Joglekar <[email protected]>
> > Reviewed-by: Thinh Nguyen <[email protected]>
> > Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> > [jstultz: forward ported to mainline, reworded commit log, reworked
> > to only check trb->ctrl as suggested by Felipe]
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > v2:
> > * Rework to only check trb->ctrl as suggested by Felipe
> > * Reword the commit message to include more of Felipe's assessment
> > ---
> > drivers/usb/dwc3/gadget.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 154f3f3e8cff..9a085eee1ae3 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2420,7 +2420,8 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
> > if (event->status & DEPEVT_STATUS_SHORT && !chain)
> > return 1;
> >
> > - if (event->status & DEPEVT_STATUS_IOC)
> > + if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
> > + (trb->ctrl & DWC3_TRB_CTRL_LST))
>
> why the LST bit here? It wasn't there before. In fact, we never set LST
> in dwc3 anymore :-)

So, it was in the patch before, just on separate lines:
https://lore.kernel.org/lkml/[email protected]/

- if (event->status & DEPEVT_STATUS_IOC)
+ if ((event->status & DEPEVT_STATUS_IOC) &&
+ (trb->ctrl & DWC3_TRB_CTRL_IOC))
+ return 1;
+
+ if ((event->status & DEPEVT_STATUS_LST) &&
+ (trb->ctrl & DWC3_TRB_CTRL_LST))
return 1;

So I just merged the two checks into one line. But I'm ok to drop the
CTRL_LST check if that really isn't used. Let me know and I'll rework
and resend.

thanks
-john

2020-01-28 18:26:48

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH v2] usb: dwc3: gadget: Check for IOC/LST bit in TRB->ctrl fields

Hi,

Felipe Balbi wrote:
> Hi,
>
> John Stultz <[email protected]> writes:
>
>> From: Anurag Kumar Vulisha <[email protected]>
>>
>> The current code in dwc3_gadget_ep_reclaim_completed_trb() will
>> check for IOC/LST bit in the event->status and returns if
>> IOC/LST bit is set. This logic doesn't work if multiple TRBs
>> are queued per request and the IOC/LST bit is set on the last
>> TRB of that request.
>>
>> Consider an example where a queued request has multiple queued
>> TRBs and IOC/LST bit is set only for the last TRB. In this case,
>> the core generates XferComplete/XferInProgress events only for
>> the last TRB (since IOC/LST are set only for the last TRB). As
>> per the logic in dwc3_gadget_ep_reclaim_completed_trb()
>> event->status is checked for IOC/LST bit and returns on the
>> first TRB. This leaves the remaining TRBs left unhandled.
>>
>> Similarly, if the gadget function enqueues an unaligned request
>> with sglist already in it, it should fail the same way, since we
>> will append another TRB to something that already uses more than
>> one TRB.
>>
>> To aviod this, this patch changes the code to check for IOC/LST
>> bits in TRB->ctrl instead.
>>
>> At a practical level, this patch resolves USB transfer stalls seen
>> with adb on dwc3 based HiKey960 after functionfs gadget added
>> scatter-gather support around v4.20.
>>
>> Cc: Felipe Balbi <[email protected]>
>> Cc: Yang Fei <[email protected]>
>> Cc: Thinh Nguyen <[email protected]>
>> Cc: Tejas Joglekar <[email protected]>
>> Cc: Andrzej Pietrasiewicz <[email protected]>
>> Cc: Jack Pham <[email protected]>
>> Cc: Todd Kjos <[email protected]>
>> Cc: Greg KH <[email protected]>
>> Cc: Linux USB List <[email protected]>
>> Cc: stable <[email protected]>
>> Tested-by: Tejas Joglekar <[email protected]>
>> Reviewed-by: Thinh Nguyen <[email protected]>
>> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
>> [jstultz: forward ported to mainline, reworded commit log, reworked
>> to only check trb->ctrl as suggested by Felipe]
>> Signed-off-by: John Stultz <[email protected]>
>> ---
>> v2:
>> * Rework to only check trb->ctrl as suggested by Felipe
>> * Reword the commit message to include more of Felipe's assessment
>> ---
>> drivers/usb/dwc3/gadget.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 154f3f3e8cff..9a085eee1ae3 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2420,7 +2420,8 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>> if (event->status & DEPEVT_STATUS_SHORT && !chain)
>> return 1;
>>
>> - if (event->status & DEPEVT_STATUS_IOC)
>> + if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
>> + (trb->ctrl & DWC3_TRB_CTRL_LST))
> why the LST bit here? It wasn't there before. In fact, we never set LST
> in dwc3 anymore :-)
>

Just a note: right now, it may be fine for non-stream endpoints to not
set the LST bit in the TRBs. For streams, we need to set this bit so the
controller know to allocate resource for different transfers of
different streams. It may be fine now if you think that it should be
added later when more fixes for streams are added, but I think it
doesn't hurt checking it now either.

BR,
Thinh

2020-01-29 07:18:21

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH v2] usb: dwc3: gadget: Check for IOC/LST bit in TRB->ctrl fields


Hi,

Thinh Nguyen <[email protected]> writes:
>> John Stultz <[email protected]> writes:
>>
>>> From: Anurag Kumar Vulisha <[email protected]>
>>>
>>> The current code in dwc3_gadget_ep_reclaim_completed_trb() will
>>> check for IOC/LST bit in the event->status and returns if
>>> IOC/LST bit is set. This logic doesn't work if multiple TRBs
>>> are queued per request and the IOC/LST bit is set on the last
>>> TRB of that request.
>>>
>>> Consider an example where a queued request has multiple queued
>>> TRBs and IOC/LST bit is set only for the last TRB. In this case,
>>> the core generates XferComplete/XferInProgress events only for
>>> the last TRB (since IOC/LST are set only for the last TRB). As
>>> per the logic in dwc3_gadget_ep_reclaim_completed_trb()
>>> event->status is checked for IOC/LST bit and returns on the
>>> first TRB. This leaves the remaining TRBs left unhandled.
>>>
>>> Similarly, if the gadget function enqueues an unaligned request
>>> with sglist already in it, it should fail the same way, since we
>>> will append another TRB to something that already uses more than
>>> one TRB.
>>>
>>> To aviod this, this patch changes the code to check for IOC/LST
>>> bits in TRB->ctrl instead.
>>>
>>> At a practical level, this patch resolves USB transfer stalls seen
>>> with adb on dwc3 based HiKey960 after functionfs gadget added
>>> scatter-gather support around v4.20.
>>>
>>> Cc: Felipe Balbi <[email protected]>
>>> Cc: Yang Fei <[email protected]>
>>> Cc: Thinh Nguyen <[email protected]>
>>> Cc: Tejas Joglekar <[email protected]>
>>> Cc: Andrzej Pietrasiewicz <[email protected]>
>>> Cc: Jack Pham <[email protected]>
>>> Cc: Todd Kjos <[email protected]>
>>> Cc: Greg KH <[email protected]>
>>> Cc: Linux USB List <[email protected]>
>>> Cc: stable <[email protected]>
>>> Tested-by: Tejas Joglekar <[email protected]>
>>> Reviewed-by: Thinh Nguyen <[email protected]>
>>> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
>>> [jstultz: forward ported to mainline, reworded commit log, reworked
>>> to only check trb->ctrl as suggested by Felipe]
>>> Signed-off-by: John Stultz <[email protected]>
>>> ---
>>> v2:
>>> * Rework to only check trb->ctrl as suggested by Felipe
>>> * Reword the commit message to include more of Felipe's assessment
>>> ---
>>> drivers/usb/dwc3/gadget.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 154f3f3e8cff..9a085eee1ae3 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2420,7 +2420,8 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>> if (event->status & DEPEVT_STATUS_SHORT && !chain)
>>> return 1;
>>>
>>> - if (event->status & DEPEVT_STATUS_IOC)
>>> + if ((trb->ctrl & DWC3_TRB_CTRL_IOC) ||
>>> + (trb->ctrl & DWC3_TRB_CTRL_LST))
>> why the LST bit here? It wasn't there before. In fact, we never set LST
>> in dwc3 anymore :-)
>
> Just a note: right now, it may be fine for non-stream endpoints to not
> set the LST bit in the TRBs. For streams, we need to set this bit so the
> controller know to allocate resource for different transfers of
> different streams. It may be fine now if you think that it should be
> added later when more fixes for streams are added, but I think it
> doesn't hurt checking it now either.

Indeed. Let's keep this version as is if we will need LST for Streams
anyway. Sorry for the noise.

--
balbi


Attachments:
signature.asc (847.00 B)

2020-01-29 07:24:20

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH v2] usb: dwc3: gadget: Check for IOC/LST bit in TRB->ctrl fields


Hi,

John Stultz <[email protected]> writes:
> From: Anurag Kumar Vulisha <[email protected]>
>
> The current code in dwc3_gadget_ep_reclaim_completed_trb() will
> check for IOC/LST bit in the event->status and returns if
> IOC/LST bit is set. This logic doesn't work if multiple TRBs
> are queued per request and the IOC/LST bit is set on the last
> TRB of that request.
>
> Consider an example where a queued request has multiple queued
> TRBs and IOC/LST bit is set only for the last TRB. In this case,
> the core generates XferComplete/XferInProgress events only for
> the last TRB (since IOC/LST are set only for the last TRB). As
> per the logic in dwc3_gadget_ep_reclaim_completed_trb()
> event->status is checked for IOC/LST bit and returns on the
> first TRB. This leaves the remaining TRBs left unhandled.
>
> Similarly, if the gadget function enqueues an unaligned request
> with sglist already in it, it should fail the same way, since we
> will append another TRB to something that already uses more than
> one TRB.
>
> To aviod this, this patch changes the code to check for IOC/LST
> bits in TRB->ctrl instead.
>
> At a practical level, this patch resolves USB transfer stalls seen
> with adb on dwc3 based HiKey960 after functionfs gadget added
> scatter-gather support around v4.20.
>
> Cc: Felipe Balbi <[email protected]>
> Cc: Yang Fei <[email protected]>
> Cc: Thinh Nguyen <[email protected]>
> Cc: Tejas Joglekar <[email protected]>
> Cc: Andrzej Pietrasiewicz <[email protected]>
> Cc: Jack Pham <[email protected]>
> Cc: Todd Kjos <[email protected]>
> Cc: Greg KH <[email protected]>
> Cc: Linux USB List <[email protected]>
> Cc: stable <[email protected]>
> Tested-by: Tejas Joglekar <[email protected]>
> Reviewed-by: Thinh Nguyen <[email protected]>
> Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> [jstultz: forward ported to mainline, reworded commit log, reworked
> to only check trb->ctrl as suggested by Felipe]
> Signed-off-by: John Stultz <[email protected]>

since v5.5 is already merged, I'll send this to Greg once -rc1 is
tagged. It's already in my testing/fixes branch waiting for a pull
request.

cheers

--
balbi


Attachments:
signature.asc (847.00 B)

2020-01-29 07:37:15

by John Stultz

[permalink] [raw]
Subject: Re: [PATCH v2] usb: dwc3: gadget: Check for IOC/LST bit in TRB->ctrl fields

On Tue, Jan 28, 2020 at 11:23 PM Felipe Balbi <[email protected]> wrote:
> John Stultz <[email protected]> writes:
> > From: Anurag Kumar Vulisha <[email protected]>
> >
> > The current code in dwc3_gadget_ep_reclaim_completed_trb() will
> > check for IOC/LST bit in the event->status and returns if
> > IOC/LST bit is set. This logic doesn't work if multiple TRBs
> > are queued per request and the IOC/LST bit is set on the last
> > TRB of that request.
> >
> > Consider an example where a queued request has multiple queued
> > TRBs and IOC/LST bit is set only for the last TRB. In this case,
> > the core generates XferComplete/XferInProgress events only for
> > the last TRB (since IOC/LST are set only for the last TRB). As
> > per the logic in dwc3_gadget_ep_reclaim_completed_trb()
> > event->status is checked for IOC/LST bit and returns on the
> > first TRB. This leaves the remaining TRBs left unhandled.
> >
> > Similarly, if the gadget function enqueues an unaligned request
> > with sglist already in it, it should fail the same way, since we
> > will append another TRB to something that already uses more than
> > one TRB.
> >
> > To aviod this, this patch changes the code to check for IOC/LST
> > bits in TRB->ctrl instead.
> >
> > At a practical level, this patch resolves USB transfer stalls seen
> > with adb on dwc3 based HiKey960 after functionfs gadget added
> > scatter-gather support around v4.20.
> >
> > Cc: Felipe Balbi <[email protected]>
> > Cc: Yang Fei <[email protected]>
> > Cc: Thinh Nguyen <[email protected]>
> > Cc: Tejas Joglekar <[email protected]>
> > Cc: Andrzej Pietrasiewicz <[email protected]>
> > Cc: Jack Pham <[email protected]>
> > Cc: Todd Kjos <[email protected]>
> > Cc: Greg KH <[email protected]>
> > Cc: Linux USB List <[email protected]>
> > Cc: stable <[email protected]>
> > Tested-by: Tejas Joglekar <[email protected]>
> > Reviewed-by: Thinh Nguyen <[email protected]>
> > Signed-off-by: Anurag Kumar Vulisha <[email protected]>
> > [jstultz: forward ported to mainline, reworded commit log, reworked
> > to only check trb->ctrl as suggested by Felipe]
> > Signed-off-by: John Stultz <[email protected]>
>
> since v5.5 is already merged, I'll send this to Greg once -rc1 is
> tagged. It's already in my testing/fixes branch waiting for a pull
> request.

Great, thanks so much for queueing this! I'll be digging on the db845c
side wrt the dma-api issue to hopefully get that one sorted as well.

Thanks again for the help and analysis!
-john

2020-01-29 11:20:57

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH v2] usb: dwc3: gadget: Check for IOC/LST bit in TRB->ctrl fields


Hi,

John Stultz <[email protected]> writes:
> On Tue, Jan 28, 2020 at 11:23 PM Felipe Balbi <[email protected]> wrote:
>> John Stultz <[email protected]> writes:
>> > From: Anurag Kumar Vulisha <[email protected]>
>> >
>> > The current code in dwc3_gadget_ep_reclaim_completed_trb() will
>> > check for IOC/LST bit in the event->status and returns if
>> > IOC/LST bit is set. This logic doesn't work if multiple TRBs
>> > are queued per request and the IOC/LST bit is set on the last
>> > TRB of that request.
>> >
>> > Consider an example where a queued request has multiple queued
>> > TRBs and IOC/LST bit is set only for the last TRB. In this case,
>> > the core generates XferComplete/XferInProgress events only for
>> > the last TRB (since IOC/LST are set only for the last TRB). As
>> > per the logic in dwc3_gadget_ep_reclaim_completed_trb()
>> > event->status is checked for IOC/LST bit and returns on the
>> > first TRB. This leaves the remaining TRBs left unhandled.
>> >
>> > Similarly, if the gadget function enqueues an unaligned request
>> > with sglist already in it, it should fail the same way, since we
>> > will append another TRB to something that already uses more than
>> > one TRB.
>> >
>> > To aviod this, this patch changes the code to check for IOC/LST
>> > bits in TRB->ctrl instead.
>> >
>> > At a practical level, this patch resolves USB transfer stalls seen
>> > with adb on dwc3 based HiKey960 after functionfs gadget added
>> > scatter-gather support around v4.20.
>> >
>> > Cc: Felipe Balbi <[email protected]>
>> > Cc: Yang Fei <[email protected]>
>> > Cc: Thinh Nguyen <[email protected]>
>> > Cc: Tejas Joglekar <[email protected]>
>> > Cc: Andrzej Pietrasiewicz <[email protected]>
>> > Cc: Jack Pham <[email protected]>
>> > Cc: Todd Kjos <[email protected]>
>> > Cc: Greg KH <[email protected]>
>> > Cc: Linux USB List <[email protected]>
>> > Cc: stable <[email protected]>
>> > Tested-by: Tejas Joglekar <[email protected]>
>> > Reviewed-by: Thinh Nguyen <[email protected]>
>> > Signed-off-by: Anurag Kumar Vulisha <[email protected]>
>> > [jstultz: forward ported to mainline, reworded commit log, reworked
>> > to only check trb->ctrl as suggested by Felipe]
>> > Signed-off-by: John Stultz <[email protected]>
>>
>> since v5.5 is already merged, I'll send this to Greg once -rc1 is
>> tagged. It's already in my testing/fixes branch waiting for a pull
>> request.
>
> Great, thanks so much for queueing this! I'll be digging on the db845c

no worries, it was way past the time :-)

> side wrt the dma-api issue to hopefully get that one sorted as well.

Thanks, that would, indeed, be great :-)

> Thanks again for the help and analysis!

no worries. If you find anything odd, just collect traces and I can help
have a look.

--
balbi


Attachments:
signature.asc (847.00 B)