Re: [PATCH] usb: dwc3: gadget: Fix dwc3_calc_trbs_left()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Felipe Balbi wrote:
> 
> Hi,
> 
> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
>> We can't depend on the TRB's HWO bit to determine if the TRB ring is
>> "full". A TRB is only available when the driver had processed it, not
>> when the controller consumed and relinquished the TRB's ownership to the
>> driver. Otherwise, the driver may overwrite unprocessed TRBs. This can
>> happen when many transfer events accumulate and the system is slow to
>> process them and/or when there are too many small requests.
>>
>> If a request is in the started_list, that means there is one or more
>> unprocessed TRBs remained. Check this instead of the TRB's HWO bit
>> whether the TRB ring is full.
>>
>> Cc: <stable@xxxxxxxxxxxxxxx>
>> Fixes: c4233573f6ee ("usb: dwc3: gadget: prepare TRBs on update transfers too")
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
>> ---
>>  drivers/usb/dwc3/gadget.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 84fe57ef5a49..1e6ddbc986ba 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -940,19 +940,19 @@ static struct dwc3_trb *dwc3_ep_prev_trb(struct dwc3_ep *dep, u8 index)
>>  
>>  static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
>>  {
>> -	struct dwc3_trb		*tmp;
>>  	u8			trbs_left;
>>  
>>  	/*
>> -	 * If enqueue & dequeue are equal than it is either full or empty.
>> -	 *
>> -	 * One way to know for sure is if the TRB right before us has HWO bit
>> -	 * set or not. If it has, then we're definitely full and can't fit any
>> -	 * more transfers in our ring.
>> +	 * If the enqueue & dequeue are equal then the TRB ring is either full
>> +	 * or empty. It's considered full when there are DWC3_TRB_NUM-1 of TRBs
>> +	 * pending to be processed by the driver.
>>  	 */
>>  	if (dep->trb_enqueue == dep->trb_dequeue) {
>> -		tmp = dwc3_ep_prev_trb(dep, dep->trb_enqueue);
>> -		if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
>> +		/*
>> +		 * If there is any request remained in the started_list at
>> +		 * this point, that means there is no TRB available.
>> +		 */
>> +		if (!list_empty(&dep->started_list))
>>  			return 0;
> 
> we could also do away with calc_trbs_left() completely if we just add an
> actual counter that gets incremented decremented together with the
> enqueue/dequeue pointers. Since we have 256 TRBs per endpoint and only
> 255 are usable, this means we can do away with a single u8 per
> endpoint. Perhaps that could be done as a second step after this fix is
> merged?
> 

Yes, that would simplify the logic. We can do that after merging this fix.

Thanks,
Thinh





[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux