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.
>
>> 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.
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.
>
> 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.
>
>> 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);
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
