Hi Anurag, On 9/6/2018 8:12 AM, Anurag Kumar Vulisha wrote: > Hi Thinh, > >> -----Original Message----- >> From: Thinh Nguyen [mailto:Thinh.Nguyen@xxxxxxxxxxxx] >> Sent: Thursday, September 06, 2018 7:28 AM >> To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Thinh Nguyen >> <Thinh.Nguyen@xxxxxxxxxxxx>; 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, >> >> On 9/5/2018 2:19 AM, Anurag Kumar Vulisha wrote: >>> 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, >>>> >>>>> + 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 >>> dep->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. >>> >> I haven't tested it, but you can try this: >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index >> d7a327eaee12..37171d46390b 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -924,8 +924,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, >> struct dwc3_trb *trb, >> struct usb_gadget *gadget = &dwc->gadget; >> enum usb_device_speed speed = gadget->speed; >> >> - dwc3_ep_inc_enq(dep); >> - >> trb->size = DWC3_TRB_SIZE_LENGTH(length); >> trb->bpl = lower_32_bits(dma); >> trb->bph = upper_32_bits(dma); >> @@ -1004,7 +1002,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, >> struct dwc3_trb *trb, >> } >> >> if ((!no_interrupt && !chain) || >> - (dwc3_calc_trbs_left(dep) == 0)) >> + (dwc3_calc_trbs_left(dep) == 1)) >> trb->ctrl |= DWC3_TRB_CTRL_IOC; >> >> if (chain) >> @@ -1013,6 +1011,8 @@ 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); >> >> + dwc3_ep_inc_enq(dep); >> + >> trb->ctrl |= DWC3_TRB_CTRL_HWO; >> >> trace_dwc3_prepare_trb(dep, trb); >> > Thanks for pointing out a solution , the fix looks good. I will test with this fix and resend the patches > > Best Regards, > Anurag Kumar Vulisha > > The rest of the patch series looks fine to me. Reviewed-by: Thinh Nguyen <thinhn@xxxxxxxxxxxx> BR, Thinh
