| Message ID | 20190717082525.891-6-takahiro.akashi@linaro.org |
|---|---|
| State | RFC |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: non-volatile variables support | expand |
Dear AKASHI Takahiro, In message <20190717082525.891-6-takahiro.akashi@linaro.org> you wrote: > Please note that the aim of this patch is to illustrate how we can > extend the existing backing storage drivers for env interfaces to > support U-Boot environment context. > > We will be able to support more devices as well as more contexts > in a similar way. Existing drivers still work exactly in the same > way as before while they are not extended yet. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > env/fat.c | 103 ++++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 84 insertions(+), 19 deletions(-) > > diff --git a/env/fat.c b/env/fat.c > index 7f74c64dfe7e..e4a672d2730a 100644 > --- a/env/fat.c > +++ b/env/fat.c > @@ -30,23 +30,44 @@ > # endif > #endif > > +static struct evn_fat_context { > + char *interface; > + char *dev_and_part; > + char *file; > +} fat_context[ENVCTX_COUNT] = { > +#if defined(CONFIG_ENV_FAT_INTERFACE) && \ > + defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && defined(CONFIG_ENV_FAT_FILE) > + [ENVCTX_UBOOT] = { > + CONFIG_ENV_FAT_INTERFACE, > + CONFIG_ENV_FAT_DEVICE_AND_PART, > + CONFIG_ENV_FAT_FILE, > + }, > +#endif > +}; Does it make sense to define this in a FAT specific way? I guess we could come up with a common structure that covers all supported storage devices and use this here. Also, the "#if defined" looks really dangerous to me, as missing #defines will go unnoticed, and in the end you are accessing uninitialized data... Did you review the patchset for memory leaks? Best regards, Wolfgang Denk
On Fri, Jul 19, 2019 at 10:21:12AM +0200, Wolfgang Denk wrote: > Dear AKASHI Takahiro, > > In message <20190717082525.891-6-takahiro.akashi@linaro.org> you wrote: > > Please note that the aim of this patch is to illustrate how we can > > extend the existing backing storage drivers for env interfaces to > > support U-Boot environment context. > > > > We will be able to support more devices as well as more contexts > > in a similar way. Existing drivers still work exactly in the same > > way as before while they are not extended yet. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > env/fat.c | 103 ++++++++++++++++++++++++++++++++++++++++++++---------- > > 1 file changed, 84 insertions(+), 19 deletions(-) > > > > diff --git a/env/fat.c b/env/fat.c > > index 7f74c64dfe7e..e4a672d2730a 100644 > > --- a/env/fat.c > > +++ b/env/fat.c > > @@ -30,23 +30,44 @@ > > # endif > > #endif > > > > +static struct evn_fat_context { > > + char *interface; > > + char *dev_and_part; > > + char *file; > > +} fat_context[ENVCTX_COUNT] = { > > +#if defined(CONFIG_ENV_FAT_INTERFACE) && \ > > + defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && defined(CONFIG_ENV_FAT_FILE) > > + [ENVCTX_UBOOT] = { > > + CONFIG_ENV_FAT_INTERFACE, > > + CONFIG_ENV_FAT_DEVICE_AND_PART, > > + CONFIG_ENV_FAT_FILE, > > + }, > > +#endif > > +}; > > Does it make sense to define this in a FAT specific way? I guess we > could come up with a common structure that covers all supported > storage devices and use this here. But a different driver has different sets of configurations required. How can we *generalize* them? > Also, the "#if defined" looks really dangerous to me, as missing > #defines will go unnoticed, and in the end you are accessing > uninitialized data... Yes, I have made this mistake before. But I think that all the drivers must be verified in any case before any changes are applied to the upstream. No? > Did you review the patchset for memory leaks? No. -Takahiro Akashi > > Best regards, > > Wolfgang Denk > > -- > 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 > If you can't explain it to a six year old, you don't understand it > yourself. - Albert Einstein
Dear Takahiro, In message <20190719083548.GU21948@linaro.org> you wrote: > > > > +#if defined(CONFIG_ENV_FAT_INTERFACE) && \ > > > + defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && defined(CONFIG_ENV_FAT_FILE) > > > + [ENVCTX_UBOOT] = { > > > + CONFIG_ENV_FAT_INTERFACE, > > > + CONFIG_ENV_FAT_DEVICE_AND_PART, > > > + CONFIG_ENV_FAT_FILE, > > > + }, > > > +#endif > > > +}; > > > > Does it make sense to define this in a FAT specific way? I guess we > > could come up with a common structure that covers all supported > > storage devices and use this here. > > But a different driver has different sets of configurations required. > How can we *generalize* them? Does it? At least all block devices probably share the properties of interface, device, partition and file name in one way or another. Or offset instead of file name when accessing the raw device. I think it should be possible to at least support all file systems with exactly the sme code - ther eis nothing special about FAT here, or am I missing something? > > Also, the "#if defined" looks really dangerous to me, as missing > > #defines will go unnoticed, and in the end you are accessing > > uninitialized data... > > Yes, I have made this mistake before. > But I think that all the drivers must be verified in any case > before any changes are applied to the upstream. No? Indeed, testing will be needed. Best regards, Wolfgang Denk
diff --git a/env/fat.c b/env/fat.c index 7f74c64dfe7e..e4a672d2730a 100644 --- a/env/fat.c +++ b/env/fat.c @@ -30,23 +30,44 @@ # endif #endif +static struct evn_fat_context { + char *interface; + char *dev_and_part; + char *file; +} fat_context[ENVCTX_COUNT] = { +#if defined(CONFIG_ENV_FAT_INTERFACE) && \ + defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && defined(CONFIG_ENV_FAT_FILE) + [ENVCTX_UBOOT] = { + CONFIG_ENV_FAT_INTERFACE, + CONFIG_ENV_FAT_DEVICE_AND_PART, + CONFIG_ENV_FAT_FILE, + }, +#endif +}; + #ifdef CMD_SAVEENV -static int env_fat_save(void) +static int env_fat_save_ext(enum env_context ctx) { env_t __aligned(ARCH_DMA_MINALIGN) env_new; + env_hdr_t *envp; struct blk_desc *dev_desc = NULL; disk_partition_t info; int dev, part; int err; loff_t size; - err = env_export(&env_new); + if (!fat_context[ctx].interface) + return -EIO; + + env_new.data_size = ENV_SIZE; + envp = (env_hdr_t *)&env_new; + err = env_export_ext(&envp, ctx); if (err) return err; - part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE, - CONFIG_ENV_FAT_DEVICE_AND_PART, - &dev_desc, &info, 1); + part = blk_get_device_part_str(fat_context[ctx].interface, + fat_context[ctx].dev_and_part, + &dev_desc, &info, 1); if (part < 0) return 1; @@ -57,28 +78,34 @@ static int env_fat_save(void) * will calling it. The missing \n is intentional. */ printf("Unable to use %s %d:%d... ", - CONFIG_ENV_FAT_INTERFACE, dev, part); + fat_context[ctx].interface, dev, part); return 1; } - err = file_fat_write(CONFIG_ENV_FAT_FILE, (void *)&env_new, 0, sizeof(env_t), - &size); + err = file_fat_write(fat_context[ctx].file, (void *)envp, 0, + sizeof(env_hdr_t) + envp->data_size, &size); if (err == -1) { /* * This printf is embedded in the messages from env_save that * will calling it. The missing \n is intentional. */ printf("Unable to write \"%s\" from %s%d:%d... ", - CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part); + fat_context[ctx].file, fat_context[ctx].interface, + dev, part); return 1; } return 0; } + +static int env_fat_save(void) +{ + return env_fat_save_ext(ENVCTX_UBOOT); +} #endif /* CMD_SAVEENV */ #ifdef LOADENV -static int env_fat_load(void) +static int env_fat_load_ext(enum env_context ctx) { ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE); struct blk_desc *dev_desc = NULL; @@ -86,14 +113,17 @@ static int env_fat_load(void) int dev, part; int err; + if (!fat_context[ctx].interface) + return -EIO; + #ifdef CONFIG_MMC - if (!strcmp(CONFIG_ENV_FAT_INTERFACE, "mmc")) + if (!strcmp(fat_context[ctx].interface, "mmc")) mmc_initialize(NULL); #endif - part = blk_get_device_part_str(CONFIG_ENV_FAT_INTERFACE, - CONFIG_ENV_FAT_DEVICE_AND_PART, - &dev_desc, &info, 1); + part = blk_get_device_part_str(fat_context[ctx].interface, + fat_context[ctx].dev_and_part, + &dev_desc, &info, 1); if (part < 0) goto err_env_relocate; @@ -104,37 +134,72 @@ static int env_fat_load(void) * will calling it. The missing \n is intentional. */ printf("Unable to use %s %d:%d... ", - CONFIG_ENV_FAT_INTERFACE, dev, part); + fat_context[ctx].interface, dev, part); goto err_env_relocate; } - err = file_fat_read(CONFIG_ENV_FAT_FILE, buf, CONFIG_ENV_SIZE); + err = file_fat_read(fat_context[ctx].file, buf, CONFIG_ENV_SIZE); if (err == -1) { /* * This printf is embedded in the messages from env_save that * will calling it. The missing \n is intentional. */ printf("Unable to read \"%s\" from %s%d:%d... ", - CONFIG_ENV_FAT_FILE, CONFIG_ENV_FAT_INTERFACE, dev, part); + fat_context[ctx].file, fat_context[ctx].interface, + dev, part); goto err_env_relocate; } - return env_import(buf, 1); + if (((env_hdr_t *)buf)->data_size <= 1) + return 0; + + return env_import_ext(buf, ctx, 1); err_env_relocate: - set_default_env(NULL, 0); + if (ctx == ENVCTX_UBOOT) + set_default_env(NULL, 0); return -EIO; } + +static int env_fat_load(void) +{ + return env_fat_load_ext(ENVCTX_UBOOT); +} #endif /* LOADENV */ +#if defined(CMD_LOADENV) || defined(CMD_SAVEENV) +static int env_fat_init_ext(enum env_context ctx) +{ + if (ctx != ENVCTX_UBOOT) + return 0; + /* + * Note: + * We can't report 0 here because low-level storage driver, + * like scsi, may not have been detected yet at boot time. + */ + return -ENOENT; +} + +static int env_fat_init(void) +{ + return env_fat_init_ext(ENVCTX_UBOOT); +} +#endif + U_BOOT_ENV_LOCATION(fat) = { .location = ENVL_FAT, ENV_NAME("FAT") #ifdef LOADENV .load = env_fat_load, + .load_ext = env_fat_load_ext, #endif #ifdef CMD_SAVEENV .save = env_save_ptr(env_fat_save), + .save_ext = env_save_ptr(env_fat_save_ext), +#endif +#if defined(CMD_LOADENV) || defined(CMD_SAVEENV) + .init = env_fat_init, + .init_ext = env_fat_init_ext, #endif };
Please note that the aim of this patch is to illustrate how we can extend the existing backing storage drivers for env interfaces to support U-Boot environment context. We will be able to support more devices as well as more contexts in a similar way. Existing drivers still work exactly in the same way as before while they are not extended yet. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- env/fat.c | 103 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 19 deletions(-)