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

[Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>]

Hi Kim,

> 
> I hope this addresses all your comments, David.

Which is why David didn't see this patch earlier. :-)


> index 213d7fb..302d5cb 100644
> --- a/libfdt/libfdt_env.h
> +++ b/libfdt/libfdt_env.h
> @@ -5,25 +5,63 @@
>  #include <stdint.h>
>  #include <string.h>
>  
> +#ifdef __CHECKER__
> +#define __force __attribute__((force))
> +#define __bitwise __attribute__((bitwise))
> +typedef uint16_t __bitwise fdt16_t;
> +typedef uint32_t __bitwise fdt32_t;
> +typedef uint64_t __bitwise fdt64_t;
> +#else
> +#define __force
> +#define __bitwise
> +typedef uint16_t fdt16_t;
> +typedef uint32_t fdt32_t;
> +typedef uint64_t fdt64_t;
> +#endif

Would this be simpler/better?

    #ifdef __CHECKER__
    #define __force __attribute__((force))
    #define __bitwise __attribute__((bitwise))
    #else
    #define __force
    #define __bitwise
    #endif

    typedef uint16_t __bitwise fdt16_t;
    typedef uint32_t __bitwise fdt32_t;
    typedef uint64_t __bitwise fdt64_t;



>  #define EXTRACT_BYTE(n)	((unsigned long long)((uint8_t *)&x)[n])
> -static inline uint16_t fdt16_to_cpu(uint16_t x)
> -{
> -	return (EXTRACT_BYTE(0) << 8) | EXTRACT_BYTE(1);
> -}
> -#define cpu_to_fdt16(x) fdt16_to_cpu(x)
> +#define __SWAB16X ((EXTRACT_BYTE(0) << 8) | EXTRACT_BYTE(1))
> +#define __SWAB32X ((EXTRACT_BYTE(0) << 24) | (EXTRACT_BYTE(1) << 16) | (EXTRACT_BYTE(2) <<
>  8) | EXTRACT_BYTE(3))
> +#define __SWAB64X ((EXTRACT_BYTE(0) << 56) | (EXTRACT_BYTE(1) << 48) | (EXTRACT_BYTE(2) <<
>  40) | (EXTRACT_BYTE(3) << 32) \
> +		  | (EXTRACT_BYTE(4) << 24) | (EXTRACT_BYTE(5) << 16) | (EXTRACT_BYTE(6) << 8
> ) | EXTRACT_BYTE(7))

I dislike function-like macros that grab global names.
Instead, can we re-work 'x' in as a parameter:

    >  #define EXTRACT_BYTE(x, n)	((unsigned long long)((uint8_t *)&x)[n])


> +#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; \
>  }

Case check that last X...

> -static inline uint64_t fdt64_to_cpu(uint64_t x)
> -{
> -	return (EXTRACT_BYTE(0) << 56) | (EXTRACT_BYTE(1) << 48) | (EXTRACT_BYTE(2) << 40) | 
> (EXTRACT_BYTE(3) << 32)
> -		| (EXTRACT_BYTE(4) << 24) | (EXTRACT_BYTE(5) << 16) | (EXTRACT_BYTE(6) << 8) 
> | EXTRACT_BYTE(7);
> +#define DEF_CPU_TO_FDT(bits) \
> +static inline fdt##bits##_t cpu_to_fdt##bits(uint##bits##_t x) \
> +{ \
> +	if (*__first_byte == 0x11) \
> +		return (__force fdt##bits##_t)x; \
> +	else \
> +		return (__force fdt##bits##_t)__SWAB##bits##X; \
>  }

...and that one.

Thanks,
jdl