Re: [PATCH] Fix race condition in omap_request_dma()

[Tony Lindgren <tony@atomide.com>]

* HU TAO-TGHK48 <taohu@motorola.com> [091106 08:45]:
> Hi, Tony
> 
> Can we merge this patch in?

Yeah, let's try to get that in as a fix.

Tony


> Thanks.
>  
> -Hu Tao
> 
> > -----Original Message-----
> > From: svenkatr@gmail.com [mailto:svenkatr@gmail.com] On 
> > Behalf Of Venkatraman S
> > Sent: Friday, November 06, 2009 8:23 PM
> > To: HU TAO-TGHK48
> > Cc: linux-omap@vger.kernel.org; Tony Lindgren; Yang 
> > Fei-AFY095; jon-hunter@ti.com; Zhou Ming-a17711; Ye Yuan.Bo-A22116
> > Subject: Re: [PATCH] Fix race condition in omap_request_dma()
> > 
> > On Fri, Nov 6, 2009 at 12:45 AM, HU TAO-TGHK48 
> > <taohu@motorola.com> wrote:
> > >
> > > Any new comments?
> > >
> > > -Hu Tao
> > >
> > >
> > I am happy with this version.
> > Regards,
> > 
> > Venkatraman.
> > 
> > >
> > >> -----Original Message-----
> > >> From: linux-omap-owner@vger.kernel.org 
> > >> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of HU 
> > TAO-TGHK48
> > >> Sent: Saturday, October 31, 2009 5:33 AM
> > >> To: Venkatraman S
> > >> Cc: linux-omap@vger.kernel.org; Tony Lindgren; Yang Fei-AFY095; 
> > >> jon-hunter@ti.com; 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 <taohu@motorola.com>
> > >> 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@motorola.com>
> > >> Signed-off-by: Tao Hu  <taohu@motorola.com>
> > >> ---
> > >>  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@gmail.com]
> > >> > Sent: Saturday, October 31, 2009 1:22 AM
> > >> > To: HU TAO-TGHK48
> > >> > Cc: linux-omap@vger.kernel.org; Tony Lindgren; Yang Fei-AFY095; 
> > >> > jon-hunter@ti.com; 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@motorola.com>
> > >> > wrote:
> > >> > > From a36dac7ee6140ffa23f0adc024964aaf3e266e5f Mon Sep 17
> > >> > 00:00:00 2001
> > >> > > From: Tao Hu <taohu@motorola.com>
> > >> > > 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@motorola.com>
> > >> > > Signed-off-by: Tao Hu  <taohu@motorola.com>
> > >> > > ---
> > >> > >  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@vger.kernel.org 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html