Re: [PATCH] init/Kconfig: Add ENDIAN attributes for all architectures using

[Paul Gortmaker]

[Re: [PATCH] init/Kconfig: Add ENDIAN attributes for all architectures using] On 01/09/2014 (Mon 10:01) H. Peter Anvin wrote:

> On 09/01/2014 09:08 AM, Paul Gortmaker wrote:
> >>>
> >> diff --git a/init/Kconfig b/init/Kconfig
> >> index ac033c3..f301cc8 100644
> >> --- a/init/Kconfig
> >> +++ b/init/Kconfig
> >> @@ -23,6 +23,12 @@ config CONSTRUCTORS
> >>  config IRQ_WORK
> >>  	bool
> >>  
> >> +config CPU_LITTLE_ENDIAN
> >> +	bool
> >> +
> >> +config CPU_BIG_ENDIAN
> >> +	bool
> > 
> > Perhaps you should take a cursory look at what already exists in tree
> > before blindly trying to add more to it?
> > 
> > $ git grep CPU_BIG_ENDIAN | wc -l
> > 88
> > 
> 
> The whole point of this patchset is to make these already widely-used
> options universal across the tree.

OK, but that was not at _all_ what I thought when looking at this...

Instead I saw a well intentioned, but perhaps not fully thought out
attempt at fixing a largely irrelevant randconfig/allmodconfig of a
1990's vintage ISDN driver coming from that x86-only era, built against
an architecture that will never use or support it (microblaze).

In today's world, we'd probably not accept a new ethernet driver or
filesystem if it was incapable of handling both BE and LE (exception
being SoC ethernet physically bound to one specific CPU, of course.)
So the justification given in the commit log for expanding the scope to
better deal with the stuff found in ISDN and the like was questionable.

Secondly, I don't think it is well known that Kbuild will tolerate
multiply defined symbols of the exact same name, and since that isn't
mentioned in the commit log (as documented and/or tested), I envisioned
this breaking powerpc and other arch who already define one (or both)
of these two.  I found multi-define _is_ documented as supported in
Documentation/kbuild but I still wonder how much it is used and how
well it handles things like in powerpc, where one of them (LE?) also
lists a "select ... " line and help text under the bool.

So if we want to do this, I'd suggest a commit log that really gets
to the intended point; something along the lines of:

  --------------

  Currently we have some architectures defining their own bool for
  CPU_LITTLE_ENDIAN and/or CPU_BIG_ENDIAN.  As of 3.17-rc3 we have:

    CPU_BIG_ENDIAN: arc, arm, arm64, c6x, mips, powerpc, sh
    CPU_LITTLE_ENDIAN: m32r, mips, powerpc, sh

  Note that the scope does not cover all arch, which reduces the utility
  value of these items in generic and/or arch independent code, as
  neither may be defined, making tests on their values inconclusive.

  To fix the above, the goal is to make both items present in an arch
  independent Kconfig.  We can do this immediately without having to
  simultaneously touch the existing arch bool definitions because...

  This change was tested on a powerpc LE config since it has help text
  and a select line, and the behaviour of both before and after the
  change was found to be ...

  --------------

Also, having the suggested change Cc'd to linux-arch would be sensible,
I think.  And one might want to make it a series, showing the follow on
arch specific commits you have planned that "select" an endian, since
the maintainers may want evidence it will be carried to completion and
they also may have something additional to say (as in the case of arch/arm
where there is already CPU_ENDIAN_BE8 and CPU_ENDIAN_BE32 Kconfig items).

Paul.
--

> 
> 	-hpa
> 
> 
--
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/