HI Felipe,
>-----Original Message----- >From: Felipe Balbi [mailto:[email protected]] >Sent: Friday, December 07, 2018 11:42 AM >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]> >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: >>>> @@ -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. > >Seems like IOC case should just test for chain flag as well: > Okay. Along with this logic the code for updating chain bit should also be modified I guess. Since the IOC bit is also set when there are not enough TRBs available, the code should be modified to not set DWC3_TRB_CTRL_CHN bit when the IOC bit is set. I will update below changes along with your suggestions and resend the patches. @@ -998,7 +998,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb, (dwc3_calc_trbs_left(dep) == 1)) trb->ctrl |= DWC3_TRB_CTRL_IOC; - if (chain) + if (chain && !(trb->ctrl & DWC3_TRB_CTRL_IOC)) trb->ctrl |= DWC3_TRB_CTRL_CHN; if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable) @@ -2372,7 +2372,7 @@ 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) + if (event->status & DEPEVT_STATUS_IOC && !chain) return 1; return 0; @@ -2399,7 +2399,7 @@ static int dwc3_gadget_ep_reclaim_trb_sg(struct dwc3_ep *dep, req->num_pending_sgs--; ret = dwc3_gadget_ep_reclaim_completed_trb(dep, req, - trb, event, status, true); + trb, event, status, (trb & DWC3_TRB_CTRL_CHN)); if (ret) break; } Thanks, Anurag Kumar Vulisha >modified drivers/usb/dwc3/gadget.c >@@ -2372,7 +2372,7 @@ 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) >+ if (event->status & DEPEVT_STATUS_IOC && !chain) > return 1; > > return 0; > >-- >balbi
