usb: dwc3: gadget: synchronize_irq dwc irq in suspend

usb: dwc3: gadget: synchronize_irq dwc irq in suspend

[Felipe Balbi]

From: Bo He <bo.he@intel.com>

We see dwc3 endpoint stopped by unwanted irq during
suspend resume test, which is caused dwc3 ep can't be started
with error "No Resource".

Here, add synchronize_irq before suspend to sync the
pending IRQ handlers complete.

Signed-off-by: Bo He <bo.he@intel.com>
Signed-off-by: Yu Wang <yu.y.wang@intel.com>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 07bd31bb2f8a..851fd44d56ad 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3379,6 +3379,8 @@ int dwc3_gadget_suspend(struct dwc3 *dwc)
 	dwc3_disconnect_gadget(dwc);
 	__dwc3_gadget_stop(dwc);
 
+	synchronize_irq(dwc->irq_gadget);
+
 	return 0;
 }
 

usb: dwc3: gadget: synchronize_irq dwc irq in suspend

[Marek Szyprowski]

Hi All,

On 2019-01-14 09:30, Felipe Balbi wrote:
> From: Bo He <bo.he@intel.com>
>
> We see dwc3 endpoint stopped by unwanted irq during
> suspend resume test, which is caused dwc3 ep can't be started
> with error "No Resource".
>
> Here, add synchronize_irq before suspend to sync the
> pending IRQ handlers complete.
>
> Signed-off-by: Bo He <bo.he@intel.com>
> Signed-off-by: Yu Wang <yu.y.wang@intel.com>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>

This patch causes following kernel BUG on Samsung Exynos based platforms
during system suspend/resume cycle:

BUG: sleeping function called from invalid context at
kernel/irq/manage.c:112
in_atomic(): 1, irqs_disabled(): 128, pid: 1601, name: rtcwake
6 locks held by rtcwake/1601:
 #0: f70ac2a2 (sb_writers#7){.+.+}, at: vfs_write+0x130/0x16c
 #1: b5fe1270 (&of->mutex){+.+.}, at: kernfs_fop_write+0xc0/0x1e4
 #2: 7e597705 (kn->count#60){.+.+}, at: kernfs_fop_write+0xc8/0x1e4
 #3: 8b3527d0 (system_transition_mutex){+.+.}, at: pm_suspend+0xc4/0xc04
 #4: fc7f1c42 (&dev->mutex){....}, at: __device_suspend+0xd8/0x74c
 #5: 4b36507e (&(&dwc->lock)->rlock){....}, at:
dwc3_gadget_suspend+0x24/0x3c
irq event stamp: 11252
hardirqs last  enabled at (11251): [<c09c54a4>]
_raw_spin_unlock_irqrestore+0x6c/0x74
hardirqs last disabled at (11252): [<c09c4d44>]
_raw_spin_lock_irqsave+0x1c/0x5c
softirqs last  enabled at (9744): [<c0102564>] __do_softirq+0x3a4/0x66c
softirqs last disabled at (9737): [<c0128528>] irq_exit+0x140/0x168
Preemption disabled at:
[<00000000>]   (null)
CPU: 7 PID: 1601 Comm: rtcwake Not tainted
5.0.0-rc3-next-20190122-00039-ga3f4ee4f8a52 #5252
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c01110f0>] (unwind_backtrace) from [<c010d120>] (show_stack+0x10/0x14)
[<c010d120>] (show_stack) from [<c09a4d04>] (dump_stack+0x90/0xc8)
[<c09a4d04>] (dump_stack) from [<c014c700>] (___might_sleep+0x22c/0x2c8)
[<c014c700>] (___might_sleep) from [<c0189d68>] (synchronize_irq+0x28/0x84)
[<c0189d68>] (synchronize_irq) from [<c05cbbf8>]
(dwc3_gadget_suspend+0x34/0x3c)
[<c05cbbf8>] (dwc3_gadget_suspend) from [<c05bd020>]
(dwc3_suspend_common+0x154/0x410)
[<c05bd020>] (dwc3_suspend_common) from [<c05bd34c>]
(dwc3_suspend+0x14/0x2c)
[<c05bd34c>] (dwc3_suspend) from [<c051c730>]
(platform_pm_suspend+0x2c/0x54)
[<c051c730>] (platform_pm_suspend) from [<c05285d4>]
(dpm_run_callback+0xa4/0x3dc)
[<c05285d4>] (dpm_run_callback) from [<c0528a40>]
(__device_suspend+0x134/0x74c)
[<c0528a40>] (__device_suspend) from [<c052c508>] (dpm_suspend+0x174/0x588)
[<c052c508>] (dpm_suspend) from [<c0182134>]
(suspend_devices_and_enter+0xc0/0xe74)
[<c0182134>] (suspend_devices_and_enter) from [<c0183658>]
(pm_suspend+0x770/0xc04)
[<c0183658>] (pm_suspend) from [<c0180ddc>] (state_store+0x6c/0xcc)
[<c0180ddc>] (state_store) from [<c09a9a70>] (kobj_attr_store+0x14/0x20)
[<c09a9a70>] (kobj_attr_store) from [<c02d6800>] (sysfs_kf_write+0x4c/0x50)
[<c02d6800>] (sysfs_kf_write) from [<c02d594c>]
(kernfs_fop_write+0xfc/0x1e4)
[<c02d594c>] (kernfs_fop_write) from [<c02593d8>] (__vfs_write+0x2c/0x160)
[<c02593d8>] (__vfs_write) from [<c0259694>] (vfs_write+0xa4/0x16c)
[<c0259694>] (vfs_write) from [<c0259870>] (ksys_write+0x40/0x8c)
[<c0259870>] (ksys_write) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
Exception stack(0xed55ffa8 to 0xed55fff0)
ffa0:                   00000004 0002a288 00000004 0002a288 00000004
00000000
ffc0: 00000004 0002a288 0002a120 00000004 00000004 00000004 befb7bec
00028160
ffe0: 00000000 befb7ac4 b6e0f608 b6e6abbc

