| Message ID | 20220421003448.4517-1-andre.przywara@arm.com |
|---|---|
| State | Superseded |
| Delegated to: | Andre Przywara |
| Headers | show |
| Series | sunxi: fix initial environment loading without MMC | expand |
Hi Andre, On 4/20/22 7:34 PM, Andre Przywara wrote: > Commit e42dad4168fe ("sunxi: use boot source for determining environment > location") changed our implementation of env_get_location() and enabled > it for every board, even those without MMC support (like the C.H.I.P. > boards). However the default fallback location of ENVL_FAT does not cope > very well without MMC support compiled in, so the board hangs when trying > to initially load the environment. > > Change the default fallback location to be ENVL_FAT only when the FAT > environment support is enabled, and use ENVL_NOWHERE and ENVL_UBI as > alternative fallbacks, when those sources are enabled. > > This fixes U-Boot loading on the C.H.I.P. boards. > > Fixes: e42dad4168fe ("sunxi: use boot source for determining environment location") > Reported-by: Chris Morgan <macroalpha82@gmail.com> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > board/sunxi/board.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > index 89324159d55..befb6076ca6 100644 > --- a/board/sunxi/board.c > +++ b/board/sunxi/board.c > @@ -132,7 +132,14 @@ void i2c_init_board(void) > */ > enum env_location env_get_location(enum env_operation op, int prio) > { > - enum env_location boot_loc = ENVL_FAT; > + enum env_location boot_loc; > + > + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) > + boot_loc = ENVL_NOWHERE; > + else if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) > + boot_loc = ENVL_FAT; > + else if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) > + boot_loc = ENVL_UBI; This could leave boot_loc uninitialized. And there is still an unconditional use of ENVL_FAT in the BOOT_DEVICE_MMCx case. > gd->env_load_prio = prio; I don't think the hook is supposed to change this variable. I'm still a bit confused on the fallback logic you have in place. Splitting it up into three blocks doesn't help. If the goal is to load the environment from the boot device, while preferring filesystems over raw block devices, I propose the following: diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 427113534b..27508bd306 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -129,26 +129,38 @@ * Try to use the environment from the boot source first. * For MMC, this means a FAT partition on the boot device (SD or eMMC). * If the raw MMC environment is also enabled, this is tried next. - * SPI flash falls back to FAT (on SD card). */ enum env_location env_get_location(enum env_operation op, int prio) { - enum env_location boot_loc = ENVL_FAT; + if (prio > 1) + return ENVL_UNKNOWN; - gd->env_load_prio = prio; + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) + return ENVL_NOWHERE; switch (sunxi_get_boot_device()) { case BOOT_DEVICE_MMC1: case BOOT_DEVICE_MMC2: - boot_loc = ENVL_FAT; + if (prio == 0) { + if (IS_ENABLED(CONFIG_ENV_IS_IN_EXT4)) + return ENVL_EXT4; + if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) + return ENVL_FAT; + } + if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) + return ENVL_MMC; break; case BOOT_DEVICE_NAND: + if (prio == 0 && IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) + return ENVL_UBI; if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND)) - boot_loc = ENVL_NAND; + return ENVL_NAND; break; case BOOT_DEVICE_SPI: + if (prio == 0 && IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) + return ENVL_UBI; if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) - boot_loc = ENVL_SPI_FLASH; + return ENVL_SPI_FLASH; break; case BOOT_DEVICE_BOARD: break; @@ -156,23 +168,6 @@ break; } - /* Always try to access the environment on the boot device first. */ - if (prio == 0) - return boot_loc; - - if (prio == 1) { - switch (boot_loc) { - case ENVL_SPI_FLASH: - return ENVL_FAT; - case ENVL_FAT: - if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) - return ENVL_MMC; - break; - default: - break; - } - } - return ENVL_UNKNOWN; } Regards, Samuel
On Sat, 23 Apr 2022 16:01:00 -0500 Samuel Holland <samuel@sholland.org> wrote: Hi Samuel, thanks for having a look and the comments. > On 4/20/22 7:34 PM, Andre Przywara wrote: > > Commit e42dad4168fe ("sunxi: use boot source for determining environment > > location") changed our implementation of env_get_location() and enabled > > it for every board, even those without MMC support (like the C.H.I.P. > > boards). However the default fallback location of ENVL_FAT does not cope > > very well without MMC support compiled in, so the board hangs when trying > > to initially load the environment. > > > > Change the default fallback location to be ENVL_FAT only when the FAT > > environment support is enabled, and use ENVL_NOWHERE and ENVL_UBI as > > alternative fallbacks, when those sources are enabled. > > > > This fixes U-Boot loading on the C.H.I.P. boards. > > > > Fixes: e42dad4168fe ("sunxi: use boot source for determining environment location") > > Reported-by: Chris Morgan <macroalpha82@gmail.com> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > board/sunxi/board.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > > index 89324159d55..befb6076ca6 100644 > > --- a/board/sunxi/board.c > > +++ b/board/sunxi/board.c > > @@ -132,7 +132,14 @@ void i2c_init_board(void) > > */ > > enum env_location env_get_location(enum env_operation op, int prio) > > { > > - enum env_location boot_loc = ENVL_FAT; > > + enum env_location boot_loc; > > + > > + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) > > + boot_loc = ENVL_NOWHERE; > > + else if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) > > + boot_loc = ENVL_FAT; > > + else if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) > > + boot_loc = ENVL_UBI; > > This could leave boot_loc uninitialized. And there is still an unconditional use > of ENVL_FAT in the BOOT_DEVICE_MMCx case. Yeah, it's a mess, and there doesn't seem to be a one-fits-all fallback value. Returning ENVL_UNKNOWN when prio is 0 definitely hangs, as does returning some source without having the corresponding driver compiled in, so getting this right *and* nice-looking is a bit tricky. > > gd->env_load_prio = prio; > > I don't think the hook is supposed to change this variable. Yeah, don't remember why I initially put that in, I must have copied that from somewhere. All I remember is that this code is touchy (as the bug report leading to that patch shows), and there are quite some corner cases to test. > I'm still a bit confused on the fallback logic you have in place. Splitting it > up into three blocks doesn't help. If the goal is to load the environment from > the boot device, while preferring filesystems over raw block devices, I propose > the following: Admittedly this gets messier, I mainly wanted to fix the regression quickly, since it already broke the release for the CHIP boards. I will have a closer look at your suggestion and check for those corner cases, but will probably go with that instead of piling up more cruft on my previous code ;-) Thanks, Andre > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > index 427113534b..27508bd306 100644 > --- a/board/sunxi/board.c > +++ b/board/sunxi/board.c > @@ -129,26 +129,38 @@ > * Try to use the environment from the boot source first. > * For MMC, this means a FAT partition on the boot device (SD or eMMC). > * If the raw MMC environment is also enabled, this is tried next. > - * SPI flash falls back to FAT (on SD card). > */ > enum env_location env_get_location(enum env_operation op, int prio) > { > - enum env_location boot_loc = ENVL_FAT; > + if (prio > 1) > + return ENVL_UNKNOWN; > > - gd->env_load_prio = prio; > + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) > + return ENVL_NOWHERE; > > switch (sunxi_get_boot_device()) { > case BOOT_DEVICE_MMC1: > case BOOT_DEVICE_MMC2: > - boot_loc = ENVL_FAT; > + if (prio == 0) { > + if (IS_ENABLED(CONFIG_ENV_IS_IN_EXT4)) > + return ENVL_EXT4; > + if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) > + return ENVL_FAT; > + } > + if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) > + return ENVL_MMC; > break; > case BOOT_DEVICE_NAND: > + if (prio == 0 && IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) > + return ENVL_UBI; > if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND)) > - boot_loc = ENVL_NAND; > + return ENVL_NAND; > break; > case BOOT_DEVICE_SPI: > + if (prio == 0 && IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) > + return ENVL_UBI; > if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) > - boot_loc = ENVL_SPI_FLASH; > + return ENVL_SPI_FLASH; > break; > case BOOT_DEVICE_BOARD: > break; > @@ -156,23 +168,6 @@ > break; > } > > - /* Always try to access the environment on the boot device first. */ > - if (prio == 0) > - return boot_loc; > - > - if (prio == 1) { > - switch (boot_loc) { > - case ENVL_SPI_FLASH: > - return ENVL_FAT; > - case ENVL_FAT: > - if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) > - return ENVL_MMC; > - break; > - default: > - break; > - } > - } > - > return ENVL_UNKNOWN; > } > > > Regards, > Samuel
On Tue, 26 Apr 2022 15:25:56 +0100 Andre Przywara <andre.przywara@arm.com> wrote: Hi Samuel, > On Sat, 23 Apr 2022 16:01:00 -0500 > Samuel Holland <samuel@sholland.org> wrote: > > Hi Samuel, > > thanks for having a look and the comments. > > > On 4/20/22 7:34 PM, Andre Przywara wrote: > > > Commit e42dad4168fe ("sunxi: use boot source for determining environment > > > location") changed our implementation of env_get_location() and enabled > > > it for every board, even those without MMC support (like the C.H.I.P. > > > boards). However the default fallback location of ENVL_FAT does not cope > > > very well without MMC support compiled in, so the board hangs when trying > > > to initially load the environment. > > > > > > Change the default fallback location to be ENVL_FAT only when the FAT > > > environment support is enabled, and use ENVL_NOWHERE and ENVL_UBI as > > > alternative fallbacks, when those sources are enabled. > > > > > > This fixes U-Boot loading on the C.H.I.P. boards. > > > > > > Fixes: e42dad4168fe ("sunxi: use boot source for determining environment location") > > > Reported-by: Chris Morgan <macroalpha82@gmail.com> > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > --- > > > board/sunxi/board.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > > > index 89324159d55..befb6076ca6 100644 > > > --- a/board/sunxi/board.c > > > +++ b/board/sunxi/board.c > > > @@ -132,7 +132,14 @@ void i2c_init_board(void) > > > */ > > > enum env_location env_get_location(enum env_operation op, int prio) > > > { > > > - enum env_location boot_loc = ENVL_FAT; > > > + enum env_location boot_loc; > > > + > > > + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) > > > + boot_loc = ENVL_NOWHERE; > > > + else if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) > > > + boot_loc = ENVL_FAT; > > > + else if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) > > > + boot_loc = ENVL_UBI; > > > > This could leave boot_loc uninitialized. And there is still an unconditional use > > of ENVL_FAT in the BOOT_DEVICE_MMCx case. > > Yeah, it's a mess, and there doesn't seem to be a one-fits-all fallback > value. Returning ENVL_UNKNOWN when prio is 0 definitely hangs, as does > returning some source without having the corresponding driver compiled in, > so getting this right *and* nice-looking is a bit tricky. > > > > gd->env_load_prio = prio; > > > > I don't think the hook is supposed to change this variable. > > Yeah, don't remember why I initially put that in, I must have copied that > from somewhere. All I remember is that this code is touchy (as the bug > report leading to that patch shows), and there are quite some corner cases > to test. > > > I'm still a bit confused on the fallback logic you have in place. Splitting it > > up into three blocks doesn't help. If the goal is to load the environment from > > the boot device, while preferring filesystems over raw block devices, I propose > > the following: > > Admittedly this gets messier, I mainly wanted to fix the regression > quickly, since it already broke the release for the CHIP boards. > I will have a closer look at your suggestion and check for those corner > cases, but will probably go with that instead of piling up more cruft on > my previous code ;-) So I was about to submit your solution, but this is suffering from the same old problem: we must not return ENVL_UNKNOWN on the first call. FEL booting triggers this: it will lead to env_init() returning a negative error value, which is fatal, as it's part of init_sequence_f[]. I think the proper fix is to treat this case the same as the -ENOENT case at the end of env_init(), but I don't dare to do this generic change this late in the cycle. My plan is to hack-fix this for sunxi, for now (see below), then propose the generic change for the next cycle. Does this sound OK? Another solution is to always select ENV_IS_NOWHERE, then return this, but I don't know if this has more side effects, so is also risky at this point. > > > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > > index 427113534b..27508bd306 100644 > > --- a/board/sunxi/board.c > > +++ b/board/sunxi/board.c > > @@ -129,26 +129,38 @@ > > * Try to use the environment from the boot source first. > > * For MMC, this means a FAT partition on the boot device (SD or eMMC). > > * If the raw MMC environment is also enabled, this is tried next. > > - * SPI flash falls back to FAT (on SD card). > > */ > > enum env_location env_get_location(enum env_operation op, int prio) > > { > > - enum env_location boot_loc = ENVL_FAT; > > + if (prio > 1) > > + return ENVL_UNKNOWN; > > > > - gd->env_load_prio = prio; > > + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) > > + return ENVL_NOWHERE; > > > > switch (sunxi_get_boot_device()) { > > case BOOT_DEVICE_MMC1: > > case BOOT_DEVICE_MMC2: > > - boot_loc = ENVL_FAT; > > + if (prio == 0) { > > + if (IS_ENABLED(CONFIG_ENV_IS_IN_EXT4)) > > + return ENVL_EXT4; > > + if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) > > + return ENVL_FAT; > > + } > > + if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) > > + return ENVL_MMC; > > break; > > case BOOT_DEVICE_NAND: > > + if (prio == 0 && IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) > > + return ENVL_UBI; > > if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND)) > > - boot_loc = ENVL_NAND; > > + return ENVL_NAND; > > break; > > case BOOT_DEVICE_SPI: > > + if (prio == 0 && IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) > > + return ENVL_UBI; > > if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) > > - boot_loc = ENVL_SPI_FLASH; > > + return ENVL_SPI_FLASH; > > break; > > case BOOT_DEVICE_BOARD: > > break; > > @@ -156,23 +168,6 @@ > > break; > > } > > > > - /* Always try to access the environment on the boot device first. */ > > - if (prio == 0) > > - return boot_loc; > > - > > - if (prio == 1) { > > - switch (boot_loc) { > > - case ENVL_SPI_FLASH: > > - return ENVL_FAT; > > - case ENVL_FAT: > > - if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) > > - return ENVL_MMC; > > - break; > > - default: > > - break; > > - } > > - } > > - + /* + * If we come here for the first call, we *must* return a valid + * environment location other than ENVL_UNKNOWN, or the setup sequence + * in board_f() will silently hang. This is arguably a bug in + * env_init(), but for now pick one environment for which we know for + * sure to have a driver for. For all defconfigs this is either FAT + * or UBI, or NOWHERE, which is already handled above. + */ + if (prio == 0) { + if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) + return ENVL_FAT; + if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) + return ENVL_UBI; + } Cheers, Andre > > return ENVL_UNKNOWN; > > } > > > > > > Regards, > > Samuel > >
Hi Andre, >>> On 4/20/22 7:34 PM, Andre Przywara wrote: >>>> Commit e42dad4168fe ("sunxi: use boot source for determining environment >>>> location") changed our implementation of env_get_location() and enabled >>>> it for every board, even those without MMC support (like the C.H.I.P. >>>> boards). However the default fallback location of ENVL_FAT does not cope >>>> very well without MMC support compiled in, so the board hangs when trying >>>> to initially load the environment. >>>> >>>> Change the default fallback location to be ENVL_FAT only when the FAT >>>> environment support is enabled, and use ENVL_NOWHERE and ENVL_UBI as >>>> alternative fallbacks, when those sources are enabled. >>>> >>>> This fixes U-Boot loading on the C.H.I.P. boards. >>>> >>>> Fixes: e42dad4168fe ("sunxi: use boot source for determining environment location") >>>> Reported-by: Chris Morgan <macroalpha82@gmail.com> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> board/sunxi/board.c | 9 ++++++++- >>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c >>>> index 89324159d55..befb6076ca6 100644 >>>> --- a/board/sunxi/board.c >>>> +++ b/board/sunxi/board.c >>>> @@ -132,7 +132,14 @@ void i2c_init_board(void) >>>> */ >>>> enum env_location env_get_location(enum env_operation op, int prio) >>>> { >>>> - enum env_location boot_loc = ENVL_FAT; >>>> + enum env_location boot_loc; >>>> + >>>> + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) >>>> + boot_loc = ENVL_NOWHERE; >>>> + else if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) >>>> + boot_loc = ENVL_FAT; >>>> + else if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) >>>> + boot_loc = ENVL_UBI; >>> >>> This could leave boot_loc uninitialized. And there is still an unconditional use >>> of ENVL_FAT in the BOOT_DEVICE_MMCx case. >> >> Yeah, it's a mess, and there doesn't seem to be a one-fits-all fallback >> value. Returning ENVL_UNKNOWN when prio is 0 definitely hangs, as does >> returning some source without having the corresponding driver compiled in, >> so getting this right *and* nice-looking is a bit tricky. >> >>>> gd->env_load_prio = prio; >>> >>> I don't think the hook is supposed to change this variable. >> >> Yeah, don't remember why I initially put that in, I must have copied that >> from somewhere. All I remember is that this code is touchy (as the bug >> report leading to that patch shows), and there are quite some corner cases >> to test. >> >>> I'm still a bit confused on the fallback logic you have in place. Splitting it >>> up into three blocks doesn't help. If the goal is to load the environment from >>> the boot device, while preferring filesystems over raw block devices, I propose >>> the following: >> >> Admittedly this gets messier, I mainly wanted to fix the regression >> quickly, since it already broke the release for the CHIP boards. >> I will have a closer look at your suggestion and check for those corner >> cases, but will probably go with that instead of piling up more cruft on >> my previous code ;-) > > So I was about to submit your solution, but this is suffering from the > same old problem: we must not return ENVL_UNKNOWN on the first call. > FEL booting triggers this: it will lead to env_init() returning a > negative error value, which is fatal, as it's part of init_sequence_f[]. > I think the proper fix is to treat this case the same as the -ENOENT > case at the end of env_init(), but I don't dare to do this generic > change this late in the cycle. > My plan is to hack-fix this for sunxi, for now (see below), then > propose the generic change for the next cycle. Does this sound OK? > Another solution is to always select ENV_IS_NOWHERE, then return this, > but I don't know if this has more side effects, so is also risky at > this point. Yes, this sounds reasonable to me. Regards, Samuel >>> >>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c >>> index 427113534b..27508bd306 100644 >>> --- a/board/sunxi/board.c >>> +++ b/board/sunxi/board.c >>> @@ -129,26 +129,38 @@ >>> * Try to use the environment from the boot source first. >>> * For MMC, this means a FAT partition on the boot device (SD or eMMC). >>> * If the raw MMC environment is also enabled, this is tried next. >>> - * SPI flash falls back to FAT (on SD card). >>> */ >>> enum env_location env_get_location(enum env_operation op, int prio) >>> { >>> - enum env_location boot_loc = ENVL_FAT; >>> + if (prio > 1) >>> + return ENVL_UNKNOWN; >>> >>> - gd->env_load_prio = prio; >>> + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) >>> + return ENVL_NOWHERE; >>> >>> switch (sunxi_get_boot_device()) { >>> case BOOT_DEVICE_MMC1: >>> case BOOT_DEVICE_MMC2: >>> - boot_loc = ENVL_FAT; >>> + if (prio == 0) { >>> + if (IS_ENABLED(CONFIG_ENV_IS_IN_EXT4)) >>> + return ENVL_EXT4; >>> + if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) >>> + return ENVL_FAT; >>> + } >>> + if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) >>> + return ENVL_MMC; >>> break; >>> case BOOT_DEVICE_NAND: >>> + if (prio == 0 && IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) >>> + return ENVL_UBI; >>> if (IS_ENABLED(CONFIG_ENV_IS_IN_NAND)) >>> - boot_loc = ENVL_NAND; >>> + return ENVL_NAND; >>> break; >>> case BOOT_DEVICE_SPI: >>> + if (prio == 0 && IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) >>> + return ENVL_UBI; >>> if (IS_ENABLED(CONFIG_ENV_IS_IN_SPI_FLASH)) >>> - boot_loc = ENVL_SPI_FLASH; >>> + return ENVL_SPI_FLASH; >>> break; >>> case BOOT_DEVICE_BOARD: >>> break; >>> @@ -156,23 +168,6 @@ >>> break; >>> } >>> >>> - /* Always try to access the environment on the boot device first. */ >>> - if (prio == 0) >>> - return boot_loc; >>> - >>> - if (prio == 1) { >>> - switch (boot_loc) { >>> - case ENVL_SPI_FLASH: >>> - return ENVL_FAT; >>> - case ENVL_FAT: >>> - if (IS_ENABLED(CONFIG_ENV_IS_IN_MMC)) >>> - return ENVL_MMC; >>> - break; >>> - default: >>> - break; >>> - } >>> - } >>> - > > + /* > + * If we come here for the first call, we *must* return a valid > + * environment location other than ENVL_UNKNOWN, or the setup sequence > + * in board_f() will silently hang. This is arguably a bug in > + * env_init(), but for now pick one environment for which we know for > + * sure to have a driver for. For all defconfigs this is either FAT > + * or UBI, or NOWHERE, which is already handled above. > + */ > + if (prio == 0) { > + if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) > + return ENVL_FAT; > + if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) > + return ENVL_UBI; > + } > > Cheers, > Andre > >>> return ENVL_UNKNOWN; >>> } >>> >>> >>> Regards, >>> Samuel >> >> >
On Thu, Jun 23, 2022 at 08:21:01PM -0500, Samuel Holland wrote: > Hi Andre, > > >>> On 4/20/22 7:34 PM, Andre Przywara wrote: > >>>> Commit e42dad4168fe ("sunxi: use boot source for determining environment > >>>> location") changed our implementation of env_get_location() and enabled > >>>> it for every board, even those without MMC support (like the C.H.I.P. > >>>> boards). However the default fallback location of ENVL_FAT does not cope > >>>> very well without MMC support compiled in, so the board hangs when trying > >>>> to initially load the environment. > >>>> > >>>> Change the default fallback location to be ENVL_FAT only when the FAT > >>>> environment support is enabled, and use ENVL_NOWHERE and ENVL_UBI as > >>>> alternative fallbacks, when those sources are enabled. > >>>> > >>>> This fixes U-Boot loading on the C.H.I.P. boards. > >>>> > >>>> Fixes: e42dad4168fe ("sunxi: use boot source for determining environment location") > >>>> Reported-by: Chris Morgan <macroalpha82@gmail.com> > >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> > >>>> --- > >>>> board/sunxi/board.c | 9 ++++++++- > >>>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c > >>>> index 89324159d55..befb6076ca6 100644 > >>>> --- a/board/sunxi/board.c > >>>> +++ b/board/sunxi/board.c > >>>> @@ -132,7 +132,14 @@ void i2c_init_board(void) > >>>> */ > >>>> enum env_location env_get_location(enum env_operation op, int prio) > >>>> { > >>>> - enum env_location boot_loc = ENVL_FAT; > >>>> + enum env_location boot_loc; > >>>> + > >>>> + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) > >>>> + boot_loc = ENVL_NOWHERE; > >>>> + else if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) > >>>> + boot_loc = ENVL_FAT; > >>>> + else if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) > >>>> + boot_loc = ENVL_UBI; > >>> > >>> This could leave boot_loc uninitialized. And there is still an unconditional use > >>> of ENVL_FAT in the BOOT_DEVICE_MMCx case. > >> > >> Yeah, it's a mess, and there doesn't seem to be a one-fits-all fallback > >> value. Returning ENVL_UNKNOWN when prio is 0 definitely hangs, as does > >> returning some source without having the corresponding driver compiled in, > >> so getting this right *and* nice-looking is a bit tricky. > >> > >>>> gd->env_load_prio = prio; > >>> > >>> I don't think the hook is supposed to change this variable. > >> > >> Yeah, don't remember why I initially put that in, I must have copied that > >> from somewhere. All I remember is that this code is touchy (as the bug > >> report leading to that patch shows), and there are quite some corner cases > >> to test. > >> > >>> I'm still a bit confused on the fallback logic you have in place. Splitting it > >>> up into three blocks doesn't help. If the goal is to load the environment from > >>> the boot device, while preferring filesystems over raw block devices, I propose > >>> the following: > >> > >> Admittedly this gets messier, I mainly wanted to fix the regression > >> quickly, since it already broke the release for the CHIP boards. > >> I will have a closer look at your suggestion and check for those corner > >> cases, but will probably go with that instead of piling up more cruft on > >> my previous code ;-) > > > > So I was about to submit your solution, but this is suffering from the > > same old problem: we must not return ENVL_UNKNOWN on the first call. > > FEL booting triggers this: it will lead to env_init() returning a > > negative error value, which is fatal, as it's part of init_sequence_f[]. > > I think the proper fix is to treat this case the same as the -ENOENT > > case at the end of env_init(), but I don't dare to do this generic > > change this late in the cycle. > > My plan is to hack-fix this for sunxi, for now (see below), then > > propose the generic change for the next cycle. Does this sound OK? > > Another solution is to always select ENV_IS_NOWHERE, then return this, > > but I don't know if this has more side effects, so is also risky at > > this point. > > Yes, this sounds reasonable to me. Also reasonable sounding to me, and sorting out things properly post v2022.07 with some generic change / behavior fix.
diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 89324159d55..befb6076ca6 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -132,7 +132,14 @@ void i2c_init_board(void) */ enum env_location env_get_location(enum env_operation op, int prio) { - enum env_location boot_loc = ENVL_FAT; + enum env_location boot_loc; + + if (IS_ENABLED(CONFIG_ENV_IS_NOWHERE)) + boot_loc = ENVL_NOWHERE; + else if (IS_ENABLED(CONFIG_ENV_IS_IN_FAT)) + boot_loc = ENVL_FAT; + else if (IS_ENABLED(CONFIG_ENV_IS_IN_UBI)) + boot_loc = ENVL_UBI; gd->env_load_prio = prio;
Commit e42dad4168fe ("sunxi: use boot source for determining environment location") changed our implementation of env_get_location() and enabled it for every board, even those without MMC support (like the C.H.I.P. boards). However the default fallback location of ENVL_FAT does not cope very well without MMC support compiled in, so the board hangs when trying to initially load the environment. Change the default fallback location to be ENVL_FAT only when the FAT environment support is enabled, and use ENVL_NOWHERE and ENVL_UBI as alternative fallbacks, when those sources are enabled. This fixes U-Boot loading on the C.H.I.P. boards. Fixes: e42dad4168fe ("sunxi: use boot source for determining environment location") Reported-by: Chris Morgan <macroalpha82@gmail.com> Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- board/sunxi/board.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)