Re: [PATCH] sunxi: fix initial environment loading without MMC

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4325 bytes --]

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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]