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

[Felipe Balbi]

Hi,

(I have added you to another thread which is where we'll be collecting
discussion about this, however ...)

Alan Stern  writes:
> On Fri, 14 Oct 2016, Felipe Balbi wrote:
>
>> 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, >config->functions, list) {
>> +   spin_unlock_irq(>lock);
>> if (f->disable)
>> f->disable(f);
>> +   spin_lock_irq(>lock);
>>  
>> bitmap_zero(f->endpoints, 32);
>> }
>> 
>> Alan, do you remember if we have a requirement for not holding locks
>> when calling usb_ep_disable()? I can't find Documentation about it, but
>> history shows me that usb_ep_disable() was called without locks and IRQs
>> enabled. Do you think we should update documentation about this?
>
> I don't think there is any requirement for interrupts to be enabled 
> when usb_ep_disable() runs.  At least, a quick check shows that both 
> net2280 and dummy-hcd use spin_lock_irqsave() rather than spin_lock() 
> in their disable routines.
>
> Holding locks is a different story.  It should be okay for a gadget 
> driver to hold one of its own locks when disabling an endpoint (which 
> means that the gadget's disable routine shouldn't wait for a concurrent 
> giveback to finish), but we might want to avoid holding a lock in the 
> composite core.  Although even that might be okay -- I can't think of 
> any reason why a udc driver would need to call back into the composite 
> core while disabling an endpoint.  It should be a pretty self-contained 
> operation.

True, but how do we handle controllers which need to wait for an
interrupt in order to cancel a transfer? Maybe we should change the
calling context of usb_ep_disable() so that it _must_ be called with
IRQs enabled?

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

(I have added you to another thread which is where we'll be collecting
discussion about this, however ...)

Alan Stern  writes:
> On Fri, 14 Oct 2016, Felipe Balbi wrote:
>
>> 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, >config->functions, list) {
>> +   spin_unlock_irq(>lock);
>> if (f->disable)
>> f->disable(f);
>> +   spin_lock_irq(>lock);
>>  
>> bitmap_zero(f->endpoints, 32);
>> }
>> 
>> Alan, do you remember if we have a requirement for not holding locks
>> when calling usb_ep_disable()? I can't find Documentation about it, but
>> history shows me that usb_ep_disable() was called without locks and IRQs
>> enabled. Do you think we should update documentation about this?
>
> I don't think there is any requirement for interrupts to be enabled 
> when usb_ep_disable() runs.  At least, a quick check shows that both 
> net2280 and dummy-hcd use spin_lock_irqsave() rather than spin_lock() 
> in their disable routines.
>
> Holding locks is a different story.  It should be okay for a gadget 
> driver to hold one of its own locks when disabling an endpoint (which 
> means that the gadget's disable routine shouldn't wait for a concurrent 
> giveback to finish), but we might want to avoid holding a lock in the 
> composite core.  Although even that might be okay -- I can't think of 
> any reason why a udc driver would need to call back into the composite 
> core while disabling an endpoint.  It should be a pretty self-contained 
> operation.

True, but how do we handle controllers which need to wait for an
interrupt in order to cancel a transfer? Maybe we should change the
calling context of usb_ep_disable() so that it _must_ be called with
IRQs enabled?

-- 
balbi


signature.asc
Description: PGP signature

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

[Alan Stern]

On Fri, 14 Oct 2016, Felipe Balbi wrote:

> 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, >config->functions, list) {
> +   spin_unlock_irq(>lock);
> if (f->disable)
> f->disable(f);
> +   spin_lock_irq(>lock);
>  
> bitmap_zero(f->endpoints, 32);
> }
> 
> Alan, do you remember if we have a requirement for not holding locks
> when calling usb_ep_disable()? I can't find Documentation about it, but
> history shows me that usb_ep_disable() was called without locks and IRQs
> enabled. Do you think we should update documentation about this?

I don't think there is any requirement for interrupts to be enabled 
when usb_ep_disable() runs.  At least, a quick check shows that both 
net2280 and dummy-hcd use spin_lock_irqsave() rather than spin_lock() 
in their disable routines.

Holding locks is a different story.  It should be okay for a gadget 
driver to hold one of its own locks when disabling an endpoint (which 
means that the gadget's disable routine shouldn't wait for a concurrent 
giveback to finish), but we might want to avoid holding a lock in the 
composite core.  Although even that might be okay -- I can't think of 
any reason why a udc driver would need to call back into the composite 
core while disabling an endpoint.  It should be a pretty self-contained 
operation.

Alan Stern

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

[Alan Stern]

On Fri, 14 Oct 2016, Felipe Balbi wrote:

> 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, >config->functions, list) {
> +   spin_unlock_irq(>lock);
> if (f->disable)
> f->disable(f);
> +   spin_lock_irq(>lock);
>  
> bitmap_zero(f->endpoints, 32);
> }
> 
> Alan, do you remember if we have a requirement for not holding locks
> when calling usb_ep_disable()? I can't find Documentation about it, but
> history shows me that usb_ep_disable() was called without locks and IRQs
> enabled. Do you think we should update documentation about this?

I don't think there is any requirement for interrupts to be enabled 
when usb_ep_disable() runs.  At least, a quick check shows that both 
net2280 and dummy-hcd use spin_lock_irqsave() rather than spin_lock() 
in their disable routines.

