All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] dfu: remove UPDATE_TFTP
@ 2020-07-15 10:45 Heinrich Schuchardt
  2020-07-15 11:24 ` Lukasz Majewski
  2020-07-16  1:36 ` AKASHI Takahiro
  0 siblings, 2 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-07-15 10:45 UTC (permalink / raw)
  To: u-boot

Using UPDATE_TFTP the firmware can be updated from tFTP by writing to NOR
flash. The same is possible by defining a dfu command in CONFIG_PREBOOT.

The dfu command cannot only write to NOR but also to other devices. In
README.dfutfp UPDATE_TFTP has been marked as deprecated. It is not used
by any board.

Remove tFTP update via CONFIG_UPDATE_TFTP.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
Currently Gitlab CI lacks a test for dfu tftp. I will try to set one up.
When it is properly tested I will send a final patch series.
But beforehand I would like to know if eliminating UPDATE_TFTP is ok.

Best regards

Heinrich
---
 README             |   8 ---
 common/Kconfig     |  17 ------
 common/main.c      |   3 -
 common/update.c    | 147 +--------------------------------------------
 doc/README.dfutftp |   3 -
 doc/README.update  |  97 ------------------------------
 6 files changed, 2 insertions(+), 273 deletions(-)
 delete mode 100644 doc/README.update

diff --git a/README b/README
index 2384966a39..c53f72cfa6 100644
--- a/README
+++ b/README
@@ -2104,14 +2104,6 @@ The following options need to be configured:

 		Please see board_init_f function.

-- Automatic software updates via TFTP server
-		CONFIG_UPDATE_TFTP
-		CONFIG_UPDATE_TFTP_CNT_MAX
-		CONFIG_UPDATE_TFTP_MSEC_MAX
-
-		These options enable and control the auto-update feature;
-		for a more detailed description refer to doc/README.update.
-
 - MTD Support (mtdparts command, UBI support)
 		CONFIG_MTD_UBI_WL_THRESHOLD
 		This parameter defines the maximum difference between the highest
diff --git a/common/Kconfig b/common/Kconfig
index 67b3818fde..ca42ba37b7 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1014,23 +1014,6 @@ endmenu

 menu "Update support"

-config UPDATE_TFTP
-	bool "Auto-update using fitImage via TFTP"
-	depends on FIT
-	help
-	  This option allows performing update of NOR with data in fitImage
-	  sent via TFTP boot.
-
-config UPDATE_TFTP_CNT_MAX
-	int "The number of connection retries during auto-update"
-	default 0
-	depends on UPDATE_TFTP
-
-config UPDATE_TFTP_MSEC_MAX
-	int "Delay in mSec to wait for the TFTP server during auto-update"
-	default 100
-	depends on UPDATE_TFTP
-
 config ANDROID_AB
 	bool "Android A/B updates"
 	default n
diff --git a/common/main.c b/common/main.c
index 4b3cd302c3..62ab3344e5 100644
--- a/common/main.c
+++ b/common/main.c
@@ -50,9 +50,6 @@ void main_loop(void)
 	if (IS_ENABLED(CONFIG_USE_PREBOOT))
 		run_preboot_environment_command();

-	if (IS_ENABLED(CONFIG_UPDATE_TFTP))
-		update_tftp(0UL, NULL, NULL);
-
 	s = bootdelay_process();
 	if (cli_process_fdt(&s))
 		cli_secure_boot_cmd(s);
diff --git a/common/update.c b/common/update.c
index c8dd346a09..caf74e63db 100644
--- a/common/update.c
+++ b/common/update.c
@@ -14,10 +14,6 @@
 #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature"
 #endif

-#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH)
-#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour"
-#endif
-
 #include <command.h>
 #include <env.h>
 #include <flash.h>
@@ -26,7 +22,6 @@
 #include <malloc.h>
 #include <dfu.h>
 #include <errno.h>
-#include <mtd/cfi_flash.h>

 /* env variable holding the location of the update file */
 #define UPDATE_FILE_ENV		"updatefile"
@@ -46,10 +41,7 @@

 extern ulong tftp_timeout_ms;
 extern int tftp_timeout_count_max;
-#ifdef CONFIG_MTD_NOR_FLASH
-extern flash_info_t flash_info[];
-static uchar *saved_prot_info;
-#endif
+
 static int update_load(char *filename, ulong msec_max, int cnt_max, ulong addr)
 {
 	int size, rv;
@@ -98,122 +90,6 @@ static int update_load(char *filename, ulong msec_max, int cnt_max, ulong addr)
 	return rv;
 }

-#ifdef CONFIG_MTD_NOR_FLASH
-static int update_flash_protect(int prot, ulong addr_first, ulong addr_last)
-{
-	uchar *sp_info_ptr;
-	ulong s;
-	int i, bank, cnt;
-	flash_info_t *info;
-
-	sp_info_ptr = NULL;
-
-	if (prot == 0) {
-		saved_prot_info =
-			calloc(CONFIG_SYS_MAX_FLASH_BANKS * CONFIG_SYS_MAX_FLASH_SECT, 1);
-		if (!saved_prot_info)
-			return 1;
-	}
-
-	for (bank = 0; bank < CONFIG_SYS_MAX_FLASH_BANKS; ++bank) {
-		cnt = 0;
-		info = &flash_info[bank];
-
-		/* Nothing to do if the bank doesn't exist */
-		if (info->sector_count == 0)
-			return 0;
-
-		/* Point to current bank protection information */
-		sp_info_ptr = saved_prot_info + (bank * CONFIG_SYS_MAX_FLASH_SECT);
-
-		/*
-		 * Adjust addr_first or addr_last if we are on bank boundary.
-		 * Address space between banks must be continuous for other
-		 * flash functions (like flash_sect_erase or flash_write) to
-		 * succeed. Banks must also be numbered in correct order,
-		 * according to increasing addresses.
-		 */
-		if (addr_last > info->start[0] + info->size - 1)
-			addr_last = info->start[0] + info->size - 1;
-		if (addr_first < info->start[0])
-			addr_first = info->start[0];
-
-		for (i = 0; i < info->sector_count; i++) {
-			/* Save current information about protected sectors */
-			if (prot == 0) {
-				s = info->start[i];
-				if ((s >= addr_first) && (s <= addr_last))
-					sp_info_ptr[i] = info->protect[i];
-
-			}
-
-			/* Protect/unprotect sectors */
-			if (sp_info_ptr[i] == 1) {
-#if defined(CONFIG_SYS_FLASH_PROTECTION)
-				if (flash_real_protect(info, i, prot))
-					return 1;
-#else
-				info->protect[i] = prot;
-#endif
-				cnt++;
-			}
-		}
-
-		if (cnt) {
-			printf("%sProtected %d sectors\n",
-						prot ? "": "Un-", cnt);
-		}
-	}
-
-	if((prot == 1) && saved_prot_info)
-		free(saved_prot_info);
-
-	return 0;
-}
-#endif
-
-static int update_flash(ulong addr_source, ulong addr_first, ulong size)
-{
-#ifdef CONFIG_MTD_NOR_FLASH
-	ulong addr_last = addr_first + size - 1;
-
-	/* round last address to the sector boundary */
-	if (flash_sect_roundb(&addr_last) > 0)
-		return 1;
-
-	if (addr_first >= addr_last) {
-		printf("Error: end address exceeds addressing space\n");
-		return 1;
-	}
-
-	/* remove protection on processed sectors */
-	if (update_flash_protect(0, addr_first, addr_last) > 0) {
-		printf("Error: could not unprotect flash sectors\n");
-		return 1;
-	}
-
-	printf("Erasing 0x%08lx - 0x%08lx", addr_first, addr_last);
-	if (flash_sect_erase(addr_first, addr_last) > 0) {
-		printf("Error: could not erase flash\n");
-		return 1;
-	}
-
-	printf("Copying to flash...");
-	if (flash_write((char *)addr_source, addr_first, size) > 0) {
-		printf("Error: could not copy to flash\n");
-		return 1;
-	}
-	printf("done\n");
-
-	/* enable protection on processed sectors */
-	if (update_flash_protect(1, addr_first, addr_last) > 0) {
-		printf("Error: could not protect flash sectors\n");
-		return 1;
-	}
-#endif
-	return 0;
-}
-
 static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
 						ulong *fladdr, ulong *size)
 {
@@ -235,20 +111,9 @@ int update_tftp(ulong addr, char *interface, char *devstring)
 	char *filename, *env_addr, *fit_image_name;
 	ulong update_addr, update_fladdr, update_size;
 	int images_noffset, ndepth, noffset;
-	bool update_tftp_dfu;
 	int ret = 0;
 	void *fit;

-	if (interface == NULL && devstring == NULL) {
-		update_tftp_dfu = false;
-	} else if (interface && devstring) {
-		update_tftp_dfu = true;
-	} else {
-		pr_err("Interface: %s and devstring: %s not supported!\n",
-		      interface, devstring);
-		return -EINVAL;
-	}
-
 	/* use already present image */
 	if (addr)
 		goto got_update_file;
@@ -315,15 +180,7 @@ got_update_file:
 			goto next_node;
 		}

