[v7,09/10] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields

Commit Message

Anurag Kumar Vulisha Dec. 1, 2018, 11:13 a.m. UTC

The present 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 makes the remaining
TRBs left unhandled.
To aviod this, changed the code to check for IOC/LST bits in both
event->status & TRB->ctrl. This patch does the same.

Signed-off-by: Anurag Kumar Vulisha <[e-mail]>
Reviewed-by: Thinh Nguyen <[e-mail]>
Tested-by: Tejas Joglekar <[e-mail]>
---
 Changes in v7:
	1. None

 Changes in v6:
	1. None

 Changes in v5:
	1. None

 Changes in v4:
	1. None

 Changes in v3:
	1. None

 Changes in v2:
	1. None
---
 drivers/usb/dwc3/gadget.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Felipe Balbi Dec. 5, 2018, 9:07 a.m. UTC | #1

Hi,

Anurag Kumar Vulisha <[e-mail]> writes:
> The present 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 makes the remaining
> TRBs left unhandled.
> To aviod this, changed the code to check for IOC/LST bits in both
> event->status & TRB->ctrl. This patch does the same.
>
> Signed-off-by: Anurag Kumar Vulisha <[e-mail]>
> Reviewed-by: Thinh Nguyen <[e-mail]>
> Tested-by: Tejas Joglekar <[e-mail]>
> ---
>  Changes in v7:
> 	1. None
>
>  Changes in v6:
> 	1. None
>
>  Changes in v5:
> 	1. None
>
>  Changes in v4:
> 	1. None
>
>  Changes in v3:
> 	1. None
>
>  Changes in v2:
> 	1. None
> ---
>  drivers/usb/dwc3/gadget.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 9ddc9fd..216179e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2286,7 +2286,12 @@ 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 | DEPEVT_STATUS_LST))
> +	if ((event->status & DEPEVT_STATUS_IOC) &&
> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
> +		return 1;

this shouldn't be necessary. According to databook, event->status
contains the bits from the completed TRB.  Which means that
event->status & IOC will always be equal to trb->ctrl & IOC.

Can you further describe the situation here? Perhaps share tracepoints
exposing the problem?

Anurag Kumar Vulisha Dec. 5, 2018, 7:01 p.m. UTC | #2

Hi Felipe,
>-----Original Message-----
>From: Felipe Balbi [mailto:[e-mail]]
>Sent: Wednesday, December 05, 2018 2:38 PM
>To: Anurag Kumar Vulisha <[e-mail]>; Greg Kroah-Hartman
><[e-mail]>; Shuah Khan <[e-mail]>; Alan Stern
><[e-mail]>; Johan Hovold <[e-mail]>; Jaejoong Kim
><[e-mail]>; Benjamin Herrenschmidt <[e-mail]>;
>Roger Quadros <[e-mail]>; Manu Gautam <[e-mail]>;
>[e-mail]; Bart Van Assche <[e-mail]>; Mike
>Christie <[e-mail]>; Matthew Wilcox <[e-mail]>; Colin Ian
>King <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; Thinh Nguyen <[e-mail]>; Tejas Joglekar
><[e-mail]>; Ajay Yugalkishore Pandey <[e-mail]>;
>Anurag Kumar Vulisha <[e-mail]>
>Subject: Re: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status
>and TRB->ctrl fields
>
>
>Hi,
>
>Anurag Kumar Vulisha <[e-mail]> writes:
>> The present 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 makes the remaining
>> TRBs left unhandled.
>> To aviod this, changed the code to check for IOC/LST bits in both
>> event->status & TRB->ctrl. This patch does the same.
>>
>> Signed-off-by: Anurag Kumar Vulisha <[e-mail]>
>> Reviewed-by: Thinh Nguyen <[e-mail]>
>> Tested-by: Tejas Joglekar <[e-mail]>
>> ---
>>  Changes in v7:
>> 	1. None
>>
>>  Changes in v6:
>> 	1. None
>>
>>  Changes in v5:
>> 	1. None
>>
>>  Changes in v4:
>> 	1. None
>>
>>  Changes in v3:
>> 	1. None
>>
>>  Changes in v2:
>> 	1. None
>> ---
>>  drivers/usb/dwc3/gadget.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 9ddc9fd..216179e 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2286,7 +2286,12 @@ 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 | DEPEVT_STATUS_LST))
>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>> +		return 1;
>
>this shouldn't be necessary. According to databook, event->status
>contains the bits from the completed TRB.  Which means that
>event->status & IOC will always be equal to trb->ctrl & IOC.
>
Thanks for reviewing this patch. Lets consider an example where a request
has num_sgs > 0 and each sg is mapped to a TRB and the last TRB has the
IOC bit set. Once the controller is done with the transfer, it  generates 
XferInProgress for the last TRB (since IOC bit is set). As a part of trb reclaim
process  dwc3_gadget_ep_reclaim_trb_sg() calls
dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
the event already has the IOC bit set, the loop is exited from the loop at the
very first TRB and the remaining TRBs (mapped to the sglist) are left unhandled.
To avoid this we modified the code to exit only if both TRB & event has the IOC
bit set.
 
