Revert "usb: dwc3: gadget: remove unnecessary _irqsave()"

Message ID 1444677889-4758-1-git-send-email-balbi@ti.com
State Accepted
Commit e5f68b4a3e7b006f209aba078d6a5ff3a78dd783
Headers show

Commit Message

Felipe Balbi Oct. 12, 2015, 7:24 p.m.
This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8.

That commit was causing a lockdep splat with g_ether and that
was interfering with proper functionality.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/usb/dwc3/gadget.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

John Youn Oct. 13, 2015, 2:09 a.m. | #1
On 10/12/2015 12:25 PM, Felipe Balbi wrote:
> This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8.
> 
> That commit was causing a lockdep splat with g_ether and that
> was interfering with proper functionality.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/usb/dwc3/gadget.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index cca806e09e5b..81bfb9ad1e2e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2642,15 +2642,16 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
>  static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc)
>  {
>  	struct dwc3 *dwc = _dwc;
> +	unsigned long flags;
>  	irqreturn_t ret = IRQ_NONE;
>  	int i;
>  
> -	spin_lock(&dwc->lock);
> +	spin_lock_irqsave(&dwc->lock, flags);
>  
>  	for (i = 0; i < dwc->num_event_buffers; i++)
>  		ret |= dwc3_process_event_buf(dwc, i);
>  
> -	spin_unlock(&dwc->lock);
> +	spin_unlock_irqrestore(&dwc->lock, flags);
>  
>  	return ret;
>  }
> 

Hi Felipe,

This seems related to a problem we've been tracking down the past
few days. This commit, 70f3a9ca, and commit a66c275b both cause
regression on one of our systems running mass-storage gadget.

a66c275b was introduced first, and until 70f3a9ca is introduced
if we revert just that, it works fine. 70f3a9ca causes similar
issues. So we must revert both in order to get back to a working
state.

Failure happens during enumeration and appears to be a race
condition in the event handling.

Attached are driver logs/traces for the failure with a66c275b.


John
Felipe Balbi Oct. 13, 2015, 4:04 p.m. | #2
Hi,

John Youn <John.Youn@synopsys.com> writes:
> On 10/12/2015 12:25 PM, Felipe Balbi wrote:
>> This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8.
>> 
>> That commit was causing a lockdep splat with g_ether and that
>> was interfering with proper functionality.
>> 
>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>> ---
>>  drivers/usb/dwc3/gadget.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index cca806e09e5b..81bfb9ad1e2e 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2642,15 +2642,16 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
>>  static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc)
>>  {
>>  	struct dwc3 *dwc = _dwc;
>> +	unsigned long flags;
>>  	irqreturn_t ret = IRQ_NONE;
>>  	int i;
>>  
>> -	spin_lock(&dwc->lock);
>> +	spin_lock_irqsave(&dwc->lock, flags);
>>  
>>  	for (i = 0; i < dwc->num_event_buffers; i++)
>>  		ret |= dwc3_process_event_buf(dwc, i);
>>  
>> -	spin_unlock(&dwc->lock);
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>  
>>  	return ret;
>>  }
>> 
>
> Hi Felipe,
>
> This seems related to a problem we've been tracking down the past
> few days. This commit, 70f3a9ca, and commit a66c275b both cause
> regression on one of our systems running mass-storage gadget.

I can't see how a66c275b could cause any issues. Our top half runs in
hardirq context with IRQs disabled; this has been discussed several times.

> a66c275b was introduced first, and until 70f3a9ca is introduced
> if we revert just that, it works fine. 70f3a9ca causes similar
> issues. So we must revert both in order to get back to a working
> state.
>
> Failure happens during enumeration and appears to be a race
> condition in the event handling.
>
> Attached are driver logs/traces for the failure with a66c275b.

All I see is a bunch of Start Transfer commands which failed. That's
odd. Which version of the core are you using ? Dwc3 or dwc31 ?
John Youn Oct. 14, 2015, 3:47 a.m. | #3
On 10/13/2015 9:05 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn <John.Youn@synopsys.com> writes:
>> On 10/12/2015 12:25 PM, Felipe Balbi wrote:
>>> This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8.
>>>
>>> That commit was causing a lockdep splat with g_ether and that
>>> was interfering with proper functionality.
>>>
>>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index cca806e09e5b..81bfb9ad1e2e 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2642,15 +2642,16 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
>>>  static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc)
>>>  {
>>>  	struct dwc3 *dwc = _dwc;
>>> +	unsigned long flags;
>>>  	irqreturn_t ret = IRQ_NONE;
>>>  	int i;
>>>  
>>> -	spin_lock(&dwc->lock);
>>> +	spin_lock_irqsave(&dwc->lock, flags);
>>>  
>>>  	for (i = 0; i < dwc->num_event_buffers; i++)
>>>  		ret |= dwc3_process_event_buf(dwc, i);
>>>  
>>> -	spin_unlock(&dwc->lock);
>>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>>  
>>>  	return ret;
>>>  }
>>>
>>
>> Hi Felipe,
>>
>> This seems related to a problem we've been tracking down the past
>> few days. This commit, 70f3a9ca, and commit a66c275b both cause
>> regression on one of our systems running mass-storage gadget.
> 
> I can't see how a66c275b could cause any issues. Our top half runs in
> hardirq context with IRQs disabled; this has been discussed several times.
> 