Holding locks is a different story.  It should be okay for a gadget 
driver to hold one of its own locks when disabling an endpoint (which 
means that the gadget's disable routine shouldn't wait for a concurrent 
giveback to finish), but we might want to avoid holding a lock in the 
composite core.  Although even that might be okay -- I can't think of 
any reason why a udc driver would need to call back into the composite 
core while disabling an endpoint.  It should be a pretty self-contained 
operation.

Alan Stern

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  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 
 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 
 Signed-off-by: Felipe Balbi 
 ---
  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 
  #include 
  #include 
 +#include 

  #include 
  #include 
 @@ -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_headpending_list;
 struct list_headstarted_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);
 

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  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 
 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 
 Signed-off-by: Felipe Balbi 
 ---
  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 
  #include 
  #include 
 +#include 

  #include 
  #include 
 @@ -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_headpending_list;
 struct list_headstarted_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(>wait_end_transfer);
 +
 if 

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

[Baolin Wang]

Hi Felipe,

On 14 October 2016 at 15:46, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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 
>>> 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 
>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>  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 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include 
>>>  #include 
>>> @@ -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_headpending_list;
>>> struct list_headstarted_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(>wait_end_transfer);

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

[Baolin Wang]

Hi Felipe,

On 14 October 2016 at 15:46, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  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 
>>> 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 
>>> Signed-off-by: Felipe Balbi 
>>> ---
>>>  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 
>>>  #include 
>>>  #include 
>>> +#include 
>>>
>>>  #include 
>>>  #include 
>>> @@ -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_headpending_list;
>>> struct list_headstarted_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(>wait_end_transfer);
>>> +
>>> if (usb_endpoint_xfer_control(desc))
>>> return 0;
>>>
>>> @@ -1151,6 

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

[Felipe Balbi]

Hi,

Baolin Wang  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 
>> 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 
>> Signed-off-by: Felipe Balbi 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -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_headpending_list;
>> struct list_headstarted_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(>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)
>> 

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

[Felipe Balbi]

Hi,

Baolin Wang  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 
>> 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 
>> Signed-off-by: Felipe Balbi 
>> ---
>>  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 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #include 
>> @@ -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_headpending_list;
>> struct list_headstarted_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(>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 & 

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

[Baolin Wang]

Hi Felipe,

On 13 October 2016 at 21:34, Felipe Balbi  wrote:
>
>
> Hi,
>
> Baolin Wang  writes:
>>> Baolin Wang  writes:
>> Baolin Wang  writes:
>> I'm thinking this is a bug in configfs interface of Gadget API, not
>> dwc3. The only reason for this to happen would be if we get to
>> ->udc_stop() with endpoints still enabled.
>>
>> Can you check if that's the case? i.e. can you check if any endpoints
>> are still enabled when we get here?
>
> No, it is not any endpoints are still enabled. Like I said in commit
> message, we will start end transfer command when disable the endpoint,
> if the end transfer command complete event comes after we have freed
> the gadget irq, it will trigger the interrupt line all the time to
> crash the system.

 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 
> 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 
> Signed-off-by: Felipe Balbi 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -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_headpending_list;
> struct list_headstarted_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;
> +
> 

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

[Baolin Wang]

Hi Felipe,

On 13 October 2016 at 21:34, Felipe Balbi  wrote:
>
>
> Hi,
>
> Baolin Wang  writes:
>>> Baolin Wang  writes:
>> Baolin Wang  writes:
>> I'm thinking this is a bug in configfs interface of Gadget API, not
>> dwc3. The only reason for this to happen would be if we get to
>> ->udc_stop() with endpoints still enabled.
>>
>> Can you check if that's the case? i.e. can you check if any endpoints
>> are still enabled when we get here?
>
> No, it is not any endpoints are still enabled. Like I said in commit
> message, we will start end transfer command when disable the endpoint,
> if the end transfer command complete event comes after we have freed
> the gadget irq, it will trigger the interrupt line all the time to
> crash the system.

 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 
> 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 
> Signed-off-by: Felipe Balbi 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -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_headpending_list;
> struct list_headstarted_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 

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  writes:
> Baolin Wang  writes:
> I'm thinking this is a bug in configfs interface of Gadget API, not
> dwc3. The only reason for this to happen would be if we get to
> ->udc_stop() with endpoints still enabled.
>
> Can you check if that's the case? i.e. can you check if any endpoints
> are still enabled when we get here?

 No, it is not any endpoints are still enabled. Like I said in commit
 message, we will start end transfer command when disable the endpoint,
 if the end transfer command complete event comes after we have freed
 the gadget irq, it will trigger the interrupt line all the time to
 crash the system.
>>>
>>> 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 
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 
Signed-off-by: Felipe Balbi 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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_headpending_list;
struct list_headstarted_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
+++ 

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>> Baolin Wang  writes:
> Baolin Wang  writes:
> I'm thinking this is a bug in configfs interface of Gadget API, not
> dwc3. The only reason for this to happen would be if we get to
> ->udc_stop() with endpoints still enabled.
>
> Can you check if that's the case? i.e. can you check if any endpoints
> are still enabled when we get here?

 No, it is not any endpoints are still enabled. Like I said in commit
 message, we will start end transfer command when disable the endpoint,
 if the end transfer command complete event comes after we have freed
 the gadget irq, it will trigger the interrupt line all the time to
 crash the system.
>>>
>>> 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 
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 
Signed-off-by: Felipe Balbi 
---
 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 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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_headpending_list;
struct list_headstarted_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);
  

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

