* [PATCH] usb: cdnsp: Fix deadlock issue in cdnsp_thread_irq_handler
@ 2021-05-20 9:42 Pawel Laszczak
2021-05-22 9:54 ` Peter Chen
0 siblings, 1 reply; 5+ messages in thread
From: Pawel Laszczak @ 2021-05-20 9:42 UTC (permalink / raw)
To: peter.chen; +Cc: gregkh, linux-usb, kurahul, Pawel Laszczak, stable
From: Pawel Laszczak <pawell@cadence.com>
Patch fixes the critical issue caused by deadlock which has been detected
during testing NCM class.
The root cause of issue is spin_lock/spin_unlock instruction instead
spin_lock_irqsave/spin_lock_irqrestore in cdnsp_thread_irq_handler
function.
Cc: stable@vger.kernel.org
Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
drivers/usb/cdns3/cdnsp-ring.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
index 5f0513c96c04..68972746e363 100644
--- a/drivers/usb/cdns3/cdnsp-ring.c
+++ b/drivers/usb/cdns3/cdnsp-ring.c
@@ -1517,13 +1517,14 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
{
struct cdnsp_device *pdev = (struct cdnsp_device *)data;
union cdnsp_trb *event_ring_deq;
+ unsigned long flags;
int counter = 0;
- spin_lock(&pdev->lock);
+ spin_lock_irqsave(&pdev->lock, flags);
if (pdev->cdnsp_state & (CDNSP_STATE_HALTED | CDNSP_STATE_DYING)) {
cdnsp_died(pdev);
- spin_unlock(&pdev->lock);
+ spin_unlock_irqrestore(&pdev->lock, flags);
return IRQ_HANDLED;
}
@@ -1539,7 +1540,7 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
cdnsp_update_erst_dequeue(pdev, event_ring_deq, 1);
- spin_unlock(&pdev->lock);
+ spin_unlock_irqrestore(&pdev->lock, flags);
return IRQ_HANDLED;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] usb: cdnsp: Fix deadlock issue in cdnsp_thread_irq_handler
2021-05-20 9:42 [PATCH] usb: cdnsp: Fix deadlock issue in cdnsp_thread_irq_handler Pawel Laszczak
@ 2021-05-22 9:54 ` Peter Chen
2021-05-24 10:56 ` Pawel Laszczak
0 siblings, 1 reply; 5+ messages in thread
From: Peter Chen @ 2021-05-22 9:54 UTC (permalink / raw)
To: Pawel Laszczak; +Cc: gregkh, linux-usb, kurahul, stable
On 21-05-20 11:42:24, Pawel Laszczak wrote:
> From: Pawel Laszczak <pawell@cadence.com>
>
> Patch fixes the critical issue caused by deadlock which has been detected
> during testing NCM class.
>
> The root cause of issue is spin_lock/spin_unlock instruction instead
> spin_lock_irqsave/spin_lock_irqrestore in cdnsp_thread_irq_handler
> function.
Pawel, would you please explain more about why the deadlock occurs with
current code, and why this patch could fix it?
Peter
>
> Cc: stable@vger.kernel.org
> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> ---
> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
> index 5f0513c96c04..68972746e363 100644
> --- a/drivers/usb/cdns3/cdnsp-ring.c
> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> @@ -1517,13 +1517,14 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
> {
> struct cdnsp_device *pdev = (struct cdnsp_device *)data;
> union cdnsp_trb *event_ring_deq;
> + unsigned long flags;
> int counter = 0;
>
> - spin_lock(&pdev->lock);
> + spin_lock_irqsave(&pdev->lock, flags);
>
> if (pdev->cdnsp_state & (CDNSP_STATE_HALTED | CDNSP_STATE_DYING)) {
> cdnsp_died(pdev);
> - spin_unlock(&pdev->lock);
> + spin_unlock_irqrestore(&pdev->lock, flags);
> return IRQ_HANDLED;
> }
>
> @@ -1539,7 +1540,7 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
>
> cdnsp_update_erst_dequeue(pdev, event_ring_deq, 1);
>
> - spin_unlock(&pdev->lock);
> + spin_unlock_irqrestore(&pdev->lock, flags);
>
> return IRQ_HANDLED;
> }
> --
> 2.25.1
>
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] usb: cdnsp: Fix deadlock issue in cdnsp_thread_irq_handler
2021-05-22 9:54 ` Peter Chen
@ 2021-05-24 10:56 ` Pawel Laszczak
2021-05-25 0:53 ` Peter Chen
0 siblings, 1 reply; 5+ messages in thread
From: Pawel Laszczak @ 2021-05-24 10:56 UTC (permalink / raw)
To: Peter Chen; +Cc: gregkh, linux-usb, Rahul Kumar, stable
>
>
>On 21-05-20 11:42:24, Pawel Laszczak wrote:
>> From: Pawel Laszczak <pawell@cadence.com>
>>
>> Patch fixes the critical issue caused by deadlock which has been detected
>> during testing NCM class.
>>
>> The root cause of issue is spin_lock/spin_unlock instruction instead
>> spin_lock_irqsave/spin_lock_irqrestore in cdnsp_thread_irq_handler
>> function.
>
>Pawel, would you please explain more about why the deadlock occurs with
>current code, and why this patch could fix it?
>
I'm going to add such description to commit message:
Lack of spin_lock_irqsave causes that during handling threaded
interrupt inside spin_lock/spin_unlock section the ethernet
subsystem invokes eth_start_xmit function from interrupt context
which in turn starts queueing free requests in cdnsp driver.
Consequently, the deadlock occurs inside cdnsp_gadget_ep_queue
on spin_lock_irqsave instruction. Replacing spin_lock/spin_unlock
with spin_lock_irqsave/spin_lock_irqrestor causes disableing
interrupts and blocks queuing requests by ethernet subsystem until
cdnsp_thread_irq_handler ends..
I hope this description is sufficient.
Thanks,
Pawel
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
>> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> ---
>> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
>> index 5f0513c96c04..68972746e363 100644
>> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> @@ -1517,13 +1517,14 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
>> {
>> struct cdnsp_device *pdev = (struct cdnsp_device *)data;
>> union cdnsp_trb *event_ring_deq;
>> + unsigned long flags;
>> int counter = 0;
>>
>> - spin_lock(&pdev->lock);
>> + spin_lock_irqsave(&pdev->lock, flags);
>>
>> if (pdev->cdnsp_state & (CDNSP_STATE_HALTED | CDNSP_STATE_DYING)) {
>> cdnsp_died(pdev);
>> - spin_unlock(&pdev->lock);
>> + spin_unlock_irqrestore(&pdev->lock, flags);
>> return IRQ_HANDLED;
>> }
>>
>> @@ -1539,7 +1540,7 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
>>
>> cdnsp_update_erst_dequeue(pdev, event_ring_deq, 1);
>>
>> - spin_unlock(&pdev->lock);
>> + spin_unlock_irqrestore(&pdev->lock, flags);
>>
>> return IRQ_HANDLED;
>> }
>> --
>> 2.25.1
>>
>
>--
>
>Thanks,
>Peter Chen
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] usb: cdnsp: Fix deadlock issue in cdnsp_thread_irq_handler
2021-05-24 10:56 ` Pawel Laszczak
@ 2021-05-25 0:53 ` Peter Chen
2021-05-25 5:11 ` Pawel Laszczak
0 siblings, 1 reply; 5+ messages in thread
From: Peter Chen @ 2021-05-25 0:53 UTC (permalink / raw)
To: Pawel Laszczak; +Cc: gregkh, linux-usb, Rahul Kumar, stable
On 21-05-24 10:56:23, Pawel Laszczak wrote:
> >
> >
> >On 21-05-20 11:42:24, Pawel Laszczak wrote:
> >> From: Pawel Laszczak <pawell@cadence.com>
> >>
> >> Patch fixes the critical issue caused by deadlock which has been detected
> >> during testing NCM class.
> >>
> >> The root cause of issue is spin_lock/spin_unlock instruction instead
> >> spin_lock_irqsave/spin_lock_irqrestore in cdnsp_thread_irq_handler
> >> function.
> >
> >Pawel, would you please explain more about why the deadlock occurs with
> >current code, and why this patch could fix it?
> >
>
> I'm going to add such description to commit message:
>
> Lack of spin_lock_irqsave causes that during handling threaded
> interrupt inside spin_lock/spin_unlock section the ethernet
> subsystem invokes eth_start_xmit function from interrupt context
> which in turn starts queueing free requests in cdnsp driver.
> Consequently, the deadlock occurs inside cdnsp_gadget_ep_queue
> on spin_lock_irqsave instruction. Replacing spin_lock/spin_unlock
> with spin_lock_irqsave/spin_lock_irqrestor causes disableing
%s/disableing/disabling
> interrupts and blocks queuing requests by ethernet subsystem until
> cdnsp_thread_irq_handler ends..
>
> I hope this description is sufficient.
A call stack graph may be better, like [1]
[1]: https://www.spinics.net/lists/linux-usb/msg211931.html
Peter
>
> Thanks,
> Pawel
>
> >>
> >> Cc: stable@vger.kernel.org
> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
> >> ---
> >> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++---
> >> 1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
> >> index 5f0513c96c04..68972746e363 100644
> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
> >> @@ -1517,13 +1517,14 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
> >> {
> >> struct cdnsp_device *pdev = (struct cdnsp_device *)data;
> >> union cdnsp_trb *event_ring_deq;
> >> + unsigned long flags;
> >> int counter = 0;
> >>
> >> - spin_lock(&pdev->lock);
> >> + spin_lock_irqsave(&pdev->lock, flags);
> >>
> >> if (pdev->cdnsp_state & (CDNSP_STATE_HALTED | CDNSP_STATE_DYING)) {
> >> cdnsp_died(pdev);
> >> - spin_unlock(&pdev->lock);
> >> + spin_unlock_irqrestore(&pdev->lock, flags);
> >> return IRQ_HANDLED;
> >> }
> >>
> >> @@ -1539,7 +1540,7 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
> >>
> >> cdnsp_update_erst_dequeue(pdev, event_ring_deq, 1);
> >>
> >> - spin_unlock(&pdev->lock);
> >> + spin_unlock_irqrestore(&pdev->lock, flags);
> >>
> >> return IRQ_HANDLED;
> >> }
> >> --
> >> 2.25.1
> >>
> >
> >--
> >
> >Thanks,
> >Peter Chen
>
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] usb: cdnsp: Fix deadlock issue in cdnsp_thread_irq_handler
2021-05-25 0:53 ` Peter Chen
@ 2021-05-25 5:11 ` Pawel Laszczak
0 siblings, 0 replies; 5+ messages in thread
From: Pawel Laszczak @ 2021-05-25 5:11 UTC (permalink / raw)
To: Peter Chen; +Cc: gregkh, linux-usb, Rahul Kumar, stable
>
>On 21-05-24 10:56:23, Pawel Laszczak wrote:
>> >
>> >
>> >On 21-05-20 11:42:24, Pawel Laszczak wrote:
>> >> From: Pawel Laszczak <pawell@cadence.com>
>> >>
>> >> Patch fixes the critical issue caused by deadlock which has been detected
>> >> during testing NCM class.
>> >>
>> >> The root cause of issue is spin_lock/spin_unlock instruction instead
>> >> spin_lock_irqsave/spin_lock_irqrestore in cdnsp_thread_irq_handler
>> >> function.
>> >
>> >Pawel, would you please explain more about why the deadlock occurs with
>> >current code, and why this patch could fix it?
>> >
>>
>> I'm going to add such description to commit message:
>>
>> Lack of spin_lock_irqsave causes that during handling threaded
>> interrupt inside spin_lock/spin_unlock section the ethernet
>> subsystem invokes eth_start_xmit function from interrupt context
>> which in turn starts queueing free requests in cdnsp driver.
>> Consequently, the deadlock occurs inside cdnsp_gadget_ep_queue
>> on spin_lock_irqsave instruction. Replacing spin_lock/spin_unlock
>> with spin_lock_irqsave/spin_lock_irqrestor causes disableing
>
>%s/disableing/disabling
>
>> interrupts and blocks queuing requests by ethernet subsystem until
>> cdnsp_thread_irq_handler ends..
>>
>> I hope this description is sufficient.
>
>A call stack graph may be better, like [1]
>
>[1]: https://urldefense.com/v3/__https://www.spinics.net/lists/linux-
>usb/msg211931.html__;!!EHscmS1ygiU1lA!WCxrsHL4OWKd3iPdyymp-dQlVCoiHM9QzjIZPUC6-Dm6cnpU5CRLLOHrgiYSRA$
After putting online extra cpus, I've manage to catch call trace showing the deadlock issue:
[ 223.051673] smp: csd: Detected non-responsive CSD lock (#1) on CPU#0, waiting 5000000015 ns for CPU#02 do_sync_core+0x0/0x30(0x0).
[ 223.051690] smp: csd: CSD lock (#1) unresponsive.
[ 223.051693] Sending NMI from CPU 0 to CPUs 2:
[ 223.052690] NMI backtrace for cpu 2
[ 223.052692] CPU: 2 PID: 3053 Comm: irq/16-0000:01: Tainted: G OE 5.11.0-rc1+ #5
[ 223.052693] Hardware name: ASUS All Series/Q87T, BIOS 0908 07/22/2014
[ 223.052693] RIP: 0010:native_queued_spin_lock_slowpath+0x61/0x1d0
[ 223.052695] Code: 0f ba 2f 08 0f 92 c0 0f b6 c0 c1 e0 08 89 c2 8b 07 30 e4 09 d0 a9 00 01 ff ff 75 47 85 c0 74 0e 8b 07 84 c0 74 08 f3 90 8b 07 <84> c0 75 f8 b8 01 00 00 00 5d 66 89 07 c3 8b 37 b8 00 02 00 00 81
[ 223.052696] RSP: 0018:ffffbc494011cde0 EFLAGS: 00000002
[ 223.052698] RAX: 0000000000000101 RBX: ffff9ee8116b4a68 RCX: 0000000000000000
[ 223.052699] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9ee8116b4658
[ 223.052700] RBP: ffffbc494011cde0 R08: 0000000000000001 R09: 0000000000000000
[ 223.052701] R10: ffff9ee8116b4670 R11: 0000000000000000 R12: ffff9ee8116b4658
[ 223.052702] R13: ffff9ee8116b4670 R14: 0000000000000246 R15: ffff9ee8116b4658
[ 223.052702] FS: 0000000000000000(0000) GS:ffff9ee89b400000(0000) knlGS:0000000000000000
[ 223.052703] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 223.052704] CR2: 00007f7bcc41a830 CR3: 000000007a612003 CR4: 00000000001706e0
[ 223.052705] Call Trace:
[ 223.052706] <IRQ>
[ 223.052706] do_raw_spin_lock+0xc0/0xd0
[ 223.052707] _raw_spin_lock_irqsave+0x95/0xa0
[ 223.052708] cdnsp_gadget_ep_queue.cold+0x88/0x107 [cdnsp_udc_pci]
[ 223.052708] usb_ep_queue+0x35/0x110
[ 223.052709] eth_start_xmit+0x220/0x3d0 [u_ether]
[ 223.052710] ncm_tx_timeout+0x34/0x40 [usb_f_ncm]
[ 223.052711] ? ncm_free_inst+0x50/0x50 [usb_f_ncm]
[ 223.052711] __hrtimer_run_queues+0xac/0x440
[ 223.052712] hrtimer_run_softirq+0x8c/0xb0
[ 223.052713] __do_softirq+0xcf/0x428
[ 223.052713] asm_call_irq_on_stack+0x12/0x20
[ 223.052714] </IRQ>
[ 223.052715] do_softirq_own_stack+0x61/0x70
[ 223.052715] irq_exit_rcu+0xc1/0xd0
[ 223.052716] sysvec_apic_timer_interrupt+0x52/0xb0
[ 223.052717] asm_sysvec_apic_timer_interrupt+0x12/0x20
[ 223.052718] RIP: 0010:do_raw_spin_trylock+0x18/0x40
[ 223.052719] Code: ff ff 66 90 eb 86 66 66 2e 0f 1f 84 00 00 00 00 00 90 0f 1f 44 00 00 55 8b 07 48 89 e5 85 c0 75 29 ba 01 00 00 00 f0 0f b1 17 <75> 1e 65 8b 05 2f 77 ee 70 89 47 08 5d 65 48 8b 04 25 40 7e 01 00
[ 223.052720] RSP: 0018:ffffbc494138bda8 EFLAGS: 00000246
[ 223.052721] RAX: 0000000000000000 RBX: ffff9ee8116b4658 RCX: 0000000000000000
[ 223.052722] RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff9ee8116b4658
[ 223.052723] RBP: ffffbc494138bda8 R08: 0000000000000001 R09: 0000000000000000
[ 223.052724] R10: ffff9ee8116b4670 R11: 0000000000000000 R12: ffff9ee8116b4658
[ 223.052725] R13: ffff9ee8116b4670 R14: ffff9ee7b5c73d80 R15: ffff9ee8116b4000
[ 223.052726] _raw_spin_lock+0x3d/0x70
[ 223.052726] ? cdnsp_thread_irq_handler.cold+0x32/0x112c [cdnsp_udc_pci]
[ 223.052727] cdnsp_thread_irq_handler.cold+0x32/0x112c [cdnsp_udc_pci]
[ 223.052728] ? cdnsp_remove_request+0x1f0/0x1f0 [cdnsp_udc_pci]
[ 223.052729] ? cdnsp_thread_irq_handler+0x5/0xa0 [cdnsp_udc_pci]
[ 223.052730] ? irq_thread+0xa0/0x1c0
[ 223.052730] irq_thread_fn+0x28/0x60
[ 223.052731] irq_thread+0x105/0x1c0
[ 223.052732] ? __kthread_parkme+0x42/0x90
[ 223.052732] ? irq_forced_thread_fn+0x90/0x90
[ 223.052733] ? wake_threads_waitq+0x30/0x30
[ 223.052734] ? irq_thread_check_affinity+0xe0/0xe0
[ 223.052735] kthread+0x12a/0x160
[ 223.052735] ? kthread_park+0x90/0x90
[ 223.052736] ret_from_fork+0x22/0x30
Pawel
>
>Peter
>
>>
>> Thanks,
>> Pawel
>>
>> >>
>> >> Cc: stable@vger.kernel.org
>> >> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
>> >> Signed-off-by: Pawel Laszczak <pawell@cadence.com>
>> >> ---
>> >> drivers/usb/cdns3/cdnsp-ring.c | 7 ++++---
>> >> 1 file changed, 4 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/cdns3/cdnsp-ring.c b/drivers/usb/cdns3/cdnsp-ring.c
>> >> index 5f0513c96c04..68972746e363 100644
>> >> --- a/drivers/usb/cdns3/cdnsp-ring.c
>> >> +++ b/drivers/usb/cdns3/cdnsp-ring.c
>> >> @@ -1517,13 +1517,14 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
>> >> {
>> >> struct cdnsp_device *pdev = (struct cdnsp_device *)data;
>> >> union cdnsp_trb *event_ring_deq;
>> >> + unsigned long flags;
>> >> int counter = 0;
>> >>
>> >> - spin_lock(&pdev->lock);
>> >> + spin_lock_irqsave(&pdev->lock, flags);
>> >>
>> >> if (pdev->cdnsp_state & (CDNSP_STATE_HALTED | CDNSP_STATE_DYING)) {
>> >> cdnsp_died(pdev);
>> >> - spin_unlock(&pdev->lock);
>> >> + spin_unlock_irqrestore(&pdev->lock, flags);
>> >> return IRQ_HANDLED;
>> >> }
>> >>
>> >> @@ -1539,7 +1540,7 @@ irqreturn_t cdnsp_thread_irq_handler(int irq, void *data)
>> >>
>> >> cdnsp_update_erst_dequeue(pdev, event_ring_deq, 1);
>> >>
>> >> - spin_unlock(&pdev->lock);
>> >> + spin_unlock_irqrestore(&pdev->lock, flags);
>> >>
>> >> return IRQ_HANDLED;
>> >> }
>> >> --
>> >> 2.25.1
>> >>
>> >
>> >--
>> >
>> >Thanks,
>> >Peter Chen
>>
>
>--
>
>Thanks,
>Peter Chen
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-25 5:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20 9:42 [PATCH] usb: cdnsp: Fix deadlock issue in cdnsp_thread_irq_handler Pawel Laszczak
2021-05-22 9:54 ` Peter Chen
2021-05-24 10:56 ` Pawel Laszczak
2021-05-25 0:53 ` Peter Chen
2021-05-25 5:11 ` Pawel Laszczak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.