Hi,
John Youn <John.Youn@xxxxxxxxxxxx> writes:
> On 5/19/2016 12:51 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> John Youn <johnyoun@xxxxxxxxxxxx> writes:
>>> This patch fixes up some issues related to the trb_left calculation.
>>>
>>> This calculation sometimes included the link trb slot in the trbs_left
>>> and sometimes didn't.
>>
>> good catch. But this patch seems like it can be broken into smaller
>> pieces. See below
>
> Ok, I can split it up.
thanks :-)
>>> In the case where the dequeue < enqueue, this number does not include
>>> the link trb and should be used as-is. Otherwise it will include the
>>> link_trb and should be decremented to reflect the actual amount of free
>>> trb slots. The fixed calculation will be the proper amount of free TRB
>>> slots left, taking into account whether a link TRB takes up one of the
>>> slots.
>>
>> this is one fix :-)
>>
>>> When checking for full or empty where enqueue == dequeue, the pointer
>>> may be 0. In that case previous TRB is the one before the link TRB. Use
>>> that to check the HWO bit to see if we are full or empty.
>>
>> if enqueue == dequeue, we could be anywhere in the ring. So previous TRB
>> might not be the one before the link. Care to further explain this case?
>
> The enqueue and dequeue can both be 0 and full and index 0 is treated
> just like any other index in the ring. That has to be the case as
> whenever we increment we don't ever point to a link TRB. We pretend it
> doesn't exist and that it is just a 255-TRB circular queue. How we end
> up full at index 0 is that they are both initially 0, then we enqueue
> 255 TRBs without the hardware getting a chance to process anything
> yet. The enqueue will wrap and point to 0 again, and then it will be
> full.
oh, you're right. I completely missed the case were gadget driver fills
up the ring in one go.
> We check for empty/full in the same way, except that at index 0, when
> we look at the previous TRB, we must wrap around backwards, skip the
> link trb, and look at the TRB prior to the link TRB.
right
>> It's also a separate fix, btw.
>>
>>> In case DWC3_TRB_NUM is ever set lower than 256, mod trbs_left result by
>>> DWC3_TRB_NUM.
>>
>> I don't get this. Where did we have % 256? I can only see % DWC3_TRB_NUM.
>
> Right. That was taken care of in the trb_enqueue/deqeuue increment
> functions. But it also needs to be accounted for in the trbs_left
> calculation which was just returning (trb->dequeue - trb->enqueue).
>
> For example if you queue one trb the calculation would
> 0x00 - 0x01 = 0xFF (255), minus 1 for link trb = 254 TRB slots
> free. But if DWC3_TRB_NUM = 8, then I have 254 % 8 = 6 slots free.
okay.
>>> In dwc3_prepare_trbs, if trbs_left is 0, do nothing and return early.
>>
>> another fix.
>>
>>> When an EP is enabled, initialized the TRB pool to clean up any stale
>>> data. When the previous TRB was not properly cleaned up on disconnect,
>>> the empty/full checking logic sometimes failed causing the EP to stop
>>> processing requests.
>>
>> another fix.
>>
>>> Signed-off-by: John Youn <johnyoun@xxxxxxxxxxxx>
>>> ---
>>> drivers/usb/dwc3/gadget.c | 37 ++++++++++++++++++++++++++++++-------
>>> 1 file changed, 30 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 3eaef22..a76634b 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -561,6 +561,12 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
>>> if (usb_endpoint_xfer_control(desc))
>>> goto out;
>>>
>>> + /* Initialize the TRB pool */
>>> + dep->trb_dequeue = 0;
>>> + dep->trb_enqueue = 0;
>>> + memset(dep->trb_pool, 0,
>>> + sizeof(struct dwc3_trb) * DWC3_TRB_NUM);
>>
>> this is a separate fix. A very good one, actually ;-)
>>
>>> /* Link TRB. The HWO bit is never reset */
>>> trb_st_hw = &dep->trb_pool[0];
>>>
>>> @@ -849,6 +855,8 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
>>> static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
>>> {
>>> struct dwc3_trb *tmp;
>>> + u8 tmp_index;
>>> + u8 trbs_left;
>>>
>>> /*
>>> * If enqueue & dequeue are equal than it is either full or empty.
>>> @@ -858,17 +866,29 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
>>> * more transfers in our ring.
>>> */
>>> if (dep->trb_enqueue == dep->trb_dequeue) {
>>> - /* If we're full, enqueue/dequeue are > 0 */
>>> - if (dep->trb_enqueue) {
>>> - tmp = &dep->trb_pool[dep->trb_enqueue - 1];
>>> - if (tmp->ctrl & DWC3_TRB_CTRL_HWO)
>>> - return 0;
>>> - }
>>> + if (!dep->trb_enqueue)
>>> + /*
>>> + * The TRB just before the zeroth one is the
>>> + * one just before the LINK TRB.
>>> + */
>>> + tmp_index = DWC3_TRB_NUM - 2;
>>
>> this seems wrong. If both enqueue and dequeue are 0, it means our entire
>> ring is empty, and we can use (by default) 255 TRBs, with one space left
>> for the link. This DWC3_TRB_NUM - 2 will give me 254, instead of 255.
>>
>
> See above explanation. Though this could be simplified so that the
> handling of index 0 is enclosed in a function like:
>
> trb = dwc3_prev_trb(dep, dep->enqueue);
I like dwc3_prev_trb(), actually.
--
balbi
Attachment:
signature.asc
Description: PGP signature
