[U-Boot] [PATCH 4/4] Make FIT support really optional

[U-Boot] [PATCH 4/4] Make FIT support really optional

[Carlos Santos]

Due to some mistakes in the source code, it was not possible to really
turn FIT support off. This commit fixes the problem by means of the
following changes:

- Enclose "bootm_host_load_image" and "bootm_host_load_images" between
  checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.

- Enclose the declaration of "bootm_host_load_images" between checks for
  CONFIG_FIT_SIGNATURE, in common/bootm.h.

- Condition the compilation and linking of fit_common.o fit_image.o
  image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.

Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
---
 common/bootm.c  | 2 ++
 include/bootm.h | 2 ++
 tools/Makefile  | 6 ++----
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index c965326..ab477ba 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -891,6 +891,7 @@ void memmove_wd(void *to, void *from, size_t len, ulong chunksz)
 	memmove(to, from, len);
 }
 
+#if defined(CONFIG_FIT_SIGNATURE)
 static int bootm_host_load_image(const void *fit, int req_image_type)
 {
 	const char *fit_uname_config = NULL;
@@ -955,5 +956,6 @@ int bootm_host_load_images(const void *fit, int cfg_noffset)
 	/* Return the first error we found */
 	return err;
 }
+#endif
 
 #endif /* ndef USE_HOSTCC */
diff --git a/include/bootm.h b/include/bootm.h
index 4981377..94d62a1 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -41,7 +41,9 @@ void lynxkdi_boot(image_header_t *hdr);
 
 boot_os_fn *bootm_os_get_boot_func(int os);
 
+#if defined(CONFIG_FIT_SIGNATURE)
 int bootm_host_load_images(const void *fit, int cfg_noffset);
+#endif
 
 int boot_selected_os(int argc, char * const argv[], int state,
 		     bootm_headers_t *images, boot_os_fn *boot_fn);
diff --git a/tools/Makefile b/tools/Makefile
index da50e1b..0a3d279 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -54,6 +54,7 @@ mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o
 hostprogs-y += dumpimage mkimage
 hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
 
+FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o
 FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o
 # Flattened device tree objects
 LIBFDT_OBJS := $(addprefix lib/libfdt/, \
@@ -68,18 +69,15 @@ ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
 # common objs for dumpimage and mkimage
 dumpimage-mkimage-objs := aisimage.o \
 			atmelimage.o \
+			$(FIT_OBJS-y) \
 			$(FIT_SIG_OBJS-y) \
 			common/bootm.o \
 			lib/crc32.o \
 			default_image.o \
 			lib/fdtdec_common.o \
 			lib/fdtdec.o \
-			fit_common.o \
-			fit_image.o \
 			gpimage.o \
 			gpimage-common.o \
-			common/image-fit.o \
-			image-host.o \
 			common/image.o \
 			imagetool.o \
 			imximage.o \
-- 
2.7.4

[U-Boot] [PATCH v2] Make FIT support really optional

[Carlos Santos]

Due to some mistakes in the source code, it was not possible to really
turn FIT support off. This commit fixes the problem by means of the
following changes:

- Enclose "bootm_host_load_image" and "bootm_host_load_images" between
  checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.

- Enclose the declaration of "bootm_host_load_images" between checks for
  CONFIG_FIT_SIGNATURE, in common/bootm.h.

- Condition the compilation and linking of fit_common.o fit_image.o
  image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.

Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
---
Changes v1 -> v2
 Rebased to the top of master branch.

 common/bootm.c  | 2 ++
 include/bootm.h | 2 ++
 tools/Makefile  | 6 ++----
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index 4941414..bbf3f97 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -901,6 +901,7 @@ void memmove_wd(void *to, void *from, size_t len, ulong chunksz)
 	memmove(to, from, len);
 }
 
+#if defined(CONFIG_FIT_SIGNATURE)
 static int bootm_host_load_image(const void *fit, int req_image_type)
 {
 	const char *fit_uname_config = NULL;
@@ -965,5 +966,6 @@ int bootm_host_load_images(const void *fit, int cfg_noffset)
 	/* Return the first error we found */
 	return err;
 }