>Can you further describe the situation here? Perhaps share tracepoints
>exposing the problem?

Sure, will capture the traces and reply back

Thanks,
Anurag Kumar Vulisha

Felipe Balbi Dec. 7, 2018, 6:11 a.m. UTC | #3

Hi,

Anurag Kumar Vulisha <[e-mail]> writes:
>>> @@ -2286,7 +2286,12 @@ 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 | DEPEVT_STATUS_LST))
>>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>>> +		return 1;
>>
>>this shouldn't be necessary. According to databook, event->status
>>contains the bits from the completed TRB.  Which means that
>>event->status & IOC will always be equal to trb->ctrl & IOC.
>>
> Thanks for reviewing this patch. Lets consider an example where a request
> has num_sgs > 0 and each sg is mapped to a TRB and the last TRB has the
> IOC bit set. Once the controller is done with the transfer, it  generates 
> XferInProgress for the last TRB (since IOC bit is set). As a part of trb reclaim
> process  dwc3_gadget_ep_reclaim_trb_sg() calls
> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
> the event already has the IOC bit set, the loop is exited from the loop at the
> very first TRB and the remaining TRBs (mapped to the sglist) are left unhandled.
> To avoid this we modified the code to exit only if both TRB & event has the IOC
> bit set.

Seems like IOC case should just test for chain flag as well:

modified   drivers/usb/dwc3/gadget.c
@@ -2372,7 +2372,7 @@ 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 (event->status & DEPEVT_STATUS_IOC && !chain)
 		return 1;
 
 	return 0;

Anurag Kumar Vulisha Dec. 8, 2018, 7:03 p.m. UTC | #4

HI Felipe,

>-----Original Message-----
>From: Felipe Balbi [mailto:[e-mail]]
>Sent: Friday, December 07, 2018 11:42 AM
>To: Anurag Kumar Vulisha <[e-mail]>; Greg Kroah-Hartman
><[e-mail]>; Shuah Khan <[e-mail]>; Alan Stern
><[e-mail]>; Johan Hovold <[e-mail]>; Jaejoong Kim
><[e-mail]>; Benjamin Herrenschmidt <[e-mail]>;
>Roger Quadros <[e-mail]>; Manu Gautam <[e-mail]>;
>[e-mail]; Bart Van Assche <[e-mail]>; Mike
>Christie <[e-mail]>; Matthew Wilcox <[e-mail]>; Colin Ian
>King <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; Thinh Nguyen <[e-mail]>; Tejas Joglekar
><[e-mail]>; Ajay Yugalkishore Pandey <[e-mail]>
>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status
>and TRB->ctrl fields
>
>
>Hi,
>
>Anurag Kumar Vulisha <[e-mail]> writes:
>>>> @@ -2286,7 +2286,12 @@ 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 | DEPEVT_STATUS_LST))
>>>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>>>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>>>> +		return 1;
>>>
>>>this shouldn't be necessary. According to databook, event->status
>>>contains the bits from the completed TRB.  Which means that
>>>event->status & IOC will always be equal to trb->ctrl & IOC.
>>>
>> Thanks for reviewing this patch. Lets consider an example where a
>> request has num_sgs > 0 and each sg is mapped to a TRB and the last
>> TRB has the IOC bit set. Once the controller is done with the
>> transfer, it  generates XferInProgress for the last TRB (since IOC bit
>> is set). As a part of trb reclaim process
>> dwc3_gadget_ep_reclaim_trb_sg() calls
>> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
>> the event already has the IOC bit set, the loop is exited from the
>> loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left
>unhandled.
>> To avoid this we modified the code to exit only if both TRB & event
>> has the IOC bit set.
>
>Seems like IOC case should just test for chain flag as well:
>

