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

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

[Tom Rini]

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@gmail.com>
Cc: Sam Protsenko <joe.skb7@gmail.com>
Signed-off-by: Tom Rini <trini@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);
 ulong android_image_get_end(const struct andr_img_hdr *hdr);
 ulong android_image_get_kload(const struct andr_img_hdr *hdr);
 ulong android_image_get_kcomp(const struct andr_img_hdr *hdr);
-- 
2.17.1

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

[Eugeniu Rosca]

Hi Tom,
Cc: Masahiro Yamada

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@gmail.com>
> Cc: Sam Protsenko <joe.skb7@gmail.com>
> Signed-off-by: Tom Rini <trini@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'.

That's because the 'image.h' itself seems to ensure the definition
of u32 and friends, as can be understood from below test:

 --------------------- 8< --------------------------

1. Create a dummy C file which includes <image.h>

$ cat cmd/my-test.c                                
#define FORCE_BREAKAGE
#include <image.h>

2. Force a build-time error on including "int-ll64.h", which defines
u32 and friends for all architectures, as per commit 3747bdbb2bb83a
("arch: types.h: factor out fixed width typedefs to int-ll64.h")
(thanks Yamada-san):

$ git diff  -- include/asm-generic/int-ll64.h
diff --git a/include/asm-generic/int-ll64.h b/include/asm-generic/int-ll64.h
index 7451718a61cd..f90dc2826071 100644
--- a/include/asm-generic/int-ll64.h
+++ b/include/asm-generic/int-ll64.h
@@ -15,6 +15,10 @@
  * header files exported to user space
  */

+#ifdef FORCE_BREAKAGE
+#error "Included, thus providing (s,u){8,16,32,64}"
+#endif
+
 typedef __signed__ char __s8;
 typedef unsigned char __u8;

3. Compile the newly created C file for sandbox

$ make cmd/my-test.o
  CC      cmd/my-test.o
In file included from ./arch/sandbox/include/asm/types.h:9,
                 from include/linux/types.h:5,
                 from include/linux/string.h:4,
                 from include/compiler.h:126,
                 from include/image.h:18,
                 from cmd/my-test.c:3:
include/asm-generic/int-ll64.h:19:2: error: #error "Included, thus providing (s,u){8,16,32,64}"

 --------------------- 8< --------------------------

So, to summarize, I don't fully understand the motivation behind this
change, unless there is a long-term goal to keep image.h (and/or other
U-Boot headers) free of u32 and Co.

Just to contrast the frequency of 'uint' vs 'u32' in v5.5 Linux release:

$ git log --oneline -S"\bu32\b" --pickaxe-regex v5.4..v5.5 | wc -l
  1020
$ git log --oneline -S"\buint\b" --pickaxe-regex v5.4..v5.5 | wc -l
  35

Assuming you still want the change, since there is no regression:
Reviewed-by: Eugeniu Rosca <roscaeugeniu@gmail.com>

-- 
Best Regards
Eugeniu Rosca

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

[Tom Rini]

On Sun, Feb 16, 2020 at 05:23:14PM +0100, Eugeniu Rosca wrote:
> Hi Tom,
> Cc: Masahiro Yamada
> 
> 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@gmail.com>
> > Cc: Sam Protsenko <joe.skb7@gmail.com>
> > Signed-off-by: Tom Rini <trini@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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200216/42f6ca21/attachment.sig>

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

[Eugeniu Rosca]

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@gmail.com>
> > > Cc: Sam Protsenko <joe.skb7@gmail.com>
> > > Signed-off-by: Tom Rini <trini@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.

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

-- 
Best Regards
Eugeniu Rosca

[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@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@gmail.com>
> > > > Cc: Sam Protsenko <joe.skb7@gmail.com>
> > > > Signed-off-by: Tom Rini <trini@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

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

[Eugeniu Rosca]

Hello Yamada-san,

Thank you for your precious thoughts!

On Mon, Feb 17, 2020 at 04:45:57PM +0900, Masahiro Yamada wrote:
> 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.

I think this is a safe, simple and non-intrusive solution,
so I have pushed https://patchwork.ozlabs.org/patch/1239098/.

> However, forbidding u32 for tools is questionable to me.

Since Linux has never restricted 'u32' in its in-tree tooling, I
totally support this statement.

> 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------------------------
> 

Since this has greater impact compared to s/u32/uint32_t/, I leave
up to Tom to decide on the most suitable approach.

> 
> 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.
> 

Agreed, since below v3.16 commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d944c4eebcf4c0

> 
> 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.
> 
> 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.

The above sounds like food for thought on how to mitigate the problem
long-term.

> 
> That's my random thought.

Many thanks!

-- 
Best Regards
Eugeniu Rosca

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

[Tom Rini]

On Mon, Feb 17, 2020 at 04:45:57PM +0900, Masahiro Yamada wrote:
> Hi Eugeniu, Tom,
> 
> 
> On Mon, Feb 17, 2020 at 7:05 AM Eugeniu Rosca <roscaeugeniu@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@gmail.com>
> > > > > Cc: Sam Protsenko <joe.skb7@gmail.com>
> > > > > Signed-off-by: Tom Rini <trini@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.

Yes, we should not further expand that file.  We should indeed get rid
of it.

> 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.

Yes, it's just another thing on the TODO list that's not been re-synced
in a while.

> > 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.

For this specific problem, yes, the CONFIG_SPL_GUARD around Android
stuff should get moved and perhaps a later step of moving it elsewhere.
Reducing image.h itself would be largely a matter of moving the "legacy"
image format code and FIT image format code into separate headers but
keeping the notice about "not a derived work" on it.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200217/6cd54a53/attachment.sig>