| Message ID | 20230920151245.2786420-1-james.hilliard1@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [1/1] bootloader/uboot: support environment in ubi | expand |
Hi James, On 20.09.23 17:12, James Hilliard wrote: > When using uboot env in UBI in combination with CONFIG_UBIATTACH we > may fail to interface with the environment as scan_mtd_devices would > not be run until actually running an update. > > To handle this case safely add an erase flag to scan_mtd_devices so > that we can attempt to attach UBI partitions in the event that our > call to libuboot_read_config fails with a -EINVAL return without > performing potentially problematic erase operations prior to running > a swu update. I understand the issue, but my concern is if this is the way to solve it. SWUpdate tries to figure out which can be the error in libubootenv, and then it tries to solve this re-running implicitly an ubiattach(). Shouldn't we fix this use case in libubootenv, instead ? The library interpretes the configuration, it knows if an UBI volume is available, and it can solve it. What do you think ? Best regards, Stefano > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > --- > bootloader/uboot.c | 23 +++++++++++++++++++---- > core/stream_interface.c | 2 +- > corelib/mtd-interface.c | 8 +++++--- > include/flash.h | 2 +- > 4 files changed, 26 insertions(+), 9 deletions(-) > > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > index a2f0794..3456132 100644 > --- a/bootloader/uboot.c > +++ b/bootloader/uboot.c > @@ -18,6 +18,9 @@ > #include "generated/autoconf.h" > #include "util.h" > #include "dlfcn.h" > +#ifdef CONFIG_UBIATTACH > +#include "flash.h" > +#endif > #include "bootloader.h" > > #include <libuboot.h> > @@ -39,17 +42,29 @@ static struct { > > static int bootloader_initialize(struct uboot_ctx **ctx) > { > - if (libuboot.initialize(ctx, NULL) < 0) { > + int ret; > + > + ret = libuboot.initialize(ctx, NULL); > + if (ret < 0) { > ERROR("Error: environment not initialized"); > return -ENODEV; > } > - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { > + ret = libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV); > +#ifdef CONFIG_UBIATTACH > + if (ret == -EINVAL) { > + scan_mtd_devices(false); > + ret = libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV); > + } > +#endif > + if (ret < 0) { > ERROR("Configuration file %s wrong or corrupted", CONFIG_UBOOT_FWENV); > return -EINVAL; > } > - if (libuboot.open(*ctx) < 0) { > + ret = libuboot.open(*ctx); > + if (ret < 0) { > WARN("Cannot read environment, using default"); > - if (libuboot.load_file(*ctx, CONFIG_UBOOT_DEFAULTENV) < 0) { > + ret = libuboot.load_file(*ctx, CONFIG_UBOOT_DEFAULTENV); > + if (ret < 0) { > ERROR("Error: Cannot read default environment from file"); > return -ENODATA; > } > diff --git a/core/stream_interface.c b/core/stream_interface.c > index 3f0b952..823ed54 100644 > --- a/core/stream_interface.c > +++ b/core/stream_interface.c > @@ -604,7 +604,7 @@ void *network_initializer(void *data) > if (!ret) { > #ifdef CONFIG_MTD > mtd_cleanup(); > - scan_mtd_devices(); > + scan_mtd_devices(true); > #endif > /* > * extract the meta data and relevant parts > diff --git a/corelib/mtd-interface.c b/corelib/mtd-interface.c > index 22e86bd..a394de1 100644 > --- a/corelib/mtd-interface.c > +++ b/corelib/mtd-interface.c > @@ -328,7 +328,7 @@ static void scan_for_ubi_devices(void) > } > > #if defined(CONFIG_UBIATTACH) > -static void scan_ubi_partitions(int mtd) > +static void scan_ubi_partitions(int mtd, bool erase) > { > struct flash_description *flash = get_flash_info(); > libubi_t libubi = flash->libubi; > @@ -365,6 +365,8 @@ static void scan_ubi_partitions(int mtd) > do { > err = ubi_attach(libubi, DEFAULT_CTRL_DEV, &mtd_info->req); > if (err) { > + if (!erase) > + return; > if (mtd_info->has_ubi && !tryattach) { > TRACE("cannot attach mtd%d ..try erasing", mtd); > if (flash_erase(mtd)) { > @@ -390,7 +392,7 @@ static void scan_ubi_partitions(int mtd) > #endif > #endif > > -int scan_mtd_devices (void) > +int scan_mtd_devices (bool erase) > { > int err; > struct flash_description *flash = get_flash_info(); > @@ -493,7 +495,7 @@ int scan_mtd_devices (void) > if (flash->libubi && !flash->mtd_info[i].skipubi && > !flash->mtd_info[i].scanned && > flash->mtd_info[i].mtd.type != MTD_UBIVOLUME) > - scan_ubi_partitions(i); > + scan_ubi_partitions(i, erase); > } > #endif > #endif > diff --git a/include/flash.h b/include/flash.h > index 3f73079..e549770 100644 > --- a/include/flash.h > +++ b/include/flash.h > @@ -47,7 +47,7 @@ void ubi_umount(const char *mntpoint); > void mtd_init(void); > void mtd_set_ubiblacklist(char *mtdlist); > void ubi_init(void); > -int scan_mtd_devices (void); > +int scan_mtd_devices (bool erase); > void mtd_cleanup (void); > int get_mtd_from_device(char *s); > int get_mtd_from_name(const char *s);
On Wed, Sep 20, 2023 at 1:31 PM Stefano Babic <stefano.babic@swupdate.org> wrote: > > Hi James, > > On 20.09.23 17:12, James Hilliard wrote: > > When using uboot env in UBI in combination with CONFIG_UBIATTACH we > > may fail to interface with the environment as scan_mtd_devices would > > not be run until actually running an update. > > > > To handle this case safely add an erase flag to scan_mtd_devices so > > that we can attempt to attach UBI partitions in the event that our > > call to libuboot_read_config fails with a -EINVAL return without > > performing potentially problematic erase operations prior to running > > a swu update. > > I understand the issue, but my concern is if this is the way to solve > it. SWUpdate tries to figure out which can be the error in libubootenv, > and then it tries to solve this re-running implicitly an ubiattach(). > > Shouldn't we fix this use case in libubootenv, instead ? The library > interpretes the configuration, it knows if an UBI volume is available, > and it can solve it. What do you think ? Well it doesn't really know if the volume is available because the format in the uboot configuration doesn't specify which mtd partition to attach to find the UBI volume. > > Best regards, > Stefano > > > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com> > > --- > > bootloader/uboot.c | 23 +++++++++++++++++++---- > > core/stream_interface.c | 2 +- > > corelib/mtd-interface.c | 8 +++++--- > > include/flash.h | 2 +- > > 4 files changed, 26 insertions(+), 9 deletions(-) > > > > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > > index a2f0794..3456132 100644 > > --- a/bootloader/uboot.c > > +++ b/bootloader/uboot.c > > @@ -18,6 +18,9 @@ > > #include "generated/autoconf.h" > > #include "util.h" > > #include "dlfcn.h" > > +#ifdef CONFIG_UBIATTACH > > +#include "flash.h" > > +#endif > > #include "bootloader.h" > > > > #include <libuboot.h> > > @@ -39,17 +42,29 @@ static struct { > > > > static int bootloader_initialize(struct uboot_ctx **ctx) > > { > > - if (libuboot.initialize(ctx, NULL) < 0) { > > + int ret; > > + > > + ret = libuboot.initialize(ctx, NULL); > > + if (ret < 0) { > > ERROR("Error: environment not initialized"); > > return -ENODEV; > > } > > - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { > > + ret = libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV); > > +#ifdef CONFIG_UBIATTACH > > + if (ret == -EINVAL) { > > + scan_mtd_devices(false); > > + ret = libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV); > > + } > > +#endif > > + if (ret < 0) { > > ERROR("Configuration file %s wrong or corrupted", CONFIG_UBOOT_FWENV); > > return -EINVAL; > > } > > - if (libuboot.open(*ctx) < 0) { > > + ret = libuboot.open(*ctx); > > + if (ret < 0) { > > WARN("Cannot read environment, using default"); > > - if (libuboot.load_file(*ctx, CONFIG_UBOOT_DEFAULTENV) < 0) { > > + ret = libuboot.load_file(*ctx, CONFIG_UBOOT_DEFAULTENV); > > + if (ret < 0) { > > ERROR("Error: Cannot read default environment from file"); > > return -ENODATA; > > } > > diff --git a/core/stream_interface.c b/core/stream_interface.c > > index 3f0b952..823ed54 100644 > > --- a/core/stream_interface.c > > +++ b/core/stream_interface.c > > @@ -604,7 +604,7 @@ void *network_initializer(void *data) > > if (!ret) { > > #ifdef CONFIG_MTD > > mtd_cleanup(); > > - scan_mtd_devices(); > > + scan_mtd_devices(true); > > #endif > > /* > > * extract the meta data and relevant parts > > diff --git a/corelib/mtd-interface.c b/corelib/mtd-interface.c > > index 22e86bd..a394de1 100644 > > --- a/corelib/mtd-interface.c > > +++ b/corelib/mtd-interface.c > > @@ -328,7 +328,7 @@ static void scan_for_ubi_devices(void) > > } > > > > #if defined(CONFIG_UBIATTACH) > > -static void scan_ubi_partitions(int mtd) > > +static void scan_ubi_partitions(int mtd, bool erase) > > { > > struct flash_description *flash = get_flash_info(); > > libubi_t libubi = flash->libubi; > > @@ -365,6 +365,8 @@ static void scan_ubi_partitions(int mtd) > > do { > > err = ubi_attach(libubi, DEFAULT_CTRL_DEV, &mtd_info->req); > > if (err) { > > + if (!erase) > > + return; > > if (mtd_info->has_ubi && !tryattach) { > > TRACE("cannot attach mtd%d ..try erasing", mtd); > > if (flash_erase(mtd)) { > > @@ -390,7 +392,7 @@ static void scan_ubi_partitions(int mtd) > > #endif > > #endif > > > > -int scan_mtd_devices (void) > > +int scan_mtd_devices (bool erase) > > { > > int err; > > struct flash_description *flash = get_flash_info(); > > @@ -493,7 +495,7 @@ int scan_mtd_devices (void) > > if (flash->libubi && !flash->mtd_info[i].skipubi && > > !flash->mtd_info[i].scanned && > > flash->mtd_info[i].mtd.type != MTD_UBIVOLUME) > > - scan_ubi_partitions(i); > > + scan_ubi_partitions(i, erase); > > } > > #endif > > #endif > > diff --git a/include/flash.h b/include/flash.h > > index 3f73079..e549770 100644 > > --- a/include/flash.h > > +++ b/include/flash.h > > @@ -47,7 +47,7 @@ void ubi_umount(const char *mntpoint); > > void mtd_init(void); > > void mtd_set_ubiblacklist(char *mtdlist); > > void ubi_init(void); > > -int scan_mtd_devices (void); > > +int scan_mtd_devices (bool erase); > > void mtd_cleanup (void); > > int get_mtd_from_device(char *s); > > int get_mtd_from_name(const char *s);
Hi James, On 20.09.23 21:57, James Hilliard wrote: > On Wed, Sep 20, 2023 at 1:31 PM Stefano Babic > <stefano.babic@swupdate.org> wrote: >> >> Hi James, >> >> On 20.09.23 17:12, James Hilliard wrote: >>> When using uboot env in UBI in combination with CONFIG_UBIATTACH we >>> may fail to interface with the environment as scan_mtd_devices would >>> not be run until actually running an update. >>> >>> To handle this case safely add an erase flag to scan_mtd_devices so >>> that we can attempt to attach UBI partitions in the event that our >>> call to libuboot_read_config fails with a -EINVAL return without >>> performing potentially problematic erase operations prior to running >>> a swu update. >> >> I understand the issue, but my concern is if this is the way to solve >> it. SWUpdate tries to figure out which can be the error in libubootenv, >> and then it tries to solve this re-running implicitly an ubiattach(). >> >> Shouldn't we fix this use case in libubootenv, instead ? The library >> interpretes the configuration, it knows if an UBI volume is available, >> and it can solve it. What do you think ? > > Well it doesn't really know if the volume is available because the format > in the uboot configuration doesn't specify which mtd partition to attach > to find the UBI volume. The original uboot configuration is pretty scarce and does not cover the case you have. In fact, u-boot's tool requires that the MTD is already attached, and just allow to use names for volume instead of hardcoded volume index, but nothing more. That means, this requires that you attach the device at boot before starting any access to the env. This has always been with old tools, But libubootenv allows a more flexible configuration file (yaml). This allows to add features and improve syntax: https://github.com/sbabic/libubootenv/blob/master/docs/fw_env_config.md and we can add the link between MTD device and volume name. Best regards, Stefano > >> >> Best regards, >> Stefano >> >>> >>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com> >>> --- >>> bootloader/uboot.c | 23 +++++++++++++++++++---- >>> core/stream_interface.c | 2 +- >>> corelib/mtd-interface.c | 8 +++++--- >>> include/flash.h | 2 +- >>> 4 files changed, 26 insertions(+), 9 deletions(-) >>> >>> diff --git a/bootloader/uboot.c b/bootloader/uboot.c >>> index a2f0794..3456132 100644 >>> --- a/bootloader/uboot.c >>> +++ b/bootloader/uboot.c >>> @@ -18,6 +18,9 @@ >>> #include "generated/autoconf.h" >>> #include "util.h" >>> #include "dlfcn.h" >>> +#ifdef CONFIG_UBIATTACH >>> +#include "flash.h" >>> +#endif >>> #include "bootloader.h" >>> >>> #include <libuboot.h> >>> @@ -39,17 +42,29 @@ static struct { >>> >>> static int bootloader_initialize(struct uboot_ctx **ctx) >>> { >>> - if (libuboot.initialize(ctx, NULL) < 0) { >>> + int ret; >>> + >>> + ret = libuboot.initialize(ctx, NULL); >>> + if (ret < 0) { >>> ERROR("Error: environment not initialized"); >>> return -ENODEV; >>> } >>> - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { >>> + ret = libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV); >>> +#ifdef CONFIG_UBIATTACH >>> + if (ret == -EINVAL) { >>> + scan_mtd_devices(false); >>> + ret = libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV); >>> + } >>> +#endif >>> + if (ret < 0) { >>> ERROR("Configuration file %s wrong or corrupted", CONFIG_UBOOT_FWENV); >>> return -EINVAL; >>> } >>> - if (libuboot.open(*ctx) < 0) { >>> + ret = libuboot.open(*ctx); >>> + if (ret < 0) { >>> WARN("Cannot read environment, using default"); >>> - if (libuboot.load_file(*ctx, CONFIG_UBOOT_DEFAULTENV) < 0) { >>> + ret = libuboot.load_file(*ctx, CONFIG_UBOOT_DEFAULTENV); >>> + if (ret < 0) { >>> ERROR("Error: Cannot read default environment from file"); >>> return -ENODATA; >>> } >>> diff --git a/core/stream_interface.c b/core/stream_interface.c >>> index 3f0b952..823ed54 100644 >>> --- a/core/stream_interface.c >>> +++ b/core/stream_interface.c >>> @@ -604,7 +604,7 @@ void *network_initializer(void *data) >>> if (!ret) { >>> #ifdef CONFIG_MTD >>> mtd_cleanup(); >>> - scan_mtd_devices(); >>> + scan_mtd_devices(true); >>> #endif >>> /* >>> * extract the meta data and relevant parts >>> diff --git a/corelib/mtd-interface.c b/corelib/mtd-interface.c >>> index 22e86bd..a394de1 100644 >>> --- a/corelib/mtd-interface.c >>> +++ b/corelib/mtd-interface.c >>> @@ -328,7 +328,7 @@ static void scan_for_ubi_devices(void) >>> } >>> >>> #if defined(CONFIG_UBIATTACH) >>> -static void scan_ubi_partitions(int mtd) >>> +static void scan_ubi_partitions(int mtd, bool erase) >>> { >>> struct flash_description *flash = get_flash_info(); >>> libubi_t libubi = flash->libubi; >>> @@ -365,6 +365,8 @@ static void scan_ubi_partitions(int mtd) >>> do { >>> err = ubi_attach(libubi, DEFAULT_CTRL_DEV, &mtd_info->req); >>> if (err) { >>> + if (!erase) >>> + return; >>> if (mtd_info->has_ubi && !tryattach) { >>> TRACE("cannot attach mtd%d ..try erasing", mtd); >>> if (flash_erase(mtd)) { >>> @@ -390,7 +392,7 @@ static void scan_ubi_partitions(int mtd) >>> #endif >>> #endif >>> >>> -int scan_mtd_devices (void) >>> +int scan_mtd_devices (bool erase) >>> { >>> int err; >>> struct flash_description *flash = get_flash_info(); >>> @@ -493,7 +495,7 @@ int scan_mtd_devices (void) >>> if (flash->libubi && !flash->mtd_info[i].skipubi && >>> !flash->mtd_info[i].scanned && >>> flash->mtd_info[i].mtd.type != MTD_UBIVOLUME) >>> - scan_ubi_partitions(i); >>> + scan_ubi_partitions(i, erase); >>> } >>> #endif >>> #endif >>> diff --git a/include/flash.h b/include/flash.h >>> index 3f73079..e549770 100644 >>> --- a/include/flash.h >>> +++ b/include/flash.h >>> @@ -47,7 +47,7 @@ void ubi_umount(const char *mntpoint); >>> void mtd_init(void); >>> void mtd_set_ubiblacklist(char *mtdlist); >>> void ubi_init(void); >>> -int scan_mtd_devices (void); >>> +int scan_mtd_devices (bool erase); >>> void mtd_cleanup (void); >>> int get_mtd_from_device(char *s); >>> int get_mtd_from_name(const char *s); >
diff --git a/bootloader/uboot.c b/bootloader/uboot.c index a2f0794..3456132 100644 --- a/bootloader/uboot.c +++ b/bootloader/uboot.c @@ -18,6 +18,9 @@ #include "generated/autoconf.h" #include "util.h" #include "dlfcn.h" +#ifdef CONFIG_UBIATTACH +#include "flash.h" +#endif #include "bootloader.h" #include <libuboot.h> @@ -39,17 +42,29 @@ static struct { static int bootloader_initialize(struct uboot_ctx **ctx) { - if (libuboot.initialize(ctx, NULL) < 0) { + int ret; + + ret = libuboot.initialize(ctx, NULL); + if (ret < 0) { ERROR("Error: environment not initialized"); return -ENODEV; } - if (libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV) < 0) { + ret = libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV); +#ifdef CONFIG_UBIATTACH + if (ret == -EINVAL) { + scan_mtd_devices(false); + ret = libuboot.read_config(*ctx, CONFIG_UBOOT_FWENV); + } +#endif + if (ret < 0) { ERROR("Configuration file %s wrong or corrupted", CONFIG_UBOOT_FWENV); return -EINVAL; } - if (libuboot.open(*ctx) < 0) { + ret = libuboot.open(*ctx); + if (ret < 0) { WARN("Cannot read environment, using default"); - if (libuboot.load_file(*ctx, CONFIG_UBOOT_DEFAULTENV) < 0) { + ret = libuboot.load_file(*ctx, CONFIG_UBOOT_DEFAULTENV); + if (ret < 0) { ERROR("Error: Cannot read default environment from file"); return -ENODATA; } diff --git a/core/stream_interface.c b/core/stream_interface.c index 3f0b952..823ed54 100644 --- a/core/stream_interface.c +++ b/core/stream_interface.c @@ -604,7 +604,7 @@ void *network_initializer(void *data) if (!ret) { #ifdef CONFIG_MTD mtd_cleanup(); - scan_mtd_devices(); + scan_mtd_devices(true); #endif /* * extract the meta data and relevant parts diff --git a/corelib/mtd-interface.c b/corelib/mtd-interface.c index 22e86bd..a394de1 100644 --- a/corelib/mtd-interface.c +++ b/corelib/mtd-interface.c @@ -328,7 +328,7 @@ static void scan_for_ubi_devices(void) } #if defined(CONFIG_UBIATTACH) -static void scan_ubi_partitions(int mtd) +static void scan_ubi_partitions(int mtd, bool erase) { struct flash_description *flash = get_flash_info(); libubi_t libubi = flash->libubi; @@ -365,6 +365,8 @@ static void scan_ubi_partitions(int mtd) do { err = ubi_attach(libubi, DEFAULT_CTRL_DEV, &mtd_info->req); if (err) { + if (!erase) + return; if (mtd_info->has_ubi && !tryattach) { TRACE("cannot attach mtd%d ..try erasing", mtd); if (flash_erase(mtd)) { @@ -390,7 +392,7 @@ static void scan_ubi_partitions(int mtd) #endif #endif -int scan_mtd_devices (void) +int scan_mtd_devices (bool erase) { int err; struct flash_description *flash = get_flash_info(); @@ -493,7 +495,7 @@ int scan_mtd_devices (void) if (flash->libubi && !flash->mtd_info[i].skipubi && !flash->mtd_info[i].scanned && flash->mtd_info[i].mtd.type != MTD_UBIVOLUME) - scan_ubi_partitions(i); + scan_ubi_partitions(i, erase); } #endif #endif diff --git a/include/flash.h b/include/flash.h index 3f73079..e549770 100644 --- a/include/flash.h +++ b/include/flash.h @@ -47,7 +47,7 @@ void ubi_umount(const char *mntpoint); void mtd_init(void); void mtd_set_ubiblacklist(char *mtdlist); void ubi_init(void); -int scan_mtd_devices (void); +int scan_mtd_devices (bool erase); void mtd_cleanup (void); int get_mtd_from_device(char *s); int get_mtd_from_name(const char *s);
When using uboot env in UBI in combination with CONFIG_UBIATTACH we may fail to interface with the environment as scan_mtd_devices would not be run until actually running an update. To handle this case safely add an erase flag to scan_mtd_devices so that we can attempt to attach UBI partitions in the event that our call to libuboot_read_config fails with a -EINVAL return without performing potentially problematic erase operations prior to running a swu update. Signed-off-by: James Hilliard <james.hilliard1@gmail.com> --- bootloader/uboot.c | 23 +++++++++++++++++++---- core/stream_interface.c | 2 +- corelib/mtd-interface.c | 8 +++++--- include/flash.h | 2 +- 4 files changed, 26 insertions(+), 9 deletions(-)