Re: frv build failure in mainline kernel

[Matthias Kaehlcke]

El Tue, May 16, 2017 at 11:22:07AM -0700 Guenter Roeck ha dit:

> On Tue, May 16, 2017 at 10:58:20AM -0700, Matthias Kaehlcke wrote:
> > El Tue, May 16, 2017 at 10:21:30AM -0700 Guenter Roeck ha dit:
> > 
> > > On Tue, May 16, 2017 at 05:04:32PM +0100, David Howells wrote:
> > > > Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> > > > 
> > > > > Turns out not here because ____cacheline_aligned_in_smp includes
> > > > > __page_aligned_data which also declares the section, at least on x86.
> > > > 
> > > > If there's any sort of section specification, that should suffice, I think.
> > > > The problem might come that jiffies and jiffies_64 don't coincide at the same
> > > > address because FRV is BE not LE.
> > > > 
> > > > It ought to be possible to make jiffies 64-bit on FRV since it has double-word
> > > > instructions that can load/store aligned 64-bit values atomically to/from a
> > > > register pair.  That might require some compiler magic, though.  I'll have to
> > > > try and work out if that's possible.
> > > > 
> > > I tried:
> > > 
> > > diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
> > > index 36872fbb815d..26d0655c4422 100644
> > > --- a/include/linux/jiffies.h
> > > +++ b/include/linux/jiffies.h
> > > @@ -64,13 +64,15 @@ extern int register_refined_jiffies(long clock_tick_rate);
> > >  /* TICK_USEC is the time between ticks in usec assuming fake USER_HZ */
> > >  #define TICK_USEC ((1000000UL + USER_HZ/2) / USER_HZ)
> > >  
> > > +#define __jiffy_data  __attribute__((section(".data")))
> > > +
> > >  /*
> > >   * The 64-bit value is not atomic - you MUST NOT read it
> > >   * without sampling the sequence number in jiffies_lock.
> > >   * get_jiffies_64() will do this for you as appropriate.
> > >   */
> > > -extern u64 __cacheline_aligned_in_smp jiffies_64;
> > > -extern unsigned long volatile __cacheline_aligned_in_smp jiffies;
> > > +extern u64 __cacheline_aligned_in_smp __jiffy_data jiffies_64;
> > > +extern unsigned long volatile __cacheline_aligned_in_smp __jiffy_data jiffies;
> > > 
> > > This is what I get when building defconfig:
> > > 
> > >   CC      arch/x86/kernel/asm-offsets.s
> > > In file included from ./include/linux/ktime.h:25:0,
> > >                  from ./include/linux/rcupdate.h:46,
> > >                  from ./include/linux/srcu.h:33,
> > >                  from ./include/linux/notifier.h:15,
> > >                  from ./include/linux/memory_hotplug.h:6,
> > >                  from ./include/linux/mmzone.h:757,
> > >                  from ./include/linux/gfp.h:5,
> > >                  from ./include/linux/slab.h:14,
> > >                  from ./include/linux/crypto.h:24,
> > >                  from arch/x86/kernel/asm-offsets.c:8:
> > > ./include/linux/jiffies.h:74:52: error: section of âjiffies_64â conflicts with previous declaration
> > >  extern u64 __cacheline_aligned_in_smp __jiffy_data jiffies_64;
> > >                                                     ^
> > > ./include/linux/jiffies.h:75:71: error: section of âjiffiesâ conflicts with previous declaration
> > >  extern unsigned long volatile __cacheline_aligned_in_smp __jiffy_data jiffies;
> > >                                                                        ^
> > > Kbuild:56: recipe for target 'arch/x86/kernel/asm-offsets.s' failed
> > > make[1]: *** [arch/x86/kernel/asm-offsets.s] Error 1
> > > Makefile:1061: recipe for target 'prepare0' failed
> > > make: *** [prepare0] Error 2
> > 
> > Thanks for giving it a try!
> > 
> > What would work is this:
> > 
> > diff --git a/arch/frv/include/asm/cache.h b/arch/frv/include/asm/cache.h
> > index 2797163b8f4f..25bf4e85f582 100644
> > --- a/arch/frv/include/asm/cache.h
> > +++ b/arch/frv/include/asm/cache.h
> > @@ -20,4 +20,6 @@
> >  #define __cacheline_aligned    __attribute__((aligned(L1_CACHE_BYTES)))
> >  #define ____cacheline_aligned  __attribute__((aligned(L1_CACHE_BYTES)))
> >  
> > +#define __jiffies_arch_xyz     __attribute__((__section__(".data")))
> > +
> >  #endif
> > diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
> > index 36872fbb815d..8904eb52928d 100644
> > --- a/include/linux/jiffies.h
> > +++ b/include/linux/jiffies.h
> > @@ -70,7 +70,7 @@ extern int register_refined_jiffies(long clock_tick_rate);
> >   * get_jiffies_64() will do this for you as appropriate.
> >   */
> >  extern u64 __cacheline_aligned_in_smp jiffies_64;
> > -extern unsigned long volatile __cacheline_aligned_in_smp jiffies;
> > +extern unsigned long volatile __cacheline_aligned_in_smp __jiffies_arch_xyz jiffies;
> >  
> >  #if (BITS_PER_LONG < 64)
> >  u64 get_jiffies_64(void);
> > 
> > 
> > We'd only have to find a halfway decent name for __jiffies_arch_xyz.
> 
> From __jiffy_data: __jiffy_arch_data or __jiffies_arch_data.

Sounds good(ish)

> You'd also need
> 
> #ifndef __jiffy_arch_data
> #define __jiffy_arch_data
> #endif

Yep

> That assumes that __cacheline_aligned_in_smp is not defined to be part of 
> a different section for the architecture, which I guess should be acceptable
> here (but maybe asks for a respective comment).

Ack

This solution looks reasonable, I'll prepare a patch shortly.

Thanks

Matthias