Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq

From: Felipe Balbi
Date: Fri Oct 14 2016 - 05:51:01 EST

Next message: Borislav Petkov: "Re: [PATCH] x86: remove duplicate rtit status MSR macro"
Previous message: Sudeep Holla: "Re: [patch] irqchip/gicv3: fix a loop timeout"
In reply to: Baolin Wang: "Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq"
Next in thread: Alan Stern: "Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>>>>>>>>>>> I see what the problem is. Databook tells us we *must* set CMDIOC when
>>>>>>>>>>> issuing EndTransfer command and we should always wait for Command
>>>>>>>>>>> Complete IRQ. However, we took a shortcut and just delayed for 100us
>>>>>>>>>>> after issuing End Transfer.
>>>>>>>>>>
>>>>>>>>>> Yes, but 100us delay is not enough in some scenarios, like changing
>>>>>>>>>> function with configfs frequently, we will met problems.
>>>>>>>>>
>>>>>>>>> heh, 100us *is* enough. However we still get an IRQ because we requested
>>>>>>>>> for it. If you want to fix this, then you need to find a way to get rid
>>>>>>>>> of that 100us udelay() and add a proper wait_for_completion() to delay
>>>>>>>>> execution until command complete IRQ fires up.
>>>>>>>>
>>>>>>>> I haven't tested this yet, but it shows the idea (I think we might still
>>>>>>>> have a race with ep_queue if we're still waiting for End Transfer, but
>>>>>>>
>>>>>>> Yes, maybe we need check DWC3_EP_END_TRANSFER_PENDING flag when
>>>>>>> queuing one request.
>>>>>>
>>>>>> Yeah, I'll add that check later. I still need to make sure we would
>>>>>> still try to kick any transfers that might have been queued while
>>>>>> waiting for End Transfer Command Complete IRQ.
>>>>>
>>>>> OK. But I still worried if there are other races in some places which
>>>>
>>>> There are no other places where this needs to be sorted out.
>>>>
>>>>> is not easy to find out by testing. No introducing race condition
>>>>> would be one better solution, anyway I would like to test the patch
>>>>> you send out firstly.
>>>>
>>>> Right, but your patch was working around another problem, rather then
>>>> fixing it.
>>>>
>>>> Here's another version which should make sure everything remains working
>>>> as it should.
>>>>
>>>> 8<------------------------------------------------------------------------
>>>> From 1f922a902a21d5cee32082be18bbfbf1d46137e4 Mon Sep 17 00:00:00 2001
>>>> From: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>>> Date: Thu, 13 Oct 2016 14:09:47 +0300
>>>> Subject: [PATCH] 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>
>>>> ---
>>>> drivers/usb/dwc3/core.h | 10 +++++++++-
>>>> drivers/usb/dwc3/gadget.c | 31 ++++++++++++++++++++++++++++---
>>>> 2 files changed, 37 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index e878366ead00..cf495d932252 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>
>>>> @@ -504,6 +505,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
>>>> @@ -529,6 +531,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;
>>>>
>>>> @@ -545,7 +549,8 @@ 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)
>>>> +#define DWC3_EP_KICK_POST_END_TRANSFER (1 << 8)
>>>> /* This last one is specific to EP0 */
>>>> #define DWC3_EP0_DIR_IN (1 << 31)
>>>>
>>>> @@ -1047,6 +1052,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 << 24)) >> 24)
>>>> } __packed;
>>>>
>>>> /**
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 3ba05b12e49a..a3f81b5ba901 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;
>>>>
>>>> @@ -1151,6 +1153,11 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>>>> if (!dwc3_calc_trbs_left(dep))
>>>> return 0;
>>>>
>>>> + if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) {
>>>> + dep->flags &= DWC3_EP_KICK_POST_END_TRANSFER;
>>>> + return 0;
>>>> + }
>>>> +
>>>> ret = __dwc3_gadget_kick_transfer(dep, 0);
>>>> if (ret && ret != -EBUSY)
>>>> dwc3_trace(trace_dwc3_gadget,
>>>> @@ -2156,6 +2163,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>>>> {
>>>> struct dwc3_ep *dep;
>>>> u8 epnum = event->endpoint_number;
>>>> + u8 cmd;
>>>>
>>>> dep = dwc->eps[epnum];
>>>>
>>>> @@ -2200,8 +2208,13 @@ 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;
>>>
>>> I think you missed wake_up(&dep->wait_end_transfer) function here.
>>>
>>>> + break;
>>>> + case DWC3_DEPEVT_RXTXFIFOEVT:
>>>> break;
>>>> }
>>>> }
>>>> @@ -2246,6 +2259,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
>>>> }
>>>>
>>>> static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
>>>> +__releases(&dwc->lock) __acquires(&dwc->lock)
>>>> {
>>>> struct dwc3_ep *dep;
>>>> struct dwc3_gadget_ep_cmd_params params;
>>>> @@ -2295,11 +2309,22 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
>>>> memset(&params, 0, sizeof(params));
>>>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>>>> WARN_ON_ONCE(ret);
>>>> +
>>>> + if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) {
>>>> + dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
>>>> + wait_event_lock_irq(dep->wait_end_transfer,
>>>> + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
>>>> + dwc->lock);
>>>> + }
>>>
>>> I've tested your patch, unfortunately it can not work on my platform.
>>> Since we can not issue wait_event_lock_irq() function in atomic
>>> context, we will hold another 'cdev->lock' in composite_disconnect()
>>> function. The dump stack as below:
>>
>> argh, we have nested spinlocks :-( Well, we shouldn't call
>> usb_ep_disable() with locks held AFAICR. So the following should be
>> enough:
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 919d7d1b611c..2e9359c58eb9 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -732,8 +732,10 @@ static void reset_config(struct usb_composite_dev *cdev)
>> DBG(cdev, "reset config\n");
>>
>> list_for_each_entry(f, &cdev->config->functions, list) {
>> + spin_unlock_irq(&cdev->lock);
>> if (f->disable)
>> f->disable(f);
>> + spin_lock_irq(&cdev->lock);
>>
>> bitmap_zero(f->endpoints, 32);
>> }
>
> After applying above modification, there are no obvious errors, but I
> still met problem: I can not change usb function with configfs. I need
> some time to find out why.

I'm also looking into this and testing it out here on SKL.

--
balbi

Attachment: signature.asc
Description: PGP signature

Next message: Borislav Petkov: "Re: [PATCH] x86: remove duplicate rtit status MSR macro"
Previous message: Sudeep Holla: "Re: [patch] irqchip/gicv3: fix a loop timeout"
In reply to: Baolin Wang: "Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq"
Next in thread: Alan Stern: "Re: [PATCH v2] usb: dwc3: gadget: Wait for end transfer complete before free irq"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]