| Message ID | 1534508695-12642-2-git-send-email-anurag.kumar.vulisha@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver | expand |
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@xilinx.com> > --- > 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. BR, Thinh
Hi Thinh, Thanks for spending your time in reviewing this code, please find my comments inline >-----Original Message----- >From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com] >Sent: Wednesday, September 05, 2018 11:04 AM >To: Anurag Kumar Vulisha <anuragku@xilinx.com>; balbi@kernel.org; >gregkh@linuxfoundation.org >Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >kernel@vger.kernel.org >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@xilinx.com> >> --- >> 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. Thanks, Anurag Kumar Vulisha
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@synopsys.com] >> Sent: Wednesday, September 05, 2018 11:04 AM >> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; balbi@kernel.org; >> gregkh@linuxfoundation.org >> Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >> kernel@vger.kernel.org >> 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@xilinx.com> >>> --- >>> 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
Hi Thinh, >-----Original Message----- >From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com] >Sent: Thursday, September 06, 2018 7:28 AM >To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen ><Thinh.Nguyen@synopsys.com>; balbi@kernel.org; gregkh@linuxfoundation.org >Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >kernel@vger.kernel.org >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@synopsys.com] >>> Sent: Wednesday, September 05, 2018 11:04 AM >>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; balbi@kernel.org; >>> gregkh@linuxfoundation.org >>> Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >>> kernel@vger.kernel.org >>> 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
Hi Anurag, On 9/6/2018 8:12 AM, Anurag Kumar Vulisha wrote: > Hi Thinh, > >> -----Original Message----- >> From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com] >> Sent: Thursday, September 06, 2018 7:28 AM >> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen >> <Thinh.Nguyen@synopsys.com>; balbi@kernel.org; gregkh@linuxfoundation.org >> Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >> kernel@vger.kernel.org >> 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@synopsys.com] >>>> Sent: Wednesday, September 05, 2018 11:04 AM >>>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; balbi@kernel.org; >>>> gregkh@linuxfoundation.org >>>> Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux- >>>> kernel@vger.kernel.org >>>> 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@synopsys.com> BR, Thinh
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); }
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@xilinx.com> --- Changes in v2: 1. Changed the commit message --- drivers/usb/dwc3/gadget.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)