[U-Boot] [PATCH 00/32] Initial sparse fix series

Fri Oct 19 02:43:24 CEST 2012

On Thu, Oct 18, 2012 at 05:30:22PM -0500, Kim Phillips wrote:
> On Thu, 18 Oct 2012 23:11:12 +1100
> David Gibson <david at gibson.dropbear.id.au> wrote:
> 
> > On Wed, Oct 17, 2012 at 08:19:23PM -0400, Jerry Van Baren wrote:
> > > Hi David, Jon,
> > > 
> > > Kim Phillips created a series of patches to change variable declarations
> > > that are big endian to be __be32/__be64.  Since the device tree is
> > > defined to be big endian, he created a patch to mark the appropriate
> > > libfdt entities as __be*.
> > 
> > So, in general I approve the idea of having endian annotations.
> > Obviously we do need to make sure it doesn't break things.
> 
> cool.
> 
> > > On 10/16/2012 08:28 PM, Kim Phillips wrote:
> > > > This 32-patch series only begins to address making u-boot source more
> > > > 'sparseable,' or sparse-clean, ultimately to catch type, address space,
> > > > and endianness mismatches and generally improve code quality. E.g., in this
> > > > initial dose whose main purpose is to reduce the output volume to workable
> > > > levels, a couple of endianness bugs are found and fixed in
> > > > of_bus_default_translate() and fdt_get_base_address().  See [PATCH 14/32]
> > > > common/fdt_support.c: sparse fixes.
> > > 
> > > Upside: This is very good for identifying endian errors early.
> > > Downside: It could break/complicate non-linux uses of libfdt.
> > 
> > This shouldn't be an inherent problem - we can always have the default
> > behaviour be that be32 etc. are #defined to be uint32_t, and we only
> > turn on "real" annotations when we have the right setup.
> > 
> > It does complicate things a bit, but I think it should be manageable.
> > 
> > I'd much prefer to see this done against the upstream dtc/libfdt, of
> > course, rather than the u-boot copy.
> 
> understood.
> 
> > > What are your thoughts on this quest?
> > > 
> > > [snip]
> > > 
> > > > Note that there are two libfdt dependencies:
> > > 
> > > [snipped #1, u-boot-fdt dependency, NBD]
> > > 
> > > > 2. potential upstream dtc change dependencies, due to having to attribute base
> > > > device tree header types to __be32 in include/libfdt.  See patch 19/32
> > > > "include/fdt.h: sparse fixes".  It is unknown whether such changes would
> > > > be welcome to dtc (but there's a way to find out :).
> > > 
> > > [snip]
> > > 
> > > > Build-tested in both endians :).  Please help test.
> > > > 
> > > > Thanks,
> > > > 
> > > > Kim
> > > 
> > > Below is the actual patch for reference (it was in a separate email).
> > > The impact in terms of changed lines is pretty small.  I'm not sure how
> > > this impacts non-linux / non-gcc systems since the sparse checker comes
> > > from a linux background and uses gcc extensions.
> > > 
> > > Possibly this could be handled a definition:
> > > 
> > > #ifndef _LINUX_TYPES_H
> > > typedef uint32_t __be32
> > > typedef uint64_t __be64
> > > #endif
> > 
> > Yes, something along those lines would be appropriate, although I
> > think that condition isn't right.
> 
> right, we don't want all uint32_t's to be one endian, we just want
> fdt32 types to have big endian annotation when being checked by a
> checker such as sparse.

Right.

> > > >  include/fdt.h         | 33 +++++++++++++++++----------------
> > > >  include/fdt_support.h |  2 ++
> > > >  include/libfdt.h      |  4 ++--
> > > >  lib/libfdt/fdt.c      |  2 +-
> > > >  lib/libfdt/fdt_ro.c   |  2 +-
> > > >  lib/libfdt/fdt_rw.c   |  4 ++--
> > > >  lib/libfdt/fdt_sw.c   |  4 ++--
> > > >  lib/libfdt/fdt_wip.c  |  2 +-
> > > >  8 files changed, 28 insertions(+), 25 deletions(-)
> > > > 
> > > > diff --git a/include/fdt.h b/include/fdt.h
> > > > index c51212e..1b7f044 100644
> > > > --- a/include/fdt.h
> > > > +++ b/include/fdt.h
> > > > @@ -2,40 +2,41 @@
> > > >  #define _FDT_H
> > > >  
> > > >  #ifndef __ASSEMBLY__
> > > > +#include <asm/byteorder.h>
> > 
> > This change, however, is not acceptable.  libfdt is supposed to
> > compile in a wide range of environments (such as bootloaders and
> > firmwares) which may be very different from a typical Unix userland
> > environment.  As such all the headers are built to have minimal
> > dependencies on external libraries.  The byteorder headers are, alas,
> > horribly non-portable even amongst otherwise similar Unix systems, so
> > right out for libfdt.
> > 
> > In fact, the way this is supposed to work is that the *only* external
> > header directly included by the fdt headers is libfdt_env.h.  It is
> > supplied by the surrounding build environment and is responsible for
> > providing the minimal things that libfdt does require.  Roughly
> > speaking that's: stdint like types, some byteswap functions and a some
> > string.h prototypes - exactly what's required should be better
> > documented than it currently is.  libfdt_env.h can do this either by
> > including appropriate pre-existing headers from the environment, or by
> > directly defining the required things, whichever makes sense.  The
> > libfdt_env.h which is shipped with libfdt is essentially just an
> > example, for use in the environment of "POSIX like userspace".
> > 
> > Anyway, I think the right way to handle this is to decide on a name
> > like __FDT_ANNOTATIONS__ or something.
> 
> this is named __CHECKER__ in linux and u-boot.  Ifdef __CHECKER__,
> then the code is being parsed by a checker.

Ok, that name works for me.

> >  In fdt.h we have
> > 
> > #ifndef __FDT_ANNOTATIONS__
> > typedef uint32_t fdt32_t;
> > /* etc */
> > #endif
> > 
> > And libfdt is reworked to use these fdt32_t and so forth types.  Then,
> > by default everything should compile fine, but with no extra checking.
> > Environments which have sparse or a similar checker available can
> > #define __FDT_ANNOTATIONS__ from their version of libfdt_env.h in
> > which case they would also be required to define the annotated integer
> > types as necessary for their checker.
> > 
> > For convenience on Linux systems we can have the "default"
> > libfdt_env.h shipped with libfdt go either way depending on SPARSE,
> > LINUX_TYPES or whatever suitable pre-existing preprocessor tags we can
> > locate.
> 
> understood, that's how I envisaged things going too.
> 
> Let me see what I can do.

Sounds good.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson