[PATCH v2 1/3] dmaengine: mxs-dma: add dma support for i.MX23/28

[Shawn Guo]

On Fri, Feb 18, 2011 at 02:22:39PM +0100, Arnd Bergmann wrote:
> On Friday 18 February 2011, Shawn Guo wrote:
> > > > +static dma_cookie_t mxs_dma_assign_cookie(struct mxs_dma_chan *mxs_chan)
> > > > +{
> > > > +   dma_cookie_t cookie = mxs_chan->chan.cookie;
> > > > +
> > > > +   if (++cookie < 0)
> > > > +           cookie = 1;
> > > > +
> > > > +   mxs_chan->chan.cookie = cookie;
> > > > +   mxs_chan->desc.cookie = cookie;
> > > > +
> > > > +   return cookie;
> > > > +}
> > > > +
> > > > +static struct mxs_dma_chan *to_mxs_dma_chan(struct dma_chan *chan)
> > > > +{
> > > > +   return container_of(chan, struct mxs_dma_chan, chan);
> > > > +}
> > > > +
> > > > +static dma_cookie_t mxs_dma_tx_submit(struct dma_async_tx_descriptor *tx)
> > > > +{
> > > > +   struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(tx->chan);
> > > > +   dma_cookie_t cookie;
> > > > +   unsigned long flags;
> > > > +
> > > > +   spin_lock_irqsave(&mxs_chan->lock, flags);
> > > > +
> > > > +   cookie = mxs_dma_assign_cookie(mxs_chan);
> > > > +
> > > > +   mxs_dma_enable_chan(mxs_chan);
> > > > +
> > > > +   spin_unlock_irqrestore(&mxs_chan->lock, flags);
> > > > +
> > > > +   return cookie;
> > > > +}
> > > 
> > > Just like the MMC driver, you use spin_lock_irqsave() in the functions that
> > > are called by other drivers, but then don't actually lock in the interrupt
> > > handler. This is clearly wrong, as I explained before:
> > > 
> > > The interrupt handler may already be running on another CPU, so it
> > > is not at all serialized with this code.
> > > 
> > I may be wrong on this point, but spin_lock_irqsave/irqrestore pair
> > is only for the protection you described?  I learnt that the pair
> > should be used to protect the critical region, where I'm not sure
> > if it's called from interrupt context.  Here I'm intending to protect
> > the dma registers accessed in mxs_dma_enable_chan.
> 
> To generalize this further, a lock should protect specific data,
> not portions of the code, and ideally you should document
> exactly what you protect near the definition of the lock.
> 
> If you use the lock to protect all the DMA registers, you should use
> it consistently, including inside of the interrupt handler, where
> you access some other registers.
> 
> If you can prove that the registers used in the IRQ handler are
> totally independent of the other ones, you don't need to use the
> lock there, but then I'd advise documenting your assumptions.
> 
When I change code to use readl/writel pair, I found the register
protection in mxs_dma_enable_chan is actually unnecessary, because
the registers are either channel specific or get set/clear address
to avoid read-modify-write operation.

So I'm removing the locking there.

> > Thanks for teaching.  I intend to take the fix that Russell offered.
> > (Thanks, Russell).  Does that look good to you, or spin_lock_bh
> > version should be used than spin_lock_irqsave?
> 
> Russell's change looks correct to me.
> 
> Regarding the choice between spin_lock_bh and spin_lock_irqsave,
> I always prefer to use the one that expresses best to the reader
> what the intention is. spin_lock_irqsave() is stronger than
> spin_lock_bh(), and can always be used in its place.
> 
> A short form of the strict rules (there are better documentations
> out there) is:
> 
> * If all users are outside of interrupt or tasklet context,
>   use a bare spin_lock().
> 
> * If one user is in a tasklet context, use spin_lock() inside
>   the tasklet, but spin_lock_bh() outside, to prevent the
>   tasklet from interrupting the critical section. Code that
>   can be called in either tasklet or regular context needs
>   to use spin_lock_bh() as well.
> 
> * If one user is in interrupt context, use spin_lock() inside
>   of the interrupt handler, but spin_lock_irq() in tasklet
>   and normal context (not spin_lock_irqsave()), to prevent
>   the interrupt from happening during the critical section.
> 
> * Use spin_lock_irqsave() only for functions that can be called
>   in either interrupt or non-interrupt context. Most drivers
>   don't need this at all.
> 
> The simplified rule would be to always use spin_lock_irqsave(),
> because that does not require you to understand what you are doing.
> 
> My position is that it is better to use the stricter rules, because
> that documents that you actually do understand what you are doing ;-)
> It's also slightly more efficient, because it avoids having to
> save the interrupt status in a variable.
> 
Thanks for this document.  It helps.

I'm changing the tasklet to have callback only, and move others into
interrupt handler, so that I can remove the locking completely from
the driver.

-- 
Regards,
Shawn