[PATCH] image.h: Change android_image_get_dtb* to use uint and not u32

[Masahiro Yamada]

Hi Eugeniu, Tom,


On Mon, Feb 17, 2020 at 7:05 AM Eugeniu Rosca <roscaeugeniu at gmail.com> wrote:
>
> Hi Tom,
>
> On Sun, Feb 16, 2020 at 11:53:23AM -0500, Tom Rini wrote:
> > On Sun, Feb 16, 2020 at 05:23:14PM +0100, Eugeniu Rosca wrote:
> > > On Fri, Feb 14, 2020 at 12:38:19PM -0500, Tom Rini wrote:
> > > > The image.h header can be used fairly widely in U-Boot builds.  We
> > > > cannot use u32 here as it may be used in cases where we don't have that
> > > > typedef available and don't want to expose it either.  Use uint instead
> > > > here.
> > > >
> > > > Cc: Eugeniu Rosca <roscaeugeniu at gmail.com>
> > > > Cc: Sam Protsenko <joe.skb7 at gmail.com>
> > > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > > > ---
> > > >  include/image.h | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/image.h b/include/image.h
> > > > index b316d167d8d7..1dc3b48d8689 100644
> > > > --- a/include/image.h
> > > > +++ b/include/image.h
> > > > @@ -1425,9 +1425,9 @@ int android_image_get_ramdisk(const struct andr_img_hdr *hdr,
> > > >                         ulong *rd_data, ulong *rd_len);
> > > >  int android_image_get_second(const struct andr_img_hdr *hdr,
> > > >                         ulong *second_data, ulong *second_len);
> > > > -bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size);
> > > > -bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
> > > > -                             u32 *size);
> > > > +bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, uint *size);
> > > > +bool android_image_get_dtb_by_index(ulong hdr_addr, uint index, ulong *addr,
> > > > +                             uint *size);
> > >
> > > While I think the change is harmless and brings some consistency and
> > > visual comfort when reviewing the types employed in 'include/image.h',
> > > I can hardly imagine a real-life breakage introduced by u32 in
> > > 'include/image.h'.
> >
> > I ran in to this in practice with
> > http://patchwork.ozlabs.org/project/uboot/list/?series=155410&state=*
> > applied.
>
> Applying this series to u-boot/master, I am running into below build
> failure [1], which I believe is something you try to fix in this patch.
>
> It looks to me that U-Boot's 'include/image.h' is used not only by
> files which are compiled for the target device, but also by files
> located in 'tools/', which are compiled for the host with -DUSE_HOSTCC.
> After inspecting the 'tools/' path of U-Boot repository, it looks like
> the definition of 'u32' is indeed missing there, so I believe that's
> the root cause of the build failure.


If you need a fixed-width type,
you can use uint32_t if you like.

It is already used.  See line 183 of include/image.h

typedef struct image_header {
        uint32_t ih_magic; /* Image Header Magic Number */


include/compiler.h includes <stdint.h> when USE_HOSTCC is defined.



However, forbidding u32 for tools is questionable to me.
u32 and uint32_t should be always interchangeable.


Perhaps, does the following patch work?  (untested)
--------------------->8------------------------
diff --git a/include/compiler.h b/include/compiler.h
index ed74c272b8c5..f2a4adfbc7e4 100644
--- a/include/compiler.h
+++ b/include/compiler.h
@@ -61,6 +61,9 @@

 #include <time.h>

+typedef uint8_t u8;
+typedef uint16_t u16;
+typedef uint32_t u32;
 typedef uint8_t __u8;
 typedef uint16_t __u16;
 typedef uint32_t __u32;
--------------------->8------------------------



BTW, I think include/compiler.h in U-Boot is ugly.


Linux kernel uses
tools/include/linux/types.h
for defining {u8,u16,u32,u64} for the tools space.


Barebox also adopted a similar approach.

When compiling files for tools,
<linux/types.h> actually includes
scripts/include/linux/types.h
instead of include/linux/types.h


Perhaps, U-Boot could do similar,
but I have never got around to it.



>
> W.r.t. 'android_image_*' functions, I really doubt that they were
> designed to be compiled with USE_HOSTCC. If so, then IMHO we shouldn't
> try to make them compliant with USE_HOSTCC compilation, since this
> will impose additional constraints/requirements to the development style
> of those functions. IMHO we should just hide the android_image functions
> on enabling -DUSE_HOSTCC, as shown in [2]. What's your view on that?
> [1] Build error after applying to u-boot/master below series:
>    http://patchwork.ozlabs.org/project/uboot/list/?series=155410&state=*
>
> In file included from include/u-boot/rsa-mod-exp.h:10,
>                  from ./tools/../lib/rsa/rsa-verify.c:22,
>                  from tools/lib/rsa/rsa-verify.c:1:
> include/image.h:1440:58: error: unknown type name ‘u32’
>  1440 | bool android_image_get_dtbo(ulong hdr_addr, ulong *addr, u32 *size);
>       |                                                          ^~~
> include/image.h:1441:53: error: unknown type name ‘u32’
>  1441 | bool android_image_get_dtb_by_index(ulong hdr_addr, u32 index, ulong *addr,
>       |                                                     ^~~
> include/image.h:1442:9: error: unknown type name ‘u32’
>  1442 |         u32 *size);
>       |         ^~~
>   HOSTCC  tools/asn1_compiler
> make[1]: *** [scripts/Makefile.host:114: tools/lib/rsa/rsa-verify.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
>   HOSTLD  tools/mkenvimage
> make: *** [Makefile:1728: tools] Error 2
>
> [2] Hide the android_image_* functions when USE_HOSTCC is enabled
> diff --git a/include/image.h b/include/image.h
> index ebec329582eb..0cdb2165fdaf 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1429,7 +1429,7 @@ struct cipher_algo *image_get_cipher_algo(const char *full_name);
>  #endif /* CONFIG_FIT_VERBOSE */
>  #endif /* CONFIG_FIT */
>
> -#if defined(CONFIG_ANDROID_BOOT_IMAGE)
> +#if defined(CONFIG_ANDROID_BOOT_IMAGE) && !defined(USE_HOSTCC)
>  struct andr_img_hdr;
>  int android_image_check_header(const struct andr_img_hdr *hdr);
>  int android_image_get_kernel(const struct andr_img_hdr *hdr, int verify,
> @@ -1449,7 +1449,7 @@ void android_print_contents(const struct andr_img_hdr *hdr);
>  bool android_image_print_dtb_contents(ulong hdr_addr);
>  #endif
>
> -#endif /* CONFIG_ANDROID_BOOT_IMAGE */
> +#endif /* CONFIG_ANDROID_BOOT_IMAGE && !USE_HOSTCC */
>
>  /**
>   * board_fit_config_name_match() - Check for a matching board name
>


Maybe U-Boot shares too much code
between U-Boot space and tooling space?

include/image.h of U-Boot is 1520 lines.
include/image.h of Barebox is 258 lines.

But, I am not tracking how they diverged.

Shrinking the interface between U-Boot space and
tooling space will provide a better maintainability.

ifdef would work. Perhaps, splitting the header might be even better.


That's my random thought.
I have not looked into the detail, though.


--
Best Regards

Masahiro Yamada