+#endif
 
 #endif /* ndef USE_HOSTCC */
diff --git a/include/bootm.h b/include/bootm.h
index 4981377..94d62a1 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -41,7 +41,9 @@ void lynxkdi_boot(image_header_t *hdr);
 
 boot_os_fn *bootm_os_get_boot_func(int os);
 
+#if defined(CONFIG_FIT_SIGNATURE)
 int bootm_host_load_images(const void *fit, int cfg_noffset);
+#endif
 
 int boot_selected_os(int argc, char * const argv[], int state,
 		     bootm_headers_t *images, boot_os_fn *boot_fn);
diff --git a/tools/Makefile b/tools/Makefile
index 63355aa..05122d5 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -54,6 +54,7 @@ mkenvimage-objs := mkenvimage.o os_support.o lib/crc32.o
 hostprogs-y += dumpimage mkimage
 hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
 
+FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o
 FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o
 # Flattened device tree objects
 LIBFDT_OBJS := $(addprefix lib/libfdt/, \
@@ -68,18 +69,15 @@ ROCKCHIP_OBS = lib/rc4.o rkcommon.o rkimage.o rksd.o rkspi.o
 # common objs for dumpimage and mkimage
 dumpimage-mkimage-objs := aisimage.o \
 			atmelimage.o \
+			$(FIT_OBJS-y) \
 			$(FIT_SIG_OBJS-y) \
 			common/bootm.o \
 			lib/crc32.o \
 			default_image.o \
 			lib/fdtdec_common.o \
 			lib/fdtdec.o \
-			fit_common.o \
-			fit_image.o \
 			gpimage.o \
 			gpimage-common.o \
-			common/image-fit.o \
-			image-host.o \
 			common/image.o \
 			imagetool.o \
 			imximage.o \
-- 
2.7.4

[U-Boot] [PATCH v2] Make FIT support really optional

[Otavio Salvador]

Hello Carlos,

On Fri, Jun 3, 2016 at 4:16 PM, Carlos Santos <casantos@datacom.ind.br> wrote:
> Due to some mistakes in the source code, it was not possible to really
> turn FIT support off. This commit fixes the problem by means of the
> following changes:
>
> - Enclose "bootm_host_load_image" and "bootm_host_load_images" between
>   checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
>
> - Enclose the declaration of "bootm_host_load_images" between checks for
>   CONFIG_FIT_SIGNATURE, in common/bootm.h.
>
> - Condition the compilation and linking of fit_common.o fit_image.o
>   image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
>
> Signed-off-by: Carlos Santos <casantos@datacom.ind.br>

Reviewed-by: Otavio Salvador <otavio@ossystems.com.br>

Thanks for resending it; I am adding Tom on Cc as this is likely going
straight for his tree.

-- 
Otavio Salvador                             O.S. Systems
http://www.ossystems.com.br        http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854            Mobile: +1 (347) 903-9750

[U-Boot] [PATCH v2] Make FIT support really optional

[Tom Rini]

On Fri, Jun 03, 2016 at 04:16:26PM -0300, Carlos Santos wrote:

> Due to some mistakes in the source code, it was not possible to really
> turn FIT support off. This commit fixes the problem by means of the
> following changes:
> 
> - Enclose "bootm_host_load_image" and "bootm_host_load_images" between
>   checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
> 
> - Enclose the declaration of "bootm_host_load_images" between checks for
>   CONFIG_FIT_SIGNATURE, in common/bootm.h.
> 
> - Condition the compilation and linking of fit_common.o fit_image.o
>   image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
> 
> Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
> ---
> Changes v1 -> v2
>  Rebased to the top of master branch.
> 
>  common/bootm.c  | 2 ++
>  include/bootm.h | 2 ++
>  tools/Makefile  | 6 ++----
>  3 files changed, 6 insertions(+), 4 deletions(-)

So, why?  I don't like the idea of making FIT support in mkimage
conditional.  This makes the life of distribution people harder, not
easier.  The functions in common/bootm.c should be being discarded in
U-Boot itself when we don't have CONFIG_FIT_SIGNATURE.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160604/8a1937d2/attachment.sig>

[U-Boot] [PATCH v2] Make FIT support really optional

[Carlos Santos]

> From: "Tom Rini" <trini@konsulko.com>
> To: "Carlos Santos" <casantos@datacom.ind.br>
> Cc: u-boot at lists.denx.de
> Sent: Saturday, June 4, 2016 10:06:58 AM
> Subject: Re: [U-Boot] [PATCH v2] Make FIT support really optional

> On Fri, Jun 03, 2016 at 04:16:26PM -0300, Carlos Santos wrote:
> 
>> Due to some mistakes in the source code, it was not possible to really
>> turn FIT support off. This commit fixes the problem by means of the
>> following changes:
>> 
>> - Enclose "bootm_host_load_image" and "bootm_host_load_images" between
>>   checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
>> 
>> - Enclose the declaration of "bootm_host_load_images" between checks for
>>   CONFIG_FIT_SIGNATURE, in common/bootm.h.
>> 
>> - Condition the compilation and linking of fit_common.o fit_image.o
>>   image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
>> 
>> Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
>> ---
>> Changes v1 -> v2
>>  Rebased to the top of master branch.
>> 
>>  common/bootm.c  | 2 ++
>>  include/bootm.h | 2 ++
>>  tools/Makefile  | 6 ++----
>>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> So, why?  I don't like the idea of making FIT support in mkimage
> conditional.

If FIT is not to be conditional then what's the purpose of the CONFIG_FIT_SIGNATURE configuration option? Looks like it exists exactly to make FIT support conditional, which seems to be a reasonable approach, since it helps to reduce the size of the boot loader.

> This makes the life of distribution people harder, not
> easier.  The functions in common/bootm.c should be being discarded in
> U-Boot itself when we don't have CONFIG_FIT_SIGNATURE.  Thanks!

The patch exists because of "distribution people". I sent a patch to Buildroot[1] which was refused because it added dependencies on DTC and evolved to several follow-ups [2,3,4].

1. http://patchwork.ozlabs.org/patch/618486/
2. http://patchwork.ozlabs.org/patch/619278/
3. http://patchwork.ozlabs.org/patch/619696/
4. http://patchwork.ozlabs.org/patch/629988/

Carlos Santos (Casantos)
DATACOM, P&D

[U-Boot] [PATCH v2] Make FIT support really optional

[Carlos Santos]

> From: "Carlos Santos" <casantos@datacom.ind.br>
> To: "Tom Rini" <trini@konsulko.com>
> Cc: u-boot at lists.denx.de
> Sent: Saturday, June 4, 2016 2:39:22 PM
> Subject: Re: [U-Boot] [PATCH v2] Make FIT support really optional

>> From: "Tom Rini" <trini@konsulko.com>
>> To: "Carlos Santos" <casantos@datacom.ind.br>
>> Cc: u-boot at lists.denx.de
>> Sent: Saturday, June 4, 2016 10:06:58 AM
>> Subject: Re: [U-Boot] [PATCH v2] Make FIT support really optional
> 
>> On Fri, Jun 03, 2016 at 04:16:26PM -0300, Carlos Santos wrote:
>> 
>>> Due to some mistakes in the source code, it was not possible to really
>>> turn FIT support off. This commit fixes the problem by means of the
>>> following changes:
>>> 
>>> - Enclose "bootm_host_load_image" and "bootm_host_load_images" between
>>>   checks for CONFIG_FIT_SIGNATURE, in common/bootm.c.
>>> 
>>> - Enclose the declaration of "bootm_host_load_images" between checks for
>>>   CONFIG_FIT_SIGNATURE, in common/bootm.h.
>>> 
>>> - Condition the compilation and linking of fit_common.o fit_image.o
>>>   image-host.o common/image-fit.o to CONFIG_FIT=y, in tools/Makefile.
>>> 
>>> Signed-off-by: Carlos Santos <casantos@datacom.ind.br>
>>> ---
>>> Changes v1 -> v2
>>>  Rebased to the top of master branch.
>>> 
>>>  common/bootm.c  | 2 ++
>>>  include/bootm.h | 2 ++
>>>  tools/Makefile  | 6 ++----
>>>  3 files changed, 6 insertions(+), 4 deletions(-)
>> 
>> So, why?  I don't like the idea of making FIT support in mkimage
>> conditional.
> 
> If FIT is not to be conditional then what's the purpose of the
> CONFIG_FIT_SIGNATURE configuration option? Looks like it exists exactly to make
> FIT support conditional, which seems to be a reasonable approach, since it
> helps to reduce the size of the boot loader.

Sorry, I meant "what is the purpose of the CONFIG_FIT option". 

>> This makes the life of distribution people harder, not
>> easier.  The functions in common/bootm.c should be being discarded in
>> U-Boot itself when we don't have CONFIG_FIT_SIGNATURE.  Thanks!
> 
> The patch exists because of "distribution people". I sent a patch to
> Buildroot[1] which was refused because it added dependencies on DTC and evolved
> to several follow-ups [2,3,4].
> 
> 1. http://patchwork.ozlabs.org/patch/618486/
> 2. http://patchwork.ozlabs.org/patch/619278/
> 3. http://patchwork.ozlabs.org/patch/619696/
> 4. http://patchwork.ozlabs.org/patch/629988/
> 
> Carlos Santos (Casantos)
> DATACOM, P&D

[U-Boot] [PATCH v2] Make FIT support really optional

[Thomas Petazzoni]

Carlos, Tom,

On Sat, 4 Jun 2016 14:39:22 -0300 (BRT), Carlos Santos wrote:

> > So, why?  I don't like the idea of making FIT support in mkimage
> > conditional.  
> 
> If FIT is not to be conditional then what's the purpose of the
> CONFIG_FIT_SIGNATURE configuration option? Looks like it exists
> exactly to make FIT support conditional, which seems to be a
> reasonable approach, since it helps to reduce the size of the boot
> loader.

CONFIG_FIT_SIGNATURE is I guess optional because it requires OpenSSL at
*build* time and the U-Boot developers don't want to force everyone to
have OpenSSL available to build U-Boot.

However, FIT support does not require any special build dependency, so
probably there's little interest from the U-Boot folks to make it
optional.

> > This makes the life of distribution people harder, not
> > easier.  The functions in common/bootm.c should be being discarded
> > in U-Boot itself when we don't have CONFIG_FIT_SIGNATURE.  Thanks!  
> 
> The patch exists because of "distribution people". I sent a patch to
> Buildroot[1] which was refused because it added dependencies on DTC
> and evolved to several follow-ups [2,3,4].

Right, *but* it is not because we make FIT support optional in
Buildroot that we have to make it optional in U-Boot.

We can perfectly have an option in Buildroot to enable/disable FIT
support which does *not* enable/disable FIT support in the U-Boot, but
only ensures that the relevant runtime dependencies (i.e DTC) are
enabled.

Of course, if the U-Boot developers want to make FIT support in the
bootloader itself an optional feature, why not, but it's clearly not a
requirement from our side.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[U-Boot] [PATCH v2] Make FIT support really optional

[Carlos Santos]

> From: "Thomas Petazzoni" <thomas.petazzoni@free-electrons.com>
> To: "Carlos Santos" <casantos@datacom.ind.br>
> Cc: "Tom Rini" <trini@konsulko.com>, u-boot at lists.denx.de
> Sent: Tuesday, June 7, 2016 5:37:46 PM
> Subject: Re: [U-Boot] [PATCH v2] Make FIT support really optional

> Carlos, Tom,
> 
> On Sat, 4 Jun 2016 14:39:22 -0300 (BRT), Carlos Santos wrote:
> 
>> > So, why?  I don't like the idea of making FIT support in mkimage
>> > conditional.
>> 
>> If FIT is not to be conditional then what's the purpose of the
>> CONFIG_FIT_SIGNATURE configuration option? Looks like it exists
>> exactly to make FIT support conditional, which seems to be a
>> reasonable approach, since it helps to reduce the size of the boot
>> loader.
> 
> CONFIG_FIT_SIGNATURE is I guess optional because it requires OpenSSL at
> *build* time and the U-Boot developers don't want to force everyone to
> have OpenSSL available to build U-Boot.
> 
> However, FIT support does not require any special build dependency, so
> probably there's little interest from the U-Boot folks to make it
> optional.

There is already a configuration that makes FIT optional (CONFIG_FIT) but it is partially broken because it does not really remove FIT-related functionality from mkimage. That's the reason why it was not possible to disable FIT in the Buildroot package. My patch aims to fix that defect.

Carlos Santos (Casantos)
DATACOM, P&D

[U-Boot] [PATCH v2] Make FIT support really optional

[Thomas Petazzoni]

Hello,

On Tue, 7 Jun 2016 21:18:17 -0300 (BRT), Carlos Santos wrote:

> There is already a configuration that makes FIT optional (CONFIG_FIT)
> but it is partially broken because it does not really remove
> FIT-related functionality from mkimage. That's the reason why it was
> not possible to disable FIT in the Buildroot package. My patch aims
> to fix that defect.

Ah, indeed, I forgot that the CONFIG_FIT option already existed. In
this case, I agree that it should either exist and work, or not exist.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[U-Boot] [PATCH v2] Make FIT support really optional

[Tom Rini]

On Wed, Jun 08, 2016 at 07:47:12AM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 7 Jun 2016 21:18:17 -0300 (BRT), Carlos Santos wrote:
> 
> > There is already a configuration that makes FIT optional (CONFIG_FIT)
> > but it is partially broken because it does not really remove
> > FIT-related functionality from mkimage. That's the reason why it was
> > not possible to disable FIT in the Buildroot package. My patch aims
> > to fix that defect.
> 
> Ah, indeed, I forgot that the CONFIG_FIT option already existed. In
> this case, I agree that it should either exist and work, or not exist.

Here is the problem with host tools living along side configurable
binaries and sharing code.  With respect to mkimage, FIT support is
non-optional.  The only reason that FIT signature stuff is optional is
that it introduces too high of a set of dependencies for everyone else
to have installed to even compile mkimage which is in turn required by
the vast majority of targets.  If the failing message that we couldn't
run dtc is not clear enoguh, I am happy to take a patch to make the
problem and solution clearer.

I would go so far as to say that requiring bison and m4 (along with dtc)
to already be compiled in buildroot is not a step too far, but I'm an
OpenEmbedded guy so it's possible I'm just dependency-happy :)  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160608/3d0d0167/attachment.sig>

