Re: [patch 2.6.31-rc7 2/3] spi: McSPI saves CHCONFx too

[Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>]

On Wed, 2 Sep 2009 08:53:16 -0700 David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:

> On Tuesday 01 September 2009, Andrew Morton wrote:
> > On Thu, 27 Aug 2009 20:21:11 -0700
> > David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > 
> > > From: Tero Kristo <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > > 
> > > Previous restore was lazy and only restored CHxCONF when it was needed by a
> > > specific chip select. This could cause occasional errors on an SPI bus where
> > > multiple chip selects are in use.
> > > 
> > > Signed-off-by: Tero Kristo <tero.kristo-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> > > Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> > > ---
> > >  drivers/spi/omap2_mcspi.c |   16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > --- a/drivers/spi/omap2_mcspi.c
> > > +++ b/drivers/spi/omap2_mcspi.c
> > > @@ -134,6 +134,7 @@ struct omap2_mcspi_cs {
> > >  	void __iomem		*base;
> > >  	unsigned long		phys;
> > >  	int			word_len;
> > > +	struct list_head	node;
> > >  	/* Context save and restore shadow register */
> > >  	u32			chconf0;
> > >  };
> > > @@ -145,6 +146,7 @@ struct omap2_mcspi_regs {
> > >  	u32 sysconfig;
> > >  	u32 modulctrl;
> > >  	u32 wakeupenable;
> > > +	struct list_head cs;
> > >  };
> > 
> > Which locking protects these new lists?
> 
> One's the head, one's the instance ... the list is used in
> activation and deactivation paths, so the same locks which
> already protect those paths.
> 

Which locks are those?

<looks for 10 seconds>

omap2_mcspi_work() calls omap2_mcspi_enable_clocks() under mcspi->lock

omap2_mcspi_setup() calls omap2_mcspi_enable_clocks() not under mcspi->lock

The locking here is insufficiently clear and possibly buggy.

Please revisit.  If it's missing, add it.  If it's unneeded, document
it.  If it's actually present and correct, document it.


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day 
trial. Simplify your report design, integration and deployment - and focus on 
what you do best, core application coding. Discover what's new with 
Crystal Reports now.  http://p.sf.net/sfu/bobj-july