[alsa-devel] [PATCH] OMAP: McBSP: Do not use extensive spin locks for dma_op_mode

[Eero Nurkkala]

On Wed, 2009-10-28 at 06:52 +0100, Ujfalusi Peter (Nokia-D/Tampere)
wrote:
> On Tuesday 27 October 2009 16:00:00 ext Jarkko Nikula wrote:
> > On Tue, 27 Oct 2009 13:07:23 +0200
> > 
> > Eero Nurkkala <ext-eero.nurkkala at nokia.com> wrote:
> > > Otherwise, that spinlocking is highly unnecessary and things are
> > > far better without than with it. The only case it could be useful
> > > is in SMPs, but OMAPs are not such quite yet - and when they
> > > are, things will need to be re-though anyway.
> > 
> > Following commit is suggesting that mcbsp code *must* be SMP safe
> > already now:
> > 
> > commit a5b92cc348299c20be215b9f4b50853ecfbf3864
> > Author: Syed Rafiuddin <rafiuddin.syed at ti.com>
> > Date:   Tue Jul 28 18:57:10 2009 +0530
> > 
> >     ARM: OMAP4: Add McBSP support
> 
> Yeah, but I think this locking issue has nothing to do with SMP safe or not.
> On playback start in omap_mcbsp_request the mcbsp->free is cleared.
> Further modification to the dma_op_mode in dma_op_mode_store is not allowed if 
> the mcbsp port is in use, thus the dma_op_mode is protected against change while 
> the port is in use (ensuring that the mode is same in omap34xx_mcbsp_request and 
> omap_mcbsp_get_dma_op_mode functions). This alone makes the use of spinlock 
> around the dma_op_mode unnecessary.
> 

Yeah, maybe I took the SMP safeness into play without looking any code,
my bad =) I was remembering a different version of this McBSP thing, now
that I looked into it, it looked different.

Right, I reviewed the code, and it was first looking really bad at
sound/soc/omap/omap-mcbsp.c, where it calls omap_mcbsp_get_dma_op_mode()
from different places. However, it's not an issue because in:
arch/arm/plat-omap/mcbsp.c : dma_op_mode_store(),
the dma_op_mode is written only if the mcbsp is unoccupied. So it is SMP
safe.

..and a single read is always atomic, so this is buggy code:

301         spin_lock_irq(&mcbsp->lock);
302         dma_op_mode = mcbsp->dma_op_mode;
303         spin_unlock_irq(&mcbsp->lock);
304 
305         return dma_op_mode;

The spinlocks are unnecessary. In the above example, you get the same
with just "return mcbsp->dma_op_mode;"

-> Peter's patch is a good cleanup.