Any new comments?
-Hu Tao
> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of HU TAO-TGHK48
> Sent: Saturday, October 31, 2009 5:33 AM
> To: Venkatraman S
> 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()
>
> 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
>
--
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
