Hi, Venkatraman
Agree with you. And just found omap_free_dma() need to be fixed too.
New patch attached.
>From 7aa22ece591365c2dcd73c799d93781497322ff1 Mon Sep 17 00:00:00 2001
From: Tao Hu <taohu@xxxxxxxxxxxx>
Date: Fri, 30 Oct 2009 16:24:25 -0500
Subject: [PATCH] Fix race condition in omap dma driver
The bug could cause irq enable bit of one DMA channel is
cleared/set unexpectedly when 2 (or more) drivers are calling
omap_request_dma()/omap_free_dma() simultaneously
Signed-off-by: Fei Yang <AFY095@xxxxxxxxxxxx>
Signed-off-by: Tao Hu <taohu@xxxxxxxxxxxx>
---
arch/arm/plat-omap/dma.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
index cd53b28..5767899 100755
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -673,13 +673,16 @@ static inline void disable_lnk(int lch)
static inline void omap2_enable_irq_lch(int lch)
{
u32 val;
+ unsigned long flags;
if (!cpu_class_is_omap2())
return;
+ spin_lock_irqsave(&dma_chan_lock, flags);
val = dma_read(IRQENABLE_L0);
val |= 1 << lch;
dma_write(val, IRQENABLE_L0);
+ spin_unlock_irqrestore(&dma_chan_lock, flags);
}
int omap_request_dma(int dev_id, const char *dev_name,
@@ -788,10 +791,13 @@ void omap_free_dma(int lch)
if (cpu_class_is_omap2()) {
u32 val;
+
+ spin_lock_irqsave(&dma_chan_lock, flags);
/* Disable interrupts */
val = dma_read(IRQENABLE_L0);
val &= ~(1 << lch);
dma_write(val, IRQENABLE_L0);
+ spin_unlock_irqrestore(&dma_chan_lock, flags);
/* Clear the CSR register and IRQ status register */
dma_write(OMAP2_DMA_CSR_CLEAR_MASK, CSR(lch));
--
1.5.4.3
> -----Original Message-----
> From: Venkatraman S [mailto:svenkatr@xxxxxxxxx]
> Sent: Saturday, October 31, 2009 1:22 AM
> To: HU TAO-TGHK48
> Cc: linux-omap@xxxxxxxxxxxxxxx; Tony Lindgren; Yang
> Fei-AFY095; jon-hunter@xxxxxx; Zhou Ming-a17711; Ye Yuan.Bo-A22116
> Subject: Re: [PATCH] Fix race condition in omap_request_dma()
>
> On Fri, Oct 30, 2009 at 9:00 AM, HU TAO-TGHK48
> <taohu@xxxxxxxxxxxx> wrote:
> > From a36dac7ee6140ffa23f0adc024964aaf3e266e5f Mon Sep 17
> 00:00:00 2001
> > From: Tao Hu <taohu@xxxxxxxxxxxx>
> > Date: Thu, 29 Oct 2009 17:17:21 -0500
> > Subject: [PATCH] Fix race condition in omap_request_dma()
> >
> > The bug could cause irq enable bit of one DMA channel is cleared
> > unexpectedly when 2 (or more) drivers are calling
> > omap_request_dma() simultaneously
> >
> > Signed-off-by: Fei Yang <AFY095@xxxxxxxxxxxx>
> > Signed-off-by: Tao Hu <taohu@xxxxxxxxxxxx>
> > ---
> > arch/arm/plat-omap/dma.c | 2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/dma.c
> b/arch/arm/plat-omap/dma.c index
> > cd53b28..6895484 100755
> > --- a/arch/arm/plat-omap/dma.c
> > +++ b/arch/arm/plat-omap/dma.c
> > @@ -749,11 +749,13 @@ int omap_request_dma(int dev_id, const char
> > *dev_name,
> > }
> >
> > if (cpu_class_is_omap2()) {
> > + spin_lock_irqsave(&dma_chan_lock, flags);
> > omap2_enable_irq_lch(free_ch);
> > omap_enable_channel_irq(free_ch);
> > /* Clear the CSR register and IRQ status register */
> > dma_write(OMAP2_DMA_CSR_CLEAR_MASK, CSR(free_ch));
> > dma_write(1 << free_ch, IRQSTATUS_L0);
> > + spin_unlock_irqrestore(&dma_chan_lock, flags);
> > }
>
> Nice catch. I think the lock needs to be applied only for the
> global registers which are changed using the read - update -
> write method.
> The dma_write to per channel configuration registers need not
> be locked.
> Hence the lock is better placed "within" omap2_enable_irq_lch
> function. That way, if it's reused, the locking would also be
> in place.
> Thank you !
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
