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.