| Message ID | 275d9901f239926baf949612e99bae437fc1836f.1511864667.git-series.maxime.ripard@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | env: Multiple env support and env transition for sunxi | expand |
Hi Maxime, On 28/11/17 10:24, Maxime Ripard wrote: > In preparation for the multiple environment support, let's introduce two > new parameters to the environment driver lookup function: the priority and > operation. > > The operation parameter is meant to identify, obviously, the operation you > might want to perform on the environment. I wonder if this is a bit too generic. Wouldn't a simple: bool is_save_op (or some more clever name) be sufficient? At least this is actually all we need, if I understand the code correctly. But it's up to you, I can see advantages for both approaches. > The priority is a number passed to identify the environment priority you > want to retrieve. The lowest priority parameter (0) will be the primary > source. > > Combining the two parameters allow you to support multiple environments > through different priorities, and to change those priorities between read > and writes operations. > > This is especially useful to implement migration mechanisms where you want > to always use the same environment first, be it to read or write, while the > common case is more likely to use the same environment it has read from to > write it to. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > env/env.c | 122 +++++++++++++++++++++++++------------------ > include/environment.h | 7 ++- > 2 files changed, 80 insertions(+), 49 deletions(-) > > diff --git a/env/env.c b/env/env.c > index 97ada5b5a6fd..673bfa6ba41b 100644 > --- a/env/env.c > +++ b/env/env.c > @@ -26,8 +26,11 @@ static struct env_driver *_env_driver_lookup(enum env_location loc) > return NULL; > } > > -static enum env_location env_get_location(void) > +static enum env_location env_get_location(enum env_operation op, int prio) > { > + if (prio >= 1) > + return ENVL_UNKNOWN; > + > if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM) > return ENVL_EEPROM; > else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT) > @@ -52,11 +55,14 @@ static enum env_location env_get_location(void) > return ENVL_UNKNOWN; > } > > -static struct env_driver *env_driver_lookup(void) > +static struct env_driver *env_driver_lookup(enum env_operation op, int prio) > { > - enum env_location loc = env_get_location(); > + enum env_location loc = env_get_location(op, prio); > struct env_driver *drv; > > + if (loc == ENVL_UNKNOWN) > + return NULL; > + > drv = _env_driver_lookup(loc); > if (!drv) { > debug("%s: No environment driver for location %d\n", __func__, > @@ -69,83 +75,101 @@ static struct env_driver *env_driver_lookup(void) > > int env_get_char(int index) > { > - struct env_driver *drv = env_driver_lookup(); > - int ret; > + struct env_driver *drv; > + int prio; > > if (gd->env_valid == ENV_INVALID) > return default_environment[index]; > - if (!drv) > - return -ENODEV; > - if (!drv->get_char) > - return *(uchar *)(gd->env_addr + index); Did you deliberately remove this fallback from the new code below? > - ret = drv->get_char(index); > - if (ret < 0) { > - debug("%s: Environment failed to load (err=%d)\n", > - __func__, ret); > + > + for (prio = 0; (drv = env_driver_lookup(ENVO_GET_CHAR, prio)); prio++) { > + int ret; > + > + if (!drv->get_char) > + continue; > + > + ret = drv->get_char(index); > + if (!ret) > + return 0; > + > + debug("%s: Environment %s failed to load (err=%d)\n", __func__, > + drv->name, ret); > } > > - return ret; > + return -ENODEV; > } > > int env_load(void) > { > - struct env_driver *drv = env_driver_lookup(); > - int ret = 0; > + struct env_driver *drv; > + int prio; > > - if (!drv) > - return -ENODEV; > - if (!drv->load) > - return 0; > - ret = drv->load(); > - if (ret) { > - debug("%s: Environment failed to load (err=%d)\n", __func__, > - ret); > - return ret; > + for (prio = 0; (drv = env_driver_lookup(ENVO_LOAD, prio)); prio++) { > + int ret; > + > + if (!drv->load) > + continue; Is this change to the algorithm acceptable? Formerly we returned 0 on finding a drv->load pointer to be NULL, now it's -ENODEV, plus we try to find other sources first. Or is that solved later somehow with the env_has_init bitmap? > + > + ret = drv->load(); > + if (!ret) > + return 0; > + > + debug("%s: Environment %s failed to load (err=%d)\n", __func__, > + drv->name, ret); > } > > - return 0; > + return -ENODEV; > } > > int env_save(void) > { > - struct env_driver *drv = env_driver_lookup(); > - int ret; > + struct env_driver *drv; > + int prio; > > - if (!drv) > - return -ENODEV; > - if (!drv->save) > - return -ENOSYS; > - > - printf("Saving Environment to %s...\n", drv->name); > - ret = drv->save(); > - if (ret) { > - debug("%s: Environment failed to save (err=%d)\n", __func__, > - ret); > - return ret; > + for (prio = 0; (drv = env_driver_lookup(ENVO_SAVE, prio)); prio++) { > + int ret; > + > + if (!drv->save) > + continue; Same as for env_load(). Cheers, Andre > + > + printf("Saving Environment to %s...\n", drv->name); > + ret = drv->save(); > + if (!ret) > + return 0; > + > + debug("%s: Environment %s failed to save (err=%d)\n", __func__, > + drv->name, ret); > } > > - return 0; > + return -ENODEV; > } > > int env_init(void) > { > - struct env_driver *drv = env_driver_lookup(); > + struct env_driver *drv; > int ret = -ENOENT; > + int prio; > + > + for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) { > + if (!drv->init) > + continue; > > - if (!drv) > - return -ENODEV; > - if (drv->init) > ret = drv->init(); > + if (!ret) > + return 0; > + > + debug("%s: Environment %s failed to init (err=%d)\n", __func__, > + drv->name, ret); > + } > + > + if (!prio) > + return -ENODEV; > + > if (ret == -ENOENT) { > gd->env_addr = (ulong)&default_environment[0]; > gd->env_valid = ENV_VALID; > > return 0; > - } else if (ret) { > - debug("%s: Environment failed to init (err=%d)\n", __func__, > - ret); > - return ret; > } > > - return 0; > + return ret; > } > diff --git a/include/environment.h b/include/environment.h > index 226e3ef2d23a..7fa8b98cc0db 100644 > --- a/include/environment.h > +++ b/include/environment.h > @@ -215,6 +215,13 @@ enum env_location { > ENVL_UNKNOWN, > }; > > +enum env_operation { > + ENVO_GET_CHAR, > + ENVO_INIT, > + ENVO_LOAD, > + ENVO_SAVE, > +}; > + > struct env_driver { > const char *name; > enum env_location location; >
Hi Andre, On Tue, Dec 05, 2017 at 10:09:04AM +0000, Andre Przywara wrote: > On 28/11/17 10:24, Maxime Ripard wrote: > > In preparation for the multiple environment support, let's introduce two > > new parameters to the environment driver lookup function: the priority and > > operation. > > > > The operation parameter is meant to identify, obviously, the operation you > > might want to perform on the environment. > > I wonder if this is a bit too generic. Wouldn't a simple: > bool is_save_op (or some more clever name) > be sufficient? At least this is actually all we need, if I understand > the code correctly. > But it's up to you, I can see advantages for both approaches. I'm not really sure what you meant, but I'll try to explain the various constraints we have: - We need to init all the environments, no matter their priority, without any particular ordering - We need to load the environments by descending order of priority, until a valid one is found. I guess get_char is in the same situation, but I'm not really sure what it's supposed to be doing. - We need to store the environments. Now, this gets tricky. The current code only implement two cases: you store the environment where you loaded it from, which is the current case, and forcing always the same order, like we did on sunxi. But you might envision without pulling your hairs too much a situation for example that we would have loaded our environment from a read-only location (either because it is physically read-only, or because you cannot modify it (ie. signed environments)). In this case, you will need to implement a fallback strategy that is probably going to be by board, probably along the line of trying to save in our environments by ascending order, starting with the environment we loaded from. tl; dr: we might need that flexibility for future use cases that are not clear yet. > > int env_load(void) > > { > > - struct env_driver *drv = env_driver_lookup(); > > - int ret = 0; > > + struct env_driver *drv; > > + int prio; > > > > - if (!drv) > > - return -ENODEV; > > - if (!drv->load) > > - return 0; > > - ret = drv->load(); > > - if (ret) { > > - debug("%s: Environment failed to load (err=%d)\n", __func__, > > - ret); > > - return ret; > > + for (prio = 0; (drv = env_driver_lookup(ENVO_LOAD, prio)); prio++) { > > + int ret; > > + > > + if (!drv->load) > > + continue; > > Is this change to the algorithm acceptable? Formerly we returned 0 on > finding a drv->load pointer to be NULL, now it's -ENODEV, plus we try to > find other sources first. I couldn't really find a better way to deal with this. You're right that the current algorithm is that if we don't have a driver, return ENODEV, and if we don't have a load function, return 0. When dealing with multiple enviroments, you would obviously have to return -ENODEV if you didn't find any, but I couldn't find a proper way to decide when we would return 0: should we return 0 when no environment has a load function? only the first one we encounter? Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Hi Maxime, On 28 November 2017 at 03:24, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > In preparation for the multiple environment support, let's introduce two > new parameters to the environment driver lookup function: the priority and > operation. > > The operation parameter is meant to identify, obviously, the operation you > might want to perform on the environment. > > The priority is a number passed to identify the environment priority you > want to retrieve. The lowest priority parameter (0) will be the primary > source. > > Combining the two parameters allow you to support multiple environments > through different priorities, and to change those priorities between read > and writes operations. > > This is especially useful to implement migration mechanisms where you want > to always use the same environment first, be it to read or write, while the > common case is more likely to use the same environment it has read from to > write it to. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > env/env.c | 122 +++++++++++++++++++++++++------------------ > include/environment.h | 7 ++- > 2 files changed, 80 insertions(+), 49 deletions(-) > > diff --git a/env/env.c b/env/env.c > index 97ada5b5a6fd..673bfa6ba41b 100644 > --- a/env/env.c > +++ b/env/env.c > @@ -26,8 +26,11 @@ static struct env_driver *_env_driver_lookup(enum env_location loc) > return NULL; > } > > -static enum env_location env_get_location(void) > +static enum env_location env_get_location(enum env_operation op, int prio) > { > + if (prio >= 1) > + return ENVL_UNKNOWN; > + > if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM) > return ENVL_EEPROM; > else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT) > @@ -52,11 +55,14 @@ static enum env_location env_get_location(void) > return ENVL_UNKNOWN; > } > > -static struct env_driver *env_driver_lookup(void) > +static struct env_driver *env_driver_lookup(enum env_operation op, int prio) Can you please document the function args? The operation of prio needs to be described somewhere. > { > - enum env_location loc = env_get_location(); > + enum env_location loc = env_get_location(op, prio); > struct env_driver *drv; > > + if (loc == ENVL_UNKNOWN) > + return NULL; Why is this needed? Shouldn't _env_driver_lookup() return NULL in this case anyway? > + > drv = _env_driver_lookup(loc); > if (!drv) { > debug("%s: No environment driver for location %d\n", __func__, > @@ -69,83 +75,101 @@ static struct env_driver *env_driver_lookup(void) > > int env_get_char(int index) > { > - struct env_driver *drv = env_driver_lookup(); > - int ret; > + struct env_driver *drv; > + int prio; > > if (gd->env_valid == ENV_INVALID) > return default_environment[index]; > - if (!drv) > - return -ENODEV; > - if (!drv->get_char) > - return *(uchar *)(gd->env_addr + index); > - ret = drv->get_char(index); > - if (ret < 0) { > - debug("%s: Environment failed to load (err=%d)\n", > - __func__, ret); > + > + for (prio = 0; (drv = env_driver_lookup(ENVO_GET_CHAR, prio)); prio++) { > + int ret; > + > + if (!drv->get_char) > + continue; > + > + ret = drv->get_char(index); > + if (!ret) > + return 0; > + > + debug("%s: Environment %s failed to load (err=%d)\n", __func__, > + drv->name, ret); > } > > - return ret; > + return -ENODEV; > } > > int env_load(void) > { > - struct env_driver *drv = env_driver_lookup(); > - int ret = 0; > + struct env_driver *drv; > + int prio; > > - if (!drv) > - return -ENODEV; > - if (!drv->load) > - return 0; > - ret = drv->load(); > - if (ret) { > - debug("%s: Environment failed to load (err=%d)\n", __func__, > - ret); > - return ret; > + for (prio = 0; (drv = env_driver_lookup(ENVO_LOAD, prio)); prio++) { > + int ret; > + > + if (!drv->load) > + continue; > + > + ret = drv->load(); > + if (!ret) > + return 0; > + > + debug("%s: Environment %s failed to load (err=%d)\n", __func__, > + drv->name, ret); > } > > - return 0; > + return -ENODEV; > } > > int env_save(void) > { > - struct env_driver *drv = env_driver_lookup(); > - int ret; > + struct env_driver *drv; > + int prio; > > - if (!drv) > - return -ENODEV; > - if (!drv->save) > - return -ENOSYS; > - > - printf("Saving Environment to %s...\n", drv->name); > - ret = drv->save(); > - if (ret) { > - debug("%s: Environment failed to save (err=%d)\n", __func__, > - ret); > - return ret; > + for (prio = 0; (drv = env_driver_lookup(ENVO_SAVE, prio)); prio++) { > + int ret; > + > + if (!drv->save) > + continue; > + > + printf("Saving Environment to %s...\n", drv->name); > + ret = drv->save(); > + if (!ret) > + return 0; So we get no advice of error? I think it should print a message here, like ...failed or ...done. Also if no driver succeeded I suggest returning one of the errors returned from drv-save(). > + > + debug("%s: Environment %s failed to save (err=%d)\n", __func__, > + drv->name, ret); > } > > - return 0; > + return -ENODEV; > } > > int env_init(void) > { > - struct env_driver *drv = env_driver_lookup(); > + struct env_driver *drv; > int ret = -ENOENT; > + int prio; > + > + for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) { > + if (!drv->init) > + continue; > > - if (!drv) > - return -ENODEV; > - if (drv->init) > ret = drv->init(); > + if (!ret) > + return 0; > + > + debug("%s: Environment %s failed to init (err=%d)\n", __func__, > + drv->name, ret); > + } > + > + if (!prio) > + return -ENODEV; > + > if (ret == -ENOENT) { > gd->env_addr = (ulong)&default_environment[0]; > gd->env_valid = ENV_VALID; > > return 0; > - } else if (ret) { > - debug("%s: Environment failed to init (err=%d)\n", __func__, > - ret); > - return ret; > } > > - return 0; > + return ret; > } > diff --git a/include/environment.h b/include/environment.h > index 226e3ef2d23a..7fa8b98cc0db 100644 > --- a/include/environment.h > +++ b/include/environment.h > @@ -215,6 +215,13 @@ enum env_location { > ENVL_UNKNOWN, > }; > comment here > +enum env_operation { > + ENVO_GET_CHAR, I think ENVOP would be a better prefix. > + ENVO_INIT, > + ENVO_LOAD, > + ENVO_SAVE, > +}; > + > struct env_driver { > const char *name; > enum env_location location; > -- > git-series 0.9.1 Regards, Simon
Hi Simon, On Thu, Dec 28, 2017 at 08:13:23PM -0700, Simon Glass wrote: > > -static struct env_driver *env_driver_lookup(void) > > +static struct env_driver *env_driver_lookup(enum env_operation op, int prio) > > Can you please document the function args? The operation of prio needs > to be described somewhere. Will do. > > { > > - enum env_location loc = env_get_location(); > > + enum env_location loc = env_get_location(op, prio); > > struct env_driver *drv; > > > > + if (loc == ENVL_UNKNOWN) > > + return NULL; > > Why is this needed? Shouldn't _env_driver_lookup() return NULL in this > case anyway? I just thought that making it obvious and explicit (especially when the behaviour of _env_driver_lookup might change) would be a good thing, but I can remove it if you want. > > - if (!drv) > > - return -ENODEV; > > - if (!drv->save) > > - return -ENOSYS; > > - > > - printf("Saving Environment to %s...\n", drv->name); > > - ret = drv->save(); > > - if (ret) { > > - debug("%s: Environment failed to save (err=%d)\n", __func__, > > - ret); > > - return ret; > > + for (prio = 0; (drv = env_driver_lookup(ENVO_SAVE, prio)); prio++) { > > + int ret; > > + > > + if (!drv->save) > > + continue; > > + > > + printf("Saving Environment to %s...\n", drv->name); > > + ret = drv->save(); > > + if (!ret) > > + return 0; > > So we get no advice of error? I think it should print a message here, > like ...failed or ...done. This is addressed in the next patch > Also if no driver succeeded I suggest returning one of the errors > returned from drv-save(). And this was one of the comments made in this version, so it's going to be addressed in the next iteration. > > --- a/include/environment.h > > +++ b/include/environment.h > > @@ -215,6 +215,13 @@ enum env_location { > > ENVL_UNKNOWN, > > }; > > > > comment here > > > +enum env_operation { > > + ENVO_GET_CHAR, > > I think ENVOP would be a better prefix. That works for me. Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
diff --git a/env/env.c b/env/env.c index 97ada5b5a6fd..673bfa6ba41b 100644 --- a/env/env.c +++ b/env/env.c @@ -26,8 +26,11 @@ static struct env_driver *_env_driver_lookup(enum env_location loc) return NULL; } -static enum env_location env_get_location(void) +static enum env_location env_get_location(enum env_operation op, int prio) { + if (prio >= 1) + return ENVL_UNKNOWN; + if IS_ENABLED(CONFIG_ENV_IS_IN_EEPROM) return ENVL_EEPROM; else if IS_ENABLED(CONFIG_ENV_IS_IN_FAT) @@ -52,11 +55,14 @@ static enum env_location env_get_location(void) return ENVL_UNKNOWN; } -static struct env_driver *env_driver_lookup(void) +static struct env_driver *env_driver_lookup(enum env_operation op, int prio) { - enum env_location loc = env_get_location(); + enum env_location loc = env_get_location(op, prio); struct env_driver *drv; + if (loc == ENVL_UNKNOWN) + return NULL; + drv = _env_driver_lookup(loc); if (!drv) { debug("%s: No environment driver for location %d\n", __func__, @@ -69,83 +75,101 @@ static struct env_driver *env_driver_lookup(void) int env_get_char(int index) { - struct env_driver *drv = env_driver_lookup(); - int ret; + struct env_driver *drv; + int prio; if (gd->env_valid == ENV_INVALID) return default_environment[index]; - if (!drv) - return -ENODEV; - if (!drv->get_char) - return *(uchar *)(gd->env_addr + index); - ret = drv->get_char(index); - if (ret < 0) { - debug("%s: Environment failed to load (err=%d)\n", - __func__, ret); + + for (prio = 0; (drv = env_driver_lookup(ENVO_GET_CHAR, prio)); prio++) { + int ret; + + if (!drv->get_char) + continue; + + ret = drv->get_char(index); + if (!ret) + return 0; + + debug("%s: Environment %s failed to load (err=%d)\n", __func__, + drv->name, ret); } - return ret; + return -ENODEV; } int env_load(void) { - struct env_driver *drv = env_driver_lookup(); - int ret = 0; + struct env_driver *drv; + int prio; - if (!drv) - return -ENODEV; - if (!drv->load) - return 0; - ret = drv->load(); - if (ret) { - debug("%s: Environment failed to load (err=%d)\n", __func__, - ret); - return ret; + for (prio = 0; (drv = env_driver_lookup(ENVO_LOAD, prio)); prio++) { + int ret; + + if (!drv->load) + continue; + + ret = drv->load(); + if (!ret) + return 0; + + debug("%s: Environment %s failed to load (err=%d)\n", __func__, + drv->name, ret); } - return 0; + return -ENODEV; } int env_save(void) { - struct env_driver *drv = env_driver_lookup(); - int ret; + struct env_driver *drv; + int prio; - if (!drv) - return -ENODEV; - if (!drv->save) - return -ENOSYS; - - printf("Saving Environment to %s...\n", drv->name); - ret = drv->save(); - if (ret) { - debug("%s: Environment failed to save (err=%d)\n", __func__, - ret); - return ret; + for (prio = 0; (drv = env_driver_lookup(ENVO_SAVE, prio)); prio++) { + int ret; + + if (!drv->save) + continue; + + printf("Saving Environment to %s...\n", drv->name); + ret = drv->save(); + if (!ret) + return 0; + + debug("%s: Environment %s failed to save (err=%d)\n", __func__, + drv->name, ret); } - return 0; + return -ENODEV; } int env_init(void) { - struct env_driver *drv = env_driver_lookup(); + struct env_driver *drv; int ret = -ENOENT; + int prio; + + for (prio = 0; (drv = env_driver_lookup(ENVO_INIT, prio)); prio++) { + if (!drv->init) + continue; - if (!drv) - return -ENODEV; - if (drv->init) ret = drv->init(); + if (!ret) + return 0; + + debug("%s: Environment %s failed to init (err=%d)\n", __func__, + drv->name, ret); + } + + if (!prio) + return -ENODEV; + if (ret == -ENOENT) { gd->env_addr = (ulong)&default_environment[0]; gd->env_valid = ENV_VALID; return 0; - } else if (ret) { - debug("%s: Environment failed to init (err=%d)\n", __func__, - ret); - return ret; } - return 0; + return ret; } diff --git a/include/environment.h b/include/environment.h index 226e3ef2d23a..7fa8b98cc0db 100644 --- a/include/environment.h +++ b/include/environment.h @@ -215,6 +215,13 @@ enum env_location { ENVL_UNKNOWN, }; +enum env_operation { + ENVO_GET_CHAR, + ENVO_INIT, + ENVO_LOAD, + ENVO_SAVE, +}; + struct env_driver { const char *name; enum env_location location;
In preparation for the multiple environment support, let's introduce two new parameters to the environment driver lookup function: the priority and operation. The operation parameter is meant to identify, obviously, the operation you might want to perform on the environment. The priority is a number passed to identify the environment priority you want to retrieve. The lowest priority parameter (0) will be the primary source. Combining the two parameters allow you to support multiple environments through different priorities, and to change those priorities between read and writes operations. This is especially useful to implement migration mechanisms where you want to always use the same environment first, be it to read or write, while the common case is more likely to use the same environment it has read from to write it to. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- env/env.c | 122 +++++++++++++++++++++++++------------------ include/environment.h | 7 ++- 2 files changed, 80 insertions(+), 49 deletions(-)