diff mbox series

[v4,03/16] common: update: add a generic interface for FIT image

Message ID 20200722060539.15168-4-takahiro.akashi@linaro.org
State Superseded
Headers show
Series efi_loader: add capsule update support | expand

Commit Message

AKASHI Takahiro July 22, 2020, 6:05 a.m. UTC 
The main purpose of this patch is to separate a generic interface for
updating firmware using DFU drivers from "auto-update" via tftp.

This function will also be used in implementing UEFI capsule update
in a later commit.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

---
 common/Kconfig      | 14 +++++++++
 common/Makefile     |  3 +-
 common/update.c     | 71 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/dfu/Kconfig |  1 +
 include/image.h     | 12 ++++++++
 5 files changed, 99 insertions(+), 2 deletions(-)

-- 
2.27.0

Comments

Heinrich Schuchardt July 22, 2020, 1:07 p.m. UTC | #1 
On 22.07.20 08:05, AKASHI Takahiro wrote:
> The main purpose of this patch is to separate a generic interface for

> updating firmware using DFU drivers from "auto-update" via tftp.

>

> This function will also be used in implementing UEFI capsule update

> in a later commit.

>

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> ---

>  common/Kconfig      | 14 +++++++++

>  common/Makefile     |  3 +-

>  common/update.c     | 71 +++++++++++++++++++++++++++++++++++++++++++++

>  drivers/dfu/Kconfig |  1 +

>  include/image.h     | 12 ++++++++

>  5 files changed, 99 insertions(+), 2 deletions(-)

>

> diff --git a/common/Kconfig b/common/Kconfig

> index ca42ba37b726..86568dec2e25 100644

> --- a/common/Kconfig

> +++ b/common/Kconfig

> @@ -1014,6 +1014,20 @@ endmenu

>

>  menu "Update support"

>

> +config UPDATE_COMMON

> +	bool

> +	default n

> +	select DFU_ALT


Why do we need separate symbols DFU_ALT and DFU_COMMON?

> +

> +config UPDATE_FIT

> +	bool "Firmware update using fitImage"

> +	depends on FIT

> +	depends on DFU

> +	select UPDATE_COMMON

> +	help

> +	  This option allows performing update of DFU-capable storage with

> +	  data in fitImage.

> +

>  config ANDROID_AB

>  	bool "Android A/B updates"

>  	default n

> diff --git a/common/Makefile b/common/Makefile

> index 2e7a090588d9..bcf352d01652 100644

> --- a/common/Makefile

> +++ b/common/Makefile

> @@ -53,8 +53,7 @@ obj-$(CONFIG_LCD_ROTATION) += lcd_console_rotation.o

>  obj-$(CONFIG_LCD_DT_SIMPLEFB) += lcd_simplefb.o

>  obj-$(CONFIG_LYNXKDI) += lynxkdi.o

>  obj-$(CONFIG_MENU) += menu.o

> -obj-$(CONFIG_UPDATE_TFTP) += update.o

> -obj-$(CONFIG_DFU_TFTP) += update.o

> +obj-$(CONFIG_UPDATE_COMMON) += update.o

>  obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o

>  obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o

>

> diff --git a/common/update.c b/common/update.c

> index f82d77cc0be9..2c75b37f19e6 100644

> --- a/common/update.c

> +++ b/common/update.c

> @@ -23,6 +23,7 @@

>  #include <dfu.h>

>  #include <errno.h>

>

> +#ifdef CONFIG_DFU_TFTP

>  /* env variable holding the location of the update file */

>  #define UPDATE_FILE_ENV		"updatefile"

>

> @@ -89,6 +90,7 @@ static int update_load(char *filename, ulong msec_max, int cnt_max, ulong addr)

>

>  	return rv;

>  }

> +#endif /* CONFIG_DFU_TFTP */

>

>  static int update_fit_getparams(const void *fit, int noffset, ulong *addr,

>  						ulong *fladdr, ulong *size)

> @@ -106,6 +108,7 @@ static int update_fit_getparams(const void *fit, int noffset, ulong *addr,

>  	return 0;

>  }

>

> +#ifdef CONFIG_DFU_TFTP

>  int update_tftp(ulong addr, char *interface, char *devstring)

>  {

>  	char *filename, *env_addr, *fit_image_name;

> @@ -194,3 +197,71 @@ next_node:

>

>  	return ret;

>  }

> +#endif /* CONFIG_DFU_UPDATE */


Why do we need all those #ifdef? The linker removes all unused functions.

