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 ;-)
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
