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.
> @@ -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
Attachment:
signature.asc
Description: PGP signature
