Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> writes:
> 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.
sounds good:
Acked-by: Felipe Balbi <balbi@xxxxxxxxxx>
--
balbi
