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
> ---
> 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.
--
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
