Re: [RT] seqlocks: use of PICK_FUNCTION breaks kernel compile whenCONFIG_GENERIC_TIME is NOT set

[Steven Rostedt]

On Wed, 2007-10-17 at 07:48 -0700, Daniel Walker wrote:
> Steven, 
> 
> Could you include this patch, it was submitted on the list but you
> didn't get CC'd ..

Thanks, it may have been a while before I see this (/me has been nose
deep in debugging).

> > On Tue, 2007-10-16 at 10:35 +0200, Remy Bohmer wrote:
> > > Hello Daniel and Ingo,
> > > 
> > > I use 2.6.23-rt1 and the patch of Daniel below which seems to be in
> > > there breaks the compilation of the RT-kernel when CONFIG_GENERIC_TIME
> > > is NOT set.
> > > 
> > > It breaks in the do_gettimeofday(struct timeval *tv) code in the
> > > architecture specific code
> > > where there is a call to: seq = read_seqbegin_irqsave(&xtime_lock, flags);
> > > The PICK_FUNCTION implementation does not return anything, so the
> > > compile breaks here.
> > > 
> > > I am figuring out how to solve this problem nicely, but I have not
> > > found a nice solution yet. Attached I have put my hack to make it
> > > compile on ARM again, but maybe one of you can do a better proposal to
> > > fix this.
> > 
> > Here's another fix for this. I compile tested for ARM (!GENERIC_TIME) ,
> > but I didn't boot test .. Could you boot test this patch?
> > 
> > ---
> > 
> > Move the read_seqbegin() call up in the chain so the value 
> > can be returned
> > 
> > Signed-off-by: Daniel Walker <dwalker@xxxxxxxxxx>
> > 
> > ---
> >  include/linux/seqlock.h |    7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > Index: linux-2.6.23/include/linux/seqlock.h
> > ===================================================================
> > --- linux-2.6.23.orig/include/linux/seqlock.h
> > +++ linux-2.6.23/include/linux/seqlock.h
> > @@ -285,21 +285,20 @@ unsigned long __read_seqbegin_irqsave_ra
> >  	unsigned long flags;
> >  
> >  	local_irq_save(flags);
> > -	__read_seqbegin_raw(sl);
> >  	return flags;
> >  }
> >  
> >  static __always_inline unsigned long __read_seqbegin_irqsave(seqlock_t *sl)
> >  {
> > -	__read_seqbegin(sl);
> >  	return 0;
> >  }

The above should be renamed. They have nothing to do with seqlocks now.

> >  
> >  #define read_seqbegin_irqsave(lock, flags)			\
> > -do {								\
> > +({								\
> >  	flags = PICK_SEQ_OP_RET(__read_seqbegin_irqsave_raw,	\
> >  		__read_seqbegin_irqsave, lock);			\
> > -} while (0)
> > +	read_seqbegin(lock);					\
> > +})

Yes, definitely the raw and unraw functions should be called
local_irqsave_raw or something else that makes this obvious to what it
is doing.

-- Steve


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/