We should move update_tftp() to drivers/dfu/dfu_tftp.c

Best regards

Heinrich

> +

> +#ifdef CONFIG_UPDATE_FIT

> +/**

> + * fit_update - update storage with FIT image

> + * @fit:	Pointer to FIT image

> + *

> + * Update firmware on storage using FIT image as input.

> + * The storage area to be update will be identified by the name

> + * in FIT and matching it to "dfu_alt_info" variable.

> + *

> + * Return:      0 - on success, non-zero - otherwise

> + */

> +int fit_update(const void *fit)

> +{

> +	char *fit_image_name;

> +	ulong update_addr, update_fladdr, update_size;

> +	int images_noffset, ndepth, noffset;

> +	int ret = 0;

> +

> +	if (!fit)

> +		return -EINVAL;

> +

> +	if (!fit_check_format((void *)fit)) {

> +		printf("Bad FIT format of the update file, aborting auto-update\n");

> +		return -EINVAL;

> +	}

> +

> +	/* process updates */

> +	images_noffset = fdt_path_offset(fit, FIT_IMAGES_PATH);

> +

> +	ndepth = 0;

> +	noffset = fdt_next_node(fit, images_noffset, &ndepth);

> +	while (noffset >= 0 && ndepth > 0) {

> +		if (ndepth != 1)

> +			goto next_node;

> +

> +		fit_image_name = (char *)fit_get_name(fit, noffset, NULL);

> +		printf("Processing update '%s' :", fit_image_name);

> +

> +		if (!fit_image_verify(fit, noffset)) {

> +			printf("Error: invalid update hash, aborting\n");

> +			ret = 1;

> +			goto next_node;

> +		}

> +

> +		printf("\n");

> +		if (update_fit_getparams(fit, noffset, &update_addr,

> +					 &update_fladdr, &update_size)) {

> +			printf("Error: can't get update parameters, aborting\n");

> +			ret = 1;

> +			goto next_node;

> +		}

> +

> +		if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) {

> +			ret = dfu_write_by_name(fit_image_name,

> +						(void *)update_addr,

> +						update_size, NULL, NULL);

> +			if (ret)

> +				return ret;

> +		}

> +next_node:

> +		noffset = fdt_next_node(fit, noffset, &ndepth);

> +	}

> +

> +	return ret;

> +}

> +#endif /* CONFIG_UPDATE_FIT */

> diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig

> index d680b28ecf51..df0585c4fc83 100644

> --- a/drivers/dfu/Kconfig

> +++ b/drivers/dfu/Kconfig

> @@ -22,6 +22,7 @@ config DFU_TFTP

>  	bool "DFU via TFTP"

>  	select DFU_ALT

>  	select DFU_OVER_TFTP

> +	select UPDATE_COMMON

>  	help

>  	  This option allows performing update of DFU-managed medium with data

>  	  sent via TFTP boot.

> diff --git a/include/image.h b/include/image.h

> index 9a5a87dbf870..dce2997f9a6a 100644

> --- a/include/image.h

> +++ b/include/image.h

> @@ -1592,4 +1592,16 @@ struct fit_loadable_tbl {

>  		.handler = _handler, \

>  	}

>

> +/**

> + * fit_update - update storage with FIT image

> + * @fit:        Pointer to FIT image

> + *

> + * Update firmware on storage using FIT image as input.

> + * The storage area to be update will be indentified by the name

> + * in FIT and matching it to "dfu_alt_info" variable.

> + *

> + * Return:      0 on success, non-zero otherwise

> + */

> +int fit_update(const void *fit);

> +

>  #endif	/* __IMAGE_H__ */

>
AKASHI Takahiro July 29, 2020, 5:33 a.m. UTC | #2 
Heinrich,

On Wed, Jul 22, 2020 at 03:07:51PM +0200, Heinrich Schuchardt wrote:
> On 22.07.20 08:05, AKASHI Takahiro wrote:

> > The main purpose of this patch is to separate a generic interface for

> > updating firmware using DFU drivers from "auto-update" via tftp.

> >

> > This function will also be used in implementing UEFI capsule update

> > in a later commit.

> >

> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

> > ---

> >  common/Kconfig      | 14 +++++++++

> >  common/Makefile     |  3 +-

> >  common/update.c     | 71 +++++++++++++++++++++++++++++++++++++++++++++

> >  drivers/dfu/Kconfig |  1 +

> >  include/image.h     | 12 ++++++++