[Baolin Wang]

Hi Felipe,

On 13 October 2016 at 19:23, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
 Baolin Wang  writes:
 I'm thinking this is a bug in configfs interface of Gadget API, not
 dwc3. The only reason for this to happen would be if we get to
 ->udc_stop() with endpoints still enabled.

 Can you check if that's the case? i.e. can you check if any endpoints
 are still enabled when we get here?
>>>
>>> No, it is not any endpoints are still enabled. Like I said in commit
>>> message, we will start end transfer command when disable the endpoint,
>>> if the end transfer command complete event comes after we have freed
>>> the gadget irq, it will trigger the interrupt line all the time to
>>> crash the system.
>>
>> 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
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.

>
>>> that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
>>> before calling kick_transfer). Could you have a look and, perhaps, run a
>>> test?
>>
>> Sure. I will test it and send out the result tomorrow.
>
> Thank you
>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi Felipe,

On 13 October 2016 at 19:23, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
 Baolin Wang  writes:
 I'm thinking this is a bug in configfs interface of Gadget API, not
 dwc3. The only reason for this to happen would be if we get to
 ->udc_stop() with endpoints still enabled.

 Can you check if that's the case? i.e. can you check if any endpoints
 are still enabled when we get here?
>>>
>>> No, it is not any endpoints are still enabled. Like I said in commit
>>> message, we will start end transfer command when disable the endpoint,
>>> if the end transfer command complete event comes after we have freed
>>> the gadget irq, it will trigger the interrupt line all the time to
>>> crash the system.
>>
>> 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
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.

>
>>> that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
>>> before calling kick_transfer). Could you have a look and, perhaps, run a
>>> test?
>>
>> Sure. I will test it and send out the result tomorrow.
>
> Thank you
>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>> Baolin Wang  writes:
>>> I'm thinking this is a bug in configfs interface of Gadget API, not
>>> dwc3. The only reason for this to happen would be if we get to
>>> ->udc_stop() with endpoints still enabled.
>>>
>>> Can you check if that's the case? i.e. can you check if any endpoints
>>> are still enabled when we get here?
>>
>> No, it is not any endpoints are still enabled. Like I said in commit
>> message, we will start end transfer command when disable the endpoint,
>> if the end transfer command complete event comes after we have freed
>> the gadget irq, it will trigger the interrupt line all the time to
>> crash the system.
>
> 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.

>> that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
>> before calling kick_transfer). Could you have a look and, perhaps, run a
>> test?
>
> Sure. I will test it and send out the result tomorrow.

Thank you

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
>>> Baolin Wang  writes:
>>> I'm thinking this is a bug in configfs interface of Gadget API, not
>>> dwc3. The only reason for this to happen would be if we get to
>>> ->udc_stop() with endpoints still enabled.
>>>
>>> Can you check if that's the case? i.e. can you check if any endpoints
>>> are still enabled when we get here?
>>
>> No, it is not any endpoints are still enabled. Like I said in commit
>> message, we will start end transfer command when disable the endpoint,
>> if the end transfer command complete event comes after we have freed
>> the gadget irq, it will trigger the interrupt line all the time to
>> crash the system.
>
> 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.

>> that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
>> before calling kick_transfer). Could you have a look and, perhaps, run a
>> test?
>
> Sure. I will test it and send out the result tomorrow.

Thank you

-- 
balbi


signature.asc
Description: PGP signature

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

[Baolin Wang]

On 13 October 2016 at 18:56, Felipe Balbi  wrote:
>
> Hi,
>
> Felipe Balbi  writes:
>> Hi,
>>
>> Baolin Wang  writes:
>> I'm thinking this is a bug in configfs interface of Gadget API, not
>> dwc3. The only reason for this to happen would be if we get to
>> ->udc_stop() with endpoints still enabled.
>>
>> Can you check if that's the case? i.e. can you check if any endpoints
>> are still enabled when we get here?
>
> No, it is not any endpoints are still enabled. Like I said in commit
> message, we will start end transfer command when disable the endpoint,
> if the end transfer command complete event comes after we have freed
> the gadget irq, it will trigger the interrupt line all the time to
> crash the system.

 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.

> that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
> before calling kick_transfer). Could you have a look and, perhaps, run a
> test?

Sure. I will test it and send out the result tomorrow.

>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index e878366ead00..24a77e9f9bba 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -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_headpending_list;
> struct list_headstarted_list;
>
> +   wait_queue_head_t   wait_end_transfer;
> +
> spinlock_t  lock;
> void __iomem*regs;
>
> @@ -545,7 +549,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)
>
> @@ -1047,6 +1051,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..8037bff43485 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(>wait_end_transfer);
> +
> if (usb_endpoint_xfer_control(desc))
> return 0;
>
> @@ -2156,6 +2158,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 +2203,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;
> +   break;
> +   case DWC3_DEPEVT_RXTXFIFOEVT:
> break;
> }
>  }
> @@ -2246,6 +2254,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
>  }
>
>  static void dwc3_stop_active_transfer(struct 

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

[Baolin Wang]

On 13 October 2016 at 18:56, Felipe Balbi  wrote:
>
> Hi,
>
> Felipe Balbi  writes:
>> Hi,
>>
>> Baolin Wang  writes:
>> I'm thinking this is a bug in configfs interface of Gadget API, not
>> dwc3. The only reason for this to happen would be if we get to
>> ->udc_stop() with endpoints still enabled.
>>
>> Can you check if that's the case? i.e. can you check if any endpoints
>> are still enabled when we get here?
>
> No, it is not any endpoints are still enabled. Like I said in commit
> message, we will start end transfer command when disable the endpoint,
> if the end transfer command complete event comes after we have freed
> the gadget irq, it will trigger the interrupt line all the time to
> crash the system.

 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.

> that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
> before calling kick_transfer). Could you have a look and, perhaps, run a
> test?

