Re: [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
 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 

Nit below, but

Acked-by: Joe Hershberger 

> ---
> 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 = _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@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rasmus Villemoes]

On 2018-09-27 22:45, Rasmus Villemoes 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 
> ---
> Add logic to ensure a corrupt copy gets replaced, even if fw_setenv
> wouldn't change anything in the good copy.

ping.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

[Rasmus Villemoes]

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 
---
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 = {
@@ -508,6 +509,9 @@ int fw_env_flush(struct env_opts *opts)
if (!opts)
opts = _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@lists.denx.de
https://lists.denx.de/listinfo/u-boot