-		if (!update_tftp_dfu) {
-			if (update_flash(update_addr, update_fladdr,
-					 update_size)) {
-				printf("Error: can't flash update, aborting\n");
-				ret = 1;
-				goto next_node;
-			}
-		} else if (fit_image_check_type(fit, noffset,
-						IH_TYPE_FIRMWARE)) {
+		if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) {
 			ret = dfu_tftp_write(fit_image_name, update_addr,
 					     update_size, interface, devstring);
 			if (ret)
diff --git a/doc/README.dfutftp b/doc/README.dfutftp
index a3341bbb61..b279f542a1 100644
--- a/doc/README.dfutftp
+++ b/doc/README.dfutftp
@@ -52,9 +52,6 @@ Environment variables

 The "dfu tftp" command can be used in the "preboot" environment variable
 (when it is enabled by defining CONFIG_PREBOOT).
-This is the preferable way of using this command in the early boot stage
-as opposed to legacy update_tftp() function invocation.
-

 Beagle Bone Black (BBB) setup
 -----------------------------
diff --git a/doc/README.update b/doc/README.update
deleted file mode 100644
index bf4379279e..0000000000
--- a/doc/README.update
+++ /dev/null
@@ -1,97 +0,0 @@
-Automatic software update from a TFTP server
-============================================
-
-Overview
---------
-
-This feature allows to automatically store software updates present on a TFTP
-server in NOR Flash. In more detail: a TFTP transfer of a file given in
-environment variable 'updatefile' from server 'serverip' is attempted during
-boot. The update file should be a FIT file, and can contain one or more
-updates. Each update in the update file has an address in NOR Flash where it
-should be placed, updates are also protected with a SHA-1 checksum. If the
-TFTP transfer is successful, the hash of each update is verified, and if the
-verification is positive, the update is stored in Flash.
-
-The auto-update feature is enabled by the CONFIG_UPDATE_TFTP macro:
-
-#define CONFIG_UPDATE_TFTP		1
-
-
-Note that when enabling auto-update, Flash support must be turned on.  Also,
-one must enable FIT and LIBFDT support:
-
-#define CONFIG_FIT		1
-#define CONFIG_OF_LIBFDT	1
-
-The auto-update feature uses the following configuration knobs:
-
-- CONFIG_UPDATE_LOAD_ADDR
-
-  Normally, TFTP transfer of the update file is done to the address specified
-  in environment variable 'loadaddr'. If this variable is not present, the
-  transfer is made to the address given in CONFIG_UPDATE_LOAD_ADDR (0x100000
-  by default).
-
-- CONFIG_UPDATE_TFTP_CNT_MAX
-  CONFIG_UPDATE_TFTP_MSEC_MAX
-
-  These knobs control the timeouts during initial connection to the TFTP
-  server. Since a transfer is attempted during each boot, it is undesirable to
-  have a long delay when a TFTP server is not present.
-  CONFIG_UPDATE_TFTP_MSEC_MAX specifies the number of milliseconds to wait for
-  the server to respond to initial connection, and CONFIG_UPDATE_TFTP_CNT_MAX
-  gives the number of such connection retries. CONFIG_UPDATE_TFTP_CNT_MAX must
-  be non-negative and is 0 by default, CONFIG_UPDATE_TFTP_MSEC_MAX must be
-  positive and is 100 by default.
-
-Since the update file is in FIT format, it is created from an *.its file using
-the mkimage tool. dtc tool with support for binary includes, e.g. in version
-1.2.0 or later, must also be available on the system where the update file is
-to be prepared. Refer to the doc/uImage.FIT/ directory for more details on FIT
-images.
-
-
-Example .its files
-------------------
-
-- doc/uImage.FIT/update_uboot.its
-
-  A simple example that can be used to create an update file for automatically
-  replacing U-Boot image on a system.
-
-  Assuming that an U-Boot image u-boot.bin is present in the current working
-  directory, and that the address given in the 'load' property in the
-  'update_uboot.its' file is where the U-Boot is stored in Flash, the
-  following command will create the actual update file 'update_uboot.itb':
-
-  mkimage -f update_uboot.its update_uboot.itb
-
-  Place 'update_uboot.itb' on a TFTP server, for example as
-  '/tftpboot/update_uboot.itb', and set the 'updatefile' variable
-  appropriately, for example in the U-Boot prompt:
-
-  setenv updatefile /tftpboot/update_uboot.itb
-  saveenv
-
-  Now, when the system boots up and the update TFTP server specified in the
-  'serverip' environment variable is accessible, the new U-Boot image will be
-  automatically stored in Flash.
-
-  NOTE: do make sure that the 'u-boot.bin' image used to create the update
-  file is a good, working image. Also make sure that the address in Flash
-  where the update will be placed is correct. Making mistake here and
-  attempting the auto-update can render the system unusable.
-
-- doc/uImage.FIT/update3.its
-
-  An example containing three updates. It can be used to update Linux kernel,
-  ramdisk and FDT blob stored in Flash. The procedure for preparing the update
-  file is similar to the example above.
-
-TFTP update via DFU
--------------------
-
-- It is now possible to update firmware (bootloader, kernel, rootfs, etc.) via
-  TFTP by using DFU (Device Firmware Upgrade). More information can be found in
-  ./doc/README.dfutftp documentation entry.
--
2.27.0

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-15 10:45 [RFC] dfu: remove UPDATE_TFTP Heinrich Schuchardt
@ 2020-07-15 11:24 ` Lukasz Majewski
  2020-07-15 11:49   ` Heinrich Schuchardt
  2020-07-16  1:36 ` AKASHI Takahiro
  1 sibling, 1 reply; 14+ messages in thread
From: Lukasz Majewski @ 2020-07-15 11:24 UTC (permalink / raw)
  To: u-boot

Hi Heinrich,

> Using UPDATE_TFTP the firmware can be updated from tFTP by writing to
> NOR flash. The same is possible by defining a dfu command in
> CONFIG_PREBOOT.
> 
> The dfu command cannot only write to NOR but also to other devices. In
> README.dfutfp UPDATE_TFTP has been marked as deprecated.

Could you also write in the proper README the steps necessary to have
the same functionality as with UPDATE_TFTP with dfu and preboot? I
think that it would be good to have it written down in ./doc.

> It is not
> used by any board.
> 
> Remove tFTP update via CONFIG_UPDATE_TFTP.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> Currently Gitlab CI lacks a test for dfu tftp. I will try to set one
> up. When it is properly tested I will send a final patch series.
> But beforehand I would like to know if eliminating UPDATE_TFTP is ok.
> 
> Best regards
> 
> Heinrich
> ---
>  README             |   8 ---
>  common/Kconfig     |  17 ------
>  common/main.c      |   3 -
>  common/update.c    | 147
> +-------------------------------------------- doc/README.dfutftp |
> 3 - doc/README.update  |  97 ------------------------------
>  6 files changed, 2 insertions(+), 273 deletions(-)
>  delete mode 100644 doc/README.update
> 
> diff --git a/README b/README
> index 2384966a39..c53f72cfa6 100644
> --- a/README
> +++ b/README
> @@ -2104,14 +2104,6 @@ The following options need to be configured:
> 
>  		Please see board_init_f function.
> 
> -- Automatic software updates via TFTP server
> -		CONFIG_UPDATE_TFTP
> -		CONFIG_UPDATE_TFTP_CNT_MAX
> -		CONFIG_UPDATE_TFTP_MSEC_MAX
> -
> -		These options enable and control the auto-update
> feature;
> -		for a more detailed description refer to
> doc/README.update. -
>  - MTD Support (mtdparts command, UBI support)
>  		CONFIG_MTD_UBI_WL_THRESHOLD
>  		This parameter defines the maximum difference
> between the highest diff --git a/common/Kconfig b/common/Kconfig
> index 67b3818fde..ca42ba37b7 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -1014,23 +1014,6 @@ endmenu
> 
>  menu "Update support"
> 
> -config UPDATE_TFTP
> -	bool "Auto-update using fitImage via TFTP"
> -	depends on FIT
> -	help
> -	  This option allows performing update of NOR with data in
> fitImage
> -	  sent via TFTP boot.
> -
> -config UPDATE_TFTP_CNT_MAX
> -	int "The number of connection retries during auto-update"
> -	default 0
> -	depends on UPDATE_TFTP
> -
> -config UPDATE_TFTP_MSEC_MAX
> -	int "Delay in mSec to wait for the TFTP server during
> auto-update"
> -	default 100
> -	depends on UPDATE_TFTP
> -
>  config ANDROID_AB
>  	bool "Android A/B updates"
>  	default n
> diff --git a/common/main.c b/common/main.c
> index 4b3cd302c3..62ab3344e5 100644
> --- a/common/main.c
> +++ b/common/main.c
> @@ -50,9 +50,6 @@ void main_loop(void)
>  	if (IS_ENABLED(CONFIG_USE_PREBOOT))
>  		run_preboot_environment_command();
> 
> -	if (IS_ENABLED(CONFIG_UPDATE_TFTP))
> -		update_tftp(0UL, NULL, NULL);
> -
>  	s = bootdelay_process();
>  	if (cli_process_fdt(&s))
>  		cli_secure_boot_cmd(s);
> diff --git a/common/update.c b/common/update.c
> index c8dd346a09..caf74e63db 100644
> --- a/common/update.c
> +++ b/common/update.c
> @@ -14,10 +14,6 @@
>  #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update
> feature" #endif
> 
> -#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH)
> -#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for
> legacy behaviour" -#endif
> -
>  #include <command.h>
>  #include <env.h>
>  #include <flash.h>
> @@ -26,7 +22,6 @@
>  #include <malloc.h>
>  #include <dfu.h>
>  #include <errno.h>
> -#include <mtd/cfi_flash.h>
> 
>  /* env variable holding the location of the update file */
>  #define UPDATE_FILE_ENV		"updatefile"
> @@ -46,10 +41,7 @@
> 
>  extern ulong tftp_timeout_ms;
>  extern int tftp_timeout_count_max;
> -#ifdef CONFIG_MTD_NOR_FLASH
> -extern flash_info_t flash_info[];
> -static uchar *saved_prot_info;
> -#endif
> +
>  static int update_load(char *filename, ulong msec_max, int cnt_max,
> ulong addr) {
>  	int size, rv;
> @@ -98,122 +90,6 @@ static int update_load(char *filename, ulong
> msec_max, int cnt_max, ulong addr) return rv;
>  }
> 
> -#ifdef CONFIG_MTD_NOR_FLASH
> -static int update_flash_protect(int prot, ulong addr_first, ulong
> addr_last) -{
> -	uchar *sp_info_ptr;
> -	ulong s;
> -	int i, bank, cnt;
> -	flash_info_t *info;
> -
> -	sp_info_ptr = NULL;
> -
> -	if (prot == 0) {
> -		saved_prot_info =
> -			calloc(CONFIG_SYS_MAX_FLASH_BANKS *
> CONFIG_SYS_MAX_FLASH_SECT, 1);
> -		if (!saved_prot_info)
> -			return 1;
> -	}
> -
> -	for (bank = 0; bank < CONFIG_SYS_MAX_FLASH_BANKS; ++bank) {
> -		cnt = 0;
> -		info = &flash_info[bank];
> -
> -		/* Nothing to do if the bank doesn't exist */
> -		if (info->sector_count == 0)
> -			return 0;
> -
> -		/* Point to current bank protection information */
> -		sp_info_ptr = saved_prot_info + (bank *
> CONFIG_SYS_MAX_FLASH_SECT); -
> -		/*
> -		 * Adjust addr_first or addr_last if we are on bank
> boundary.
> -		 * Address space between banks must be continuous
> for other
> -		 * flash functions (like flash_sect_erase or
> flash_write) to
> -		 * succeed. Banks must also be numbered in correct
> order,
> -		 * according to increasing addresses.
> -		 */
> -		if (addr_last > info->start[0] + info->size - 1)
> -			addr_last = info->start[0] + info->size - 1;
> -		if (addr_first < info->start[0])
> -			addr_first = info->start[0];
> -
> -		for (i = 0; i < info->sector_count; i++) {
> -			/* Save current information about protected
> sectors */
> -			if (prot == 0) {
> -				s = info->start[i];
> -				if ((s >= addr_first) && (s <=
> addr_last))
> -					sp_info_ptr[i] =
> info->protect[i]; -
> -			}
> -
> -			/* Protect/unprotect sectors */
> -			if (sp_info_ptr[i] == 1) {
> -#if defined(CONFIG_SYS_FLASH_PROTECTION)
> -				if (flash_real_protect(info, i,
> prot))
> -					return 1;
> -#else
> -				info->protect[i] = prot;
> -#endif
> -				cnt++;
> -			}
> -		}
> -
> -		if (cnt) {
> -			printf("%sProtected %d sectors\n",
> -						prot ? "": "Un-",
> cnt);
> -		}
> -	}
> -
> -	if((prot == 1) && saved_prot_info)
> -		free(saved_prot_info);
> -
> -	return 0;
> -}
> -#endif
> -
> -static int update_flash(ulong addr_source, ulong addr_first, ulong
> size) -{
> -#ifdef CONFIG_MTD_NOR_FLASH
> -	ulong addr_last = addr_first + size - 1;
> -
> -	/* round last address to the sector boundary */
> -	if (flash_sect_roundb(&addr_last) > 0)
> -		return 1;
> -
> -	if (addr_first >= addr_last) {
> -		printf("Error: end address exceeds addressing
> space\n");
> -		return 1;
> -	}
> -
> -	/* remove protection on processed sectors */
> -	if (update_flash_protect(0, addr_first, addr_last) > 0) {
> -		printf("Error: could not unprotect flash sectors\n");
> -		return 1;
> -	}
> -
> -	printf("Erasing 0x%08lx - 0x%08lx", addr_first, addr_last);
> -	if (flash_sect_erase(addr_first, addr_last) > 0) {
> -		printf("Error: could not erase flash\n");
> -		return 1;
> -	}
> -
> -	printf("Copying to flash...");
> -	if (flash_write((char *)addr_source, addr_first, size) > 0) {
> -		printf("Error: could not copy to flash\n");
> -		return 1;
> -	}
> -	printf("done\n");
> -
> -	/* enable protection on processed sectors */
> -	if (update_flash_protect(1, addr_first, addr_last) > 0) {
> -		printf("Error: could not protect flash sectors\n");
> -		return 1;
> -	}
> -#endif
> -	return 0;
> -}
> -
>  static int update_fit_getparams(const void *fit, int noffset, ulong
> *addr, ulong *fladdr, ulong *size)
>  {
> @@ -235,20 +111,9 @@ int update_tftp(ulong addr, char *interface,
> char *devstring) char *filename, *env_addr, *fit_image_name;
>  	ulong update_addr, update_fladdr, update_size;
>  	int images_noffset, ndepth, noffset;
> -	bool update_tftp_dfu;
>  	int ret = 0;
>  	void *fit;
> 
> -	if (interface == NULL && devstring == NULL) {
> -		update_tftp_dfu = false;
> -	} else if (interface && devstring) {
> -		update_tftp_dfu = true;
> -	} else {
> -		pr_err("Interface: %s and devstring: %s not
> supported!\n",
> -		      interface, devstring);
> -		return -EINVAL;
> -	}
> -
>  	/* use already present image */
>  	if (addr)
>  		goto got_update_file;
> @@ -315,15 +180,7 @@ got_update_file:
>  			goto next_node;
>  		}
> 
> -		if (!update_tftp_dfu) {
> -			if (update_flash(update_addr, update_fladdr,
> -					 update_size)) {
> -				printf("Error: can't flash update,
> aborting\n");
> -				ret = 1;
> -				goto next_node;
> -			}
> -		} else if (fit_image_check_type(fit, noffset,
> -						IH_TYPE_FIRMWARE)) {
> +		if (fit_image_check_type(fit, noffset,
> IH_TYPE_FIRMWARE)) { ret = dfu_tftp_write(fit_image_name, update_addr,
>  					     update_size, interface,
> devstring); if (ret)
> diff --git a/doc/README.dfutftp b/doc/README.dfutftp
> index a3341bbb61..b279f542a1 100644
> --- a/doc/README.dfutftp
> +++ b/doc/README.dfutftp
> @@ -52,9 +52,6 @@ Environment variables
> 
>  The "dfu tftp" command can be used in the "preboot" environment
> variable (when it is enabled by defining CONFIG_PREBOOT).
> -This is the preferable way of using this command in the early boot
> stage -as opposed to legacy update_tftp() function invocation.
> -
> 
>  Beagle Bone Black (BBB) setup
>  -----------------------------
> diff --git a/doc/README.update b/doc/README.update
> deleted file mode 100644
> index bf4379279e..0000000000
> --- a/doc/README.update
> +++ /dev/null
> @@ -1,97 +0,0 @@
> -Automatic software update from a TFTP server
> -============================================
> -
> -Overview
> ---------
> -
> -This feature allows to automatically store software updates present
> on a TFTP -server in NOR Flash. In more detail: a TFTP transfer of a
> file given in -environment variable 'updatefile' from server
> 'serverip' is attempted during -boot. The update file should be a FIT
> file, and can contain one or more -updates. Each update in the update
> file has an address in NOR Flash where it -should be placed, updates
> are also protected with a SHA-1 checksum. If the -TFTP transfer is
> successful, the hash of each update is verified, and if the
> -verification is positive, the update is stored in Flash. -
> -The auto-update feature is enabled by the CONFIG_UPDATE_TFTP macro:
> -
> -#define CONFIG_UPDATE_TFTP		1
> -
> -
> -Note that when enabling auto-update, Flash support must be turned
> on.  Also, -one must enable FIT and LIBFDT support:
> -
> -#define CONFIG_FIT		1
> -#define CONFIG_OF_LIBFDT	1
> -
> -The auto-update feature uses the following configuration knobs:
> -
> -- CONFIG_UPDATE_LOAD_ADDR
> -
> -  Normally, TFTP transfer of the update file is done to the address
> specified
> -  in environment variable 'loadaddr'. If this variable is not
> present, the
> -  transfer is made to the address given in CONFIG_UPDATE_LOAD_ADDR
> (0x100000
> -  by default).
> -
> -- CONFIG_UPDATE_TFTP_CNT_MAX
> -  CONFIG_UPDATE_TFTP_MSEC_MAX
> -
> -  These knobs control the timeouts during initial connection to the
> TFTP
> -  server. Since a transfer is attempted during each boot, it is
> undesirable to
> -  have a long delay when a TFTP server is not present.
> -  CONFIG_UPDATE_TFTP_MSEC_MAX specifies the number of milliseconds
> to wait for
> -  the server to respond to initial connection, and
> CONFIG_UPDATE_TFTP_CNT_MAX
> -  gives the number of such connection retries.
> CONFIG_UPDATE_TFTP_CNT_MAX must
> -  be non-negative and is 0 by default, CONFIG_UPDATE_TFTP_MSEC_MAX
> must be
> -  positive and is 100 by default.
> -
> -Since the update file is in FIT format, it is created from an *.its
> file using -the mkimage tool. dtc tool with support for binary
> includes, e.g. in version -1.2.0 or later, must also be available on
> the system where the update file is -to be prepared. Refer to the
> doc/uImage.FIT/ directory for more details on FIT -images.
> -
> -
> -Example .its files
> -------------------
> -
> -- doc/uImage.FIT/update_uboot.its
> -
> -  A simple example that can be used to create an update file for
> automatically
> -  replacing U-Boot image on a system.
> -
> -  Assuming that an U-Boot image u-boot.bin is present in the current
> working
> -  directory, and that the address given in the 'load' property in the
> -  'update_uboot.its' file is where the U-Boot is stored in Flash, the
> -  following command will create the actual update file
> 'update_uboot.itb': -
> -  mkimage -f update_uboot.its update_uboot.itb
> -
> -  Place 'update_uboot.itb' on a TFTP server, for example as
> -  '/tftpboot/update_uboot.itb', and set the 'updatefile' variable
> -  appropriately, for example in the U-Boot prompt:
> -
> -  setenv updatefile /tftpboot/update_uboot.itb
> -  saveenv
> -
> -  Now, when the system boots up and the update TFTP server specified
> in the
> -  'serverip' environment variable is accessible, the new U-Boot
> image will be
> -  automatically stored in Flash.
> -
> -  NOTE: do make sure that the 'u-boot.bin' image used to create the
> update
> -  file is a good, working image. Also make sure that the address in
> Flash
> -  where the update will be placed is correct. Making mistake here and
> -  attempting the auto-update can render the system unusable.
> -
> -- doc/uImage.FIT/update3.its
> -
> -  An example containing three updates. It can be used to update
> Linux kernel,
> -  ramdisk and FDT blob stored in Flash. The procedure for preparing
> the update
> -  file is similar to the example above.
> -
> -TFTP update via DFU
> --------------------
> -
> -- It is now possible to update firmware (bootloader, kernel, rootfs,
> etc.) via
> -  TFTP by using DFU (Device Firmware Upgrade). More information can
> be found in
> -  ./doc/README.dfutftp documentation entry.
> --
> 2.27.0
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200715/45de4912/attachment.sig>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-15 11:24 ` Lukasz Majewski
@ 2020-07-15 11:49   ` Heinrich Schuchardt
  2020-07-15 13:23     ` Lukasz Majewski
  0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-07-15 11:49 UTC (permalink / raw)
  To: u-boot

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 15.07.20 13:24, Lukasz Majewski wrote:
> Hi Heinrich,
>
>> Using UPDATE_TFTP the firmware can be updated from tFTP by
>> writing to NOR flash. The same is possible by defining a dfu
>> command in CONFIG_PREBOOT.
>>
>> The dfu command cannot only write to NOR but also to other
>> devices. In README.dfutfp UPDATE_TFTP has been marked as
>> deprecated.
>
> Could you also write in the proper README the steps necessary to
> have the same functionality as with UPDATE_TFTP with dfu and
> preboot? I think that it would be good to have it written down in
> ./doc.
>

doc/README.dfutftp already describes the necessary steps as:

* enable CONFIG_DFU_TFTP
* enable CONFIG_PREBOOT
* put a string like "dfu tftp 0 mmc 0" into the "preboot" env variable

Do you want me to explicitly write that "dfu tftp 0 mtd 0" replaces
the former auto-update functionality?

Best regards

Heinrich


>> It is not used by any board.
>>
>> Remove tFTP update via CONFIG_UPDATE_TFTP.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> ---
>> Currently Gitlab CI lacks a test for dfu tftp. I will try to set
>> one up. When it is properly tested I will send a final patch
>> series. But beforehand I would like to know if eliminating
>> UPDATE_TFTP is ok.
>>
>> Best regards
>>
>> Heinrich
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEbcT5xx8ppvoGt20zxIHbvCwFGsQFAl8O7SYACgkQxIHbvCwF
GsSp9RAAkNdFFkYpaSit5367azX56eAt4eiHyD1bqCEYJB1Z3+f3P0xM0NjD2B+T
QNVHHNbE+fvEftCT5Sl28mZIhqTpqZnMv5TFAyj4c1NxEmHnBFoeQcNb9vqzNaX3
8txdrYV3Nx5fKZlFDa2NqX0/+2NMx4DjqnN2P71IaFrp98qQMR0b62OQOAR9CwNE
YEW+usIRyE/cPHvvYMzKTS/8e/jJTRQXtFJvFjU3xh3/4s4/fbQCIbJZwMACsMrJ
hYAoT0d2+VNWBEg/i0q/uUmUh5OuwnrSsYaI3MdqV5DpNHcsF6UTOhT6svE+MibV
v4gMO7D2yfX7Zob6cLGU9hmnio32U0xx4jRhWNZtphuX1mI4psKtZp5k9FdRGjgn
+hrbkjG1y8xbytFiw5quCk07E34x8AttHDDM76wF5wu7WrH0U4HBjoNucj9Q2S6j
ZBD3IP/bOwpO3x32tpcFid+GaWy7NRTQ6OtOL4jF/Qgjk7F714wP1N5+cO4qB+kF
QFaRw+4zOGo0W0DLD9WyexDuY7QyNDbfXYOE6ao/pXGfFfWCa2WJCt0JUv1TnH5L
AbCqaNILWDZDJ0YBaPjJCqbeiikCG859nr/AgEPR8ns8jJ2i/2ditk9yUuha27Ub
mLBjbhV9PYo+5K07xMkVis+DIIgCc3xOJtL8DoviiJMO0xLBKos=
=IzXA
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-15 11:49   ` Heinrich Schuchardt
@ 2020-07-15 13:23     ` Lukasz Majewski
  2020-07-15 14:06       ` Michal Simek
  0 siblings, 1 reply; 14+ messages in thread
From: Lukasz Majewski @ 2020-07-15 13:23 UTC (permalink / raw)
  To: u-boot

Hi Heinrich,

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> On 15.07.20 13:24, Lukasz Majewski wrote:
> > Hi Heinrich,
> >  
> >> Using UPDATE_TFTP the firmware can be updated from tFTP by
> >> writing to NOR flash. The same is possible by defining a dfu
> >> command in CONFIG_PREBOOT.
> >>
> >> The dfu command cannot only write to NOR but also to other
> >> devices. In README.dfutfp UPDATE_TFTP has been marked as
> >> deprecated.  
> >
> > Could you also write in the proper README the steps necessary to
> > have the same functionality as with UPDATE_TFTP with dfu and
> > preboot? I think that it would be good to have it written down in
> > ./doc.
> >  
> 
> doc/README.dfutftp already describes the necessary steps as:
> 
> * enable CONFIG_DFU_TFTP
> * enable CONFIG_PREBOOT
> * put a string like "dfu tftp 0 mmc 0" into the "preboot" env variable
> 
> Do you want me to explicitly write that "dfu tftp 0 mtd 0" replaces
> the former auto-update functionality?

No, this is fine. Thanks.

> 
> Best regards
> 
> Heinrich
> 
> 
> >> It is not used by any board.
> >>
> >> Remove tFTP update via CONFIG_UPDATE_TFTP.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> ---
> >> Currently Gitlab CI lacks a test for dfu tftp. I will try to set
> >> one up. When it is properly tested I will send a final patch
> >> series. But beforehand I would like to know if eliminating
> >> UPDATE_TFTP is ok.
> >>
> >> Best regards
> >>
> >> Heinrich  
> -----BEGIN PGP SIGNATURE-----
> 
> iQIzBAEBCAAdFiEEbcT5xx8ppvoGt20zxIHbvCwFGsQFAl8O7SYACgkQxIHbvCwF
> GsSp9RAAkNdFFkYpaSit5367azX56eAt4eiHyD1bqCEYJB1Z3+f3P0xM0NjD2B+T
> QNVHHNbE+fvEftCT5Sl28mZIhqTpqZnMv5TFAyj4c1NxEmHnBFoeQcNb9vqzNaX3
> 8txdrYV3Nx5fKZlFDa2NqX0/+2NMx4DjqnN2P71IaFrp98qQMR0b62OQOAR9CwNE
> YEW+usIRyE/cPHvvYMzKTS/8e/jJTRQXtFJvFjU3xh3/4s4/fbQCIbJZwMACsMrJ
> hYAoT0d2+VNWBEg/i0q/uUmUh5OuwnrSsYaI3MdqV5DpNHcsF6UTOhT6svE+MibV
> v4gMO7D2yfX7Zob6cLGU9hmnio32U0xx4jRhWNZtphuX1mI4psKtZp5k9FdRGjgn
> +hrbkjG1y8xbytFiw5quCk07E34x8AttHDDM76wF5wu7WrH0U4HBjoNucj9Q2S6j
> ZBD3IP/bOwpO3x32tpcFid+GaWy7NRTQ6OtOL4jF/Qgjk7F714wP1N5+cO4qB+kF
> QFaRw+4zOGo0W0DLD9WyexDuY7QyNDbfXYOE6ao/pXGfFfWCa2WJCt0JUv1TnH5L
> AbCqaNILWDZDJ0YBaPjJCqbeiikCG859nr/AgEPR8ns8jJ2i/2ditk9yUuha27Ub
> mLBjbhV9PYo+5K07xMkVis+DIIgCc3xOJtL8DoviiJMO0xLBKos=
> =IzXA
> -----END PGP SIGNATURE-----




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200715/1215cead/attachment.sig>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-15 13:23     ` Lukasz Majewski
@ 2020-07-15 14:06       ` Michal Simek
  2020-07-15 14:42         ` Heinrich Schuchardt
  0 siblings, 1 reply; 14+ messages in thread
From: Michal Simek @ 2020-07-15 14:06 UTC (permalink / raw)
  To: u-boot



On 15. 07. 20 15:23, Lukasz Majewski wrote:
> Hi Heinrich,
>
> On 15.07.20 13:24, Lukasz Majewski wrote:
>>>> Hi Heinrich,
>>>>  
>>>>> Using UPDATE_TFTP the firmware can be updated from tFTP by
>>>>> writing to NOR flash. The same is possible by defining a dfu
>>>>> command in CONFIG_PREBOOT.
>>>>>
>>>>> The dfu command cannot only write to NOR but also to other
>>>>> devices. In README.dfutfp UPDATE_TFTP has been marked as
>>>>> deprecated.  
>>>>
>>>> Could you also write in the proper README the steps necessary to
>>>> have the same functionality as with UPDATE_TFTP with dfu and
>>>> preboot? I think that it would be good to have it written down in
>>>> ./doc.
>>>>  
> 
> doc/README.dfutftp already describes the necessary steps as:
> 
> * enable CONFIG_DFU_TFTP
> * enable CONFIG_PREBOOT
> * put a string like "dfu tftp 0 mmc 0" into the "preboot" env variable
> 
> Do you want me to explicitly write that "dfu tftp 0 mtd 0" replaces
> the former auto-update functionality?

I am curious if that command is really dfu tftp 0 mtd 0.

I have just tested it on zynqmp with copying images to ram and command
is just dfu tftp ram 0.
It means I think it would be worth to list options which are really used
by different back ends because it is not fully clear from help how to
exactly define it.

dfu tftp [<interface> <dev>] [<addr>]
  - device firmware upgrade via TFTP
    on device <dev>, attached to interface
    <interface>
    [<addr>] - address where FIT image has been stored

Thanks,
Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200715/ca1d8383/attachment.sig>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-15 14:06       ` Michal Simek
@ 2020-07-15 14:42         ` Heinrich Schuchardt
  2020-07-15 14:48           ` Michal Simek
  2020-07-16  1:52           ` AKASHI Takahiro
  0 siblings, 2 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-07-15 14:42 UTC (permalink / raw)
  To: u-boot

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 15.07.20 16:06, Michal Simek wrote:
>
>
> On 15. 07. 20 15:23, Lukasz Majewski wrote:
>> Hi Heinrich,
>>
>> On 15.07.20 13:24, Lukasz Majewski wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>>> Using UPDATE_TFTP the firmware can be updated from tFTP
>>>>>> by writing to NOR flash. The same is possible by defining
>>>>>> a dfu command in CONFIG_PREBOOT.
>>>>>>
>>>>>> The dfu command cannot only write to NOR but also to
>>>>>> other devices. In README.dfutfp UPDATE_TFTP has been
>>>>>> marked as deprecated.
>>>>>
>>>>> Could you also write in the proper README the steps
>>>>> necessary to have the same functionality as with
>>>>> UPDATE_TFTP with dfu and preboot? I think that it would be
>>>>> good to have it written down in ./doc.
>>>>>
>>
>> doc/README.dfutftp already describes the necessary steps as:
>>
>> * enable CONFIG_DFU_TFTP * enable CONFIG_PREBOOT * put a string
>> like "dfu tftp 0 mmc 0" into the "preboot" env variable
>>
>> Do you want me to explicitly write that "dfu tftp 0 mtd 0"
>> replaces the former auto-update functionality?
>
> I am curious if that command is really dfu tftp 0 mtd 0.

Lukasz was asking about the replacement of the update that was
executed when running with CONFIG_UPDATE_TFTP=y. This function that I
am removing could only write to NOR flash. The update defined via
CONFIG_PREBOOT is much more flexible and can server other storage media.

The different accessible devices are described in doc/README.dfu.
"ram" is one of them.

Best regards

Heinrich

>
> I have just tested it on zynqmp with copying images to ram and
> command is just dfu tftp ram 0. It means I think it would be worth
> to list options which are really used by different back ends
> because it is not fully clear from help how to exactly define it.
>
> dfu tftp [<interface> <dev>] [<addr>] - device firmware upgrade via
> TFTP on device <dev>, attached to interface <interface> [<addr>] -
> address where FIT image has been stored
>
> Thanks, Michal
>

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEbcT5xx8ppvoGt20zxIHbvCwFGsQFAl8PFcUACgkQxIHbvCwF
GsSwLQ//bU8AISZ5q4cgLcnAVPYnBPbVtoPXlyRROnX6XW3eDjq506Ah0Rk8UwYH
lhLSM1m+bfJZzVtWUjymlRGjuaB1HLzcyd1PbfK0+50mMaVnFBV5PMU4ue6Ju+zT
BukpUvtQ4KLpooo2QhjCz4/oXmur782RUJGq6hb7mDbk6R0+L8coqoiglLid5374
GdOQhgTAa8jWu0QxIBbdeOOrTSZz1kOBaShYffKXfBCaUfMOf/2T6sxDrSuTB8/N
WTeKdbTAsx5BVEY8s9EaAy6a7kWf8mb/7LdooFUMQ2noNbdw/ucfiNCA+H2JNTK1
GjvzDgViWAF0A0sXu6c1fqFT+4xo2lpRvG0wXsz3zRwpggmlAVX5PPi7o4Xj2HVF
7HPBVxz4AjJ3YpOpyLm0rEVEXnwBdke/jy/2r0VRyn5QMFaqAMEkrA39/wHrjj6t
0s4EUeuy5IjMf51zUrTAE/J9xHowLwjldODRgwmWoEYShJRgZp+J0yr2nhGTwpyS
91ESrHzkok/5BPkYFYGulBeeJFDOBegA7N7xec5e6YsXrWEPDiOpS5I1AfbycceV
lERhJpHIgPPjpaMgNcATKeti85o7veNe31IMoW758i5kbjQFOB8NlvtSSu97/2/K
eB/dIrOdnzC+URaun+MEt0T4HYPYgEuGAsBPQJKYrTw0sFhJRkg=
=JXjF
-----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-15 14:42         ` Heinrich Schuchardt
@ 2020-07-15 14:48           ` Michal Simek
  2020-07-21  3:03             ` Heinrich Schuchardt
  2020-07-16  1:52           ` AKASHI Takahiro
  1 sibling, 1 reply; 14+ messages in thread
From: Michal Simek @ 2020-07-15 14:48 UTC (permalink / raw)
  To: u-boot



On 15. 07. 20 16:42, Heinrich Schuchardt wrote:
> On 15.07.20 16:06, Michal Simek wrote:
> 
> 
>> On 15. 07. 20 15:23, Lukasz Majewski wrote:
>>> Hi Heinrich,
>>>
>>> On 15.07.20 13:24, Lukasz Majewski wrote:
>>>>>> Hi Heinrich,
>>>>>>
>>>>>>> Using UPDATE_TFTP the firmware can be updated from tFTP
>>>>>>> by writing to NOR flash. The same is possible by defining
>>>>>>> a dfu command in CONFIG_PREBOOT.
>>>>>>>
>>>>>>> The dfu command cannot only write to NOR but also to
>>>>>>> other devices. In README.dfutfp UPDATE_TFTP has been
>>>>>>> marked as deprecated.
>>>>>>
>>>>>> Could you also write in the proper README the steps
>>>>>> necessary to have the same functionality as with
>>>>>> UPDATE_TFTP with dfu and preboot? I think that it would be
>>>>>> good to have it written down in ./doc.
>>>>>>
>>>
>>> doc/README.dfutftp already describes the necessary steps as:
>>>
>>> * enable CONFIG_DFU_TFTP * enable CONFIG_PREBOOT * put a string
>>> like "dfu tftp 0 mmc 0" into the "preboot" env variable
>>>
>>> Do you want me to explicitly write that "dfu tftp 0 mtd 0"
>>> replaces the former auto-update functionality?
> 
>> I am curious if that command is really dfu tftp 0 mtd 0.
> 
> Lukasz was asking about the replacement of the update that was
> executed when running with CONFIG_UPDATE_TFTP=y. This function that I
> am removing could only write to NOR flash. The update defined via
> CONFIG_PREBOOT is much more flexible and can server other storage media.

I have really not a problem with removing it.

> 
> The different accessible devices are described in doc/README.dfu.
> "ram" is one of them.

yes and link to README.dfutftp where only one options is listed
as "dfu tftp 0 mmc 0" which is definitely only one option which you can
use. Would be really good to list that options there and also retest them.

It means as the part of removal we should check if README.dfutftp is
really up to date because I don't think it is clear how users should use
it for different cases. (USB cases are described properly in README.dfu
for different backends).

Thanks,
Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200715/18dc5d6a/attachment.sig>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-15 10:45 [RFC] dfu: remove UPDATE_TFTP Heinrich Schuchardt
  2020-07-15 11:24 ` Lukasz Majewski
@ 2020-07-16  1:36 ` AKASHI Takahiro
  2020-07-16  4:13   ` Heinrich Schuchardt
  1 sibling, 1 reply; 14+ messages in thread
From: AKASHI Takahiro @ 2020-07-16  1:36 UTC (permalink / raw)
  To: u-boot

Heinrich,

On Wed, Jul 15, 2020 at 12:45:18PM +0200, Heinrich Schuchardt wrote:
> Using UPDATE_TFTP the firmware can be updated from tFTP by writing to NOR
> flash. The same is possible by defining a dfu command in CONFIG_PREBOOT.
> 
> The dfu command cannot only write to NOR but also to other devices. In
> README.dfutfp UPDATE_TFTP has been marked as deprecated. It is not used
> by any board.
> 
> Remove tFTP update via CONFIG_UPDATE_TFTP.

This description is not accurate.

> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> Currently Gitlab CI lacks a test for dfu tftp. I will try to set one up.
> When it is properly tested I will send a final patch series.
> But beforehand I would like to know if eliminating UPDATE_TFTP is ok.
> 
> Best regards
> 
> Heinrich
> ---
>  README             |   8 ---
>  common/Kconfig     |  17 ------
>  common/main.c      |   3 -
>  common/update.c    | 147 +--------------------------------------------
>  doc/README.dfutftp |   3 -
>  doc/README.update  |  97 ------------------------------
>  6 files changed, 2 insertions(+), 273 deletions(-)
>  delete mode 100644 doc/README.update
> 
> diff --git a/README b/README
> index 2384966a39..c53f72cfa6 100644
> --- a/README
> +++ b/README
> @@ -2104,14 +2104,6 @@ The following options need to be configured:
> 
>  		Please see board_init_f function.
> 
> -- Automatic software updates via TFTP server
> -		CONFIG_UPDATE_TFTP
> -		CONFIG_UPDATE_TFTP_CNT_MAX
> -		CONFIG_UPDATE_TFTP_MSEC_MAX
> -
> -		These options enable and control the auto-update feature;
> -		for a more detailed description refer to doc/README.update.
> -
>  - MTD Support (mtdparts command, UBI support)
>  		CONFIG_MTD_UBI_WL_THRESHOLD
>  		This parameter defines the maximum difference between the highest
> diff --git a/common/Kconfig b/common/Kconfig
> index 67b3818fde..ca42ba37b7 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -1014,23 +1014,6 @@ endmenu
> 
>  menu "Update support"
> 
> -config UPDATE_TFTP
> -	bool "Auto-update using fitImage via TFTP"
> -	depends on FIT
> -	help
> -	  This option allows performing update of NOR with data in fitImage
> -	  sent via TFTP boot.

NAK.
If this configuration is removed, common/update.c won't be compiled in.
There is a chain of dependency:
DFU -> DFU_TFTP -> DFU_OVER_TFTP ->(implicitly) UPDATE_TFTP

> -config UPDATE_TFTP_CNT_MAX
> -	int "The number of connection retries during auto-update"
> -	default 0
> -	depends on UPDATE_TFTP
> -
> -config UPDATE_TFTP_MSEC_MAX
> -	int "Delay in mSec to wait for the TFTP server during auto-update"
> -	default 100
> -	depends on UPDATE_TFTP
> -
>  config ANDROID_AB
>  	bool "Android A/B updates"
>  	default n
> diff --git a/common/main.c b/common/main.c
> index 4b3cd302c3..62ab3344e5 100644
> --- a/common/main.c
> +++ b/common/main.c
> @@ -50,9 +50,6 @@ void main_loop(void)
>  	if (IS_ENABLED(CONFIG_USE_PREBOOT))
>  		run_preboot_environment_command();
> 
> -	if (IS_ENABLED(CONFIG_UPDATE_TFTP))
> -		update_tftp(0UL, NULL, NULL);
> -
>  	s = bootdelay_process();
>  	if (cli_process_fdt(&s))
>  		cli_secure_boot_cmd(s);
> diff --git a/common/update.c b/common/update.c
> index c8dd346a09..caf74e63db 100644
> --- a/common/update.c
> +++ b/common/update.c
> @@ -14,10 +14,6 @@
>  #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update feature"
>  #endif
> 
> -#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH)
> -#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for legacy behaviour"
> -#endif
> -
>  #include <command.h>
>  #include <env.h>
>  #include <flash.h>
> @@ -26,7 +22,6 @@
>  #include <malloc.h>
>  #include <dfu.h>
>  #include <errno.h>
> -#include <mtd/cfi_flash.h>
> 
>  /* env variable holding the location of the update file */
>  #define UPDATE_FILE_ENV		"updatefile"
> @@ -46,10 +41,7 @@
> 
>  extern ulong tftp_timeout_ms;
>  extern int tftp_timeout_count_max;
> -#ifdef CONFIG_MTD_NOR_FLASH
> -extern flash_info_t flash_info[];
> -static uchar *saved_prot_info;
> -#endif
> +
>  static int update_load(char *filename, ulong msec_max, int cnt_max, ulong addr)
>  {
>  	int size, rv;
> @@ -98,122 +90,6 @@ static int update_load(char *filename, ulong msec_max, int cnt_max, ulong addr)
>  	return rv;
>  }
> 
> -#ifdef CONFIG_MTD_NOR_FLASH
> -static int update_flash_protect(int prot, ulong addr_first, ulong addr_last)
> -{
> -	uchar *sp_info_ptr;
> -	ulong s;
> -	int i, bank, cnt;
> -	flash_info_t *info;
> -
> -	sp_info_ptr = NULL;
> -
> -	if (prot == 0) {
> -		saved_prot_info =
> -			calloc(CONFIG_SYS_MAX_FLASH_BANKS * CONFIG_SYS_MAX_FLASH_SECT, 1);
> -		if (!saved_prot_info)
> -			return 1;
> -	}
> -
> -	for (bank = 0; bank < CONFIG_SYS_MAX_FLASH_BANKS; ++bank) {
> -		cnt = 0;
> -		info = &flash_info[bank];
> -
> -		/* Nothing to do if the bank doesn't exist */
> -		if (info->sector_count == 0)
> -			return 0;
> -
> -		/* Point to current bank protection information */
> -		sp_info_ptr = saved_prot_info + (bank * CONFIG_SYS_MAX_FLASH_SECT);
> -
> -		/*
> -		 * Adjust addr_first or addr_last if we are on bank boundary.
> -		 * Address space between banks must be continuous for other
> -		 * flash functions (like flash_sect_erase or flash_write) to
> -		 * succeed. Banks must also be numbered in correct order,
> -		 * according to increasing addresses.
> -		 */
> -		if (addr_last > info->start[0] + info->size - 1)
> -			addr_last = info->start[0] + info->size - 1;
> -		if (addr_first < info->start[0])
> -			addr_first = info->start[0];
> -
> -		for (i = 0; i < info->sector_count; i++) {
> -			/* Save current information about protected sectors */
> -			if (prot == 0) {
> -				s = info->start[i];
> -				if ((s >= addr_first) && (s <= addr_last))
> -					sp_info_ptr[i] = info->protect[i];
> -
> -			}
> -
> -			/* Protect/unprotect sectors */
> -			if (sp_info_ptr[i] == 1) {
> -#if defined(CONFIG_SYS_FLASH_PROTECTION)
> -				if (flash_real_protect(info, i, prot))
> -					return 1;
> -#else
> -				info->protect[i] = prot;
> -#endif
> -				cnt++;
> -			}
> -		}
> -
> -		if (cnt) {
> -			printf("%sProtected %d sectors\n",
> -						prot ? "": "Un-", cnt);
> -		}
> -	}
> -
> -	if((prot == 1) && saved_prot_info)
> -		free(saved_prot_info);
> -
> -	return 0;
> -}
> -#endif
> -
> -static int update_flash(ulong addr_source, ulong addr_first, ulong size)
> -{
> -#ifdef CONFIG_MTD_NOR_FLASH
> -	ulong addr_last = addr_first + size - 1;
> -
> -	/* round last address to the sector boundary */
> -	if (flash_sect_roundb(&addr_last) > 0)
> -		return 1;
> -
> -	if (addr_first >= addr_last) {
> -		printf("Error: end address exceeds addressing space\n");
> -		return 1;
> -	}
> -
> -	/* remove protection on processed sectors */
> -	if (update_flash_protect(0, addr_first, addr_last) > 0) {
> -		printf("Error: could not unprotect flash sectors\n");
> -		return 1;
> -	}
> -
> -	printf("Erasing 0x%08lx - 0x%08lx", addr_first, addr_last);
> -	if (flash_sect_erase(addr_first, addr_last) > 0) {
> -		printf("Error: could not erase flash\n");
> -		return 1;
> -	}
> -
> -	printf("Copying to flash...");
> -	if (flash_write((char *)addr_source, addr_first, size) > 0) {
> -		printf("Error: could not copy to flash\n");
> -		return 1;
> -	}
> -	printf("done\n");
> -
> -	/* enable protection on processed sectors */
> -	if (update_flash_protect(1, addr_first, addr_last) > 0) {
> -		printf("Error: could not protect flash sectors\n");
> -		return 1;
> -	}
> -#endif
> -	return 0;
> -}
> -
>  static int update_fit_getparams(const void *fit, int noffset, ulong *addr,
>  						ulong *fladdr, ulong *size)
>  {
> @@ -235,20 +111,9 @@ int update_tftp(ulong addr, char *interface, char *devstring)
>  	char *filename, *env_addr, *fit_image_name;
>  	ulong update_addr, update_fladdr, update_size;
>  	int images_noffset, ndepth, noffset;
> -	bool update_tftp_dfu;
>  	int ret = 0;
>  	void *fit;
> 
> -	if (interface == NULL && devstring == NULL) {
> -		update_tftp_dfu = false;
> -	} else if (interface && devstring) {
> -		update_tftp_dfu = true;
> -	} else {
> -		pr_err("Interface: %s and devstring: %s not supported!\n",
> -		      interface, devstring);
> -		return -EINVAL;
> -	}
> -
>  	/* use already present image */
>  	if (addr)
>  		goto got_update_file;
> @@ -315,15 +180,7 @@ got_update_file:
>  			goto next_node;
>  		}
> 
> -		if (!update_tftp_dfu) {
> -			if (update_flash(update_addr, update_fladdr,
> -					 update_size)) {
> -				printf("Error: can't flash update, aborting\n");
> -				ret = 1;
> -				goto next_node;
> -			}
> -		} else if (fit_image_check_type(fit, noffset,
> -						IH_TYPE_FIRMWARE)) {
> +		if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) {
>  			ret = dfu_tftp_write(fit_image_name, update_addr,
>  					     update_size, interface, devstring);
>  			if (ret)
> diff --git a/doc/README.dfutftp b/doc/README.dfutftp
> index a3341bbb61..b279f542a1 100644
> --- a/doc/README.dfutftp
> +++ b/doc/README.dfutftp
> @@ -52,9 +52,6 @@ Environment variables
> 
>  The "dfu tftp" command can be used in the "preboot" environment variable
>  (when it is enabled by defining CONFIG_PREBOOT).
> -This is the preferable way of using this command in the early boot stage
> -as opposed to legacy update_tftp() function invocation.
> -

There are still two places in which README.update file is referred to
while you try to delete it.


>  Beagle Bone Black (BBB) setup
>  -----------------------------
> diff --git a/doc/README.update b/doc/README.update
> deleted file mode 100644
> index bf4379279e..0000000000
> --- a/doc/README.update
> +++ /dev/null
> @@ -1,97 +0,0 @@
> -Automatic software update from a TFTP server
> -============================================
> -
> -Overview
> ---------
> -
> -This feature allows to automatically store software updates present on a TFTP
> -server in NOR Flash. In more detail: a TFTP transfer of a file given in
> -environment variable 'updatefile' from server 'serverip' is attempted during
> -boot. The update file should be a FIT file, and can contain one or more
> -updates. Each update in the update file has an address in NOR Flash where it
> -should be placed, updates are also protected with a SHA-1 checksum. If the
> -TFTP transfer is successful, the hash of each update is verified, and if the
> -verification is positive, the update is stored in Flash.
> -
> -The auto-update feature is enabled by the CONFIG_UPDATE_TFTP macro:
> -
> -#define CONFIG_UPDATE_TFTP		1
> -
> -

If you really want to delete this file, you must move the following
text to somewhere else.
Otherwise, FIT related information will be lost.

-Takahiro Akashi

> -Note that when enabling auto-update, Flash support must be turned on.  Also,
> -one must enable FIT and LIBFDT support:
> -
> -#define CONFIG_FIT		1
> -#define CONFIG_OF_LIBFDT	1
> -
> -The auto-update feature uses the following configuration knobs:
> -
> -- CONFIG_UPDATE_LOAD_ADDR
> -
> -  Normally, TFTP transfer of the update file is done to the address specified
> -  in environment variable 'loadaddr'. If this variable is not present, the
> -  transfer is made to the address given in CONFIG_UPDATE_LOAD_ADDR (0x100000
> -  by default).
> -
> -- CONFIG_UPDATE_TFTP_CNT_MAX
> -  CONFIG_UPDATE_TFTP_MSEC_MAX
> -
> -  These knobs control the timeouts during initial connection to the TFTP
> -  server. Since a transfer is attempted during each boot, it is undesirable to
> -  have a long delay when a TFTP server is not present.
> -  CONFIG_UPDATE_TFTP_MSEC_MAX specifies the number of milliseconds to wait for
> -  the server to respond to initial connection, and CONFIG_UPDATE_TFTP_CNT_MAX
> -  gives the number of such connection retries. CONFIG_UPDATE_TFTP_CNT_MAX must
> -  be non-negative and is 0 by default, CONFIG_UPDATE_TFTP_MSEC_MAX must be
> -  positive and is 100 by default.
> -
> -Since the update file is in FIT format, it is created from an *.its file using
> -the mkimage tool. dtc tool with support for binary includes, e.g. in version
> -1.2.0 or later, must also be available on the system where the update file is
> -to be prepared. Refer to the doc/uImage.FIT/ directory for more details on FIT
> -images.
> -
> -
> -Example .its files
> -------------------
> -
> -- doc/uImage.FIT/update_uboot.its
> -
> -  A simple example that can be used to create an update file for automatically
> -  replacing U-Boot image on a system.
> -
> -  Assuming that an U-Boot image u-boot.bin is present in the current working
> -  directory, and that the address given in the 'load' property in the
> -  'update_uboot.its' file is where the U-Boot is stored in Flash, the
> -  following command will create the actual update file 'update_uboot.itb':
> -
> -  mkimage -f update_uboot.its update_uboot.itb
> -
> -  Place 'update_uboot.itb' on a TFTP server, for example as
> -  '/tftpboot/update_uboot.itb', and set the 'updatefile' variable
> -  appropriately, for example in the U-Boot prompt:
> -
> -  setenv updatefile /tftpboot/update_uboot.itb
> -  saveenv
> -
> -  Now, when the system boots up and the update TFTP server specified in the
> -  'serverip' environment variable is accessible, the new U-Boot image will be
> -  automatically stored in Flash.
> -
> -  NOTE: do make sure that the 'u-boot.bin' image used to create the update
> -  file is a good, working image. Also make sure that the address in Flash
> -  where the update will be placed is correct. Making mistake here and
> -  attempting the auto-update can render the system unusable.
> -
> -- doc/uImage.FIT/update3.its
> -
> -  An example containing three updates. It can be used to update Linux kernel,
> -  ramdisk and FDT blob stored in Flash. The procedure for preparing the update
> -  file is similar to the example above.
> -
> -TFTP update via DFU
> --------------------
> -
> -- It is now possible to update firmware (bootloader, kernel, rootfs, etc.) via
> -  TFTP by using DFU (Device Firmware Upgrade). More information can be found in
> -  ./doc/README.dfutftp documentation entry.
> --
> 2.27.0
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-15 14:42         ` Heinrich Schuchardt
  2020-07-15 14:48           ` Michal Simek
@ 2020-07-16  1:52           ` AKASHI Takahiro
  2020-07-16  4:44             ` Heinrich Schuchardt
  1 sibling, 1 reply; 14+ messages in thread
From: AKASHI Takahiro @ 2020-07-16  1:52 UTC (permalink / raw)
  To: u-boot

On Wed, Jul 15, 2020 at 04:42:20PM +0200, Heinrich Schuchardt wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> On 15.07.20 16:06, Michal Simek wrote:
> >
> >
> > On 15. 07. 20 15:23, Lukasz Majewski wrote:
> >> Hi Heinrich,
> >>
> >> On 15.07.20 13:24, Lukasz Majewski wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>>> Using UPDATE_TFTP the firmware can be updated from tFTP
> >>>>>> by writing to NOR flash. The same is possible by defining
> >>>>>> a dfu command in CONFIG_PREBOOT.
> >>>>>>
> >>>>>> The dfu command cannot only write to NOR but also to
> >>>>>> other devices. In README.dfutfp UPDATE_TFTP has been
> >>>>>> marked as deprecated.
> >>>>>
> >>>>> Could you also write in the proper README the steps
> >>>>> necessary to have the same functionality as with
> >>>>> UPDATE_TFTP with dfu and preboot? I think that it would be
> >>>>> good to have it written down in ./doc.
> >>>>>
> >>
> >> doc/README.dfutftp already describes the necessary steps as:
> >>
> >> * enable CONFIG_DFU_TFTP * enable CONFIG_PREBOOT * put a string
> >> like "dfu tftp 0 mmc 0" into the "preboot" env variable
> >>
> >> Do you want me to explicitly write that "dfu tftp 0 mtd 0"
> >> replaces the former auto-update functionality?
> >
> > I am curious if that command is really dfu tftp 0 mtd 0.
> 
> Lukasz was asking about the replacement of the update that was
> executed when running with CONFIG_UPDATE_TFTP=y. This function that I
> am removing could only write to NOR flash. The update defined via
> CONFIG_PREBOOT is much more flexible and can server other storage media.

One of my concerns regarding CONFIG_PREBOOT is that it depends
on the "preboot" variable.
What this means is that, if a user unsets this variable (accidentally
or not), 'automatic firmware update' won't be executed.
This can be a critical issue if the system administrator intends to
*enforce* the update.
(I know there are more issues regarding enforcement though.)

That is why I added UEFI capsule update hook in main_loop() as
a compile-time option.

Thanks,
-Takahiro Akashi


> The different accessible devices are described in doc/README.dfu.
> "ram" is one of them.
> 
> Best regards
> 
> Heinrich
> 
> >
> > I have just tested it on zynqmp with copying images to ram and
> > command is just dfu tftp ram 0. It means I think it would be worth
> > to list options which are really used by different back ends
> > because it is not fully clear from help how to exactly define it.
> >
> > dfu tftp [<interface> <dev>] [<addr>] - device firmware upgrade via
> > TFTP on device <dev>, attached to interface <interface> [<addr>] -
> > address where FIT image has been stored
> >
> > Thanks, Michal
> >
> 
> -----BEGIN PGP SIGNATURE-----
> 
> iQIzBAEBCAAdFiEEbcT5xx8ppvoGt20zxIHbvCwFGsQFAl8PFcUACgkQxIHbvCwF
> GsSwLQ//bU8AISZ5q4cgLcnAVPYnBPbVtoPXlyRROnX6XW3eDjq506Ah0Rk8UwYH
> lhLSM1m+bfJZzVtWUjymlRGjuaB1HLzcyd1PbfK0+50mMaVnFBV5PMU4ue6Ju+zT
> BukpUvtQ4KLpooo2QhjCz4/oXmur782RUJGq6hb7mDbk6R0+L8coqoiglLid5374
> GdOQhgTAa8jWu0QxIBbdeOOrTSZz1kOBaShYffKXfBCaUfMOf/2T6sxDrSuTB8/N
> WTeKdbTAsx5BVEY8s9EaAy6a7kWf8mb/7LdooFUMQ2noNbdw/ucfiNCA+H2JNTK1
> GjvzDgViWAF0A0sXu6c1fqFT+4xo2lpRvG0wXsz3zRwpggmlAVX5PPi7o4Xj2HVF
> 7HPBVxz4AjJ3YpOpyLm0rEVEXnwBdke/jy/2r0VRyn5QMFaqAMEkrA39/wHrjj6t
> 0s4EUeuy5IjMf51zUrTAE/J9xHowLwjldODRgwmWoEYShJRgZp+J0yr2nhGTwpyS
> 91ESrHzkok/5BPkYFYGulBeeJFDOBegA7N7xec5e6YsXrWEPDiOpS5I1AfbycceV
> lERhJpHIgPPjpaMgNcATKeti85o7veNe31IMoW758i5kbjQFOB8NlvtSSu97/2/K
> eB/dIrOdnzC+URaun+MEt0T4HYPYgEuGAsBPQJKYrTw0sFhJRkg=
> =JXjF
> -----END PGP SIGNATURE-----

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-16  1:36 ` AKASHI Takahiro
@ 2020-07-16  4:13   ` Heinrich Schuchardt
  0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-07-16  4:13 UTC (permalink / raw)
  To: u-boot

Am 16. Juli 2020 03:36:13 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>Heinrich,
>
>On Wed, Jul 15, 2020 at 12:45:18PM +0200, Heinrich Schuchardt wrote:
>> Using UPDATE_TFTP the firmware can be updated from tFTP by writing to
>NOR
>> flash. The same is possible by defining a dfu command in
>CONFIG_PREBOOT.
>> 
>> The dfu command cannot only write to NOR but also to other devices.
>In
>> README.dfutfp UPDATE_TFTP has been marked as deprecated. It is not
>used
>> by any board.
>> 
>> Remove tFTP update via CONFIG_UPDATE_TFTP.
>
>This description is not accurate.

What are you missing?

>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> Currently Gitlab CI lacks a test for dfu tftp. I will try to set one
>up.
>> When it is properly tested I will send a final patch series.
>> But beforehand I would like to know if eliminating UPDATE_TFTP is ok.
>> 
>> Best regards
>> 
>> Heinrich
>> ---
>>  README             |   8 ---
>>  common/Kconfig     |  17 ------
>>  common/main.c      |   3 -
>>  common/update.c    | 147
>+--------------------------------------------
>>  doc/README.dfutftp |   3 -
>>  doc/README.update  |  97 ------------------------------
>>  6 files changed, 2 insertions(+), 273 deletions(-)
>>  delete mode 100644 doc/README.update
>> 
>> diff --git a/README b/README
>> index 2384966a39..c53f72cfa6 100644
>> --- a/README
>> +++ b/README
>> @@ -2104,14 +2104,6 @@ The following options need to be configured:
>> 
>>  		Please see board_init_f function.
>> 
>> -- Automatic software updates via TFTP server
>> -		CONFIG_UPDATE_TFTP
>> -		CONFIG_UPDATE_TFTP_CNT_MAX
>> -		CONFIG_UPDATE_TFTP_MSEC_MAX
>> -
>> -		These options enable and control the auto-update feature;
>> -		for a more detailed description refer to doc/README.update.
>> -
>>  - MTD Support (mtdparts command, UBI support)
>>  		CONFIG_MTD_UBI_WL_THRESHOLD
>>  		This parameter defines the maximum difference between the highest
>> diff --git a/common/Kconfig b/common/Kconfig
>> index 67b3818fde..ca42ba37b7 100644
>> --- a/common/Kconfig
>> +++ b/common/Kconfig
>> @@ -1014,23 +1014,6 @@ endmenu
>> 
>>  menu "Update support"
>> 
>> -config UPDATE_TFTP
>> -	bool "Auto-update using fitImage via TFTP"
>> -	depends on FIT
>> -	help
>> -	  This option allows performing update of NOR with data in fitImage
>> -	  sent via TFTP boot.
>
>NAK.
>If this configuration is removed, common/update.c won't be compiled in.
>There is a chain of dependency:
>DFU -> DFU_TFTP -> DFU_OVER_TFTP ->(implicitly) UPDATE_TFTP

No. In common/Makefile there are orginally two lines for update.o


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

>
>> -config UPDATE_TFTP_CNT_MAX
>> -	int "The number of connection retries during auto-update"
>> -	default 0
>> -	depends on UPDATE_TFTP
>> -
>> -config UPDATE_TFTP_MSEC_MAX
>> -	int "Delay in mSec to wait for the TFTP server during auto-update"
>> -	default 100
>> -	depends on UPDATE_TFTP
>> -
>>  config ANDROID_AB
>>  	bool "Android A/B updates"
>>  	default n
>> diff --git a/common/main.c b/common/main.c
>> index 4b3cd302c3..62ab3344e5 100644
>> --- a/common/main.c
>> +++ b/common/main.c
>> @@ -50,9 +50,6 @@ void main_loop(void)
>>  	if (IS_ENABLED(CONFIG_USE_PREBOOT))
>>  		run_preboot_environment_command();
>> 
>> -	if (IS_ENABLED(CONFIG_UPDATE_TFTP))
>> -		update_tftp(0UL, NULL, NULL);
>> -
>>  	s = bootdelay_process();
>>  	if (cli_process_fdt(&s))
>>  		cli_secure_boot_cmd(s);
>> diff --git a/common/update.c b/common/update.c
>> index c8dd346a09..caf74e63db 100644
>> --- a/common/update.c
>> +++ b/common/update.c
>> @@ -14,10 +14,6 @@
>>  #error "CONFIG_FIT and CONFIG_OF_LIBFDT are required for auto-update
>feature"
>>  #endif
>> 
>> -#if defined(CONFIG_UPDATE_TFTP) && !defined(CONFIG_MTD_NOR_FLASH)
>> -#error "CONFIG_UPDATE_TFTP and !CONFIG_MTD_NOR_FLASH needed for
>legacy behaviour"
>> -#endif
>> -
>>  #include <command.h>
>>  #include <env.h>
>>  #include <flash.h>
>> @@ -26,7 +22,6 @@
>>  #include <malloc.h>
>>  #include <dfu.h>
>>  #include <errno.h>
>> -#include <mtd/cfi_flash.h>
>> 
>>  /* env variable holding the location of the update file */
>>  #define UPDATE_FILE_ENV		"updatefile"
>> @@ -46,10 +41,7 @@
>> 
>>  extern ulong tftp_timeout_ms;
>>  extern int tftp_timeout_count_max;
>> -#ifdef CONFIG_MTD_NOR_FLASH
>> -extern flash_info_t flash_info[];
>> -static uchar *saved_prot_info;
>> -#endif
>> +
>>  static int update_load(char *filename, ulong msec_max, int cnt_max,
>ulong addr)
>>  {
>>  	int size, rv;
>> @@ -98,122 +90,6 @@ static int update_load(char *filename, ulong
>msec_max, int cnt_max, ulong addr)
>>  	return rv;
>>  }
>> 
>> -#ifdef CONFIG_MTD_NOR_FLASH
>> -static int update_flash_protect(int prot, ulong addr_first, ulong
>addr_last)
>> -{
>> -	uchar *sp_info_ptr;
>> -	ulong s;
>> -	int i, bank, cnt;
>> -	flash_info_t *info;
>> -
>> -	sp_info_ptr = NULL;
>> -
>> -	if (prot == 0) {
>> -		saved_prot_info =
>> -			calloc(CONFIG_SYS_MAX_FLASH_BANKS * CONFIG_SYS_MAX_FLASH_SECT,
>1);
>> -		if (!saved_prot_info)
>> -			return 1;
>> -	}
>> -
>> -	for (bank = 0; bank < CONFIG_SYS_MAX_FLASH_BANKS; ++bank) {
>> -		cnt = 0;
>> -		info = &flash_info[bank];
>> -
>> -		/* Nothing to do if the bank doesn't exist */
>> -		if (info->sector_count == 0)
>> -			return 0;
>> -
>> -		/* Point to current bank protection information */
>> -		sp_info_ptr = saved_prot_info + (bank *
>CONFIG_SYS_MAX_FLASH_SECT);
>> -
>> -		/*
>> -		 * Adjust addr_first or addr_last if we are on bank boundary.
>> -		 * Address space between banks must be continuous for other
>> -		 * flash functions (like flash_sect_erase or flash_write) to
>> -		 * succeed. Banks must also be numbered in correct order,
>> -		 * according to increasing addresses.
>> -		 */
>> -		if (addr_last > info->start[0] + info->size - 1)
>> -			addr_last = info->start[0] + info->size - 1;
>> -		if (addr_first < info->start[0])
>> -			addr_first = info->start[0];
>> -
>> -		for (i = 0; i < info->sector_count; i++) {
>> -			/* Save current information about protected sectors */
>> -			if (prot == 0) {
>> -				s = info->start[i];
>> -				if ((s >= addr_first) && (s <= addr_last))
>> -					sp_info_ptr[i] = info->protect[i];
>> -
>> -			}
>> -
>> -			/* Protect/unprotect sectors */
>> -			if (sp_info_ptr[i] == 1) {
>> -#if defined(CONFIG_SYS_FLASH_PROTECTION)
>> -				if (flash_real_protect(info, i, prot))
>> -					return 1;
>> -#else
>> -				info->protect[i] = prot;
>> -#endif
>> -				cnt++;
>> -			}
>> -		}
>> -
>> -		if (cnt) {
>> -			printf("%sProtected %d sectors\n",
>> -						prot ? "": "Un-", cnt);
>> -		}
>> -	}
>> -
>> -	if((prot == 1) && saved_prot_info)
>> -		free(saved_prot_info);
>> -
>> -	return 0;
>> -}
>> -#endif
>> -
>> -static int update_flash(ulong addr_source, ulong addr_first, ulong
>size)
>> -{
>> -#ifdef CONFIG_MTD_NOR_FLASH
>> -	ulong addr_last = addr_first + size - 1;
>> -
>> -	/* round last address to the sector boundary */
>> -	if (flash_sect_roundb(&addr_last) > 0)
>> -		return 1;
>> -
>> -	if (addr_first >= addr_last) {
>> -		printf("Error: end address exceeds addressing space\n");
>> -		return 1;
>> -	}
>> -
>> -	/* remove protection on processed sectors */
>> -	if (update_flash_protect(0, addr_first, addr_last) > 0) {
>> -		printf("Error: could not unprotect flash sectors\n");
>> -		return 1;
>> -	}
>> -
>> -	printf("Erasing 0x%08lx - 0x%08lx", addr_first, addr_last);
>> -	if (flash_sect_erase(addr_first, addr_last) > 0) {
>> -		printf("Error: could not erase flash\n");
>> -		return 1;
>> -	}
>> -
>> -	printf("Copying to flash...");
>> -	if (flash_write((char *)addr_source, addr_first, size) > 0) {
>> -		printf("Error: could not copy to flash\n");
>> -		return 1;
>> -	}
>> -	printf("done\n");
>> -
>> -	/* enable protection on processed sectors */
>> -	if (update_flash_protect(1, addr_first, addr_last) > 0) {
>> -		printf("Error: could not protect flash sectors\n");
>> -		return 1;
>> -	}
>> -#endif
>> -	return 0;
>> -}
>> -
>>  static int update_fit_getparams(const void *fit, int noffset, ulong
>*addr,
>>  						ulong *fladdr, ulong *size)
>>  {
>> @@ -235,20 +111,9 @@ int update_tftp(ulong addr, char *interface,
>char *devstring)
>>  	char *filename, *env_addr, *fit_image_name;
>>  	ulong update_addr, update_fladdr, update_size;
>>  	int images_noffset, ndepth, noffset;
>> -	bool update_tftp_dfu;
>>  	int ret = 0;
>>  	void *fit;
>> 
>> -	if (interface == NULL && devstring == NULL) {
>> -		update_tftp_dfu = false;
>> -	} else if (interface && devstring) {
>> -		update_tftp_dfu = true;
>> -	} else {
>> -		pr_err("Interface: %s and devstring: %s not supported!\n",
>> -		      interface, devstring);
>> -		return -EINVAL;
>> -	}
>> -
>>  	/* use already present image */
>>  	if (addr)
>>  		goto got_update_file;
>> @@ -315,15 +180,7 @@ got_update_file:
>>  			goto next_node;
>>  		}
>> 
>> -		if (!update_tftp_dfu) {
>> -			if (update_flash(update_addr, update_fladdr,
>> -					 update_size)) {
>> -				printf("Error: can't flash update, aborting\n");
>> -				ret = 1;
>> -				goto next_node;
>> -			}
>> -		} else if (fit_image_check_type(fit, noffset,
>> -						IH_TYPE_FIRMWARE)) {
>> +		if (fit_image_check_type(fit, noffset, IH_TYPE_FIRMWARE)) {
>>  			ret = dfu_tftp_write(fit_image_name, update_addr,
>>  					     update_size, interface, devstring);
>>  			if (ret)
>> diff --git a/doc/README.dfutftp b/doc/README.dfutftp
>> index a3341bbb61..b279f542a1 100644
>> --- a/doc/README.dfutftp
>> +++ b/doc/README.dfutftp
>> @@ -52,9 +52,6 @@ Environment variables
>> 
>>  The "dfu tftp" command can be used in the "preboot" environment
>variable
>>  (when it is enabled by defining CONFIG_PREBOOT).
>> -This is the preferable way of using this command in the early boot
>stage
>> -as opposed to legacy update_tftp() function invocation.
>> -
>
>There are still two places in which README.update file is referred to
>while you try to delete it.

Thanks for catching this.

>
>
>>  Beagle Bone Black (BBB) setup
>>  -----------------------------
>> diff --git a/doc/README.update b/doc/README.update
>> deleted file mode 100644
>> index bf4379279e..0000000000
>> --- a/doc/README.update
>> +++ /dev/null
>> @@ -1,97 +0,0 @@
>> -Automatic software update from a TFTP server
>> -============================================
>> -
>> -Overview
>> ---------
>> -
>> -This feature allows to automatically store software updates present
>on a TFTP
>> -server in NOR Flash. In more detail: a TFTP transfer of a file given
>in
>> -environment variable 'updatefile' from server 'serverip' is
>attempted during
>> -boot. The update file should be a FIT file, and can contain one or
>more
>> -updates. Each update in the update file has an address in NOR Flash
>where it
>> -should be placed, updates are also protected with a SHA-1 checksum.
>If the
>> -TFTP transfer is successful, the hash of each update is verified,
>and if the
>> -verification is positive, the update is stored in Flash.
>> -
>> -The auto-update feature is enabled by the CONFIG_UPDATE_TFTP macro:
>> -
>> -#define CONFIG_UPDATE_TFTP		1
>> -
>> -
>
>If you really want to delete this file, you must move the following
>text to somewhere else.
>Otherwise, FIT related information will be lost.

The text below is incorrect. You should not use any definition in a board include.

Dependencies should be in the Kconfig.

Best regards

Heinrich

>
>-Takahiro Akashi
>
>> -Note that when enabling auto-update, Flash support must be turned
>on.  Also,
>> -one must enable FIT and LIBFDT support:
>> -
>> -#define CONFIG_FIT		1
>> -#define CONFIG_OF_LIBFDT	1
>> -
>> -The auto-update feature uses the following configuration knobs:
>> -
>> -- CONFIG_UPDATE_LOAD_ADDR
>> -
>> -  Normally, TFTP transfer of the update file is done to the address
>specified
>> -  in environment variable 'loadaddr'. If this variable is not
>present, the
>> -  transfer is made to the address given in CONFIG_UPDATE_LOAD_ADDR
>(0x100000
>> -  by default).
>> -
>> -- CONFIG_UPDATE_TFTP_CNT_MAX
>> -  CONFIG_UPDATE_TFTP_MSEC_MAX
>> -
>> -  These knobs control the timeouts during initial connection to the
>TFTP
>> -  server. Since a transfer is attempted during each boot, it is
>undesirable to
>> -  have a long delay when a TFTP server is not present.
>> -  CONFIG_UPDATE_TFTP_MSEC_MAX specifies the number of milliseconds
>to wait for
>> -  the server to respond to initial connection, and
>CONFIG_UPDATE_TFTP_CNT_MAX
>> -  gives the number of such connection retries.
>CONFIG_UPDATE_TFTP_CNT_MAX must
>> -  be non-negative and is 0 by default, CONFIG_UPDATE_TFTP_MSEC_MAX
>must be
>> -  positive and is 100 by default.
>> -
>> -Since the update file is in FIT format, it is created from an *.its
>file using
>> -the mkimage tool. dtc tool with support for binary includes, e.g. in
>version
>> -1.2.0 or later, must also be available on the system where the
>update file is
>> -to be prepared. Refer to the doc/uImage.FIT/ directory for more
>details on FIT
>> -images.
>> -
>> -
>> -Example .its files
>> -------------------
>> -
>> -- doc/uImage.FIT/update_uboot.its
>> -
>> -  A simple example that can be used to create an update file for
>automatically
>> -  replacing U-Boot image on a system.
>> -
>> -  Assuming that an U-Boot image u-boot.bin is present in the current
>working
>> -  directory, and that the address given in the 'load' property in
>the
>> -  'update_uboot.its' file is where the U-Boot is stored in Flash,
>the
>> -  following command will create the actual update file
>'update_uboot.itb':
>> -
>> -  mkimage -f update_uboot.its update_uboot.itb
>> -
>> -  Place 'update_uboot.itb' on a TFTP server, for example as
>> -  '/tftpboot/update_uboot.itb', and set the 'updatefile' variable
>> -  appropriately, for example in the U-Boot prompt:
>> -
>> -  setenv updatefile /tftpboot/update_uboot.itb
>> -  saveenv
>> -
>> -  Now, when the system boots up and the update TFTP server specified
>in the
>> -  'serverip' environment variable is accessible, the new U-Boot
>image will be
>> -  automatically stored in Flash.
>> -
>> -  NOTE: do make sure that the 'u-boot.bin' image used to create the
>update
>> -  file is a good, working image. Also make sure that the address in
>Flash
>> -  where the update will be placed is correct. Making mistake here
>and
>> -  attempting the auto-update can render the system unusable.
>> -
>> -- doc/uImage.FIT/update3.its
>> -
>> -  An example containing three updates. It can be used to update
>Linux kernel,
>> -  ramdisk and FDT blob stored in Flash. The procedure for preparing
>the update
>> -  file is similar to the example above.
>> -
>> -TFTP update via DFU
>> --------------------
>> -
>> -- It is now possible to update firmware (bootloader, kernel, rootfs,
>etc.) via
>> -  TFTP by using DFU (Device Firmware Upgrade). More information can
>be found in
>> -  ./doc/README.dfutftp documentation entry.
>> --
>> 2.27.0
>> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-16  1:52           ` AKASHI Takahiro
@ 2020-07-16  4:44             ` Heinrich Schuchardt
  2020-07-21 11:56               ` AKASHI Takahiro
  0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-07-16  4:44 UTC (permalink / raw)
  To: u-boot

On 7/16/20 3:52 AM, AKASHI Takahiro wrote:
> On Wed, Jul 15, 2020 at 04:42:20PM +0200, Heinrich Schuchardt wrote:
> On 15.07.20 16:06, Michal Simek wrote:
>>>>
>>>>
>>>> On 15. 07. 20 15:23, Lukasz Majewski wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On 15.07.20 13:24, Lukasz Majewski wrote:
>>>>>>>> Hi Heinrich,
>>>>>>>>
>>>>>>>>> Using UPDATE_TFTP the firmware can be updated from tFTP
>>>>>>>>> by writing to NOR flash. The same is possible by defining
>>>>>>>>> a dfu command in CONFIG_PREBOOT.
>>>>>>>>>
>>>>>>>>> The dfu command cannot only write to NOR but also to
>>>>>>>>> other devices. In README.dfutfp UPDATE_TFTP has been
>>>>>>>>> marked as deprecated.
>>>>>>>>
>>>>>>>> Could you also write in the proper README the steps
>>>>>>>> necessary to have the same functionality as with
>>>>>>>> UPDATE_TFTP with dfu and preboot? I think that it would be
>>>>>>>> good to have it written down in ./doc.
>>>>>>>>
>>>>>
>>>>> doc/README.dfutftp already describes the necessary steps as:
>>>>>
>>>>> * enable CONFIG_DFU_TFTP * enable CONFIG_PREBOOT * put a string
>>>>> like "dfu tftp 0 mmc 0" into the "preboot" env variable
>>>>>
>>>>> Do you want me to explicitly write that "dfu tftp 0 mtd 0"
>>>>> replaces the former auto-update functionality?
>>>>
>>>> I am curious if that command is really dfu tftp 0 mtd 0.
>
> Lukasz was asking about the replacement of the update that was
> executed when running with CONFIG_UPDATE_TFTP=y. This function that I
> am removing could only write to NOR flash. The update defined via
> CONFIG_PREBOOT is much more flexible and can server other storage media.
>
>> One of my concerns regarding CONFIG_PREBOOT is that it depends
>> on the "preboot" variable.
>> What this means is that, if a user unsets this variable (accidentally
>> or not), 'automatic firmware update' won't be executed.
>> This can be a critical issue if the system administrator intends to
>> *enforce* the update.
>> (I know there are more issues regarding enforcement though.)

You could also defeat the old tFTP auto-update by setting "serverip" to
a reserved value like 240.0.0.0. So there is no new vulnerability
introduced here.

Anyway no board uses the old tFTP auto-update while DFU is enabled in
273 config files and DFU_TFTP in 2.

Best regards

Heinrich

>
>> That is why I added UEFI capsule update hook in main_loop() as
>> a compile-time option.
>
>> Thanks,
>> -Takahiro Akashi
>
>
> The different accessible devices are described in doc/README.dfu.
> "ram" is one of them.
>
> Best regards
>
> Heinrich
>
>>>>
>>>> I have just tested it on zynqmp with copying images to ram and
>>>> command is just dfu tftp ram 0. It means I think it would be worth
>>>> to list options which are really used by different back ends
>>>> because it is not fully clear from help how to exactly define it.
>>>>
>>>> dfu tftp [<interface> <dev>] [<addr>] - device firmware upgrade via
>>>> TFTP on device <dev>, attached to interface <interface> [<addr>] -
>>>> address where FIT image has been stored
>>>>
>>>> Thanks, Michal
>>>>
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-15 14:48           ` Michal Simek
@ 2020-07-21  3:03             ` Heinrich Schuchardt
  2020-07-21  6:31               ` Michal Simek
  0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2020-07-21  3:03 UTC (permalink / raw)
  To: u-boot

On 7/15/20 4:48 PM, Michal Simek wrote:
>
>
> On 15. 07. 20 16:42, Heinrich Schuchardt wrote:
>> On 15.07.20 16:06, Michal Simek wrote:
>>
>>
>>> On 15. 07. 20 15:23, Lukasz Majewski wrote:
>>>> Hi Heinrich,
>>>>
>>>> On 15.07.20 13:24, Lukasz Majewski wrote:
>>>>>>> Hi Heinrich,
>>>>>>>
>>>>>>>> Using UPDATE_TFTP the firmware can be updated from tFTP
>>>>>>>> by writing to NOR flash. The same is possible by defining
>>>>>>>> a dfu command in CONFIG_PREBOOT.
>>>>>>>>
>>>>>>>> The dfu command cannot only write to NOR but also to
>>>>>>>> other devices. In README.dfutfp UPDATE_TFTP has been
>>>>>>>> marked as deprecated.
>>>>>>>
>>>>>>> Could you also write in the proper README the steps
>>>>>>> necessary to have the same functionality as with
>>>>>>> UPDATE_TFTP with dfu and preboot? I think that it would be
>>>>>>> good to have it written down in ./doc.
>>>>>>>
>>>>
>>>> doc/README.dfutftp already describes the necessary steps as:
>>>>
>>>> * enable CONFIG_DFU_TFTP * enable CONFIG_PREBOOT * put a string
>>>> like "dfu tftp 0 mmc 0" into the "preboot" env variable
>>>>
>>>> Do you want me to explicitly write that "dfu tftp 0 mtd 0"
>>>> replaces the former auto-update functionality?
>>
>>> I am curious if that command is really dfu tftp 0 mtd 0.
>>
>> Lukasz was asking about the replacement of the update that was
>> executed when running with CONFIG_UPDATE_TFTP=y. This function that I
>> am removing could only write to NOR flash. The update defined via
>> CONFIG_PREBOOT is much more flexible and can server other storage media.
>
> I have really not a problem with removing it.
>
>>
>> The different accessible devices are described in doc/README.dfu.
>> "ram" is one of them.
>
> yes and link to README.dfutftp where only one options is listed
> as "dfu tftp 0 mmc 0" which is definitely only one option which you can
> use. Would be really good to list that options there and also retest them.

We will be able to have automated tests writing to the DFU "ram" and DFU
"mtd" driver once the flash driver used by QEMU works properly. See

https://lists.denx.de/pipermail/u-boot/2020-July/420835.html
mtd: cfi_flash: read device tree correctly

Best regards

Heinrich

>
> It means as the part of removal we should check if README.dfutftp is
> really up to date because I don't think it is clear how users should use
> it for different cases. (USB cases are described properly in README.dfu
> for different backends).
>
> Thanks,
> Michal
>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-21  3:03             ` Heinrich Schuchardt
@ 2020-07-21  6:31               ` Michal Simek
  0 siblings, 0 replies; 14+ messages in thread
From: Michal Simek @ 2020-07-21  6:31 UTC (permalink / raw)
  To: u-boot



On 21. 07. 20 5:03, Heinrich Schuchardt wrote:
> On 7/15/20 4:48 PM, Michal Simek wrote:
>>
>>
>> On 15. 07. 20 16:42, Heinrich Schuchardt wrote:
>>> On 15.07.20 16:06, Michal Simek wrote:
>>>
>>>
>>>> On 15. 07. 20 15:23, Lukasz Majewski wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On 15.07.20 13:24, Lukasz Majewski wrote:
>>>>>>>> Hi Heinrich,
>>>>>>>>
>>>>>>>>> Using UPDATE_TFTP the firmware can be updated from tFTP
>>>>>>>>> by writing to NOR flash. The same is possible by defining
>>>>>>>>> a dfu command in CONFIG_PREBOOT.
>>>>>>>>>
>>>>>>>>> The dfu command cannot only write to NOR but also to
>>>>>>>>> other devices. In README.dfutfp UPDATE_TFTP has been
>>>>>>>>> marked as deprecated.
>>>>>>>>
>>>>>>>> Could you also write in the proper README the steps
>>>>>>>> necessary to have the same functionality as with
>>>>>>>> UPDATE_TFTP with dfu and preboot? I think that it would be
>>>>>>>> good to have it written down in ./doc.
>>>>>>>>
>>>>>
>>>>> doc/README.dfutftp already describes the necessary steps as:
>>>>>
>>>>> * enable CONFIG_DFU_TFTP * enable CONFIG_PREBOOT * put a string
>>>>> like "dfu tftp 0 mmc 0" into the "preboot" env variable
>>>>>
>>>>> Do you want me to explicitly write that "dfu tftp 0 mtd 0"
>>>>> replaces the former auto-update functionality?
>>>
>>>> I am curious if that command is really dfu tftp 0 mtd 0.
>>>
>>> Lukasz was asking about the replacement of the update that was
>>> executed when running with CONFIG_UPDATE_TFTP=y. This function that I
>>> am removing could only write to NOR flash. The update defined via
>>> CONFIG_PREBOOT is much more flexible and can server other storage media.
>>
>> I have really not a problem with removing it.
>>
>>>
>>> The different accessible devices are described in doc/README.dfu.
>>> "ram" is one of them.
>>
>> yes and link to README.dfutftp where only one options is listed
>> as "dfu tftp 0 mmc 0" which is definitely only one option which you can
>> use. Would be really good to list that options there and also retest them.
> 
> We will be able to have automated tests writing to the DFU "ram" and DFU
> "mtd" driver once the flash driver used by QEMU works properly. See
> 
> https://lists.denx.de/pipermail/u-boot/2020-July/420835.html
> mtd: cfi_flash: read device tree correctly

Do you plan to wire it with test/py framework and run it via travis/gitlab?

Thanks,
Michal

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC] dfu: remove UPDATE_TFTP
  2020-07-16  4:44             ` Heinrich Schuchardt
@ 2020-07-21 11:56               ` AKASHI Takahiro
  0 siblings, 0 replies; 14+ messages in thread
From: AKASHI Takahiro @ 2020-07-21 11:56 UTC (permalink / raw)
  To: u-boot

On Thu, Jul 16, 2020 at 06:44:43AM +0200, Heinrich Schuchardt wrote:
> On 7/16/20 3:52 AM, AKASHI Takahiro wrote:
> > On Wed, Jul 15, 2020 at 04:42:20PM +0200, Heinrich Schuchardt wrote:
> > On 15.07.20 16:06, Michal Simek wrote:
> >>>>
> >>>>
> >>>> On 15. 07. 20 15:23, Lukasz Majewski wrote:
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On 15.07.20 13:24, Lukasz Majewski wrote:
> >>>>>>>> Hi Heinrich,
> >>>>>>>>
> >>>>>>>>> Using UPDATE_TFTP the firmware can be updated from tFTP
> >>>>>>>>> by writing to NOR flash. The same is possible by defining
> >>>>>>>>> a dfu command in CONFIG_PREBOOT.
> >>>>>>>>>
> >>>>>>>>> The dfu command cannot only write to NOR but also to
> >>>>>>>>> other devices. In README.dfutfp UPDATE_TFTP has been
> >>>>>>>>> marked as deprecated.
> >>>>>>>>
> >>>>>>>> Could you also write in the proper README the steps
> >>>>>>>> necessary to have the same functionality as with
> >>>>>>>> UPDATE_TFTP with dfu and preboot? I think that it would be
> >>>>>>>> good to have it written down in ./doc.
> >>>>>>>>
> >>>>>
> >>>>> doc/README.dfutftp already describes the necessary steps as:
> >>>>>
> >>>>> * enable CONFIG_DFU_TFTP * enable CONFIG_PREBOOT * put a string
> >>>>> like "dfu tftp 0 mmc 0" into the "preboot" env variable
> >>>>>
> >>>>> Do you want me to explicitly write that "dfu tftp 0 mtd 0"
> >>>>> replaces the former auto-update functionality?
> >>>>
> >>>> I am curious if that command is really dfu tftp 0 mtd 0.
> >
> > Lukasz was asking about the replacement of the update that was
> > executed when running with CONFIG_UPDATE_TFTP=y. This function that I
> > am removing could only write to NOR flash. The update defined via
> > CONFIG_PREBOOT is much more flexible and can server other storage media.
> >
> >> One of my concerns regarding CONFIG_PREBOOT is that it depends
> >> on the "preboot" variable.
> >> What this means is that, if a user unsets this variable (accidentally
> >> or not), 'automatic firmware update' won't be executed.
> >> This can be a critical issue if the system administrator intends to
> >> *enforce* the update.
> >> (I know there are more issues regarding enforcement though.)
> 
> You could also defeat the old tFTP auto-update by setting "serverip" to
> a reserved value like 240.0.0.0. So there is no new vulnerability
> introduced here.

?
I'm not talking about tftp here, but about a potential issue
in the current framework.

-Takahiro Akashi


> Anyway no board uses the old tFTP auto-update while DFU is enabled in
> 273 config files and DFU_TFTP in 2.
> 
> Best regards
> 
> Heinrich
> 
> >
> >> That is why I added UEFI capsule update hook in main_loop() as
> >> a compile-time option.
> >
> >> Thanks,
> >> -Takahiro Akashi
> >
> >
> > The different accessible devices are described in doc/README.dfu.
> > "ram" is one of them.
> >
> > Best regards
> >
> > Heinrich
> >
> >>>>
> >>>> I have just tested it on zynqmp with copying images to ram and
> >>>> command is just dfu tftp ram 0. It means I think it would be worth
> >>>> to list options which are really used by different back ends
> >>>> because it is not fully clear from help how to exactly define it.
> >>>>
> >>>> dfu tftp [<interface> <dev>] [<addr>] - device firmware upgrade via
> >>>> TFTP on device <dev>, attached to interface <interface> [<addr>] -
> >>>> address where FIT image has been stored
> >>>>
> >>>> Thanks, Michal
> >>>>
> >

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-07-21 11:56 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 10:45 [RFC] dfu: remove UPDATE_TFTP Heinrich Schuchardt
2020-07-15 11:24 ` Lukasz Majewski
2020-07-15 11:49   ` Heinrich Schuchardt
2020-07-15 13:23     ` Lukasz Majewski
2020-07-15 14:06       ` Michal Simek
2020-07-15 14:42         ` Heinrich Schuchardt
2020-07-15 14:48           ` Michal Simek
2020-07-21  3:03             ` Heinrich Schuchardt
2020-07-21  6:31               ` Michal Simek
2020-07-16  1:52           ` AKASHI Takahiro
2020-07-16  4:44             ` Heinrich Schuchardt
2020-07-21 11:56               ` AKASHI Takahiro
2020-07-16  1:36 ` AKASHI Takahiro
2020-07-16  4:13   ` Heinrich Schuchardt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.