On Tue, Oct 30, 2012 at 04:57:54PM -0500, Kim Phillips wrote:
Projects such as linux and u-boot run sparse on libfdt. libfdt contains the notion of endianness via usage of endian conversion functions such as fdt32_to_cpu. As such, in order to pass endian checks, libfdt has to annotate its fdt variables as big endian. This patch does that ifdef __CHECKER__ (a symbol sparse defines), for two new fdt types: fdt32_t and fdt64_t, and subsequently silences warnings emitted by sparse when parsing libfdt.
Signed-off-by: Kim Phillips kim.phillips@freescale.com
note: wasn't sure whether to introduce the new fdt32 types, or just have libfdt use __be32 directly.
I prefer having the fdt32 types, to match the cpu_to_fdt32() and so forth functions we already use.
So I like this in principle, but a couple of nits.
First, I'd really like to see an accompanying patch that adds targets to the dtc makefiles to run sparse over the sources. I couldn't really test this, because I couldn't figure out quite what options I needed to invoke sparse with to get it to work properly.
And I'd also like to see the default libfdt_env.h updated to supply the necessary sparse stuff - including the necessary __force casts in its byteswap functions.
[snip]
diff --git a/libfdt/fdt.h b/libfdt/fdt.h index 48ccfd9..0d9c856 100644 --- a/libfdt/fdt.h +++ b/libfdt/fdt.h @@ -3,46 +3,54 @@
#ifndef __ASSEMBLY__
+#ifdef __CHECKER__
So, I'd prefer not to use __CHECKER__ directly here. I'd rather we defined a new specific symbol, that libfdt_env.h can set based on __CHECKER__ if it wants. Let's say _FDT_SPARSE The reason is because..
+typedef __be32 fdt32_t; +typedef __be64 fdt64_t;
..just running sparse does *not* immediately give you __be32 and __be64 types - those have to be defined in terms of __attribute__((bitwise)) and whatnot. Your uboot env probably does that already, but the default libfdt_env.h certainly doesn't. Effectively _FDT_SPARSE is sayint two things, first that we're compiling under sparse, but also that we have suitably defined endian types in the environment.
Actually, given that libfdt_env.h is already required to provide the cpu_to_fdt32() and so forth macros, I think it's slightly neater to just require it to directly supply the fdt32_t etc. types when it defines _FDT_SPARSE as well, rather than defining them in terms of __beXX here then in terms of the attributes in the environment.