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
