Hi Felipe, >-----Original Message----- >From: Felipe Balbi [mailto:[email protected]] >Sent: Wednesday, December 05, 2018 2:38 PM >To: Anurag Kumar Vulisha <[email protected]>; Greg Kroah-Hartman ><[email protected]>; Shuah Khan <[email protected]>; Alan Stern ><[email protected]>; Johan Hovold <[email protected]>; Jaejoong Kim ><[email protected]>; Benjamin Herrenschmidt <[email protected]>; >Roger Quadros <[email protected]>; Manu Gautam <[email protected]>; >[email protected]; Bart Van Assche <[email protected]>; Mike >Christie <[email protected]>; Matthew Wilcox <[email protected]>; Colin Ian >King <[email protected]> >Cc: [email protected]; [email protected]; >[email protected]; Thinh Nguyen <[email protected]>; Tejas Joglekar ><[email protected]>; Ajay Yugalkishore Pandey <[email protected]>; >Anurag Kumar Vulisha <[email protected]> >Subject: Re: [PATCH v7 09/10] usb: dwc3: Check for IOC/LST bit in both >event->status >and TRB->ctrl fields > > >Hi, > >Anurag Kumar Vulisha <[email protected]> writes: >> The present code in dwc3_gadget_ep_reclaim_completed_trb() will check >> for IOC/LST bit in the event->status and returns if IOC/LST bit is >> set. This logic doesn't work if multiple TRBs are queued per >> request and the IOC/LST bit is set on the last TRB of that request. >> Consider an example where a queued request has multiple queued TRBs >> and IOC/LST bit is set only for the last TRB. In this case, the Core >> generates XferComplete/XferInProgress events only for the last TRB >> (since IOC/LST are set only for the last TRB). As per the logic in >> dwc3_gadget_ep_reclaim_completed_trb() event->status is checked for >> IOC/LST bit and returns on the first TRB. This makes the remaining >> TRBs left unhandled. >> To aviod this, changed the code to check for IOC/LST bits in both >> event->status & TRB->ctrl. This patch does the same. >> >> Signed-off-by: Anurag Kumar Vulisha <[email protected]> >> Reviewed-by: Thinh Nguyen <[email protected]> >> Tested-by: Tejas Joglekar <[email protected]> >> --- >> Changes in v7: >> 1. None >> >> Changes in v6: >> 1. None >> >> Changes in v5: >> 1. None >> >> Changes in v4: >> 1. None >> >> Changes in v3: >> 1. None >> >> Changes in v2: >> 1. None >> --- >> drivers/usb/dwc3/gadget.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 9ddc9fd..216179e 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2286,7 +2286,12 @@ static int >dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep, >> if (event->status & DEPEVT_STATUS_SHORT && !chain) >> return 1; >> >> - if (event->status & (DEPEVT_STATUS_IOC | DEPEVT_STATUS_LST)) >> + if ((event->status & DEPEVT_STATUS_IOC) && >> + (trb->ctrl & DWC3_TRB_CTRL_IOC)) >> + return 1; > >this shouldn't be necessary. According to databook, event->status >contains the bits from the completed TRB. Which means that >event->status & IOC will always be equal to trb->ctrl & IOC. > Thanks for reviewing this patch. Lets consider an example where a request has num_sgs > 0 and each sg is mapped to a TRB and the last TRB has the IOC bit set. Once the controller is done with the transfer, it generates XferInProgress for the last TRB (since IOC bit is set). As a part of trb reclaim process dwc3_gadget_ep_reclaim_trb_sg() calls dwc3_gadget_ep_reclaim_completed_trb() for req->num_sgs times. Since the event already has the IOC bit set, the loop is exited from the loop at the very first TRB and the remaining TRBs (mapped to the sglist) are left unhandled. To avoid this we modified the code to exit only if both TRB & event has the IOC bit set. >Can you further describe the situation here? Perhaps share tracepoints >exposing the problem?
Sure, will capture the traces and reply back Thanks, Anurag Kumar Vulisha
