[U-Boot] [PATCH v2] fw_setenv: avoid writing environment when nothing has changed

[Joe Hershberger]

On Thu, Sep 27, 2018 at 3:45 PM Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> In the case where one deletes an already-non-existing variable, or sets
> a variable to the value it already has, there is no point in writing the
> environment back, thus reducing wear on the underlying storage
> device.
>
> In the case of redundant environments, if the two environments
> differ (e.g. because one is corrupt), make sure that any call of
> fw_setenv causes the two to become synchronized, even if the fw_setenv
> call does not change anything in the good copy.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>

Nit below, but

Acked-by: Joe Hershberger <joe.hershberger at ni.com>

> ---
> Add logic to ensure a corrupt copy gets replaced, even if fw_setenv
> wouldn't change anything in the good copy.
>
>  tools/env/fw_env.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c
> index a5d75958e1..66372dad55 100644
> --- a/tools/env/fw_env.c
> +++ b/tools/env/fw_env.c
> @@ -110,6 +110,7 @@ struct environment {
>         unsigned char *flags;
>         char *data;
>         enum flag_scheme flag_scheme;
> +       int dirty;
>  };
>
>  static struct environment environment = {

It seems prudent to initialize the dirty flag here.

> @@ -508,6 +509,9 @@ int fw_env_flush(struct env_opts *opts)
>         if (!opts)
>                 opts = &default_opts;
>
> +       if (!environment.dirty)
> +               return 0;
> +
>         /*
>          * Update CRC
>          */
> @@ -553,7 +557,8 @@ int fw_env_write(char *name, char *value)
>
>         deleting = (oldval && !(value && strlen(value)));
>         creating = (!oldval && (value && strlen(value)));
> -       overwriting = (oldval && (value && strlen(value)));
> +       overwriting = (oldval && (value && strlen(value) &&
> +                                 strcmp(oldval, value)));
>
>         /* check for permission */
>         if (deleting) {
> @@ -593,6 +598,7 @@ int fw_env_write(char *name, char *value)
>                 /* Nothing to do */
>                 return 0;
>
> +       environment.dirty = 1;
>         if (deleting || overwriting) {
>                 if (*++nxt == '\0') {
>                         *env = '\0';
> @@ -1441,6 +1447,7 @@ int fw_env_open(struct env_opts *opts)
>                                 "Warning: Bad CRC, using default environment\n");
>                         memcpy(environment.data, default_environment,
>                                sizeof(default_environment));
> +                       environment.dirty = 1;
>                 }
>         } else {
>                 flag0 = *environment.flags;
> @@ -1494,6 +1501,16 @@ int fw_env_open(struct env_opts *opts)
>                 crc1_ok = (crc1 == redundant->crc);
>                 flag1 = redundant->flags;
>
> +               /*
> +                * environment.data still points to ((struct
> +                * env_image_redundant *)addr0)->data. If the two
> +                * environments differ, or one has bad crc, force a
> +                * write-out by marking the environment dirty.
> +                */
> +               if (memcmp(environment.data, redundant->data, ENV_SIZE) ||
> +                   !crc0_ok || !crc1_ok)
> +                       environment.dirty = 1;
> +
>                 if (crc0_ok && !crc1_ok) {
>                         dev_current = 0;
>                 } else if (!crc0_ok && crc1_ok) {
> @@ -1503,6 +1520,7 @@ int fw_env_open(struct env_opts *opts)
>                                 "Warning: Bad CRC, using default environment\n");
>                         memcpy(environment.data, default_environment,
>                                sizeof(default_environment));
> +                       environment.dirty = 1;
>                         dev_current = 0;
>                 } else {
>                         switch (environment.flag_scheme) {
> --
> 2.16.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot