[U-Boot] [PATCH v3 0/2] add env erase

[U-Boot] [PATCH v3 0/2] add env erase

[Frank Wunderlich]

sometimes it is needed to erase the non-volatile environment
e.g. for boot-up with builtin-environment or after resizing env

this series add basic functionality for erasing environment from storage
as a first storage-driver mmc is introduced, other needs to be added later

changes since v2:
  - move mmc-part to second patch (without function prototype)

Frank Wunderlich (2):
  env: register erase command
  env: mmc: add erase-function

 cmd/nvedit.c          | 13 +++++++++++++
 env/env.c             | 30 ++++++++++++++++++++++++++++++
 env/mmc.c             | 28 ++++++++++++++++++++++++++++
 include/environment.h | 17 +++++++++++++++++
 4 files changed, 88 insertions(+)

--
2.17.1

[U-Boot] [PATCH v3 1/2] env: register erase command

[Frank Wunderlich]

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 cmd/nvedit.c          | 13 +++++++++++++
 env/env.c             | 30 ++++++++++++++++++++++++++++++
 include/environment.h | 17 +++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 24a6cf7824..3b5c62d629 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -756,11 +756,22 @@ static int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc,
 	return env_save() ? 1 : 0;
 }

+static int do_env_erase(cmd_tbl_t *cmdtp, int flag, int argc,
+			char * const argv[])
+{
+	return env_erase() ? 1 : 0;
+}
+
 U_BOOT_CMD(
 	saveenv, 1, 0,	do_env_save,
 	"save environment variables to persistent storage",
 	""
 );
+U_BOOT_CMD(
+	eraseenv, 1, 0,	do_env_erase,
+	"erase environment variables from persistent storage",
+	""
+);
 #endif
 #endif /* CONFIG_SPL_BUILD */

@@ -1207,6 +1218,7 @@ static cmd_tbl_t cmd_env_sub[] = {
 #endif
 #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
 	U_BOOT_CMD_MKENT(save, 1, 0, do_env_save, "", ""),
+	U_BOOT_CMD_MKENT(erase, 1, 0, do_env_erase, "", ""),
 #endif
 	U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 0, do_env_set, "", ""),
 #if defined(CONFIG_CMD_ENV_EXISTS)
@@ -1282,6 +1294,7 @@ static char env_help_text[] =
 #endif
 #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
 	"env save - save environment\n"
+	"env erase - erase environment\n"
 #endif
 #if defined(CONFIG_CMD_NVEDIT_EFI)
 	"env set -e name [arg ...] - set UEFI variable; unset if 'arg' not specified\n"
diff --git a/env/env.c b/env/env.c
index 4b417b90a2..25441370e6 100644
--- a/env/env.c
+++ b/env/env.c
@@ -24,6 +24,8 @@ void env_fix_drivers(void)
 			entry->load += gd->reloc_off;
 		if (entry->save)
 			entry->save += gd->reloc_off;
+		if (entry->erase)
+			entry->erase += gd->reloc_off;
 		if (entry->init)
 			entry->init += gd->reloc_off;
 	}
@@ -254,6 +256,34 @@ int env_save(void)
 	return -ENODEV;
 }

+int env_erase(void)
+{
+	struct env_driver *drv;
+
+	drv = env_driver_lookup(ENVOP_ERASE, gd->env_load_prio);
+	if (drv) {
+		int ret;
+
+		if (!drv->erase)
+			return -ENODEV;
+
+		if (!env_has_inited(drv->location))
+			return -ENODEV;
+
+		printf("Erase Environment on %s... ", drv->name);
+		ret = drv->erase();
+		if (ret)
+			printf("Failed (%d)\n", ret);
+		else
+			printf("OK\n");
+
+		if (!ret)
+			return 0;
+	}
+
+	return -ENODEV;
+}
+
 int env_init(void)
 {
 	struct env_driver *drv;
diff --git a/include/environment.h b/include/environment.h
index cd96676141..5024cbea7e 100644
--- a/include/environment.h
+++ b/include/environment.h
@@ -200,6 +200,7 @@ enum env_operation {
 	ENVOP_INIT,	/* we want to call the init function */
 	ENVOP_LOAD,	/* we want to call the load function */
 	ENVOP_SAVE,	/* we want to call the save function */
+	ENVOP_ERASE,	/* we want to call the erase function */
 };

 struct env_driver {
@@ -225,6 +226,15 @@ struct env_driver {
 	 */
 	int (*save)(void);

+	/**
+	 * erase() - Erase the environment on storage
+	 *
+	 * This method is required for 'eraseenv' to work.
+	 *
+	 * @return 0 if OK, -ve on error
+	 */
+	int (*erase)(void);
+
 	/**
 	 * init() - Set up the initial pre-relocation environment
 	 *
@@ -303,6 +313,13 @@ int env_load(void);
  */
 int env_save(void);

+/**
+ * env_erase() - Erase the environment on storage
+ *
+ * @return 0 if OK, -ve on error
+ */
+int env_erase(void);
+
 /**
  * env_fix_drivers() - Updates envdriver as per relocation
  */
--
2.17.1

[U-Boot] [PATCH v3 2/2] env: mmc: add erase-function

[Frank Wunderlich]

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 env/mmc.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/env/mmc.c b/env/mmc.c
index c3cf35d01b..f387a53970 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -242,6 +242,33 @@ fini:
 	fini_mmc_for_env(mmc);
 	return ret;
 }
+
+static int env_mmc_erase(void)
+{
+	int dev = mmc_get_env_dev();
+	struct mmc *mmc = find_mmc_device(dev);
+	int	n, blk, cnt;
+
+	if (!mmc)
+		return CMD_RET_FAILURE;
+
+	blk = CONFIG_ENV_OFFSET / mmc->read_bl_len;
+	cnt = CONFIG_ENV_SIZE / mmc->read_bl_len;
+
+	printf("\nMMC erase env: dev # %d, block # %d (0x%x), count %d (0x%x)\n",
+	       dev, blk, blk * mmc->read_bl_len,
+	       cnt, cnt * mmc->read_bl_len);
+
+	if (mmc_getwp(mmc) == 1) {
+		printf("Error: card is write protected!\n");
+		return CMD_RET_FAILURE;
+	}
+	n = blk_derase(mmc_get_blk_desc(mmc), blk, cnt);
+	printf("%d blocks erased: %s\n", n, (n == cnt) ? "OK" : "ERROR");
+
+	return (n == cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE;
+}
+
 #endif /* CONFIG_CMD_SAVEENV && !CONFIG_SPL_BUILD */

 static inline int read_env(struct mmc *mmc, unsigned long size,
@@ -351,5 +378,6 @@ U_BOOT_ENV_LOCATION(mmc) = {
 	.load		= env_mmc_load,
 #ifndef CONFIG_SPL_BUILD
 	.save		= env_save_ptr(env_mmc_save),
+	.erase		= env_mmc_erase,
 #endif
 };
--
2.17.1

[U-Boot] [PATCH v3 1/2] env: register erase command

[Simon Goldschmidt]

On Fri, Apr 12, 2019 at 11:38 AM Frank Wunderlich
<frank-w@public-files.de> wrote:
>
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  cmd/nvedit.c          | 13 +++++++++++++
>  env/env.c             | 30 ++++++++++++++++++++++++++++++
>  include/environment.h | 17 +++++++++++++++++
>  3 files changed, 60 insertions(+)
>
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 24a6cf7824..3b5c62d629 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -756,11 +756,22 @@ static int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc,
>         return env_save() ? 1 : 0;
>  }
>
> +static int do_env_erase(cmd_tbl_t *cmdtp, int flag, int argc,
> +                       char * const argv[])
> +{
> +       return env_erase() ? 1 : 0;
> +}
> +
>  U_BOOT_CMD(
>         saveenv, 1, 0,  do_env_save,
>         "save environment variables to persistent storage",
>         ""
>  );
> +U_BOOT_CMD(
> +       eraseenv, 1, 0, do_env_erase,
> +       "erase environment variables from persistent storage",
> +       ""
> +);
>  #endif
>  #endif /* CONFIG_SPL_BUILD */
>
> @@ -1207,6 +1218,7 @@ static cmd_tbl_t cmd_env_sub[] = {
>  #endif
>  #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
>         U_BOOT_CMD_MKENT(save, 1, 0, do_env_save, "", ""),
> +       U_BOOT_CMD_MKENT(erase, 1, 0, do_env_erase, "", ""),

Guarding this with the same Kconfig option as 'saveenv' might be a bit
confusing?

>  #endif
>         U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 0, do_env_set, "", ""),
>  #if defined(CONFIG_CMD_ENV_EXISTS)
> @@ -1282,6 +1294,7 @@ static char env_help_text[] =
>  #endif
>  #if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)
>         "env save - save environment\n"
> +       "env erase - erase environment\n"
>  #endif
>  #if defined(CONFIG_CMD_NVEDIT_EFI)
>         "env set -e name [arg ...] - set UEFI variable; unset if 'arg' not specified\n"
> diff --git a/env/env.c b/env/env.c
> index 4b417b90a2..25441370e6 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -24,6 +24,8 @@ void env_fix_drivers(void)
>                         entry->load += gd->reloc_off;
>                 if (entry->save)
>                         entry->save += gd->reloc_off;
> +               if (entry->erase)
> +                       entry->erase += gd->reloc_off;
>                 if (entry->init)
>                         entry->init += gd->reloc_off;
>         }
> @@ -254,6 +256,34 @@ int env_save(void)
>         return -ENODEV;
>  }
>
> +int env_erase(void)
> +{
> +       struct env_driver *drv;
> +
> +       drv = env_driver_lookup(ENVOP_ERASE, gd->env_load_prio);
> +       if (drv) {
> +               int ret;
> +
> +               if (!drv->erase)
> +                       return -ENODEV;
> +
> +               if (!env_has_inited(drv->location))
> +                       return -ENODEV;
> +
> +               printf("Erase Environment on %s... ", drv->name);

Following the other prints, this should probably start with "Erasing".

> +               ret = drv->erase();
> +               if (ret)
> +                       printf("Failed (%d)\n", ret);
> +               else
> +                       printf("OK\n");
> +
> +               if (!ret)
> +                       return 0;
> +       }
> +
> +       return -ENODEV;
> +}
> +
>  int env_init(void)
>  {
>         struct env_driver *drv;
> diff --git a/include/environment.h b/include/environment.h
> index cd96676141..5024cbea7e 100644
> --- a/include/environment.h
> +++ b/include/environment.h
> @@ -200,6 +200,7 @@ enum env_operation {
>         ENVOP_INIT,     /* we want to call the init function */
>         ENVOP_LOAD,     /* we want to call the load function */
>         ENVOP_SAVE,     /* we want to call the save function */
> +       ENVOP_ERASE,    /* we want to call the erase function */
>  };
>
>  struct env_driver {
> @@ -225,6 +226,15 @@ struct env_driver {
>          */
>         int (*save)(void);
>
> +       /**
> +        * erase() - Erase the environment on storage
> +        *
> +        * This method is required for 'eraseenv' to work.

I know you copied this from 'save', but I think you should mention that
it's optional.

Regards,
Simon

> +        *
> +        * @return 0 if OK, -ve on error
> +        */
> +       int (*erase)(void);
> +
>         /**
>          * init() - Set up the initial pre-relocation environment
>          *
> @@ -303,6 +313,13 @@ int env_load(void);
>   */
>  int env_save(void);
>
> +/**
> + * env_erase() - Erase the environment on storage
> + *
> + * @return 0 if OK, -ve on error
> + */
> +int env_erase(void);
> +
>  /**
>   * env_fix_drivers() - Updates envdriver as per relocation
>   */
> --
> 2.17.1

[U-Boot] [PATCH v3 2/2] env: mmc: add erase-function

[Simon Goldschmidt]

On Fri, Apr 12, 2019 at 11:38 AM Frank Wunderlich
<frank-w@public-files.de> wrote:
>

Missing description here. I just saw I forgot this comment for the 1/2
patch, too.
There's no description there as well.

> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  env/mmc.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/env/mmc.c b/env/mmc.c
> index c3cf35d01b..f387a53970 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -242,6 +242,33 @@ fini:
>         fini_mmc_for_env(mmc);
>         return ret;
>  }
> +
> +static int env_mmc_erase(void)
> +{
> +       int dev = mmc_get_env_dev();
> +       struct mmc *mmc = find_mmc_device(dev);
> +       int     n, blk, cnt;

Bogus indent?

> +
> +       if (!mmc)
> +               return CMD_RET_FAILURE;
> +
> +       blk = CONFIG_ENV_OFFSET / mmc->read_bl_len;
> +       cnt = CONFIG_ENV_SIZE / mmc->read_bl_len;

Hmm, this doesn't work with redundant env. To fix this, you probably
need to touch patch 1/2, add a parameter to the command specifying
which env to erase (or both) and pass it to this callback.

> +
> +       printf("\nMMC erase env: dev # %d, block # %d (0x%x), count %d (0x%x)\n",
> +              dev, blk, blk * mmc->read_bl_len,
> +              cnt, cnt * mmc->read_bl_len);
> +
> +       if (mmc_getwp(mmc) == 1) {
> +               printf("Error: card is write protected!\n");
> +               return CMD_RET_FAILURE;
> +       }
> +       n = blk_derase(mmc_get_blk_desc(mmc), blk, cnt);
> +       printf("%d blocks erased: %s\n", n, (n == cnt) ? "OK" : "ERROR");
> +
> +       return (n == cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE;
> +}

Overall, I think this function should more resemble the save function.
That should make it easier to follow for someone trying to understand
this file.

Regards,
Simon

> +
>  #endif /* CONFIG_CMD_SAVEENV && !CONFIG_SPL_BUILD */
>
>  static inline int read_env(struct mmc *mmc, unsigned long size,
> @@ -351,5 +378,6 @@ U_BOOT_ENV_LOCATION(mmc) = {
>         .load           = env_mmc_load,
>  #ifndef CONFIG_SPL_BUILD
>         .save           = env_save_ptr(env_mmc_save),
> +       .erase          = env_mmc_erase,
>  #endif
>  };
> --
> 2.17.1

[U-Boot] [PATCH v3 2/2] env: mmc: add erase-function

[Frank Wunderlich]

Hi Simon

thanks for your review..

> > +       if (!mmc)
> > +               return CMD_RET_FAILURE;
> > +
> > +       blk = CONFIG_ENV_OFFSET / mmc->read_bl_len;
> > +       cnt = CONFIG_ENV_SIZE / mmc->read_bl_len;
>
> Hmm, this doesn't work with redundant env. To fix this, you probably
> need to touch patch 1/2, add a parameter to the command specifying
> which env to erase (or both) and pass it to this callback.

i had not understand the idea behind redundant-offset...you mean i should let user choose which offset to clear? save uses this way to determine the offset:

int copy = 0;
#ifdef CONFIG_ENV_OFFSET_REDUND
    if (gd->env_valid == ENV_VALID) //use redundant offset if default location holds valid environment (and ENV_OFFSET_REDUND is defined)??
        copy = 1;
#endif

    if (mmc_get_env_addr(mmc, copy, &offset)) {

should i use the same way or something like this (additional parameter to callback, also command "env erase" needs additional optional parameter):

#ifdef CONFIG_ENV_OFFSET_REDUND
    if (use_redund)
        blk = CONFIG_ENV_OFFSET_REDUND / mmc->read_bl_len;
    else
#endif
        blk = CONFIG_ENV_OFFSET / mmc->read_bl_len;


> > +
> > +       printf("\nMMC erase env: dev # %d, block # %d (0x%x), count %d (0x%x)\n",
> > +              dev, blk, blk * mmc->read_bl_len,
> > +              cnt, cnt * mmc->read_bl_len);
> > +
> > +       if (mmc_getwp(mmc) == 1) {
> > +               printf("Error: card is write protected!\n");
> > +               return CMD_RET_FAILURE;
> > +       }
> > +       n = blk_derase(mmc_get_blk_desc(mmc), blk, cnt);
> > +       printf("%d blocks erased: %s\n", n, (n == cnt) ? "OK" : "ERROR");
> > +
> > +       return (n == cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE;
> > +}
>
> Overall, I think this function should more resemble the save function.
> That should make it easier to follow for someone trying to understand
> this file.

i first copied the save-function and removed/changed anything to match new usecase...so the flow is same as for save. i removed the gotos because they are not needed because i think i don't need "fini_mmc_for_env(mmc);", do i?

regards Frank

[U-Boot] [PATCH v3 2/2] env: mmc: add erase-function

[Simon Goldschmidt]

On Fri, Apr 26, 2019 at 5:18 PM Frank Wunderlich
<frank-w@public-files.de> wrote:
>
> Hi Simon
>
> thanks for your review..
>
> > > +       if (!mmc)
> > > +               return CMD_RET_FAILURE;
> > > +
> > > +       blk = CONFIG_ENV_OFFSET / mmc->read_bl_len;
> > > +       cnt = CONFIG_ENV_SIZE / mmc->read_bl_len;
> >
> > Hmm, this doesn't work with redundant env. To fix this, you probably
> > need to touch patch 1/2, add a parameter to the command specifying
> > which env to erase (or both) and pass it to this callback.
>
> i had not understand the idea behind redundant-offset...you mean i should let user choose which offset to clear? save uses this way to determine the offset:

The idea is to have 2 copies of the env so if one gets corrupted, the older
one is loaded. I think what "save" does is to save to the one that hasn't been
used for loading.

In contrast to this, "erase" should probebly erase both environments to be
robust but might optionally erase one of the two copies...

By now I think it would be enough to just erase both copies as a start.

>
> int copy = 0;
> #ifdef CONFIG_ENV_OFFSET_REDUND
>     if (gd->env_valid == ENV_VALID) //use redundant offset if default location holds valid environment (and ENV_OFFSET_REDUND is defined)??
>         copy = 1;
> #endif
>
>     if (mmc_get_env_addr(mmc, copy, &offset)) {
>
> should i use the same way or something like this (additional parameter to callback, also command "env erase" needs additional optional parameter):

To make it easier to read, copy the "2-function style" used by save
where only the 2nd function calculates the block offset.

Then you can call this 2nd function twice with different offsets
when erasing redundant env.

Regards,
Simon

>
> #ifdef CONFIG_ENV_OFFSET_REDUND
>     if (use_redund)
>         blk = CONFIG_ENV_OFFSET_REDUND / mmc->read_bl_len;
>     else
> #endif
>         blk = CONFIG_ENV_OFFSET / mmc->read_bl_len;
>
>
> > > +
> > > +       printf("\nMMC erase env: dev # %d, block # %d (0x%x), count %d (0x%x)\n",
> > > +              dev, blk, blk * mmc->read_bl_len,
> > > +              cnt, cnt * mmc->read_bl_len);
> > > +
> > > +       if (mmc_getwp(mmc) == 1) {
> > > +               printf("Error: card is write protected!\n");
> > > +               return CMD_RET_FAILURE;
> > > +       }
> > > +       n = blk_derase(mmc_get_blk_desc(mmc), blk, cnt);
> > > +       printf("%d blocks erased: %s\n", n, (n == cnt) ? "OK" : "ERROR");
> > > +
> > > +       return (n == cnt) ? CMD_RET_SUCCESS : CMD_RET_FAILURE;
> > > +}
> >
> > Overall, I think this function should more resemble the save function.
> > That should make it easier to follow for someone trying to understand
> > this file.
>
> i first copied the save-function and removed/changed anything to match new usecase...so the flow is same as for save. i removed the gotos because they are not needed because i think i don't need "fini_mmc_for_env(mmc);", do i?
>
> regards Frank