Sure. I will test it and send out the result tomorrow.

>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index e878366ead00..24a77e9f9bba 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -26,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -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_headpending_list;
> struct list_headstarted_list;
>
> +   wait_queue_head_t   wait_end_transfer;
> +
> spinlock_t  lock;
> void __iomem*regs;
>
> @@ -545,7 +549,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)
>
> @@ -1047,6 +1051,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..8037bff43485 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(>wait_end_transfer);
> +
> if (usb_endpoint_xfer_control(desc))
> return 0;
>
> @@ -2156,6 +2158,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 +2203,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;
> +   break;
> +   case DWC3_DEPEVT_RXTXFIFOEVT:
> break;
> }
>  }
> @@ -2246,6 +2254,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
>  }
>
>  static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool 
> force)
> +__releases(>lock) 

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

[Felipe Balbi]

Hi,

Felipe Balbi  writes:
> Hi,
>
> Baolin Wang  writes:
> I'm thinking this is a bug in configfs interface of Gadget API, not
> dwc3. The only reason for this to happen would be if we get to
> ->udc_stop() with endpoints still enabled.
>
> Can you check if that's the case? i.e. can you check if any endpoints
> are still enabled when we get here?

 No, it is not any endpoints are still enabled. Like I said in commit
 message, we will start end transfer command when disable the endpoint,
 if the end transfer command complete event comes after we have freed
 the gadget irq, it will trigger the interrupt line all the time to
 crash the system.
>>>
>>> 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
that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
before calling kick_transfer). Could you have a look and, perhaps, run a
test?

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index e878366ead00..24a77e9f9bba 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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_headpending_list;
struct list_headstarted_list;
 
+   wait_queue_head_t   wait_end_transfer;
+
spinlock_t  lock;
void __iomem*regs;
 
@@ -545,7 +549,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)
 
@@ -1047,6 +1051,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..8037bff43485 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(>wait_end_transfer);
+
if (usb_endpoint_xfer_control(desc))
return 0;
 
@@ -2156,6 +2158,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 +2203,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;
+   break;
+   case DWC3_DEPEVT_RXTXFIFOEVT:
break;
}
 }
@@ -2246,6 +2254,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
 }
 
 static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
+__releases(>lock) __acquires(>lock)
 {
struct dwc3_ep *dep;
struct dwc3_gadget_ep_cmd_params params;
@@ -2295,6 +2304,12 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, 
u32 epnum, bool force)
memset(, 0, sizeof(params));
ret = dwc3_send_gadget_ep_cmd(dep, cmd, );
WARN_ON_ONCE(ret);
+
+   dep->flags |= 

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

[Felipe Balbi]

Hi,

Felipe Balbi  writes:
> Hi,
>
> Baolin Wang  writes:
> I'm thinking this is a bug in configfs interface of Gadget API, not
> dwc3. The only reason for this to happen would be if we get to
> ->udc_stop() with endpoints still enabled.
>
> Can you check if that's the case? i.e. can you check if any endpoints
> are still enabled when we get here?

 No, it is not any endpoints are still enabled. Like I said in commit
 message, we will start end transfer command when disable the endpoint,
 if the end transfer command complete event comes after we have freed
 the gadget irq, it will trigger the interrupt line all the time to
 crash the system.
>>>
>>> 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
that's easy to sort out by checking for DWC3_EP_END_TRANSFER_PENDING
before calling kick_transfer). Could you have a look and, perhaps, run a
test?

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index e878366ead00..24a77e9f9bba 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -26,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -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_headpending_list;
struct list_headstarted_list;
 
+   wait_queue_head_t   wait_end_transfer;
+
spinlock_t  lock;
void __iomem*regs;
 
@@ -545,7 +549,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)
 
@@ -1047,6 +1051,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..8037bff43485 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(>wait_end_transfer);
+
if (usb_endpoint_xfer_control(desc))
return 0;
 
@@ -2156,6 +2158,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 +2203,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;
+   break;
+   case DWC3_DEPEVT_RXTXFIFOEVT:
break;
}
 }
@@ -2246,6 +2254,7 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
 }
 
 static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force)
+__releases(>lock) __acquires(>lock)
 {
struct dwc3_ep *dep;
struct dwc3_gadget_ep_cmd_params params;
@@ -2295,6 +2304,12 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, 
u32 epnum, bool force)
memset(, 0, sizeof(params));
ret = dwc3_send_gadget_ep_cmd(dep, cmd, );
WARN_ON_ONCE(ret);
+
+   dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
+   

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
 I'm thinking this is a bug in configfs interface of Gadget API, not
 dwc3. The only reason for this to happen would be if we get to
 ->udc_stop() with endpoints still enabled.

 Can you check if that's the case? i.e. can you check if any endpoints
 are still enabled when we get here?
