| Message ID | 20171116092231.27740-5-maxime.ripard@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | env: Multiple env support and env transition for sunxi | expand |
On Thu, 16 Nov 2017 10:22:25 +0100 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; Is this correct? Above you mentioned that prio = 0 is the default. With this condition you may only allow prio <= 0... > + > 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; 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
Hi Maxime, On 16/11/2017 10:22, 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. > > 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); > - 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; > + Shouldn't we init all drivers that can be found? I think it is perfectly plausible that an env that you could init could fail to save/load its environment. Then we would try to save/load from the next one but with the current code, the next one wouldn't be initialized. Quentin
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(-)