On Thu, 18 Oct 2012 23:11:12 +1100 David Gibson david@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]
- 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