>>>
>>> No, it is not any endpoints are still enabled. Like I said in commit
>>> message, we will start end transfer command when disable the endpoint,
>>> if the end transfer command complete event comes after we have freed
>>> the gadget irq, it will trigger the interrupt line all the time to
>>> crash the system.
>>
>> 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.

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
 I'm thinking this is a bug in configfs interface of Gadget API, not
 dwc3. The only reason for this to happen would be if we get to
 ->udc_stop() with endpoints still enabled.

 Can you check if that's the case? i.e. can you check if any endpoints
 are still enabled when we get here?
>>>
>>> No, it is not any endpoints are still enabled. Like I said in commit
>>> message, we will start end transfer command when disable the endpoint,
>>> if the end transfer command complete event comes after we have freed
>>> the gadget irq, it will trigger the interrupt line all the time to
>>> crash the system.
>>
>> 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.

-- 
balbi


signature.asc
Description: PGP signature

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

[Baolin Wang]

Hi,

On 13 October 2016 at 15:51, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> Hi,
>>
>> On 13 October 2016 at 15:02, Felipe Balbi  wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang  writes:
 @@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
   dwc->gadget_driver  = NULL;
   spin_unlock_irqrestore(>lock, flags);

 + /*
 +  * Since the xHCI will share the same interrupt with gadget, thus 
 when
 +  * free the gadget irq, it will not shutdown this gadget irq line. 
 Then
 +  * the gadget driver can not handle the end transfer command complete
 +  * event after free the gadget irq, which will hang the system to 
 crash.
 +  *
 +  * So we should wait for the end transfer command complete event 
 before
 +  * free it to avoid this situation.
 +  */
 + dwc3_wait_command_complete(dwc);
>>>
>>> this doesn't make sense. We have already masked all interrupts before
>>> getting here. We have also, already, disabled all endpoints.
>>
>> No, you just mask the interrupts described in DEVTEN register, and you
>> did not disable the endpoint command complete event.
>
> True that
>
>>> I'm thinking this is a bug in configfs interface of Gadget API, not
>>> dwc3. The only reason for this to happen would be if we get to
>>> ->udc_stop() with endpoints still enabled.
>>>
>>> Can you check if that's the case? i.e. can you check if any endpoints
>>> are still enabled when we get here?
>>
>> No, it is not any endpoints are still enabled. Like I said in commit
>> message, we will start end transfer command when disable the endpoint,
>> if the end transfer command complete event comes after we have freed
>> the gadget irq, it will trigger the interrupt line all the time to
>> crash the system.
>
> 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.

>
> It seems as if *that* should be fixed. We should start actually waiting
> for Command Complete IRQ.
>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi,

On 13 October 2016 at 15:51, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> Hi,
>>
>> On 13 October 2016 at 15:02, Felipe Balbi  wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang  writes:
 @@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
   dwc->gadget_driver  = NULL;
   spin_unlock_irqrestore(>lock, flags);

 + /*
 +  * Since the xHCI will share the same interrupt with gadget, thus 
 when
 +  * free the gadget irq, it will not shutdown this gadget irq line. 
 Then
 +  * the gadget driver can not handle the end transfer command complete
 +  * event after free the gadget irq, which will hang the system to 
 crash.
 +  *
 +  * So we should wait for the end transfer command complete event 
 before
 +  * free it to avoid this situation.
 +  */
 + dwc3_wait_command_complete(dwc);
>>>
>>> this doesn't make sense. We have already masked all interrupts before
>>> getting here. We have also, already, disabled all endpoints.
>>
>> No, you just mask the interrupts described in DEVTEN register, and you
>> did not disable the endpoint command complete event.
>
> True that
>
>>> I'm thinking this is a bug in configfs interface of Gadget API, not
>>> dwc3. The only reason for this to happen would be if we get to
>>> ->udc_stop() with endpoints still enabled.
>>>
>>> Can you check if that's the case? i.e. can you check if any endpoints
>>> are still enabled when we get here?
>>
>> No, it is not any endpoints are still enabled. Like I said in commit
>> message, we will start end transfer command when disable the endpoint,
>> if the end transfer command complete event comes after we have freed
>> the gadget irq, it will trigger the interrupt line all the time to
>> crash the system.
>
> 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.

>
> It seems as if *that* should be fixed. We should start actually waiting
> for Command Complete IRQ.
>
> --
> balbi



-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> Hi,
>
> On 13 October 2016 at 15:02, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  writes:
>>> @@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>>   dwc->gadget_driver  = NULL;
>>>   spin_unlock_irqrestore(>lock, flags);
>>>
>>> + /*
>>> +  * Since the xHCI will share the same interrupt with gadget, thus when
>>> +  * free the gadget irq, it will not shutdown this gadget irq line. 
>>> Then
>>> +  * the gadget driver can not handle the end transfer command complete
>>> +  * event after free the gadget irq, which will hang the system to 
>>> crash.
>>> +  *
>>> +  * So we should wait for the end transfer command complete event 
>>> before
>>> +  * free it to avoid this situation.
>>> +  */
>>> + dwc3_wait_command_complete(dwc);
>>
>> this doesn't make sense. We have already masked all interrupts before
>> getting here. We have also, already, disabled all endpoints.
>
> No, you just mask the interrupts described in DEVTEN register, and you
> did not disable the endpoint command complete event.

