Hi Thinh, Thanks for spending your time in reviewing this code, please find my comments inline
>-----Original Message----- >From: Thinh Nguyen [mailto:thinh.ngu...@synopsys.com] >Sent: Wednesday, September 05, 2018 11:04 AM >To: Anurag Kumar Vulisha <anura...@xilinx.com>; ba...@kernel.org; >gre...@linuxfoundation.org >Cc: v.anuragku...@gmail.com; linux-usb@vger.kernel.org; linux- >ker...@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.vuli...@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
