On 18 October 2016 at 16:21, Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>>> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
>>>>> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>>>>> >> Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
>>>>> >>> Instead of just delaying for 100us, we should
>>>>> >>> actually wait for End Transfer Command Complete
>>>>> >>> interrupt before moving on. Note that this should
>>>>> >>> only be done if we're dealing with one of the core
>>>>> >>> revisions that actually require the interrupt before
>>>>> >>> moving on.
>>>>> >>>
>>>>> >>> Reported-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>>>>> >>> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>>>> >>
>>>>> >> I've updated this one in order to fix the comment we had about delaying
>>>>> >> 100us. No further changes were made, only the comment. Here it is:
>>>>> >>
>>>>> >> 8<------------------------------------------------------------------------
>>>>> >> From f3fa94f3171709f787a30e3c5ce69a668960b66e Mon Sep 17 00:00:00 2001
>>>>> >> From: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>>>> >> Date: Thu, 13 Oct 2016 14:09:47 +0300
>>>>> >> Subject: [PATCH v2] usb: dwc3: gadget: wait for End Transfer to complete
>>>>> >>
>>>>> >> Instead of just delaying for 100us, we should
>>>>> >> actually wait for End Transfer Command Complete
>>>>> >> interrupt before moving on. Note that this should
>>>>> >> only be done if we're dealing with one of the core
>>>>> >> revisions that actually require the interrupt before
>>>>> >> moving on.
>>>>> >>
>>>>> >> Reported-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>>>>> >> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>>>> >
>>>>> > From my testing, there are still some problems caused by the nested
>>>>> > lock, at lease I found 2 functions will issue the usb_ep_disable()
>>>>> > function with spinlock:
>>>>>
>>>>> Thanks for testing. Seems like I really need to revisit locking in the
>>>>> entire gadget API. In either case, we need to have a larger discussion
>>>>> about this situation.
>>>>>
>>>>> IMO, usb_ep_disable() and usb_ep_enable() should only be callable from
>>>>> process context, but that has never been a requirement before. Still,
>>>>> it's not far-fetched to assume that a controller (such as dwc3, but
>>>>> probably others) might sleep while cancelling a transfer because they
>>>>> need to wait for an Interrupt.
>>>>>
>>>>> In fact, we know of two controllers that need this: dwc3 and dwc3.1.
>>>>
>>>> It's not clear that this should be the deciding factor. That is, does
>>>> usb_ep_disable() need to wait until all the endpoint's outstanding
>>>> transfers have been completed before it returns? I don't think it
>>>> does.
>>>
>>> not all, no. And, frankly, this is really only a problem if we're
>>> unregistering a gadget while there are still pending transfers.
>>>
>>> The original problem statement is that we unregister a gadget, issue End
>>> Transfer, but CmdCompletion for the EndTransfer comes way too
>>> late. Baolin, can you clarify again what happens when the IRQ comes
>>> late?
>>
>> Sure. The problem I met was, when we change the USB function with
>> configfs frequently, sometimes it will hang the system to crash. The
>> reason is, we will start end transfer command when disable the
>> endpoint, but sometimes the end transfer command complete event comes
>> after we have freed the gadget irq (since the xHCI will share the same
>> interrupt number with gadget, thus when free the gadget irq, it will
>> not shutdown this gadget irq line), it will trigger the interrupt line
>> all the time.
>
> okay, thanks for describing it again. Another option would be to only
> wait_for_completion() before calling free_irq() is any endpoint has
> END_TRANSFER_PENDING flag set. Something like below:
Yeah, that is what my original patch did, but you did one better
patch, I will test it too.
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5fc437021ac7..0cb3b78d28b7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -26,6 +26,7 @@
> #include <linux/dma-mapping.h>
> #include <linux/mm.h>
> #include <linux/debugfs.h>
> +#include <linux/wait.h>
>
> #include <linux/usb/ch9.h>
> #include <linux/usb/gadget.h>
> @@ -505,6 +506,7 @@ struct dwc3_event_buffer {
> * @endpoint: usb endpoint
> * @pending_list: list of pending requests for this endpoint
> * @started_list: list of started requests on this endpoint
> + * @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
> * @trb_pool: array of transaction buffers
> @@ -530,6 +532,8 @@ struct dwc3_ep {
> struct list_head pending_list;
> struct list_head started_list;
>
> + wait_queue_head_t wait_end_transfer;
> +
> spinlock_t lock;
> void __iomem *regs;
>
> @@ -546,6 +550,7 @@ struct dwc3_ep {
> #define DWC3_EP_BUSY (1 << 4)
> #define DWC3_EP_PENDING_REQUEST (1 << 5)
> #define DWC3_EP_MISSED_ISOC (1 << 6)
> +#define DWC3_EP_END_TRANSFER_PENDING (1 << 7)
>
> /* This last one is specific to EP0 */
> #define DWC3_EP0_DIR_IN (1 << 31)
> @@ -1050,6 +1055,9 @@ struct dwc3_event_depevt {
> #define DEPEVT_TRANSFER_BUS_EXPIRY 2
>
> u32 parameters:16;
> +
> +/* For Command Complete Events */
> +#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 8)) >> 8)
> } __packed;
>
> /**
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3b53a5714df4..5929a75b3455 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -598,6 +598,8 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
> reg |= DWC3_DALEPENA_EP(dep->number);
> dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
>
> + init_waitqueue_head(&dep->wait_end_transfer);
> +
> if (usb_endpoint_xfer_control(desc))
> return 0;
>
> @@ -1783,12 +1785,28 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>
> static void __dwc3_gadget_stop(struct dwc3 *dwc)
> {
> + int epnum;
> +
> if (pm_runtime_suspended(dwc->dev))
> return;
>
> dwc3_gadget_disable_irq(dwc);
> __dwc3_gadget_ep_disable(dwc->eps[0]);
> __dwc3_gadget_ep_disable(dwc->eps[1]);
> +
> + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
> + struct dwc3_ep *dep = dwc->eps[epnum];
> +
> + if (!(dep->flags & DWC3_EP_ENABLED))
> + continue;
> +
> + if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
> + continue;
> +
> + wait_event_lock_irq(dep->wait_end_transfer,
> + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
> + dwc->lock);
> + }
> }
>
> static int dwc3_gadget_stop(struct usb_gadget *g)
> @@ -2171,6 +2189,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> {
> struct dwc3_ep *dep;
> u8 epnum = event->endpoint_number;
> + u8 cmd;
>
> dep = dwc->eps[epnum];
>
> @@ -2215,8 +2234,15 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> return;
> }
> break;
> - case DWC3_DEPEVT_RXTXFIFOEVT:
> case DWC3_DEPEVT_EPCMDCMPLT:
> + cmd = DEPEVT_PARAMETER_CMD(event->parameters);
> +
> + if (cmd == DWC3_DEPCMD_ENDTRANSFER) {
> + dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
> + wake_up(&dep->wait_end_transfer);
> + }
> + break;
> + case DWC3_DEPEVT_RXTXFIFOEVT:
> break;
> }
> }
> @@ -2269,7 +2295,8 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
>
> dep = dwc->eps[epnum];
>
> - if (!dep->resource_index)
> + if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) ||
> + !dep->resource_index)
> return;
>
> /*
> @@ -2313,8 +2340,10 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
> dep->resource_index = 0;
> dep->flags &= ~DWC3_EP_BUSY;
>
> - if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A)
> + if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) {
> + dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> udelay(100);
> + }
> }
>
> static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)
>
>
> I'll run some tests with this in a couple hours.
>
> --
> balbi
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