> >  5 files changed, 99 insertions(+), 2 deletions(-)

> >

> > diff --git a/common/Kconfig b/common/Kconfig

> > index ca42ba37b726..86568dec2e25 100644

> > --- a/common/Kconfig

> > +++ b/common/Kconfig

> > @@ -1014,6 +1014,20 @@ endmenu

> >

> >  menu "Update support"

> >

> > +config UPDATE_COMMON

> > +	bool

> > +	default n

> > +	select DFU_ALT

> 

> Why do we need separate symbols DFU_ALT and DFU_COMMON?


Because we have different compile targets.

I believe that 'update.c' should still stay in common (or preferably lib/)
because it is a kind of 'high-level' helper functions for a specific use/
subsystem, tftp update or UEFI capsule in this case, while drivers/dfu is
a low-level/generic drivers for multiple uses.

> > +

> > +config UPDATE_FIT

> > +	bool "Firmware update using fitImage"

> > +	depends on FIT

> > +	depends on DFU

> > +	select UPDATE_COMMON

> > +	help

> > +	  This option allows performing update of DFU-capable storage with

> > +	  data in fitImage.

> > +

> >  config ANDROID_AB

> >  	bool "Android A/B updates"

> >  	default n

> > diff --git a/common/Makefile b/common/Makefile

> > index 2e7a090588d9..bcf352d01652 100644

> > --- a/common/Makefile

> > +++ b/common/Makefile

> > @@ -53,8 +53,7 @@ obj-$(CONFIG_LCD_ROTATION) += lcd_console_rotation.o

> >  obj-$(CONFIG_LCD_DT_SIMPLEFB) += lcd_simplefb.o

> >  obj-$(CONFIG_LYNXKDI) += lynxkdi.o

> >  obj-$(CONFIG_MENU) += menu.o

> > -obj-$(CONFIG_UPDATE_TFTP) += update.o

> > -obj-$(CONFIG_DFU_TFTP) += update.o

> > +obj-$(CONFIG_UPDATE_COMMON) += update.o

> >  obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o

> >  obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o

> >

> > diff --git a/common/update.c b/common/update.c

> > index f82d77cc0be9..2c75b37f19e6 100644

> > --- a/common/update.c

> > +++ b/common/update.c

> > @@ -23,6 +23,7 @@

> >  #include <dfu.h>

> >  #include <errno.h>

> >

> > +#ifdef CONFIG_DFU_TFTP

> >  /* env variable holding the location of the update file */

> >  #define UPDATE_FILE_ENV		"updatefile"

> >

> > @@ -89,6 +90,7 @@ static int update_load(char *filename, ulong msec_max, int cnt_max, ulong addr)

> >

> >  	return rv;

> >  }

> > +#endif /* CONFIG_DFU_TFTP */

> >

> >  static int update_fit_getparams(const void *fit, int noffset, ulong *addr,

> >  						ulong *fladdr, ulong *size)

> > @@ -106,6 +108,7 @@ static int update_fit_getparams(const void *fit, int noffset, ulong *addr,

> >  	return 0;

> >  }

> >

> > +#ifdef CONFIG_DFU_TFTP

> >  int update_tftp(ulong addr, char *interface, char *devstring)

> >  {

> >  	char *filename, *env_addr, *fit_image_name;

> > @@ -194,3 +197,71 @@ next_node:

> >

> >  	return ret;

> >  }

> > +#endif /* CONFIG_DFU_UPDATE */

> 

> Why do we need all those #ifdef? The linker removes all unused functions.


I think this kind of use of #ifdef is quite common across
u-boot source code.
If you want to prohibit such usages, we should have
a written document/guideline.

> We should move update_tftp() to drivers/dfu/dfu_tftp.c


I can't agree. See the above.

-Takahiro Akashi

> Best regards

> 

> Heinrich

> 

> > +

> > +#ifdef CONFIG_UPDATE_FIT

> > +/**

> > + * fit_update - update storage with FIT image

> > + * @fit:	Pointer to FIT image

> > + *

> > + * Update firmware on storage using FIT image as input.

> > + * The storage area to be update will be identified by the name

> > + * in FIT and matching it to "dfu_alt_info" variable.

> > + *

> > + * Return:      0 - on success, non-zero - otherwise

> > + */

> > +int fit_update(const void *fit)