Okay. Along with this logic the code for updating chain bit should also be modified I guess.
Since the IOC bit is also set when there are not enough TRBs available, the code should be
modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update below
changes along with your suggestions and resend the patches.

@@ -998,7 +998,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb,
                        (dwc3_calc_trbs_left(dep) == 1))
                trb->ctrl |= DWC3_TRB_CTRL_IOC;
 
-       if (chain)
+       if (chain && !(trb->ctrl & DWC3_TRB_CTRL_IOC))
                trb->ctrl |= DWC3_TRB_CTRL_CHN;
 
        if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
@@ -2372,7 +2372,7 @@ 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 (event->status & DEPEVT_STATUS_IOC && !chain)
                return 1;
 
        return 0;
@@ -2399,7 +2399,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep,
                req->num_pending_sgs--;
 
                ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req,
-                               trb, event, status, true);
+                               trb, event, status, (trb & DWC3_TRB_CTRL_CHN));
                if (ret)
                        break;
        }

Thanks,
Anurag Kumar Vulisha

>modified   drivers/usb/dwc3/gadget.c
>@@ -2372,7 +2372,7 @@ 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 (event->status & DEPEVT_STATUS_IOC && !chain)
> 		return 1;
>
> 	return 0;
>
>--
>balbi

Felipe Balbi Dec. 10, 2018, 6:54 a.m. UTC | #5

Hi,

Anurag Kumar Vulisha <[e-mail]> writes:
> HI Felipe,
>
>>-----Original Message-----
>>From: Felipe Balbi [mailto:[e-mail]]
>>Sent: Friday, December 07, 2018 11:42 AM
>>To: Anurag Kumar Vulisha <[e-mail]>; Greg Kroah-Hartman
>><[e-mail]>; Shuah Khan <[e-mail]>; Alan Stern
>><[e-mail]>; Johan Hovold <[e-mail]>; Jaejoong Kim
>><[e-mail]>; Benjamin Herrenschmidt <[e-mail]>;
>>Roger Quadros <[e-mail]>; Manu Gautam <[e-mail]>;
>>[e-mail]; Bart Van Assche <[e-mail]>; Mike
>>Christie <[e-mail]>; Matthew Wilcox <[e-mail]>; Colin Ian
>>King <[e-mail]>
>>Cc: [e-mail]; [e-mail];
>>[e-mail]; Thinh Nguyen <[e-mail]>; Tejas Joglekar
>><[e-mail]>; Ajay Yugalkishore Pandey <[e-mail]>
>>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status
>>and TRB->ctrl fields
>>
>>
>>Hi,
>>
>>Anurag Kumar Vulisha <[e-mail]> writes:
>>>>> @@ -2286,7 +2286,12 @@ 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 | DEPEVT_STATUS_LST))
>>>>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>>>>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>>>>> +		return 1;
>>>>
>>>>this shouldn't be necessary. According to databook, event->status
>>>>contains the bits from the completed TRB.  Which means that
>>>>event->status & IOC will always be equal to trb->ctrl & IOC.
>>>>
>>> Thanks for reviewing this patch. Lets consider an example where a
>>> request has num_sgs > 0 and each sg is mapped to a TRB and the last
>>> TRB has the IOC bit set. Once the controller is done with the
>>> transfer, it  generates XferInProgress for the last TRB (since IOC bit
>>> is set). As a part of trb reclaim process
>>> dwc3_gadget_ep_reclaim_trb_sg() calls
>>> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
>>> the event already has the IOC bit set, the loop is exited from the
>>> loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left
>>unhandled.
>>> To avoid this we modified the code to exit only if both TRB & event
>>> has the IOC bit set.
>>
>>Seems like IOC case should just test for chain flag as well:
>>
>
> Okay. Along with this logic the code for updating chain bit should also be modified I guess.

