On Tue, 26 Apr 2022 15:25:56 +0100 Andre Przywara <andre.przyw...@arm.com> wrote:
Hi Samuel, > On Sat, 23 Apr 2022 16:01:00 -0500 > Samuel Holland <sam...@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 <macroalph...@gmail.com> > > > Signed-off-by: Andre Przywara <andre.przyw...@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 > >