| Message ID | 1555496902-10030-1-git-send-email-mark.jonas@de.bosch.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | bootloader: Return error in case of no valid environment | expand |
On 17/04/19 12:28, Mark Jonas wrote: > From: Leo Ruan <tingquan.ruan@cn.bosch.com> > > The swupdate loads environment variables from a persistent storage > (e.g. SPI flash) at first. If there is no environment in the persistent > storage, swupdate try to read environment from a specified file which > contains an initial environment of U-Boot. > If no valid environment can be found from the persistent storage and > given file, swupdate continue to work on an empty environment and store > it to the persistent storage. This leads problem on booting. > > This commit fixes the problem by making swupdate return error in case > of no valid environment in persistent storage and given file. > > Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com> > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> > --- > bootloader/uboot.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > index e57b8bd..2822763 100644 > --- a/bootloader/uboot.c > +++ b/bootloader/uboot.c > @@ -141,8 +141,11 @@ static int bootloader_initialize(struct uboot_ctx **ctx) > return -EINVAL; > } > if (libuboot_open(*ctx) < 0) { > - ERROR("Cannot read environment, using default\n"); > - libuboot_load_file(*ctx, CONFIG_UBOOT_DEFAULTENV); > + WARN("Cannot read environment, using default"); > + if (libuboot_load_file(*ctx, CONFIG_UBOOT_DEFAULTENV) < 0) { > + ERROR("Error: Cannot read default environment from file"); > + return -ENODATA; > + } > } > > return 0; > This patch is wrong - it is based on something else. This uses SWUpdate's macro for ERROR and WARN, but they are not available in libubootenv. And be careful: using them makes libubootenv GPLv2 instead of LGPLv2.1. I will apply the previous patch and reject this one. Best regards, Stefano Babic
Hi Stefano, > > The swupdate loads environment variables from a persistent storage > > (e.g. SPI flash) at first. If there is no environment in the persistent > > storage, swupdate try to read environment from a specified file which > > contains an initial environment of U-Boot. > > If no valid environment can be found from the persistent storage and > > given file, swupdate continue to work on an empty environment and store > > it to the persistent storage. This leads problem on booting. > > > > This commit fixes the problem by making swupdate return error in case > > of no valid environment in persistent storage and given file. > > > > Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com> > > Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> > > --- > > bootloader/uboot.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/bootloader/uboot.c b/bootloader/uboot.c > > index e57b8bd..2822763 100644 > > --- a/bootloader/uboot.c > > +++ b/bootloader/uboot.c > > @@ -141,8 +141,11 @@ static int bootloader_initialize(struct uboot_ctx **ctx) > > return -EINVAL; > > } > > if (libuboot_open(*ctx) < 0) { > > - ERROR("Cannot read environment, using default\n"); > > - libuboot_load_file(*ctx, CONFIG_UBOOT_DEFAULTENV); > > + WARN("Cannot read environment, using default"); > > + if (libuboot_load_file(*ctx, CONFIG_UBOOT_DEFAULTENV) < 0) { > > + ERROR("Error: Cannot read default environment from file"); > > + return -ENODATA; > > + } > > } > > > > return 0; > > > > This patch is wrong - it is based on something else. This uses > SWUpdate's macro for ERROR and WARN, but they are not available in > libubootenv. And be careful: using them makes libubootenv GPLv2 instead > of LGPLv2.1. > > I will apply the previous patch and reject this one. I have to admit that it is confusing: We patched the same (wrong!?) behavior in libubootenv (previous patch) as well as in swupdate itself (this patch). That is, this patch here is for swupdate itself. The intention is to fail the update if there is no U-Boot environment in the MTD devices AND the uboot-default-environment file cannot be loaded, too. Without the patch swupdate will continue and U-Boot will end up with a (mostly) empty environment. This would most likely brick the device. So we think it is safer to fail the update and keep the device running from the default environment compiled into U-Boot. Greetings, Mark
Hi Mark, On 19/04/19 13:21, Mark Jonas wrote: > Hi Stefano, > >>> The swupdate loads environment variables from a persistent storage >>> (e.g. SPI flash) at first. If there is no environment in the persistent >>> storage, swupdate try to read environment from a specified file which >>> contains an initial environment of U-Boot. >>> If no valid environment can be found from the persistent storage and >>> given file, swupdate continue to work on an empty environment and store >>> it to the persistent storage. This leads problem on booting. >>> >>> This commit fixes the problem by making swupdate return error in case >>> of no valid environment in persistent storage and given file. >>> >>> Signed-off-by: Leo Ruan <tingquan.ruan@cn.bosch.com> >>> Signed-off-by: Mark Jonas <mark.jonas@de.bosch.com> >>> --- >>> bootloader/uboot.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/bootloader/uboot.c b/bootloader/uboot.c >>> index e57b8bd..2822763 100644 >>> --- a/bootloader/uboot.c >>> +++ b/bootloader/uboot.c >>> @@ -141,8 +141,11 @@ static int bootloader_initialize(struct uboot_ctx **ctx) >>> return -EINVAL; >>> } >>> if (libuboot_open(*ctx) < 0) { >>> - ERROR("Cannot read environment, using default\n"); >>> - libuboot_load_file(*ctx, CONFIG_UBOOT_DEFAULTENV); >>> + WARN("Cannot read environment, using default"); >>> + if (libuboot_load_file(*ctx, CONFIG_UBOOT_DEFAULTENV) < 0) { >>> + ERROR("Error: Cannot read default environment from file"); >>> + return -ENODATA; >>> + } >>> } >>> >>> return 0; >>> >> >> This patch is wrong - it is based on something else. This uses >> SWUpdate's macro for ERROR and WARN, but they are not available in >> libubootenv. And be careful: using them makes libubootenv GPLv2 instead >> of LGPLv2.1. >> >> I will apply the previous patch and reject this one. > > I have to admit that it is confusing: We patched the same (wrong!?) > behavior in libubootenv (previous patch) as well as in swupdate itself > (this patch). > > That is, this patch here is for swupdate itself. The intention is to > fail the update if there is no U-Boot environment in the MTD devices > AND the uboot-default-environment file cannot be loaded, too. Without > the patch swupdate will continue and U-Boot will end up with a > (mostly) empty environment. This would most likely brick the device. > So we think it is safer to fail the update and keep the device running > from the default environment compiled into U-Boot. Ok - got it. Agree, this is safer, I will merge it. Regards, Stefano
diff --git a/bootloader/uboot.c b/bootloader/uboot.c index e57b8bd..2822763 100644 --- a/bootloader/uboot.c +++ b/bootloader/uboot.c @@ -141,8 +141,11 @@ static int bootloader_initialize(struct uboot_ctx **ctx) return -EINVAL; } if (libuboot_open(*ctx) < 0) { - ERROR("Cannot read environment, using default\n"); - libuboot_load_file(*ctx, CONFIG_UBOOT_DEFAULTENV); + WARN("Cannot read environment, using default"); + if (libuboot_load_file(*ctx, CONFIG_UBOOT_DEFAULTENV) < 0) { + ERROR("Error: Cannot read default environment from file"); + return -ENODATA; + } } return 0;