not really

> Since the IOC bit is also set when there are not enough TRBs available, the code should be
> modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update below
> changes along with your suggestions and resend the patches.

no. Actually I don't think we're allowed to split a scatter/gather like
that. I did that quite a while ago, but I don't think we're allowed to
do so. What we should do, in that case, is not even queue that request
until we have enough for all members of the scatter/gather. But that's a
separate patch, anyway.

Anurag Kumar Vulisha Dec. 10, 2018, 8:56 a.m. UTC | #6

Hi Felipe,

>-----Original Message-----
>From: Felipe Balbi [mailto:[e-mail]]
>Sent: Monday, December 10, 2018 12:24 PM
>To: Anurag Kumar Vulisha <[e-mail]>; Greg Kroah-Hartman
><[e-mail]>; Shuah Khan <[e-mail]>; Alan Stern
><[e-mail]>; Johan Hovold <[e-mail]>; Jaejoong Kim
><[e-mail]>; Benjamin Herrenschmidt <[e-mail]>;
>Roger Quadros <[e-mail]>; Manu Gautam <[e-mail]>;
>[e-mail]; Bart Van Assche <[e-mail]>; Mike
>Christie <[e-mail]>; Matthew Wilcox <[e-mail]>; Colin Ian
>King <[e-mail]>
>Cc: [e-mail]; [e-mail];
>[e-mail]; Thinh Nguyen <[e-mail]>; Tejas Joglekar
><[e-mail]>; Ajay Yugalkishore Pandey <[e-mail]>
>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event->status
>and TRB->ctrl fields
>
>
>Hi,
>
>Anurag Kumar Vulisha <[e-mail]> writes:
>> HI Felipe,
>>
>>>-----Original Message-----
>>>From: Felipe Balbi [mailto:[e-mail]]
>>>Sent: Friday, December 07, 2018 11:42 AM
>>>To: Anurag Kumar Vulisha <[e-mail]>; Greg Kroah-Hartman
>>><[e-mail]>; Shuah Khan <[e-mail]>; Alan Stern
>>><[e-mail]>; Johan Hovold <[e-mail]>; Jaejoong Kim
>>><[e-mail]>; Benjamin Herrenschmidt <[e-mail]>;
>>>Roger Quadros <[e-mail]>; Manu Gautam <[e-mail]>;
>>>[e-mail]; Bart Van Assche <[e-mail]>; Mike
>>>Christie <[e-mail]>; Matthew Wilcox <[e-mail]>; Colin
>Ian
>>>King <[e-mail]>
>>>Cc: [e-mail]; [e-mail];
>>>[e-mail]; Thinh Nguyen <[e-mail]>; Tejas
>Joglekar
>>><[e-mail]>; Ajay Yugalkishore Pandey
><[e-mail]>
>>>Subject: RE: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both event-
>>status
>>>and TRB->ctrl fields
>>>
>>>
>>>Hi,
>>>
>>>Anurag Kumar Vulisha <[e-mail]> writes:
>>>>>> @@ -2286,7 +2286,12 @@ 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 | DEPEVT_STATUS_LST))
>>>>>> +	if ((event->status & DEPEVT_STATUS_IOC) &&
>>>>>> +	    (trb->ctrl & DWC3_TRB_CTRL_IOC))
>>>>>> +		return 1;
>>>>>
>>>>>this shouldn't be necessary. According to databook, event->status
>>>>>contains the bits from the completed TRB.  Which means that
>>>>>event->status & IOC will always be equal to trb->ctrl & IOC.
>>>>>
>>>> Thanks for reviewing this patch. Lets consider an example where a
>>>> request has num_sgs > 0 and each sg is mapped to a TRB and the last
>>>> TRB has the IOC bit set. Once the controller is done with the
>>>> transfer, it  generates XferInProgress for the last TRB (since IOC bit
>>>> is set). As a part of trb reclaim process
>>>> dwc3_gadget_ep_reclaim_trb_sg() calls
>>>> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
>>>> the event already has the IOC bit set, the loop is exited from the
>>>> loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left
>>>unhandled.
>>>> To avoid this we modified the code to exit only if both TRB & event
>>>> has the IOC bit set.
>>>
>>>Seems like IOC case should just test for chain flag as well:
>>>
>>
>> Okay. Along with this logic the code for updating chain bit should also be modified I
>guess.
>
>not really
>
>> Since the IOC bit is also set when there are not enough TRBs available, the code
>should be
>> modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update
>below
>> changes along with your suggestions and resend the patches.
>
>no. Actually I don't think we're allowed to split a scatter/gather like
>that. I did that quite a while ago, but I don't think we're allowed to
>do so. What we should do, in that case, is not even queue that request
>until we have enough for all members of the scatter/gather. But that's a
>separate patch, anyway.
>

