| Message ID | CAK7LNAQ6MbH_NWA_7r0y4K+641oSUaDmKNMXHXLheZtLFPWxWA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | image.h: Change android_image_get_dtb* to use uint and not u32 | expand |
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!
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 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. 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.
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;