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
