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@xxxxxxxxx> 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@xxxxxx> > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
