Hi,
Pierre Le Magourou <lemagoup@xxxxxxxxx> writes:
>> 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 ?
I'll send a series of patches implementing this today or tomorrow.
--
balbi
Attachment:
signature.asc
Description: PGP signature
