On 21-05-24 10:56:23, Pawel Laszczak wrote: > > > > > >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 %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@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 > -- Thanks, Peter Chen
