| Message ID | 1253799417-19888-1-git-send-email-swami.iyer@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Swaminathan S wrote: > Race condition exists between the cppi_interrupt handler and davinci_interrupt > handler w.r.t completing a TX IO. Since DM646x has seperate DMA and USB > Endpoint interrupts cppi_interrupt handler needs to hold the lock while > operating on the endpoint. > > Signed-off-by: Swaminathan S <swami.iyer@ti.com> NAK. davinci_interrupt() calls cppi_interrupt() with this lock already held. We probably need to move the lock acquisition below that call first. WBR, Sergei
Sergei,
Pl. read through the patch description (and the associated code) before expressing your opinion. This helps to avoid the unnecessary traffic for all of us.
As in the patch description DM646x has 2 separate interrupt relating to USB subsystem. One handles DMA interrupts while other handles non-DMA interrupt.
Davinci_interrupt() handler handles non-DMA interrupts while cppi_interrupt() handler handles the DMA interrupts.
In the davinci_interrupt() handler refer to the below code snippet
---------------------------------------------------------------------------
cppi = container_of(musb->dma_controller, struct cppi, controller);
if (is_cppi_enabled() && musb->dma_controller && !cppi->irq)
retval = cppi_interrupt(irq, __hci);
----------------------------------------------------------------------------
From the above snippet you can see if the DMA irq is subscribed then davinci_interrupt() does not call the cppi_interrupt() handler. This is the case in relation to DM646x platform.
I expect you to look in the code, understand the patch and then provide you observation in future and do not expect others to do this for you.
Regards
swami
-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com]
Sent: Thursday, September 24, 2009 7:01 PM
To: Subbrathnam, Swaminathan
Cc: linux-usb@vger.kernel.org; davinci-linux-open-source@linux.davincidsp.com
Subject: Re: [PATCH] [MUSB] Fix for crash in DM646x USB when (CPPI)DMA is enabled.
Swaminathan S wrote:
> Race condition exists between the cppi_interrupt handler and davinci_interrupt
> handler w.r.t completing a TX IO. Since DM646x has seperate DMA and USB
> Endpoint interrupts cppi_interrupt handler needs to hold the lock while
> operating on the endpoint.
>
> Signed-off-by: Swaminathan S <swami.iyer@ti.com>
NAK. davinci_interrupt() calls cppi_interrupt() with this lock
already held. We probably need to move the lock acquisition below that
call first.
WBR, Sergei
Hello. Subbrathnam, Swaminathan wrote: > Sergei, > Pl. read through the patch description (and the associated code) before expressing your opinion. This helps to avoid the unnecessary traffic for all of us. > > As in the patch description DM646x has 2 separate interrupt relating to USB subsystem. One handles DMA interrupts while other handles non-DMA interrupt. > I know -- because I was the author of the cppi_interrupt(). :-) Unfortunately I have completely overlooked the locking problem (and I yet had some strange things happening with the USB storage into which I hadn't looked enough it seems). :-< > Davinci_interrupt() handler handles non-DMA interrupts while cppi_interrupt() handler handles the DMA interrupts. > > In the davinci_interrupt() handler refer to the below code snippet > > --------------------------------------------------------------------------- > cppi = container_of(musb->dma_controller, struct cppi, controller); > if (is_cppi_enabled() && musb->dma_controller && !cppi->irq) > retval = cppi_interrupt(irq, __hci); > ---------------------------------------------------------------------------- > > From the above snippet you can see if the DMA irq is subscribed then davinci_interrupt() does not call the cppi_interrupt() handler. This is the case in relation to DM646x platform. > Sigh. Look at the code just above that snippet, i.e. spiun_lock_irqsave(&musb->lock, flags), and think about *non-DM646x* case. You're causing the lock recursion in this case. > I expect you to look in the code, understand the patch and then provide you observation in future and do not expect others to do this for you. > > > Regards > swami > > -----Original Message----- > From: Sergei Shtylyov [mailto:sshtylyov@ru.mvista.com] > Sent: Thursday, September 24, 2009 7:01 PM > To: Subbrathnam, Swaminathan > Cc: linux-usb@vger.kernel.org; davinci-linux-open-source@linux.davincidsp.com > Subject: Re: [PATCH] [MUSB] Fix for crash in DM646x USB when (CPPI)DMA is enabled. > > Swaminathan S wrote: > >> Race condition exists between the cppi_interrupt handler and davinci_interrupt >> handler w.r.t completing a TX IO. Since DM646x has seperate DMA and USB >> Endpoint interrupts cppi_interrupt handler needs to hold the lock while >> operating on the endpoint. >> >> Signed-off-by: Swaminathan S <swami.iyer@ti.com> >> > > NAK. davinci_interrupt() calls cppi_interrupt() with this lock > already held. We probably need to move the lock acquisition below that > call first. > WBR, Sergei
diff --git a/drivers/usb/musb/cppi_dma.c b/drivers/usb/musb/cppi_dma.c index c3577bb..a3555e5 100644 --- a/drivers/usb/musb/cppi_dma.c +++ b/drivers/usb/musb/cppi_dma.c @@ -1154,7 +1154,9 @@ irqreturn_t cppi_interrupt(int irq, void *dev_id) struct musb_hw_ep *hw_ep = NULL; u32 rx, tx; int i, index; + unsigned long flags; + spin_lock_irqsave(&musb->lock, flags); cppi = container_of(musb->dma_controller, struct cppi, controller); tibase = musb->ctrl_base; @@ -1285,6 +1287,8 @@ irqreturn_t cppi_interrupt(int irq, void *dev_id) /* write to CPPI EOI register to re-enable interrupts */ musb_writel(tibase, DAVINCI_CPPI_EOI_REG, 0); + spin_unlock_irqrestore(&musb->lock, flags); + return IRQ_HANDLED; }
Race condition exists between the cppi_interrupt handler and davinci_interrupt handler w.r.t completing a TX IO. Since DM646x has seperate DMA and USB Endpoint interrupts cppi_interrupt handler needs to hold the lock while operating on the endpoint. Signed-off-by: Swaminathan S <swami.iyer@ti.com> --- drivers/usb/musb/cppi_dma.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)