RE: [PATCH] usb: cdnsp: Fix deadlock issue in cdnsp_thread_irq_handler

[Pawel Laszczak <pawell@cadence.com>]

>
>
>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