> ---
>  drivers/usb/dwc3/gadget.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 07bd31bb2f8a..851fd44d56ad 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3379,6 +3379,8 @@ int dwc3_gadget_suspend(struct dwc3 *dwc)
>  	dwc3_disconnect_gadget(dwc);
>  	__dwc3_gadget_stop(dwc);
>  
> +	synchronize_irq(dwc->irq_gadget);
> +
>  	return 0;
>  }
>  
>
Best regards

usb: dwc3: gadget: synchronize_irq dwc irq in suspend

[Felipe Balbi]

Hi,

Marek Szyprowski <m.szyprowski@samsung.com> writes:
> On 2019-01-14 09:30, Felipe Balbi wrote:
>> From: Bo He <bo.he@intel.com>
>>
>> We see dwc3 endpoint stopped by unwanted irq during
>> suspend resume test, which is caused dwc3 ep can't be started
>> with error "No Resource".
>>
>> Here, add synchronize_irq before suspend to sync the
>> pending IRQ handlers complete.
>>
>> Signed-off-by: Bo He <bo.he@intel.com>
>> Signed-off-by: Yu Wang <yu.y.wang@intel.com>
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> This patch causes following kernel BUG on Samsung Exynos based platforms
> during system suspend/resume cycle:

are you calling ->suspend() from ->suspend_noirq() time? Are we not
allowed to call synchronize_irq() during ->suspend()?

usb: dwc3: gadget: synchronize_irq dwc irq in suspend

[Marek Szyprowski]

Hi Felipe,

On 2019-01-28 14:30, Felipe Balbi wrote:
> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>> On 2019-01-14 09:30, Felipe Balbi wrote:
>>> From: Bo He <bo.he@intel.com>
>>>
>>> We see dwc3 endpoint stopped by unwanted irq during
>>> suspend resume test, which is caused dwc3 ep can't be started
>>> with error "No Resource".
>>>
>>> Here, add synchronize_irq before suspend to sync the
>>> pending IRQ handlers complete.
>>>
>>> Signed-off-by: Bo He <bo.he@intel.com>
>>> Signed-off-by: Yu Wang <yu.y.wang@intel.com>
>>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> This patch causes following kernel BUG on Samsung Exynos based platforms
>> during system suspend/resume cycle:
> are you calling ->suspend() from ->suspend_noirq() time? Are we not
> allowed to call synchronize_irq() during ->suspend()?

dwc3_suspend_common() calls dwc3_gadget_suspend() with dwc->lock
spinlock held. This is not the proper context for calling sleeping
functions like synchronize_irq().

Best regards

usb: dwc3: gadget: synchronize_irq dwc irq in suspend

[He, Bo]

agree your findings.

