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

[Kim Phillips]

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.

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

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

Kim