> > +{

> > +	char *fit_image_name;

> > +	ulong update_addr, update_fladdr, update_size;

> > +	int images_noffset, ndepth, noffset;

> > +	int ret = 0;

> > +

> > +	if (!fit)

> > +		return -EINVAL;

> > +

> > +	if (!fit_check_format((void *)fit)) {

> > +		printf("Bad FIT format of the update file, aborting auto-update\n");

> > +		return -EINVAL;

> > +	}

> > +

> > +	/* process updates */

> > +	images_noffset = fdt_path_offset(fit, FIT_IMAGES_PATH);

> > +

> > +	ndepth = 0;

> > +	noffset = fdt_next_node(fit, images_noffset, &ndepth);

> > +	while (noffset >= 0 && ndepth > 0) {

> > +		if (ndepth != 1)

> > +			goto next_node;

> > +

> > +		fit_image_name = (char *)fit_get_name(fit, noffset, NULL);

> > +		printf("Processing update '%s' :", fit_image_name);

> > +

> > +		if (!fit_image_verify(fit, noffset)) {

> > +			printf("Error: invalid update hash, aborting\n");

> > +			ret = 1;

> > +			goto next_node;

> > +		}

> > +

> > +		printf("\n");

> > +		if (update_fit_getparams(fit, noffset, &update_addr,

> > +					 &update_fladdr, &update_size)) {

> > +			printf("Error: can't get update parameters, aborting\n");

> > +			ret = 1;

> > +			goto next_node;

> > +		}

> > +

> > +		if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) {

> > +			ret = dfu_write_by_name(fit_image_name,

> > +						(void *)update_addr,

> > +						update_size, NULL, NULL);

> > +			if (ret)

> > +				return ret;

> > +		}

> > +next_node:

> > +		noffset = fdt_next_node(fit, noffset, &ndepth);

> > +	}

> > +

> > +	return ret;

> > +}

> > +#endif /* CONFIG_UPDATE_FIT */

> > diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig

> > index d680b28ecf51..df0585c4fc83 100644

> > --- a/drivers/dfu/Kconfig

> > +++ b/drivers/dfu/Kconfig

> > @@ -22,6 +22,7 @@ config DFU_TFTP

> >  	bool "DFU via TFTP"

> >  	select DFU_ALT

> >  	select DFU_OVER_TFTP

> > +	select UPDATE_COMMON

> >  	help

> >  	  This option allows performing update of DFU-managed medium with data

> >  	  sent via TFTP boot.

> > diff --git a/include/image.h b/include/image.h

> > index 9a5a87dbf870..dce2997f9a6a 100644

> > --- a/include/image.h

> > +++ b/include/image.h

> > @@ -1592,4 +1592,16 @@ struct fit_loadable_tbl {

> >  		.handler = _handler, \

> >  	}

> >

> > +/**

> > + * fit_update - update storage with FIT image

> > + * @fit:        Pointer to FIT image

> > + *

> > + * Update firmware on storage using FIT image as input.

> > + * The storage area to be update will be indentified by the name

> > + * in FIT and matching it to "dfu_alt_info" variable.

> > + *

> > + * Return:      0 on success, non-zero otherwise

> > + */

> > +int fit_update(const void *fit);

> > +

> >  #endif	/* __IMAGE_H__ */

> >

>
diff mbox series

Patch

diff --git a/common/Kconfig b/common/Kconfig
index ca42ba37b726..86568dec2e25 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1014,6 +1014,20 @@  endmenu
 
 menu "Update support"
 
+config UPDATE_COMMON
+	bool
+	default n
+	select DFU_ALT
+
+config UPDATE_FIT
+	bool "Firmware update using fitImage"
+	depends on FIT
+	depends on DFU
+	select UPDATE_COMMON
+	help
+	  This option allows performing update of DFU-capable storage with
+	  data in fitImage.
+
 config ANDROID_AB
 	bool "Android A/B updates"
 	default n
diff --git a/common/Makefile b/common/Makefile
index 2e7a090588d9..bcf352d01652 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -53,8 +53,7 @@  obj-$(CONFIG_LCD_ROTATION) += lcd_console_rotation.o
 obj-$(CONFIG_LCD_DT_SIMPLEFB) += lcd_simplefb.o
 obj-$(CONFIG_LYNXKDI) += lynxkdi.o
 obj-$(CONFIG_MENU) += menu.o
-obj-$(CONFIG_UPDATE_TFTP) += update.o
-obj-$(CONFIG_DFU_TFTP) += update.o
+obj-$(CONFIG_UPDATE_COMMON) += update.o
 obj-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
 obj-$(CONFIG_CMDLINE) += cli_readline.o cli_simple.o
 