so it's better move the synchronize_irq() after the spin_unlock_irqrestore().
static int dwc3_suspend_common(struct dwc3 *dwc)
{
	unsigned long	flags;

	switch (dwc->dr_mode) {
	case USB_DR_MODE_PERIPHERAL:
	case USB_DR_MODE_OTG:
		spin_lock_irqsave(&dwc->lock, flags);
		dwc3_gadget_suspend(dwc);
		spin_unlock_irqrestore(&dwc->lock, flags);
		synchronize_irq()

-----Original Message-----
From: Marek Szyprowski <m.szyprowski@samsung.com> 
Sent: Monday, January 28, 2019 10:53 PM
To: Felipe Balbi <felipe.balbi@linux.intel.com>; Linux USB <linux-usb@vger.kernel.org>
Cc: He, Bo <bo.he@intel.com>; Wang, Yu Y <yu.y.wang@intel.com>; 'Linux Samsung SOC' <linux-samsung-soc@vger.kernel.org>
Subject: Re: [PATCH] usb: dwc3: gadget: synchronize_irq dwc irq in suspend

Hi Felipe,

On 2019-01-28 14:30, Felipe Balbi wrote:
> Marek Szyprowski <m.szyprowski@samsung.com> writes:
>> On 2019-01-14 09:30, Felipe Balbi wrote:
>>> From: Bo He <bo.he@intel.com>
>>>
>>> We see dwc3 endpoint stopped by unwanted irq during suspend resume 
>>> test, which is caused dwc3 ep can't be started with error "No 
>>> Resource".
>>>
>>> Here, add synchronize_irq before suspend to sync the pending IRQ 
>>> handlers complete.
>>>
>>> Signed-off-by: Bo He <bo.he@intel.com>
>>> Signed-off-by: Yu Wang <yu.y.wang@intel.com>
>>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> This patch causes following kernel BUG on Samsung Exynos based 
>> platforms during system suspend/resume cycle:
> are you calling ->suspend() from ->suspend_noirq() time? Are we not 
> allowed to call synchronize_irq() during ->suspend()?

dwc3_suspend_common() calls dwc3_gadget_suspend() with dwc->lock spinlock held. This is not the proper context for calling sleeping functions like synchronize_irq().

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

usb: dwc3: gadget: synchronize_irq dwc irq in suspend

[Felipe Balbi]

Hi,

"He, Bo" <bo.he@intel.com> writes:
> agree your findings.
>
> so it's better move the synchronize_irq() after the spin_unlock_irqrestore().
> static int dwc3_suspend_common(struct dwc3 *dwc)
> {
> 	unsigned long	flags;
>
> 	switch (dwc->dr_mode) {
> 	case USB_DR_MODE_PERIPHERAL:
> 	case USB_DR_MODE_OTG:
> 		spin_lock_irqsave(&dwc->lock, flags);
> 		dwc3_gadget_suspend(dwc);
> 		spin_unlock_irqrestore(&dwc->lock, flags);
> 		synchronize_irq()

indeed, I missed that when I first reviewed the patch. Care to send a fix?

usb: dwc3: gadget: synchronize_irq dwc irq in suspend

[Felipe Balbi]

Hi,

"Wang, Yu1" <yu1.wang@intel.com> writes:
>> > so it's better move the synchronize_irq() after the spin_unlock_irqrestore().
>> > static int dwc3_suspend_common(struct dwc3 *dwc)
>> > {
>> > 	unsigned long	flags;
>> >
>> > 	switch (dwc->dr_mode) {
>> > 	case USB_DR_MODE_PERIPHERAL:
>> > 	case USB_DR_MODE_OTG:
>> > 		spin_lock_irqsave(&dwc->lock, flags);
>> > 		dwc3_gadget_suspend(dwc);
>> > 		spin_unlock_irqrestore(&dwc->lock, flags);
>> > 		synchronize_irq()
>> 
>> indeed, I missed that when I first reviewed the patch. Care to send a fix?
> With this new change, the dwc3 irq thread will be sync after dwc3
> stopped, is this irq thread can be handled correctly for this case?

how about this?

modified   drivers/usb/dwc3/gadget.c
@@ -3373,6 +3373,8 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
 }
 
 int dwc3_gadget_suspend(struct dwc3 *dwc)
+	__releases(dwc->lock)
+	__acquires(dwc->lock)
 {
 	if (!dwc->gadget_driver)
 		return 0;
@@ -3381,7 +3383,9 @@ int dwc3_gadget_suspend(struct dwc3 *dwc)
 	dwc3_disconnect_gadget(dwc);
 	__dwc3_gadget_stop(dwc);
 
+	spin_unlock_irq(dwc->lock);
 	synchronize_irq(dwc->irq_gadget);
+	spin_lock_irq(dwc->lock);
 
 	return 0;
 }

Not the most elegant solution. I'm open to other suggestions.

usb: dwc3: gadget: synchronize_irq dwc irq in suspend

[Marek Szyprowski]

Hi All,

On 2019-01-30 07:53, Felipe Balbi wrote:
> "Wang, Yu1" <yu1.wang@intel.com> writes:
>>>> so it's better move the synchronize_irq() after the spin_unlock_irqrestore().
>>>> static int dwc3_suspend_common(struct dwc3 *dwc)
>>>> {
>>>> 	unsigned long	flags;
>>>>
>>>> 	switch (dwc->dr_mode) {
>>>> 	case USB_DR_MODE_PERIPHERAL:
>>>> 	case USB_DR_MODE_OTG:
>>>> 		spin_lock_irqsave(&dwc->lock, flags);
>>>> 		dwc3_gadget_suspend(dwc);
>>>> 		spin_unlock_irqrestore(&dwc->lock, flags);
>>>> 		synchronize_irq()
>>> indeed, I missed that when I first reviewed the patch. Care to send a fix?
>> With this new change, the dwc3 irq thread will be sync after dwc3
>> stopped, is this irq thread can be handled correctly for this case?
> how about this?
>
> modified   drivers/usb/dwc3/gadget.c
> @@ -3373,6 +3373,8 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
>  }
>  
>  int dwc3_gadget_suspend(struct dwc3 *dwc)
> +	__releases(dwc->lock)
> +	__acquires(dwc->lock)
>  {
>  	if (!dwc->gadget_driver)
>  		return 0;
> @@ -3381,7 +3383,9 @@ int dwc3_gadget_suspend(struct dwc3 *dwc)
>  	dwc3_disconnect_gadget(dwc);
>  	__dwc3_gadget_stop(dwc);
>  
> +	spin_unlock_irq(dwc->lock);
>  	synchronize_irq(dwc->irq_gadget);
> +	spin_lock_irq(dwc->lock);
>  
>  	return 0;
>  }
>
> Not the most elegant solution. I'm open to other suggestions.

Frankly this the same as the previous proposal. spinlock is being
released just after dwc3_gadget_suspend() function, so I see no point in
playing with spinlock at the end of dwc3_gadget_suspend().

Best regards

usb: dwc3: gadget: synchronize_irq dwc irq in suspend

[Marek Szyprowski]

Hi All,

On 2019-01-30 09:19, Wang, Yu1 wrote:
>
>> "Wang, Yu1" <yu1.wang@intel.com> writes:
>>>>> so it's better move the synchronize_irq() after the spin_unlock_irqrestore().
>>>>> static int dwc3_suspend_common(struct dwc3 *dwc)
>>>>> {
>>>>> 	unsigned long	flags;
>>>>>
>>>>> 	switch (dwc->dr_mode) {
>>>>> 	case USB_DR_MODE_PERIPHERAL:
>>>>> 	case USB_DR_MODE_OTG:
>>>>> 		spin_lock_irqsave(&dwc->lock, flags);
>>>>> 		dwc3_gadget_suspend(dwc);
>>>>> 		spin_unlock_irqrestore(&dwc->lock, flags);
>>>>> 		synchronize_irq()
>>>> indeed, I missed that when I first reviewed the patch. Care to send a fix?
>>> With this new change, the dwc3 irq thread will be sync after dwc3
>>> stopped, is this irq thread can be handled correctly for this case?
>> how about this?
>>
>> modified   drivers/usb/dwc3/gadget.c
>> @@ -3373,6 +3373,8 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
>>  }
>>  
>>  int dwc3_gadget_suspend(struct dwc3 *dwc)
>> +	__releases(dwc->lock)
>> +	__acquires(dwc->lock)
>>  {
>>  	if (!dwc->gadget_driver)
>>  		return 0;
>> @@ -3381,7 +3383,9 @@ int dwc3_gadget_suspend(struct dwc3 *dwc)
>>  	dwc3_disconnect_gadget(dwc);
>>  	__dwc3_gadget_stop(dwc);
>>  
>> +	spin_unlock_irq(dwc->lock);
>>  	synchronize_irq(dwc->irq_gadget);
>> +	spin_lock_irq(dwc->lock);
>>  
>>  	return 0;
>>  }
>>
>> Not the most elegant solution. I'm open to other suggestions.
> Is there any conflict if dwc3_thread_interrupt and below three functions
> execute by parallel? 
>
> 	dwc3_gadget_run_stop(dwc, false, false);
> 	dwc3_disconnect_gadget(dwc);
> 	__dwc3_gadget_stop(dwc);
>
> That is means the dwc3_thread_interrupt is trying to handle pending
> events but another core is trying to do dwc3 stop/suspend.

Isn't that serialized by dwc->lock?

It quite late in release cycle and the issue is still not fixed. Maybe
it would make sense to revert that change and prepare a proper fix for
the next release?

Best regards