[U-Boot] [RFC PATCH] u-boot: remove driver lookup loop from env_save()
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This allows saving the environment into the same location that has been previously loaded.
This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.
The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.
Signed-off-by: Nicholas Faustini nicholas.faustini@azcomtech.com ---
env/env.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/env/env.c b/env/env.c index 5c0842a..897d197 100644 --- a/env/env.c +++ b/env/env.c @@ -211,16 +211,16 @@ int env_load(void) int env_save(void) { struct env_driver *drv; - int prio;
- for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) { + drv = env_driver_lookup(ENVOP_SAVE, 0); + if (drv) { int ret;
if (!drv->save) - continue; + return -ENODEV;
if (!env_has_inited(drv->location)) - continue; + return -ENODEV;
printf("Saving Environment to %s... ", drv->name); ret = drv->save();
Hi Nicholas,
On 10 July 2018 at 06:57, Nicholas Faustini nicholas.faustini@azcomtech.com wrote:
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This allows saving the environment into the same location that has been previously loaded.
This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.
Why is this? Should it not stop eventually? Do we need a limit on prio?
The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.
What is that? It should be possible to write the env to multiple places. Also what is the previously discovered environment?
Signed-off-by: Nicholas Faustini nicholas.faustini@azcomtech.com
env/env.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/env/env.c b/env/env.c index 5c0842a..897d197 100644 --- a/env/env.c +++ b/env/env.c @@ -211,16 +211,16 @@ int env_load(void) int env_save(void) { struct env_driver *drv;
int prio;for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {
drv = env_driver_lookup(ENVOP_SAVE, 0);if (drv) { int ret; if (!drv->save)
continue;
return -ENODEV; if (!env_has_inited(drv->location))
continue;
return -ENODEV; printf("Saving Environment to %s... ", drv->name); ret = drv->save();-- 2.7.4
Regards, Simon
Hi,
Pepperl+Fuchs GmbH, Mannheim Geschaeftsfuehrer/Managing Directors: Dr.-Ing. Gunther Kegel (Vors./CEO), Werner Guthier, Mehmet Hatiboglu Vorsitzender des Aufsichtsrats/Chairman of the supervisory board: Michael Fuchs sen. Registergericht/Register Court: AG Mannheim HRB 4713 On 10.07.2018 22:49, Simon Glass wrote:
Hi Nicholas,
On 10 July 2018 at 06:57, Nicholas Faustini nicholas.faustini@azcomtech.com wrote:
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This allows saving the environment into the same location that has been previously loaded.
This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.
Why is this? Should it not stop eventually? Do we need a limit on prio?
See my description in this mail:
https://lists.denx.de/pipermail/u-boot/2018-April/324728.html
Unfortunately, I got diverted at that time and could not follow up on this. Essentially, the question is raised what 'env save' is supposed to do with multiple environments.
Currently, env_save() effectively stores only to the location where the env has been loaded from, which is questionable. But storing to all locations or the default location might not be correct, either.
Maybe the 'env save' command might need a parameter the tells it where to save?
Regards, Simon (Goldschmidt)
The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.
What is that? It should be possible to write the env to multiple places. Also what is the previously discovered environment?
Signed-off-by: Nicholas Faustini nicholas.faustini@azcomtech.com
env/env.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/env/env.c b/env/env.c index 5c0842a..897d197 100644 --- a/env/env.c +++ b/env/env.c @@ -211,16 +211,16 @@ int env_load(void) int env_save(void) { struct env_driver *drv;
int prio;for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {
drv = env_driver_lookup(ENVOP_SAVE, 0);if (drv) { int ret; if (!drv->save)
continue;
return -ENODEV; if (!env_has_inited(drv->location))
continue;
return -ENODEV; printf("Saving Environment to %s... ", drv->name); ret = drv->save();-- 2.7.4
Regards, Simon
Wichtiger Hinweis: Diese E-Mail einschliesslich ihrer Anhaenge enthaelt vertrauliche und rechtlich geschuetzte Informationen, die nur fuer den Adressaten bestimmt sind. Sollten Sie nicht der bezeichnete Adressat sein, so teilen Sie dies bitte dem Absender umgehend mit und loeschen Sie diese Nachricht und ihre Anhaenge. Die unbefugte Weitergabe, das Anfertigen von Kopien und jede Veraenderung der E-Mail ist untersagt. Der Absender haftet nicht fuer Inhalte von veraenderten E-Mails.
Important Information: This e-mail message including its attachments contains confidential and legally protected information solely intended for the addressee. If you are not the intended addressee of this message, please contact the addresser immediately and delete this message including its attachments. The unauthorized dissemination, copying and change of this e-mail are strictly forbidden. The addresser shall not be liable for the content of such changed e-mails.
Hi,
sorry for the disclaimer in the last mail. Still don't know why this is the default here :-(
Resending without the disclaimer to make possible follow-ups cleaner:
On 10.07.2018 22:49, Simon Glass wrote:
Hi Nicholas,
On 10 July 2018 at 06:57, Nicholas Faustini nicholas.faustini@azcomtech.com wrote:
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This allows saving the environment into the same location that has been previously loaded.
This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.
Why is this? Should it not stop eventually? Do we need a limit on prio?
See my description in this mail:
https://lists.denx.de/pipermail/u-boot/2018-April/324728.html
Unfortunately, I got diverted at that time and could not follow up on this. Essentially, the question is raised what 'env save' is supposed to do with multiple environments.
Currently, env_save() effectively stores only to the location where the env has been loaded from, which is questionable. But storing to all locations or the default location might not be correct, either.
Maybe the 'env save' command might need a parameter the tells it where to save?
Regards, Simon (Goldschmidt)
The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.
What is that? It should be possible to write the env to multiple places. Also what is the previously discovered environment?
Signed-off-by: Nicholas Faustini nicholas.faustini@azcomtech.com
env/env.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/env/env.c b/env/env.c index 5c0842a..897d197 100644 --- a/env/env.c +++ b/env/env.c @@ -211,16 +211,16 @@ int env_load(void) int env_save(void) { struct env_driver *drv;
int prio;for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) {
drv = env_driver_lookup(ENVOP_SAVE, 0);if (drv) { int ret; if (!drv->save)
continue;
return -ENODEV; if (!env_has_inited(drv->location))
continue;
return -ENODEV; printf("Saving Environment to %s... ", drv->name); ret = drv->save();-- 2.7.4
Regards, Simon
Hi Simon,
thanks for your comments and clarifications. I realize that I was not super clear when describing the problem.
On mer, 2018-07-11 at 07:09 +0200, Simon Goldschmidt wrote:
Hi,
sorry for the disclaimer in the last mail. Still don't know why this is the default here :-(
Resending without the disclaimer to make possible follow-ups cleaner:
On 10.07.2018 22:49, Simon Glass wrote:
Hi Nicholas,
On 10 July 2018 at 06:57, Nicholas Faustini nicholas.faustini@azcomtech.com wrote:
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This allows saving the environment into the same location that has been previously loaded.
This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.
Why is this? Should it not stop eventually? Do we need a limit on prio?
See my description in this mail:
https://lists.denx.de/pipermail/u-boot/2018-April/324728.html
Unfortunately, I got diverted at that time and could not follow up on this. Essentially, the question is raised what 'env save' is supposed to do with multiple environments.
Currently, env_save() effectively stores only to the location where the env has been loaded from, which is questionable. But storing to all locations or the default location might not be correct, either.
I have the same feeling about env_save(). Loading in multiple environments is straightforward. Saving is not.
I personally think that it makes more sense that env_save() saves into the same location from which the enviroment has been previously loaded (that is, current implementation). Otherwise, the logic becomes error- prone since an user may env_load() from one location and env_save() into another, resulting in misalignments which requires a lot of extra logic in order to avoid such misalignments.
Maybe the 'env save' command might need a parameter the tells it where to save?
Regards, Simon (Goldschmidt)
Having a parameter to env_save() might be a solution but in my opinion it would break the priority logic: an user follows the priority logic when loading but she can work around that when saving. Moreover, having the location embedded into env_*() functions is a great nice to have imho.
Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-
env_load_location.
The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.
What is that? It should be possible to write the env to multiple places. Also what is the previously discovered environment?
Signed-off-by: Nicholas Faustini <nicholas.faustini@azcomtech.com
env/env.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/env/env.c b/env/env.c index 5c0842a..897d197 100644 --- a/env/env.c +++ b/env/env.c @@ -211,16 +211,16 @@ int env_load(void) int env_save(void) { struct env_driver *drv; - int prio;
- for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio)); prio++) { + drv = env_driver_lookup(ENVOP_SAVE, 0); + if (drv) { int ret;
if (!drv->save) - continue; + return -ENODEV;
if (!env_has_inited(drv->location)) - continue; + return -ENODEV;
printf("Saving Environment to %s... ", drv-
name);
ret = drv->save();
2.7.4
Regards, Simon
Hi,
On Wed, Jul 11, 2018 at 10:33:28AM +0200, Nicholas wrote:
Hi Simon,
thanks for your comments and clarifications. I realize that I was not super clear when describing the problem.
On mer, 2018-07-11 at 07:09 +0200, Simon Goldschmidt wrote:
Hi,
sorry for the disclaimer in the last mail. Still don't know why this is the default here :-(
Resending without the disclaimer to make possible follow-ups cleaner:
On 10.07.2018 22:49, Simon Glass wrote:
Hi Nicholas,
On 10 July 2018 at 06:57, Nicholas Faustini nicholas.faustini@azcomtech.com wrote:
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This allows saving the environment into the same location that has been previously loaded.
This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.
Why is this? Should it not stop eventually? Do we need a limit on prio?
See my description in this mail:
https://lists.denx.de/pipermail/u-boot/2018-April/324728.html
Unfortunately, I got diverted at that time and could not follow up on this. Essentially, the question is raised what 'env save' is supposed to do with multiple environments.
Currently, env_save() effectively stores only to the location where the env has been loaded from, which is questionable. But storing to all locations or the default location might not be correct, either.
I have the same feeling about env_save(). Loading in multiple environments is straightforward. Saving is not.
I personally think that it makes more sense that env_save() saves into the same location from which the enviroment has been previously loaded (that is, current implementation). Otherwise, the logic becomes error- prone since an user may env_load() from one location and env_save() into another, resulting in misalignments which requires a lot of extra logic in order to avoid such misalignments.
Or worse, you might end up erasing something that shouldn't be erased on your board.
Maybe the 'env save' command might need a parameter the tells it where to save?
Having a parameter to env_save() might be a solution but in my opinion it would break the priority logic: an user follows the priority logic when loading but she can work around that when saving. Moreover, having the location embedded into env_*() functions is a great nice to have imho.
I agree.
Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-
env_load_location.
I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.
Maxime
On 11.07.2018 11:48, Maxime Ripard wrote:
Hi,
On Wed, Jul 11, 2018 at 10:33:28AM +0200, Nicholas wrote:
Hi Simon,
thanks for your comments and clarifications. I realize that I was not super clear when describing the problem.
On mer, 2018-07-11 at 07:09 +0200, Simon Goldschmidt wrote:
Hi,
sorry for the disclaimer in the last mail. Still don't know why this is the default here :-(
Resending without the disclaimer to make possible follow-ups cleaner:
On 10.07.2018 22:49, Simon Glass wrote:
Hi Nicholas,
On 10 July 2018 at 06:57, Nicholas Faustini nicholas.faustini@azcomtech.com wrote:
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This allows saving the environment into the same location that has been previously loaded.
This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.
Why is this? Should it not stop eventually? Do we need a limit on prio?
See my description in this mail:
https://lists.denx.de/pipermail/u-boot/2018-April/324728.html
Unfortunately, I got diverted at that time and could not follow up on this. Essentially, the question is raised what 'env save' is supposed to do with multiple environments.
Currently, env_save() effectively stores only to the location where the env has been loaded from, which is questionable. But storing to all locations or the default location might not be correct, either.
I have the same feeling about env_save(). Loading in multiple environments is straightforward. Saving is not.
I personally think that it makes more sense that env_save() saves into the same location from which the enviroment has been previously loaded (that is, current implementation). Otherwise, the logic becomes error- prone since an user may env_load() from one location and env_save() into another, resulting in misalignments which requires a lot of extra logic in order to avoid such misalignments.
Or worse, you might end up erasing something that shouldn't be erased on your board.
Maybe the 'env save' command might need a parameter the tells it where to save?
Having a parameter to env_save() might be a solution but in my opinion it would break the priority logic: an user follows the priority logic when loading but she can work around that when saving. Moreover, having the location embedded into env_*() functions is a great nice to have imho.
I agree.
Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-
env_load_location.
I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.
Well, the use case I saw was that I wanted to test redundant environment storage. I admit this is not an end user use case but a developer use case, so I guess you're right.
So after fixing this endless loops I see two questions: - to which environment do we store if the one in 'env_load_location' fails to store? - to which environment do we store if all environments failed to load (currently, it seems we store to the lowest prio in this case?)
Simon
On mer, 2018-07-11 at 12:01 +0200, Simon Goldschmidt wrote:
On 11.07.2018 11:48, Maxime Ripard wrote:
Hi,
On Wed, Jul 11, 2018 at 10:33:28AM +0200, Nicholas wrote:
Hi Simon,
thanks for your comments and clarifications. I realize that I was not super clear when describing the problem.
On mer, 2018-07-11 at 07:09 +0200, Simon Goldschmidt wrote:
Hi,
sorry for the disclaimer in the last mail. Still don't know why this is the default here :-(
Resending without the disclaimer to make possible follow-ups cleaner:
On 10.07.2018 22:49, Simon Glass wrote:
Hi Nicholas,
On 10 July 2018 at 06:57, Nicholas Faustini nicholas.faustini@azcomtech.com wrote:
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This allows saving the environment into the same location that has been previously loaded.
This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.
Why is this? Should it not stop eventually? Do we need a limit on prio?
See my description in this mail:
https://lists.denx.de/pipermail/u-boot/2018-April/324728.html
Unfortunately, I got diverted at that time and could not follow up on this. Essentially, the question is raised what 'env save' is supposed to do with multiple environments.
Currently, env_save() effectively stores only to the location where the env has been loaded from, which is questionable. But storing to all locations or the default location might not be correct, either.
I have the same feeling about env_save(). Loading in multiple environments is straightforward. Saving is not.
I personally think that it makes more sense that env_save() saves into the same location from which the enviroment has been previously loaded (that is, current implementation). Otherwise, the logic becomes error- prone since an user may env_load() from one location and env_save() into another, resulting in misalignments which requires a lot of extra logic in order to avoid such misalignments.
Or worse, you might end up erasing something that shouldn't be erased on your board.
Maybe the 'env save' command might need a parameter the tells it where to save?
Having a parameter to env_save() might be a solution but in my opinion it would break the priority logic: an user follows the priority logic when loading but she can work around that when saving. Moreover, having the location embedded into env_*() functions is a great nice to have imho.
I agree.
Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-
env_load_location.
I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.
Well, the use case I saw was that I wanted to test redundant environment storage. I admit this is not an end user use case but a developer use case, so I guess you're right.
So after fixing this endless loops I see two questions:
- to which environment do we store if the one in 'env_load_location'
fails to store?
Good question. My opinion is that it strongly depends on what we want to achieve with the implementation: do we want 1) to keep the consistency between load and save, or we want 2) to guarantee to be able to load/save from at least one location?
If 1) then a failure on env_save() simply fails and doesn't store anything. In other words we are multi-env when loading but single-env when storing. We are actually binding env_save() to the last env_load()'s result.
If 2) then a failure on env_save() will try all the available locations but we are open to misalignments. Here we are full multi-env since env_save() and env_load() are not bound together.
- to which environment do we store if all environments failed to
load (currently, it seems we store to the lowest prio in this case?)
This issue exists only in 1) and it results in 'not save' the environment.
Simon
Regards, Nicholas
On Wed, Jul 11, 2018 at 12:44:23PM +0200, Nicholas wrote:
Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-> env_load_location.
I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.
Well, the use case I saw was that I wanted to test redundant environment storage. I admit this is not an end user use case but a developer use case, so I guess you're right.
So after fixing this endless loops I see two questions: - to which environment do we store if the one in 'env_load_location' fails to store?
Good question. My opinion is that it strongly depends on what we want to achieve with the implementation: do we want 1) to keep the consistency between load and save, or we want 2) to guarantee to be able to load/save from at least one location?
If 1) then a failure on env_save() simply fails and doesn't store anything. In other words we are multi-env when loading but single-env when storing. We are actually binding env_save() to the last env_load()'s result.
If 2) then a failure on env_save() will try all the available locations but we are open to misalignments. Here we are full multi-env since env_save() and env_load() are not bound together.
In that case, we don't want to store to a lower priority, but to a higher one. Otherwise, the environment will be saved just fine, but will not be read next time, which will be very confusing to the user.
And with a higher priority, you might end up overwriting something you weren't expecting to overwrite, so I'd vote 1.
On 11.07.2018 15:50, Maxime Ripard wrote:
On Wed, Jul 11, 2018 at 12:44:23PM +0200, Nicholas wrote:
Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-> env_load_location.
I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.
Well, the use case I saw was that I wanted to test redundant environment storage. I admit this is not an end user use case but a developer use case, so I guess you're right.
So after fixing this endless loops I see two questions: - to which environment do we store if the one in 'env_load_location' fails to store?
Good question. My opinion is that it strongly depends on what we want to achieve with the implementation: do we want 1) to keep the consistency between load and save, or we want 2) to guarantee to be able to load/save from at least one location?
If 1) then a failure on env_save() simply fails and doesn't store anything. In other words we are multi-env when loading but single-env when storing. We are actually binding env_save() to the last env_load()'s result.
If 2) then a failure on env_save() will try all the available locations but we are open to misalignments. Here we are full multi-env since env_save() and env_load() are not bound together.
In that case, we don't want to store to a lower priority, but to a higher one. Otherwise, the environment will be saved just fine, but will not be read next time, which will be very confusing to the user.
And with a higher priority, you might end up overwriting something you weren't expecting to overwrite, so I'd vote 1.
I agree that 1 would be best. But from reading the code, unless I'm totally wrong, it seems that the patch sent by Nicholas does not suffice:
If no location contained a valid environment (e.g. no environment written yet), the lowest priority will be written as 'gd->env_load_location' is set to the lowest priority from iterating locations in 'env_load()'.
So we might have to reset 'gd->env_load_location' to highest prio if loading the environment fails.
Simon
On Thu, Jul 12, 2018 at 09:02:26AM +0200, Simon Goldschmidt wrote:
On 11.07.2018 15:50, Maxime Ripard wrote:
On Wed, Jul 11, 2018 at 12:44:23PM +0200, Nicholas wrote:
Maybe a solution could be to have an env_save() function which acts in a similar way as proposed in my patch and an env_save_prio() function, which acts like the env_load() i.e. looking for the best working location instead of relying on what has been stored into gd-> env_load_location.
I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.
Well, the use case I saw was that I wanted to test redundant environment storage. I admit this is not an end user use case but a developer use case, so I guess you're right.
So after fixing this endless loops I see two questions: - to which environment do we store if the one in 'env_load_location' fails to store?
Good question. My opinion is that it strongly depends on what we want to achieve with the implementation: do we want 1) to keep the consistency between load and save, or we want 2) to guarantee to be able to load/save from at least one location?
If 1) then a failure on env_save() simply fails and doesn't store anything. In other words we are multi-env when loading but single-env when storing. We are actually binding env_save() to the last env_load()'s result.
If 2) then a failure on env_save() will try all the available locations but we are open to misalignments. Here we are full multi-env since env_save() and env_load() are not bound together.
In that case, we don't want to store to a lower priority, but to a higher one. Otherwise, the environment will be saved just fine, but will not be read next time, which will be very confusing to the user.
And with a higher priority, you might end up overwriting something you weren't expecting to overwrite, so I'd vote 1.
I agree that 1 would be best. But from reading the code, unless I'm totally wrong, it seems that the patch sent by Nicholas does not suffice:
If no location contained a valid environment (e.g. no environment written yet), the lowest priority will be written as 'gd->env_load_location' is set to the lowest priority from iterating locations in 'env_load()'.
So we might have to reset 'gd->env_load_location' to highest prio if loading the environment fails.
That would make sense yes.
Maxime
On 12.07.2018 09:20, Maxime Ripard wrote:
On Thu, Jul 12, 2018 at 09:02:26AM +0200, Simon Goldschmidt wrote:
On 11.07.2018 15:50, Maxime Ripard wrote:
On Wed, Jul 11, 2018 at 12:44:23PM +0200, Nicholas wrote:
> Maybe a solution could be to have an env_save() function which > acts in a similar way as proposed in my patch and an > env_save_prio() function, which acts like the env_load() > i.e. looking for the best working location instead of relying > on what has been stored into gd-> env_load_location.
I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.
Well, the use case I saw was that I wanted to test redundant environment storage. I admit this is not an end user use case but a developer use case, so I guess you're right.
So after fixing this endless loops I see two questions: - to which environment do we store if the one in 'env_load_location' fails to store?
Good question. My opinion is that it strongly depends on what we want to achieve with the implementation: do we want 1) to keep the consistency between load and save, or we want 2) to guarantee to be able to load/save from at least one location?
If 1) then a failure on env_save() simply fails and doesn't store anything. In other words we are multi-env when loading but single-env when storing. We are actually binding env_save() to the last env_load()'s result.
If 2) then a failure on env_save() will try all the available locations but we are open to misalignments. Here we are full multi-env since env_save() and env_load() are not bound together.
In that case, we don't want to store to a lower priority, but to a higher one. Otherwise, the environment will be saved just fine, but will not be read next time, which will be very confusing to the user.
And with a higher priority, you might end up overwriting something you weren't expecting to overwrite, so I'd vote 1.
I agree that 1 would be best. But from reading the code, unless I'm totally wrong, it seems that the patch sent by Nicholas does not suffice:
If no location contained a valid environment (e.g. no environment written yet), the lowest priority will be written as 'gd->env_load_location' is set to the lowest priority from iterating locations in 'env_load()'.
So we might have to reset 'gd->env_load_location' to highest prio if loading the environment fails.
That would make sense yes.
Nicholas has sent v4 of this patch meanwhile, but it's not in reply to v1.
Any comments on that?
Simon
Dear Maxime,
In message 20180711094838.zyczks6iksp772ir@flea you wrote:
I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.
If the feature is available, use cases will spring into mind.
Assume you have a eMMC based system which can also boot from other storage (say, SDCard), and you want to write the current content of the eMMC environment to the alternative boot device.
Yes, there are usually alternative approaches to perform similar actions, but an easy way to save lsewhere might be handy, especially if it comes at low cost.
Best regards,
Wolfgang Denk
On Wed, Jul 11, 2018 at 12:49:50PM +0200, Wolfgang Denk wrote:
Dear Maxime,
In message 20180711094838.zyczks6iksp772ir@flea you wrote:
I don't really see a use-case for overriding wherever the environment at the user-level actually. At the board level, for redundancy or transition, yes, definitely, but we can already do that.
If the feature is available, use cases will spring into mind.
Assume you have a eMMC based system which can also boot from other storage (say, SDCard), and you want to write the current content of the eMMC environment to the alternative boot device.
Yes, there are usually alternative approaches to perform similar actions, but an easy way to save lsewhere might be handy, especially if it comes at low cost.
Right, but that would bring a much more significant rework as to how the environments are handled than just the changes that are being discussed here. At the moment, the environment can only be stored on one device for each "types" (one raw MMC, one MTD partition, etc). The redundancy allows you to duplicate it, but you won't be able to store on just one instance of your choosing at the moment.
Maxime
Dear Maxime,
In message 20180711134735.vhhc7quuzcp42bo7@flea you wrote:
If the feature is available, use cases will spring into mind.
...
Right, but that would bring a much more significant rework as to how the environments are handled than just the changes that are being discussed here. At the moment, the environment can only be stored on one device for each "types" (one raw MMC, one MTD partition, etc). The redundancy allows you to duplicate it, but you won't be able to store on just one instance of your choosing at the moment.
I am aware of this. However, if we can pave the way for this to be added later at little or no additional cost, should we not try that?
Best regards,
Wolfgang Denk
Hi Simon,
Hi Nicholas,
On 10 July 2018 at 06:57, Nicholas Faustini nicholas.faustini@azcomtech.com wrote:
When called with ENVOP_SAVE, env_get_location() only returns the gd->env_load_location variable without actually checking for the environment location and priority. This allows saving the environment into the same location that has been previously loaded.
This behaviour causes env_save() to fall into an infinite loop when the low-level drv->save() call fails.
Why is this? Should it not stop eventually?
I can confirm that there is a problem as described here - when you fail to write to eMMC device (like e.g. 2, not 0), then we loop forever.
I do see such situation with Odroid XU3 when switching to BLK.
Do we need a limit on prio?
The env_save() function should not loop through the environment location list but it should use the previously discovered environment driver once.
What is that? It should be possible to write the env to multiple places. Also what is the previously discovered environment?
Signed-off-by: Nicholas Faustini nicholas.faustini@azcomtech.com
env/env.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/env/env.c b/env/env.c index 5c0842a..897d197 100644 --- a/env/env.c +++ b/env/env.c @@ -211,16 +211,16 @@ int env_load(void) int env_save(void) { struct env_driver *drv;
int prio;for (prio = 0; (drv = env_driver_lookup(ENVOP_SAVE, prio));prio++) {
drv = env_driver_lookup(ENVOP_SAVE, 0);if (drv) { int ret; if (!drv->save)
continue;
return -ENODEV; if (!env_has_inited(drv->location))
continue;
return -ENODEV; printf("Saving Environment to %s... ", drv->name); ret = drv->save();-- 2.7.4
Regards, Simon
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-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
participants (7)
-
Lukasz Majewski -
Maxime Ripard -
Nicholas -
Nicholas Faustini
-
Simon Glass -
Simon Goldschmidt -
Wolfgang Denk