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

[Thinh Nguyen]

Hi,

Felipe Balbi wrote:
> Hi,
>
> John Stultz <john.stultz@xxxxxxxxxx> writes:
>
>> From: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
>>
>> 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 <balbi@xxxxxxxxxx>
>> Cc: Yang Fei <fei.yang@xxxxxxxxx>
>> Cc: Thinh Nguyen <thinhn@xxxxxxxxxxxx>
>> Cc: Tejas Joglekar <tejas.joglekar@xxxxxxxxxxxx>
>> Cc: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>
>> Cc: Jack Pham <jackp@xxxxxxxxxxxxxx>
>> Cc: Todd Kjos <tkjos@xxxxxxxxxx>
>> Cc: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
>> Cc: Linux USB List <linux-usb@xxxxxxxxxxxxxxx>
>> Cc: stable <stable@xxxxxxxxxxxxxxx>
>> Tested-by: Tejas Joglekar <tejas.joglekar@xxxxxxxxxxxx>
>> Reviewed-by: Thinh Nguyen <thinhn@xxxxxxxxxxxx>
>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
>> [jstultz: forward ported to mainline, reworded commit log, reworked
>>   to only check trb->ctrl as suggested by Felipe]
>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>> ---
>> 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