RE: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one_trb()

[Anurag Kumar Vulisha]

Hi Thinh,

Thanks for spending your time in reviewing this code, please find my comments inline

>-----Original Message-----
>From: Thinh Nguyen [mailto:Thinh.Nguyen@xxxxxxxxxxxx]
>Sent: Wednesday, September 05, 2018 11:04 AM
>To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; balbi@xxxxxxxxxx;
>gregkh@xxxxxxxxxxxxxxxxxxx
>Cc: v.anuragkumar@xxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full in
>__dwc3_prepare_one_trb()
>
>Hi Anurag,
>
>On 8/17/2018 5:25 AM, Anurag Kumar Vulisha wrote:
>> Availability of TRB's are calculated using dwc3_calc_trbs_left(), which
>> determines available TRB's based on the HWO bit set in a TRB.
>>
>> __dwc3_prepare_one_trb() is called with a TRB which needs to be prepared
>> for transfer. This __dwc3_prepare_one_trb() calls dwc3_calc_trbs_left()
>> to determine total available TRBs and set IOC bit if the total available
>> TRBs are zero. Since the present working TRB(which is passed as an
>> argument to __dwc3_prepare_one_trb() )  doesn't have HWO bit already set,
>> there are chances where dwc3_calc_trbs_left() wrongly calculates this
>> present working TRB as free(since the HWO bit is not yet set). This could
>> be a problem. This patch correct this issue by setting HWO bit before
>> calling dwc3_calc_trbs_left()
>>
>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx>
>> ---
>>  Changes in v2:
>> 	1. Changed the commit message
>> ---
>>  drivers/usb/dwc3/gadget.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 69bf137..f73d219 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -990,6 +990,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep,
>struct dwc3_trb *trb,
>>  			trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
>>  	}
>>
>> +	trb->ctrl |= DWC3_TRB_CTRL_HWO;
>> +
>>  	if ((!no_interrupt && !chain) ||
>>  			(dwc3_calc_trbs_left(dep) == 0))
>>  		trb->ctrl |= DWC3_TRB_CTRL_IOC;
>> @@ -1000,8 +1002,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep
>*dep, struct dwc3_trb *trb,
>>  	if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
>>  		trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>>
>> -	trb->ctrl |= DWC3_TRB_CTRL_HWO;
>> -
>>  	trace_dwc3_prepare_trb(dep, trb);
>>  }
>>
>How do you reproduce this issue? We should not set HWO before setting
>other trb->ctrl bits. Can you provide a driver tracepoint? If there's an
>issue with the check if ((!no_interrupt && !chain) ||
>dwc3_calc_trbs_left == 0), then we may need to fix the check there.
>

This issue gets triggered very rarely on long runs when dep->trb_enqueue == dep->trb_dequeue.
In __dwc3_prepare_one_trb() , IOC bit is set when no TRBs are available, so that a complete
event can be generated and TRBs can be cleaned  after complete . Dwc3_calc_trbs_left() is called 
to determine the available TRBs, which depends on the previous TRB's HWO bit set when 
dep->trb_enqueue == dep->trb_dequeue. There are chances where the dwc3_calc_trbs_left() wrongly
returns DWC3_TRB_NUM -1 instead of 0 , even though there are no available TRBs. Please
consider the below example

1.  Consider a TRB passed to __dwc3_prepare_one_trb() is the last available TRB in the pool. 
2.  __dwc3_prepare_one_trb() calls dwc3_ep_inc_enq() which increments the dep->trb_enqueue
   before preparing the TRB and since the current TRB is the last available, incrementing
  dep->enqueue will make dep->enqueue == dep->dequeue 
3. IOC bit is set by __dwc3_prepare_one_trb()  only when dwc3_calc_trbs_left()  returns 0 (empty TRBs)
4. Since dep->enqueue == dep->dequeue and the previous TRB(the one which we are working)
    doesn't yet has the HWO bit set, dwc3_calc_trbs_left() returns DWC3_TRB_NUM -1 instead of
    zero (Though there are no available TRBs)
5. Since  Dwc3_calc_trbs_left() returns non-zero value, IOC bit is not set in __dwc3_prepare_one_trb()
   for the last TRB and no complete event is generated. Because of this no further TRBs are queued.

To avoid the above mentioned issue, I moved the code logic for setting HWO bit before setting IOC bit.
I agree that HWO bit should always be set at the last, but I didn't find any better logic for fixing this.
Please suggest if any better way to handle this situation.   

Thanks,
Anurag Kumar Vulisha