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 <[email protected]> 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 <[email protected]> Signed-off-by: Tao Hu <[email protected]> --- 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:[email protected]] > Sent: Saturday, October 31, 2009 1:22 AM > To: HU TAO-TGHK48 > Cc: [email protected]; Tony Lindgren; Yang > Fei-AFY095; [email protected]; 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 > <[email protected]> wrote: > > From a36dac7ee6140ffa23f0adc024964aaf3e266e5f Mon Sep 17 > 00:00:00 2001 > > From: Tao Hu <[email protected]> > > 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 <[email protected]> > > Signed-off-by: Tao Hu <[email protected]> > > --- > > 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
