Re: [U-Boot] [PATCH] libfdt: introduce fdt type annotation for use by endian checkers
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
Re: [U-Boot] [PATCH] libfdt: introduce fdt type annotation for use by endian checkers
On Tue, 30 Oct 2012 16:24:05 -0600 Stephen Warren wrote: > On 10/30/2012 03:57 PM, 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. > > Should libfdt patches be committed to the main dtc repository (which I > assume is also upstream for libfdt?) rather than U-Boot first? that's what this is meant to be: a patch for dtc, cc: u-boot, since it was originally a patch to u-boot's copy. Fyi, dtc sumbission guidelines dictate To: jdl, Cc: devicetree-discuss... > Otherwise, if we want to bring in a new libfdt from upstream, that would > trash all the U-Boot-specific changes in U-Boot's copy of libfdt. right, this patch is for the dtc. I assume gvb will pull it into u-boot once it's applied. Kim ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] libfdt: introduce fdt type annotation for use by endian checkers
On 10/30/2012 03:57 PM, 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. Should libfdt patches be committed to the main dtc repository (which I assume is also upstream for libfdt?) rather than U-Boot first? Otherwise, if we want to bring in a new libfdt from upstream, that would trash all the U-Boot-specific changes in U-Boot's copy of libfdt. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] libfdt: introduce fdt type annotation for use by endian checkers
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.
libfdt/fdt.c | 2 +-
libfdt/fdt.h | 50 +-
libfdt/fdt_ro.c | 2 +-
libfdt/fdt_rw.c | 4 ++--
libfdt/fdt_sw.c | 4 ++--
libfdt/fdt_wip.c | 2 +-
libfdt/libfdt.h | 32
7 files changed, 52 insertions(+), 44 deletions(-)
diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index e56833a..57faba3 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -92,7 +92,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset,
unsigned int len)
uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
{
- const uint32_t *tagp, *lenp;
+ const fdt32_t *tagp, *lenp;
uint32_t tag;
int offset = startoffset;
const char *p;
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__
+typedef __be32 fdt32_t;
+typedef __be64 fdt64_t;
+#else
+typedef uint32_t fdt32_t;
+typedef uint64_t fdt64_t;
+#endif
+
struct fdt_header {
- uint32_t magic; /* magic word FDT_MAGIC */
- uint32_t totalsize; /* total size of DT block */
- uint32_t off_dt_struct; /* offset to structure */
- uint32_t off_dt_strings; /* offset to strings */
- uint32_t off_mem_rsvmap; /* offset to memory reserve map */
- uint32_t version;/* format version */
- uint32_t last_comp_version; /* last compatible version */
+ fdt32_t magic; /* magic word FDT_MAGIC */
+ fdt32_t totalsize; /* total size of DT block */
+ fdt32_t off_dt_struct; /* offset to structure */
+ fdt32_t off_dt_strings; /* offset to strings */
+ fdt32_t off_mem_rsvmap; /* offset to memory reserve map */
+ fdt32_t version; /* format version */
+ fdt32_t last_comp_version; /* last compatible version */
/* version 2 fields below */
- uint32_t boot_cpuid_phys;/* Which physical CPU id we're
+ fdt32_t boot_cpuid_phys; /* Which physical CPU id we're
booting on */
/* version 3 fields below */
- uint32_t size_dt_strings;/* size of the strings block */
+ fdt32_t size_dt_strings; /* size of the strings block */
/* version 17 fields below */
- uint32_t size_dt_struct; /* size of the structure block */
+ fdt32_t size_dt_struct; /* size of the structure block */
};
struct fdt_reserve_entry {
- uint64_t address;
- uint64_t size;
+ fdt64_t address;
+ fdt64_t size;
};
struct fdt_node_header {
- uint32_t tag;
+ fdt32_t tag;
char name[0];
};
struct fdt_property {
- uint32_t tag;
- uint32_t len;
- uint32_t nameoff;
+ fdt32_t tag;
+ fdt32_t len;
+ fdt32_t nameoff;
char data[0];
};
#endif /* !__ASSEMBLY */
#define FDT_MAGIC 0xd00dfeed /* 4: version, 4: total size */
-#define FDT_TAGSIZEsizeof(uint32_t)
+#define FDT_TAGSIZEsizeof(fdt32_t)
#define FDT_BEGIN_NODE 0x1 /* Start node: full name */
#define FDT_END_NODE 0x2 /* End node */
@@ -51,10 +59,10 @@ struct fdt_property {
#define FDT_NOP0x4 /* nop */
#define FDT_END0x9
-#define FDT_V1_SIZE(7*sizeof(uint32_t))
-#define FDT_V2_SIZE(FDT_V1_SIZE + sizeof(uint32_t))
-#define FDT_V3_SIZE(FDT_V2_SIZE + sizeof(uint32_t))
+#define FDT_V1_SIZE(7*sizeof(fdt32_t))
+#define FDT_V2_SIZE(FDT_V1_SIZE + sizeof(fdt32_t))
+#define FDT_V3_SIZE(FDT_V2_SIZE + sizeof(fdt32_t))
#define FDT_V16_SIZE FDT_V3_SIZE
-#define FDT_V17_SIZE (FDT_V16_SIZE + sizeof(uint32_t))
+#define FDT_V17_SIZE (FDT_V16_SIZE + sizeof(fdt32_t))
#endif /* _FDT_H */
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 02b6d68..42da2bd 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -322,7 +322,7 @@ const void *fdt_getprop(const void *fdt, int nodeoffset,
uint32_t fdt_get_phandle(const void *fdt, int nodeoffset)
{
- const uint32_t *php;
+ const fdt32_t *php;
int len;
/* FIXME: This is a bit sub-optimal, since we potentially scan
diff --git a/libfdt/