Okay. I have a doubt here, not pushing the request until all sgs are mapped to enough TRBs
might remove the driver complexity but reduce the performance (since we are waiting
until enough TRBs are available). Are we okay with that?  

Thanks,
Anurag Kumar Vulisha

Felipe Balbi Dec. 10, 2018, 9:03 a.m. UTC | #7

Hi,

Anurag Kumar Vulisha <[e-mail]> writes:
>>>>> Thanks for reviewing this patch. Lets consider an example where a
>>>>> request has num_sgs > 0 and each sg is mapped to a TRB and the last
>>>>> TRB has the IOC bit set. Once the controller is done with the
>>>>> transfer, it  generates XferInProgress for the last TRB (since IOC bit
>>>>> is set). As a part of trb reclaim process
>>>>> dwc3_gadget_ep_reclaim_trb_sg() calls
>>>>> dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since
>>>>> the event already has the IOC bit set, the loop is exited from the
>>>>> loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left
>>>>unhandled.
>>>>> To avoid this we modified the code to exit only if both TRB & event
>>>>> has the IOC bit set.
>>>>
>>>>Seems like IOC case should just test for chain flag as well:
>>>>
>>>
>>> Okay. Along with this logic the code for updating chain bit should also be modified I
>>guess.
>>
>>not really
>>
>>> Since the IOC bit is also set when there are not enough TRBs available, the code
>>should be
>>> modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update
>>below
>>> changes along with your suggestions and resend the patches.
>>
>>no. Actually I don't think we're allowed to split a scatter/gather like
>>that. I did that quite a while ago, but I don't think we're allowed to
>>do so. What we should do, in that case, is not even queue that request
>>until we have enough for all members of the scatter/gather. But that's a
>>separate patch, anyway.
>>
>
> Okay. I have a doubt here, not pushing the request until all sgs are mapped to enough TRBs
> might remove the driver complexity but reduce the performance (since we are waiting
> until enough TRBs are available). Are we okay with that?  

The only other way would be to copy the buffer over to a contiguous
buffer. That will also reduce performance. I think we need to consider
how frequently this may actually happen. I dare to say we don't have any
usb function in kernel as of today that can, easily and frequently, fall
into such a situation. Besides, the performance loss can be amortized by
a deeper request queue.

IMO, this is a minor problem. But, certainly, if you have the setup,
_do_ run some benchmarking and report your findings :-)

Patch

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9ddc9fd..216179e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2286,7 +2286,12 @@  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 | DEPEVT_STATUS_LST))
+	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;
 
 	return 0;