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, >> >> On 8/17/2018 5:25 AM, Anurag Kumar Vulisha wrote: >>> Availability of TRB's are calculated using dwc3_calc_trbs_left(), which >>> determines available TRB's based on the HWO bit set in a TRB. >>> >>> __dwc3_prepare_one_trb() is called with a TRB which needs to be prepared >>> for transfer. This __dwc3_prepare_one_trb() calls dwc3_calc_trbs_left() >>> to determine total available TRBs and set IOC bit if the total available >>> TRBs are zero. Since the present working TRB(which is passed as an >>> argument to __dwc3_prepare_one_trb() ) doesn't have HWO bit already set, >>> there are chances where dwc3_calc_trbs_left() wrongly calculates this >>> present working TRB as free(since the HWO bit is not yet set). This could >>> be a problem. This patch correct this issue by setting HWO bit before >>> calling dwc3_calc_trbs_left() >>> >>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx> >>> --- >>> Changes in v2: >>> 1. Changed the commit message >>> --- >>> drivers/usb/dwc3/gadget.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 69bf137..f73d219 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -990,6 +990,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, >> struct dwc3_trb *trb, >>> trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI; >>> } >>> >>> + 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 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); BR, Thinh