True that

>> I'm thinking this is a bug in configfs interface of Gadget API, not
>> dwc3. The only reason for this to happen would be if we get to
>> ->udc_stop() with endpoints still enabled.
>>
>> Can you check if that's the case? i.e. can you check if any endpoints
>> are still enabled when we get here?
>
> No, it is not any endpoints are still enabled. Like I said in commit
> message, we will start end transfer command when disable the endpoint,
> if the end transfer command complete event comes after we have freed
> the gadget irq, it will trigger the interrupt line all the time to
> crash the system.

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.

It seems as if *that* should be fixed. We should start actually waiting
for Command Complete IRQ.

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> Hi,
>
> On 13 October 2016 at 15:02, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Baolin Wang  writes:
>>> @@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>>   dwc->gadget_driver  = NULL;
>>>   spin_unlock_irqrestore(>lock, flags);
>>>
>>> + /*
>>> +  * Since the xHCI will share the same interrupt with gadget, thus when
>>> +  * free the gadget irq, it will not shutdown this gadget irq line. 
>>> Then
>>> +  * the gadget driver can not handle the end transfer command complete
>>> +  * event after free the gadget irq, which will hang the system to 
>>> crash.
>>> +  *
>>> +  * So we should wait for the end transfer command complete event 
>>> before
>>> +  * free it to avoid this situation.
>>> +  */
>>> + dwc3_wait_command_complete(dwc);
>>
>> this doesn't make sense. We have already masked all interrupts before
>> getting here. We have also, already, disabled all endpoints.
>
> No, you just mask the interrupts described in DEVTEN register, and you
> did not disable the endpoint command complete event.

True that

>> I'm thinking this is a bug in configfs interface of Gadget API, not
>> dwc3. The only reason for this to happen would be if we get to
>> ->udc_stop() with endpoints still enabled.
>>
>> Can you check if that's the case? i.e. can you check if any endpoints
>> are still enabled when we get here?
>
> No, it is not any endpoints are still enabled. Like I said in commit
> message, we will start end transfer command when disable the endpoint,
> if the end transfer command complete event comes after we have freed
> the gadget irq, it will trigger the interrupt line all the time to
> crash the system.

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.

It seems as if *that* should be fixed. We should start actually waiting
for Command Complete IRQ.

-- 
balbi


signature.asc
Description: PGP signature

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

[Baolin Wang]

Hi,

On 13 October 2016 at 15:02, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> @@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>   dwc->gadget_driver  = NULL;
>>   spin_unlock_irqrestore(>lock, flags);
>>
>> + /*
>> +  * Since the xHCI will share the same interrupt with gadget, thus when
>> +  * free the gadget irq, it will not shutdown this gadget irq line. Then
>> +  * the gadget driver can not handle the end transfer command complete
>> +  * event after free the gadget irq, which will hang the system to 
>> crash.
>> +  *
>> +  * So we should wait for the end transfer command complete event before
>> +  * free it to avoid this situation.
>> +  */
>> + dwc3_wait_command_complete(dwc);
>
> this doesn't make sense. We have already masked all interrupts before
> getting here. We have also, already, disabled all endpoints.

No, you just mask the interrupts described in DEVTEN register, and you
did not disable the endpoint command complete event.

>
> I'm thinking this is a bug in configfs interface of Gadget API, not
> dwc3. The only reason for this to happen would be if we get to
> ->udc_stop() with endpoints still enabled.
>
> Can you check if that's the case? i.e. can you check if any endpoints
> are still enabled when we get here?

No, it is not any endpoints are still enabled. Like I said in commit
message, we will start end transfer command when disable the endpoint,
if the end transfer command complete event comes after we have freed
the gadget irq, it will trigger the interrupt line all the time to
crash the system.

-- 
Baolin.wang
Best Regards

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

[Baolin Wang]

Hi,

On 13 October 2016 at 15:02, Felipe Balbi  wrote:
>
> Hi,
>
> Baolin Wang  writes:
>> @@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>>   dwc->gadget_driver  = NULL;
>>   spin_unlock_irqrestore(>lock, flags);
>>
>> + /*
>> +  * Since the xHCI will share the same interrupt with gadget, thus when
>> +  * free the gadget irq, it will not shutdown this gadget irq line. Then
>> +  * the gadget driver can not handle the end transfer command complete
>> +  * event after free the gadget irq, which will hang the system to 
>> crash.
>> +  *
>> +  * So we should wait for the end transfer command complete event before
>> +  * free it to avoid this situation.
>> +  */
>> + dwc3_wait_command_complete(dwc);
>
> this doesn't make sense. We have already masked all interrupts before
> getting here. We have also, already, disabled all endpoints.

No, you just mask the interrupts described in DEVTEN register, and you
did not disable the endpoint command complete event.

>
> I'm thinking this is a bug in configfs interface of Gadget API, not
> dwc3. The only reason for this to happen would be if we get to
> ->udc_stop() with endpoints still enabled.
>
> Can you check if that's the case? i.e. can you check if any endpoints
> are still enabled when we get here?

No, it is not any endpoints are still enabled. Like I said in commit
message, we will start end transfer command when disable the endpoint,
if the end transfer command complete event comes after we have freed
the gadget irq, it will trigger the interrupt line all the time to
crash the system.

