[PATCH] sunxi: fix initial environment loading without MMC

[Samuel Holland]

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 at gmail.com>
>>>> Signed-off-by: Andre Przywara <andre.przywara at 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  
>>
>>
>