Hello, Le jeu. 19 juil. 2018 à 09:24, Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> a écrit : > > > Hi, > > lemagoup@xxxxxxxxx writes: > > From: Pierre Le Magourou <plemagourou@xxxxxxxxxxxxxxxxxxxx> > > > > We can't wait for END_OF_TRANSFER event in dwc3_gadget_ep_dequeue() > > because it can be called in an interruption context. > > > > TRBs are now cleared after the END_OF_TRANSFER completion, in the > > interrupt handler. > > > > Signed-off-by: Pierre Le Magourou <plemagourou@xxxxxxxxxxxxxxxxxxxx> > > --- > > you forgot to Cc linux-usb > > > drivers/usb/dwc3/core.h | 14 ++++++ > > drivers/usb/dwc3/gadget.c | 122 +++++++++++++++++++++++++++------------------- > > 2 files changed, 85 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > index 7a217a36c36b..5e2070183e3a 100644 > > --- a/drivers/usb/dwc3/core.h > > +++ b/drivers/usb/dwc3/core.h > > @@ -627,6 +627,7 @@ struct dwc3_event_buffer { > > * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer complete > > * @lock: spinlock for endpoint request queue traversal > > * @regs: pointer to first endpoint register > > + * @pending_trb: pointer to pending TRB structure > > * @trb_pool: array of transaction buffers > > * @trb_pool_dma: dma address of @trb_pool > > * @trb_enqueue: enqueue 'pointer' into TRB array > > @@ -654,6 +655,7 @@ struct dwc3_ep { > > void __iomem *regs; > > > > struct dwc3_trb *trb_pool; > > + struct dwc3_pending_trb *pending_trb; > > dma_addr_t trb_pool_dma; > > struct dwc3 *dwc; > > > > @@ -777,6 +779,18 @@ struct dwc3_trb { > > u32 ctrl; > > } __packed; > > > > +/** > > + * struct dwc3_pending_trb > > + * @dirty_trb: pointer to TRB waiting for END_TRANSFER completion > > + * @extra_trb: true if extra TRB was set to align transfer > > + * @num_pending_sgs: number of pending SGs > > + */ > > +struct dwc3_pending_trb { > > + struct dwc3_trb *dirty_trb; > > + bool extra_trb; > > + unsigned int num_pending_sgs; > > +}; > > + > > /** > > * struct dwc3_hwparams - copy of HWPARAMS registers > > * @hwparams0: GHWPARAMS0 > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 69bf137aab37..dd1f2b74723b 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -1341,6 +1341,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, > > > > struct dwc3_ep *dep = to_dwc3_ep(ep); > > struct dwc3 *dwc = dep->dwc; > > + struct dwc3_pending_trb *p_trb; > > > > unsigned long flags; > > int ret = 0; > > @@ -1367,63 +1368,29 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, > > * If request was already started, this means we had to > > * stop the transfer. With that we also need to ignore > > * all TRBs used by the request, however TRBs can only > > - * be modified after completion of END_TRANSFER > > - * command. So what we do here is that we wait for > > - * END_TRANSFER completion and only after that, we jump > > - * over TRBs by clearing HWO and incrementing dequeue > > - * pointer. > > + * be modified after completion of END_TRANSFER command. > > * > > - * Note that we have 2 possible types of transfers here: > > - * > > - * i) Linear buffer request > > - * ii) SG-list based request > > - * > > - * SG-list based requests will have r->num_pending_sgs > > - * set to a valid number (> 0). Linear requests, > > - * normally use a single TRB. > > - * > > - * For each of these two cases, if r->unaligned flag is > > - * set, one extra TRB has been used to align transfer > > - * size to wMaxPacketSize. > > - * > > - * All of these cases need to be taken into > > - * consideration so we don't mess up our TRB ring > > - * pointers. > > + * Don't wait for END_TRANSFER completion here because > > + * dwc3_gadget_ep_dequeue() can be called from within > > + * the controller's interrupt handler. Save the TRB > > + * pointer, the number of pending sgs, and the > > + * extra_trb flag, so that TRBs HWO can be cleared and > > + * dequeue pointer incremented when the END_TRANSFER > > + * completion is received. > > */ > > - wait_event_lock_irq(dep->wait_end_transfer, > > - !(dep->flags & DWC3_EP_END_TRANSFER_PENDING), > > - dwc->lock); > > > > if (!r->trb) > > goto out0; > > > > - if (r->num_pending_sgs) { > > - struct dwc3_trb *trb; > > - int i = 0; > > - > > - for (i = 0; i < r->num_pending_sgs; i++) { > > - trb = r->trb + i; > > - trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > - dwc3_ep_inc_deq(dep); > > - } > > - > > - if (r->unaligned || r->zero) { > > - trb = r->trb + r->num_pending_sgs + 1; > > - trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > - dwc3_ep_inc_deq(dep); > > - } > > - } else { > > - struct dwc3_trb *trb = r->trb; > > - > > - trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > - dwc3_ep_inc_deq(dep); > > - > > - if (r->unaligned || r->zero) { > > - trb = r->trb + 1; > > - trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > - dwc3_ep_inc_deq(dep); > > - } > > - } > > + p_trb = kzalloc(sizeof(*p_trb), GFP_KERNEL|GFP_ATOMIC); > > sorry, no. You shouldn't need this at all. The only thing you should do > here is delete all the code that's currently waiting for completion of > End Transfer, and move it all to command complete handler. > > you shouldn't need any allocation of any kind. Just remember that if the > cable is plugged, you must make sure that all TRBs have their HWO reset. Sorry, I am quite new to USB drivers. I created this structure to be able to keep the dwc3_request and get it back in the END_TRANSFER completion handler. If I understand well, in this handler, I can simply reset all TRB HWO of the requests in dep->started_list and dep->pending_list ? > > > @@ -2430,6 +2397,56 @@ static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep, > > __dwc3_gadget_start_isoc(dep); > > } > > > > +/* Clear HWO and increment dequeue pointer. > > + * > > + * 2 types of transfers are possible: > > + * > > + * i) Linear buffer request > > + * ii) SG-list based request > > + * > > + * SG-list based requests have p_trb->num_pending_sgs set to a valid number > > + * (> 0). Linear requests, normally use a single TRB. > > + * > > + * For each of these two cases, if p_trb->extra_trb flag is set, one > > + * extra TRB has been used to align transfer size to wMaxPacketSize. > > + * > > + * All of these cases need to be taken into consideration so TRB ring pointers > > + * are not messed up. > > + */ > > +static void dwc3_clear_pending_trbs(struct dwc3_ep *dep) > > +{ > > + struct dwc3_pending_trb *p_trb = dep->pending_trb; > > + struct dwc3_trb *trb; > > + > > + if (p_trb->num_pending_sgs) { > > + int i; > > + > > + for (i = 0; i < p_trb->num_pending_sgs; i++) { > > + trb = p_trb->dirty_trb + i; > > + trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > + dwc3_ep_inc_deq(dep); > > + } > > + > > + if (p_trb->extra_trb) { > > + trb = p_trb->dirty_trb + p_trb->num_pending_sgs + 1; > > + trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > + dwc3_ep_inc_deq(dep); > > + } > > + } else { > > + trb = p_trb->dirty_trb; > > + trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > + dwc3_ep_inc_deq(dep); > > + > > + if (p_trb->extra_trb) { > > + trb = p_trb->dirty_trb + 1; > > + trb->ctrl &= ~DWC3_TRB_CTRL_HWO; > > + dwc3_ep_inc_deq(dep); > > + } > > + } > > + kfree(p_trb); > > + dep->pending_trb = NULL; > > +} > > + > > static void dwc3_endpoint_interrupt(struct dwc3 *dwc, > > const struct dwc3_event_depevt *event) > > { > > @@ -2466,6 +2483,9 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, > > if (cmd == DWC3_DEPCMD_ENDTRANSFER) { > > dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING; > > wake_up(&dep->wait_end_transfer); > > this wake_up should go away. In fact, wait_end_transfer should go away completely. > > > + > > + if (dep->pending_trb) > > you don't need this pending_trb field at all. You already know you're > expecting a END_TRANSFER command to complete. > > -- > balbi