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

[David Gibson]

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.

> 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.

> 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.

> >  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.  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.

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