[U-Boot] [PATCH v2] Make FIT support really optional

[Simon Glass]

Hi,

On 8 June 2016 at 05:17, Tom Rini <trini@konsulko.com> wrote:
> On Wed, Jun 08, 2016 at 07:47:12AM +0200, Thomas Petazzoni wrote:
>> Hello,
>>
>> On Tue, 7 Jun 2016 21:18:17 -0300 (BRT), Carlos Santos wrote:
>>
>> > There is already a configuration that makes FIT optional (CONFIG_FIT)
>> > but it is partially broken because it does not really remove
>> > FIT-related functionality from mkimage. That's the reason why it was
>> > not possible to disable FIT in the Buildroot package. My patch aims
>> > to fix that defect.
>>
>> Ah, indeed, I forgot that the CONFIG_FIT option already existed. In
>> this case, I agree that it should either exist and work, or not exist.
>
> Here is the problem with host tools living along side configurable
> binaries and sharing code.  With respect to mkimage, FIT support is
> non-optional.  The only reason that FIT signature stuff is optional is
> that it introduces too high of a set of dependencies for everyone else
> to have installed to even compile mkimage which is in turn required by
> the vast majority of targets.  If the failing message that we couldn't
> run dtc is not clear enoguh, I am happy to take a patch to make the
> problem and solution clearer.

I agree. I would prefer to always support signatures in mkimage, if we can.

The only purpose of turning off FIT in U-Boot is to reduce code size.
But IMO it should be enabled by default in U-Boot. It is suppose to be
the official format...

Regards,
Simon