Re: [PATCH v3 3/4] dtc/libfdt: introduce fdt types for annotation by endian checkers

[Kim Phillips <kim.phillips@freescale.com>]

On Thu, 15 Nov 2012 15:43:40 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Nov 14, 2012 at 06:59:58PM -0600, Kim Phillips wrote:
> > +#define EXTRACT_BYTE(x, n) ((unsigned long long)((uint8_t *)&x)[n])
> > +#define __SWAB16(x) ((EXTRACT_BYTE(x, 0) << 8) | EXTRACT_BYTE(x, 1))
> > +#define __SWAB32(x) ((EXTRACT_BYTE(x, 0) << 24) | (EXTRACT_BYTE(x, 1) << 16) | \
> > +		     (EXTRACT_BYTE(x, 2) << 8) | EXTRACT_BYTE(x, 3))
> > +#define __SWAB64(x) ((EXTRACT_BYTE(x, 0) << 56) | (EXTRACT_BYTE(x, 1) << 48) | \
> > +		     (EXTRACT_BYTE(x, 2) << 40) | (EXTRACT_BYTE(x, 3) << 32) | \
> > +		     (EXTRACT_BYTE(x, 4) << 24) | (EXTRACT_BYTE(x, 5) << 16) | \
> > +		     (EXTRACT_BYTE(x, 6) << 8) | EXTRACT_BYTE(x, 7))
> 
> This is not right, or at least very misleading.  "swab" usually refers
> to an unconditional byteswap.  But the macros above are nops on a
> big-endian machine.

but they don't get called on a big endian system.  This is the name
linux uses.  If you want them renamed, please suggest names - I
can't read your mind.

> > -static inline uint32_t fdt32_to_cpu(uint32_t x)
> > -{
> > -	return (EXTRACT_BYTE(0) << 24) | (EXTRACT_BYTE(1) << 16) | (EXTRACT_BYTE(2) << 8) | EXTRACT_BYTE(3);
> > +/*
> > + * determine host endianness:
> > + * *__first_byte is 0x11 on big endian systems
> > + * *__first_byte is 0x44 on little endian systems
> > + */
> > +static const uint32_t __native = 0x11223344u;
> > +static const uint8_t *__first_byte = (const uint8_t *)&__native;
> > +
> > +#define DEF_FDT_TO_CPU(bits) \
> > +static inline uint##bits##_t fdt##bits##_to_cpu(fdt##bits##_t x) \
> > +{ \
> > +	if (*__first_byte == 0x11) \
> > +		return (__force uint##bits##_t)x; \
> > +	else \
> > +		return (__force uint##bits##_t)__SWAB##bits(x); \
> >  }
> > -#define cpu_to_fdt32(x) fdt32_to_cpu(x)
> > +DEF_FDT_TO_CPU(16)
> > +DEF_FDT_TO_CPU(32)
> > +DEF_FDT_TO_CPU(64)
> 
> In fact, I really don't see why you're rewriting the byteswapper
> functions as part of this patch.  The existing versions aren't very
> nice, but if you want to rewrite those, please do it in a separate
> patch.

This patchseries is about silencing sparse warnings in linux,
u-boot, and libfdt.  Sparse is intelligent in that if you mismatch a
native type of value 0 to a bitwise restricted type, it won't call a
warning.  The existing short-circuiting of the byteswapper
functions, i.e., defining cpu_to_fdt32(x) to fdt32_to_cpu(x) and
vice versa doesn't allow for correct attribution propagation.
Therefore I chose to allow sparse to see the actual conversion.  If
you have any other ideas on how to silence sparse in libfdt, let me
know.

Kim