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

[Peter Chen <peter.chen@kernel.org>]

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