diff --git a/common/update.c b/common/update.c
index f82d77cc0be9..2c75b37f19e6 100644
--- a/common/update.c
+++ b/common/update.c
@@ -23,6 +23,7 @@ 
 #include <dfu.h>
 #include <errno.h>
 
+#ifdef CONFIG_DFU_TFTP
 /* env variable holding the location of the update file */
 #define UPDATE_FILE_ENV		"updatefile"
 
@@ -89,6 +90,7 @@  static int update_load(char *filename, ulong msec_max, int cnt_max, ulong addr)
 
 	return rv;
 }
+#endif /* CONFIG_DFU_TFTP */
 
 static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
 						ulong *fladdr, ulong *size)
@@ -106,6 +108,7 @@  static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
 	return 0;
 }
 
+#ifdef CONFIG_DFU_TFTP
 int update_tftp(ulong addr, char *interface, char *devstring)
 {
 	char *filename, *env_addr, *fit_image_name;
@@ -194,3 +197,71 @@  next_node:
 
 	return ret;
 }
+#endif /* CONFIG_DFU_UPDATE */
+
+#ifdef CONFIG_UPDATE_FIT
+/**
+ * fit_update - update storage with FIT image
+ * @fit:	Pointer to FIT image
+ *
+ * Update firmware on storage using FIT image as input.
+ * The storage area to be update will be identified by the name
+ * in FIT and matching it to "dfu_alt_info" variable.
+ *
+ * Return:      0 - on success, non-zero - otherwise
+ */
+int fit_update(const void *fit)
+{
+	char *fit_image_name;
+	ulong update_addr, update_fladdr, update_size;
+	int images_noffset, ndepth, noffset;
+	int ret = 0;
+
+	if (!fit)
+		return -EINVAL;
+
+	if (!fit_check_format((void *)fit)) {
+		printf("Bad FIT format of the update file, aborting auto-update\n");
+		return -EINVAL;
+	}
+
+	/* process updates */
+	images_noffset = fdt_path_offset(fit, FIT_IMAGES_PATH);
+
+	ndepth = 0;
+	noffset = fdt_next_node(fit, images_noffset, &ndepth);
+	while (noffset >= 0 && ndepth > 0) {
+		if (ndepth != 1)
+			goto next_node;
+
+		fit_image_name = (char *)fit_get_name(fit, noffset, NULL);
+		printf("Processing update '%s' :", fit_image_name);
+
+		if (!fit_image_verify(fit, noffset)) {
+			printf("Error: invalid update hash, aborting\n");
+			ret = 1;
+			goto next_node;
+		}
+
+		printf("\n");
+		if (update_fit_getparams(fit, noffset, &update_addr,
+					 &update_fladdr, &update_size)) {
+			printf("Error: can't get update parameters, aborting\n");
+			ret = 1;
+			goto next_node;
+		}
+
+		if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) {
+			ret = dfu_write_by_name(fit_image_name,
+						(void *)update_addr,
+						update_size, NULL, NULL);
+			if (ret)
+				return ret;
+		}
+next_node:
+		noffset = fdt_next_node(fit, noffset, &ndepth);
+	}
+
+	return ret;
+}
+#endif /* CONFIG_UPDATE_FIT */
diff --git a/drivers/dfu/Kconfig b/drivers/dfu/Kconfig
index d680b28ecf51..df0585c4fc83 100644
--- a/drivers/dfu/Kconfig
+++ b/drivers/dfu/Kconfig
@@ -22,6 +22,7 @@  config DFU_TFTP
 	bool "DFU via TFTP"
 	select DFU_ALT
 	select DFU_OVER_TFTP
+	select UPDATE_COMMON
 	help
 	  This option allows performing update of DFU-managed medium with data
 	  sent via TFTP boot.
diff --git a/include/image.h b/include/image.h
index 9a5a87dbf870..dce2997f9a6a 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1592,4 +1592,16 @@  struct fit_loadable_tbl {
 		.handler = _handler, \
 	}
 
+/**
+ * fit_update - update storage with FIT image
+ * @fit:        Pointer to FIT image
+ *
+ * Update firmware on storage using FIT image as input.
+ * The storage area to be update will be indentified by the name
+ * in FIT and matching it to "dfu_alt_info" variable.
+ *
+ * Return:      0 on success, non-zero otherwise
+ */
+int fit_update(const void *fit);
+
 #endif	/* __IMAGE_H__ */