>
>
>On 21-05-20 11:42:24, Pawel Laszczak wrote:
>> From: Pawel Laszczak <pawell@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxx
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence USBSSP DRD Driver")
>> Signed-off-by: Pawel Laszczak <pawell@xxxxxxxxxxx>
>> ---
>> 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