-- 
Baolin.wang
Best Regards

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> @@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>   dwc->gadget_driver  = NULL;
>   spin_unlock_irqrestore(>lock, flags);
>  
> + /*
> +  * Since the xHCI will share the same interrupt with gadget, thus when
> +  * free the gadget irq, it will not shutdown this gadget irq line. Then
> +  * the gadget driver can not handle the end transfer command complete
> +  * event after free the gadget irq, which will hang the system to crash.
> +  *
> +  * So we should wait for the end transfer command complete event before
> +  * free it to avoid this situation.
> +  */
> + dwc3_wait_command_complete(dwc);

this doesn't make sense. We have already masked all interrupts before
getting here. We have also, already, disabled all endpoints.

I'm thinking this is a bug in configfs interface of Gadget API, not
dwc3. The only reason for this to happen would be if we get to
->udc_stop() with endpoints still enabled.

Can you check if that's the case? i.e. can you check if any endpoints
are still enabled when we get here?

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Baolin Wang  writes:
> @@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>   dwc->gadget_driver  = NULL;
>   spin_unlock_irqrestore(>lock, flags);
>  
> + /*
> +  * Since the xHCI will share the same interrupt with gadget, thus when
> +  * free the gadget irq, it will not shutdown this gadget irq line. Then
> +  * the gadget driver can not handle the end transfer command complete
> +  * event after free the gadget irq, which will hang the system to crash.
> +  *
> +  * So we should wait for the end transfer command complete event before
> +  * free it to avoid this situation.
> +  */
> + dwc3_wait_command_complete(dwc);

this doesn't make sense. We have already masked all interrupts before
getting here. We have also, already, disabled all endpoints.

I'm thinking this is a bug in configfs interface of Gadget API, not
dwc3. The only reason for this to happen would be if we get to
->udc_stop() with endpoints still enabled.

Can you check if that's the case? i.e. can you check if any endpoints
are still enabled when we get here?

-- 
balbi


signature.asc
Description: PGP signature

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

[Baolin Wang]

When we change the USB function with configfs frequently, sometimes it will
hang the system to crash. The reason is the gadget driver can not hanle the
end transfer complete event after free 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), which will trigger the interrupt
all the time.

Thus we should check if we need wait for the end transfer command completion
before free gadget irq.

Signed-off-by: Baolin Wang 
---
Changes since v1:
 - Simply the operation of cleaning the dep flags.
---
 drivers/usb/dwc3/core.h   |3 ++
 drivers/usb/dwc3/gadget.c |   73 +++--
 2 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 9128725..f01d8fd 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -537,6 +537,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_CMDCMPLT_BUSY  (1 << 7)
 
/* This last one is specific to EP0 */
 #define DWC3_EP0_DIR_IN(1 << 31)
@@ -746,6 +747,7 @@ struct dwc3_scratchpad_array {
  * @ep0_bounce_addr: dma address of ep0_bounce
  * @scratch_addr: dma address of scratchbuf
  * @ep0_in_setup: One control transfer is completed and enter setup phase
+ * @cmd_complete: endpoint command completion
  * @lock: for synchronizing
  * @dev: pointer to our struct device
  * @xhci: pointer to our xHCI child
@@ -845,6 +847,7 @@ struct dwc3 {
dma_addr_t  scratch_addr;
struct dwc3_request ep0_usb_req;
struct completion   ep0_in_setup;
+   struct completion   cmd_complete;
 
/* device lock */
spinlock_t  lock;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fef023a..32e3d4d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -573,6 +573,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
dep->comp_desc = comp_desc;
dep->type = usb_endpoint_type(desc);
dep->flags |= DWC3_EP_ENABLED;
+   dep->flags &= ~DWC3_EP_CMDCMPLT_BUSY;
 
reg = dwc3_readl(dwc->regs, DWC3_DALEPENA);
reg |= DWC3_DALEPENA_EP(dep->number);
@@ -650,7 +651,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
dep->endpoint.desc = NULL;
dep->comp_desc = NULL;
dep->type = 0;
-   dep->flags = 0;
+   dep->flags &= DWC3_EP_CMDCMPLT_BUSY;
 
return 0;
 }
@@ -1732,6 +1733,54 @@ static void __dwc3_gadget_stop(struct dwc3 *dwc)
__dwc3_gadget_ep_disable(dwc->eps[1]);
 }
 