Yes I followed that and I'm also not sure how this can affect
it. I was hoping you might have some idea :)

I'll look into it in more detail later and let you know.

>> a66c275b was introduced first, and until 70f3a9ca is introduced
>> if we revert just that, it works fine. 70f3a9ca causes similar
>> issues. So we must revert both in order to get back to a working
>> state.
>>
>> Failure happens during enumeration and appears to be a race
>> condition in the event handling.
>>
>> Attached are driver logs/traces for the failure with a66c275b.
> 
> All I see is a bunch of Start Transfer commands which failed. That's
> odd. Which version of the core are you using ? Dwc3 or dwc31 ?
> 

USB 3.0 


John


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi Oct. 14, 2015, 5:30 a.m. | #4
Hi,

John Youn <John.Youn@synopsys.com> writes:
> On 10/13/2015 9:05 AM, Felipe Balbi wrote:
>>> John Youn <John.Youn@synopsys.com> writes:
>>> On 10/12/2015 12:25 PM, Felipe Balbi wrote:
>>>> This reverts commit 70f3a9caa11665e9f9aace581d85d8483716a4c8.
>>>>
>>>> That commit was causing a lockdep splat with g_ether and that
>>>> was interfering with proper functionality.
>>>>
>>>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>>>> ---
>>>>  drivers/usb/dwc3/gadget.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index cca806e09e5b..81bfb9ad1e2e 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -2642,15 +2642,16 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
>>>>  static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc)
>>>>  {
>>>>  	struct dwc3 *dwc = _dwc;
>>>> +	unsigned long flags;
>>>>  	irqreturn_t ret = IRQ_NONE;
>>>>  	int i;
>>>>  
>>>> -	spin_lock(&dwc->lock);
>>>> +	spin_lock_irqsave(&dwc->lock, flags);
>>>>  
>>>>  	for (i = 0; i < dwc->num_event_buffers; i++)
>>>>  		ret |= dwc3_process_event_buf(dwc, i);
>>>>  
>>>> -	spin_unlock(&dwc->lock);
>>>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>>>  
>>>>  	return ret;
>>>>  }
>>>>
>>>
>>> Hi Felipe,
>>>
>>> This seems related to a problem we've been tracking down the past
>>> few days. This commit, 70f3a9ca, and commit a66c275b both cause
>>> regression on one of our systems running mass-storage gadget.
>> 
>> I can't see how a66c275b could cause any issues. Our top half runs in
>> hardirq context with IRQs disabled; this has been discussed several times.
>> 
>
> Yes I followed that and I'm also not sure how this can affect
> it. I was hoping you might have some idea :)
>
> I'll look into it in more detail later and let you know.

okay.

>>> a66c275b was introduced first, and until 70f3a9ca is introduced
>>> if we revert just that, it works fine. 70f3a9ca causes similar
>>> issues. So we must revert both in order to get back to a working
>>> state.
>>>
>>> Failure happens during enumeration and appears to be a race
>>> condition in the event handling.
>>>
>>> Attached are driver logs/traces for the failure with a66c275b.
>> 
>> All I see is a bunch of Start Transfer commands which failed. That's
>> odd. Which version of the core are you using ? Dwc3 or dwc31 ?
>> 
>
> USB 3.0

silicon, fpga or virtual model ? If this is virtual model, could it be a
bug in the interrupt controller core model or something like that ? Any
chance you can get your same dwc3 in FPGA with Synopsys PCIe controller
and stick it into a desktop PC and see if you can get the same behavior?

If you want, I could try it myself, but I only have HAPS-61 and would
need sd-card contents (bitfile and conf file) with the core revision
you're using. I only have an old 1.83a bitfile around.

Patch

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index cca806e09e5b..81bfb9ad1e2e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2642,15 +2642,16 @@  static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
 static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc)
 {
 	struct dwc3 *dwc = _dwc;
+	unsigned long flags;
 	irqreturn_t ret = IRQ_NONE;
 	int i;
 
-	spin_lock(&dwc->lock);
+	spin_lock_irqsave(&dwc->lock, flags);
 
 	for (i = 0; i < dwc->num_event_buffers; i++)
 		ret |= dwc3_process_event_buf(dwc, i);
 
-	spin_unlock(&dwc->lock);
+	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return ret;
 }