+static void dwc3_wait_command_complete(struct dwc3 *dwc)
+{
+   u32 epnum, epstart = 2;
+   int ret, wait_cmd_complete = 0;
+   unsigned long flags;
+
+check_next:
+   spin_lock_irqsave(>lock, flags);
+   /*
+* If the gadget has been in suspend state, then don't
+* need to wait for the end transfer complete event.
+*/
+   if (pm_runtime_suspended(dwc->dev)) {
+   spin_unlock_irqrestore(>lock, flags);
+   return;
+   }
+
+   for (epnum = epstart; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
+   struct dwc3_ep *dep;
+
+   dep = dwc->eps[epnum];
+   if (!dep)
+   continue;
+
+   if (dep->flags & DWC3_EP_CMDCMPLT_BUSY) {
+   reinit_completion(>cmd_complete);
+   epstart = epnum + 1;
+   wait_cmd_complete = 1;
+   break;
+   }
+   }
+   spin_unlock_irqrestore(>lock, flags);
+
+   /*
+* Wait for 500ms to complete the end transfer command before free irq.
+*/
+   if (wait_cmd_complete) {
+   wait_cmd_complete = 0;
+   ret = wait_for_completion_timeout(>cmd_complete,
+ msecs_to_jiffies(500));
+   if (ret == 0)
+   dev_warn(dwc->dev,
+"timeout to wait for command complete.\n");
+
+   goto check_next;
+   }
+}
+
 static int dwc3_gadget_stop(struct usb_gadget *g)
 {
struct dwc3 *dwc = gadget_to_dwc(g);
@@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
dwc->gadget_driver  = NULL;
spin_unlock_irqrestore(>lock, flags);
 
+   /*
+* Since the xHCI will share the same interrupt with gadget, thus when
+* free the gadget irq, it will not shutdown this gadget irq line. Then
+* the gadget driver can not handle the end transfer command complete
+* event after free the gadget irq, which will hang the system to crash.
+*
+

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

[Baolin Wang]

When we change the USB function with configfs frequently, sometimes it will
hang the system to crash. The reason is the gadget driver can not hanle the
end transfer complete event after free 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), which will trigger the interrupt
all the time.

Thus we should check if we need wait for the end transfer command completion
before free gadget irq.

Signed-off-by: Baolin Wang 
---
Changes since v1:
 - Simply the operation of cleaning the dep flags.
---
 drivers/usb/dwc3/core.h   |3 ++
 drivers/usb/dwc3/gadget.c |   73 +++--
 2 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 9128725..f01d8fd 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -537,6 +537,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_CMDCMPLT_BUSY  (1 << 7)
 
/* This last one is specific to EP0 */
 #define DWC3_EP0_DIR_IN(1 << 31)
@@ -746,6 +747,7 @@ struct dwc3_scratchpad_array {
  * @ep0_bounce_addr: dma address of ep0_bounce
  * @scratch_addr: dma address of scratchbuf
  * @ep0_in_setup: One control transfer is completed and enter setup phase
+ * @cmd_complete: endpoint command completion
  * @lock: for synchronizing
  * @dev: pointer to our struct device
  * @xhci: pointer to our xHCI child
@@ -845,6 +847,7 @@ struct dwc3 {
dma_addr_t  scratch_addr;
struct dwc3_request ep0_usb_req;
struct completion   ep0_in_setup;
+   struct completion   cmd_complete;
 
/* device lock */
spinlock_t  lock;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fef023a..32e3d4d 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -573,6 +573,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
dep->comp_desc = comp_desc;
dep->type = usb_endpoint_type(desc);
dep->flags |= DWC3_EP_ENABLED;
+   dep->flags &= ~DWC3_EP_CMDCMPLT_BUSY;
 
reg = dwc3_readl(dwc->regs, DWC3_DALEPENA);
reg |= DWC3_DALEPENA_EP(dep->number);
@@ -650,7 +651,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep)
dep->endpoint.desc = NULL;
dep->comp_desc = NULL;
dep->type = 0;
-   dep->flags = 0;
+   dep->flags &= DWC3_EP_CMDCMPLT_BUSY;
 
return 0;
 }
@@ -1732,6 +1733,54 @@ static void __dwc3_gadget_stop(struct dwc3 *dwc)
__dwc3_gadget_ep_disable(dwc->eps[1]);
 }
 
+static void dwc3_wait_command_complete(struct dwc3 *dwc)
+{
+   u32 epnum, epstart = 2;
+   int ret, wait_cmd_complete = 0;
+   unsigned long flags;
+
+check_next:
+   spin_lock_irqsave(>lock, flags);
+   /*
+* If the gadget has been in suspend state, then don't
+* need to wait for the end transfer complete event.
+*/
+   if (pm_runtime_suspended(dwc->dev)) {
+   spin_unlock_irqrestore(>lock, flags);
+   return;
+   }
+
+   for (epnum = epstart; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
+   struct dwc3_ep *dep;
+
+   dep = dwc->eps[epnum];
+   if (!dep)
+   continue;
+
+   if (dep->flags & DWC3_EP_CMDCMPLT_BUSY) {
+   reinit_completion(>cmd_complete);
+   epstart = epnum + 1;
+   wait_cmd_complete = 1;
+   break;
+   }
+   }
+   spin_unlock_irqrestore(>lock, flags);
+
+   /*
+* Wait for 500ms to complete the end transfer command before free irq.
+*/
+   if (wait_cmd_complete) {
+   wait_cmd_complete = 0;
+   ret = wait_for_completion_timeout(>cmd_complete,
+ msecs_to_jiffies(500));
+   if (ret == 0)
+   dev_warn(dwc->dev,
+"timeout to wait for command complete.\n");
+
+   goto check_next;
+   }
+}
+
 static int dwc3_gadget_stop(struct usb_gadget *g)
 {
struct dwc3 *dwc = gadget_to_dwc(g);
@@ -1742,6 +1791,17 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
dwc->gadget_driver  = NULL;
spin_unlock_irqrestore(>lock, flags);
 
+   /*
+* Since the xHCI will share the same interrupt with gadget, thus when
+* free the gadget irq, it will not shutdown this gadget irq line. Then
+* the gadget driver can not handle the end transfer command complete
+* event after free the gadget irq, which will hang the system to crash.
+*
+* So we should wait