[U-Boot] [PATCH 2/2] env: Add back default action of get_char in env_get_char()

[Simon Goldschmidt]

On 08.02.2018 09:47, Maxime Ripard wrote:
> On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote:
>> Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env
>> lookup function") dropped the default action if driver doesn't have
>> get_char() defined. This causes failure to get environmental
>> variables from NOR flash. Add back this default action for now.
>>
>> Signed-off-by: York Sun <york.sun at nxp.com>
>> CC: Maxime Ripard <maxime.ripard at free-electrons.com>
>> ---
>> Limited test on LS1043ARDB.
>>
>>   env/env.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/env/env.c b/env/env.c
>> index edfb575..210bae2 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -159,7 +159,7 @@ int env_get_char(int index)
>>   		int ret;
>>   
>>   		if (!drv->get_char)
>> -			continue;
>> +			return *(uchar *)(gd->env_addr + index);
> Thinking more about this, I think this would break the case where the
> first environment in your list has no get_char method, but the second
> might.


How can we decide which way is wanted? With your patch below, we might 
end up loading chars from a low-prio environment (which is not CRC 
checked) in the early boot stage. Later, we load the environment from 
another env driver with higher priority.

That's why I suggested removing the 'get_char' callback from the env 
drivers :-) Early boot stage environment lookups would still work the 
old way reading from 'gd->env_addr + index'.

Simon


> Would something like that work?
>
> ------------ 8< --------
> diff --git a/env/env.c b/env/env.c
> index 9a89832c1aaf..391c9c0df2f0 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -152,6 +152,7 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio)
>   int env_get_char(int index)
>   {
>          struct env_driver *drv;
> +       bool has_get_char = false;
>          int prio;
>   
>          if (gd->env_valid == ENV_INVALID)
> @@ -166,6 +167,7 @@ int env_get_char(int index)
>                  if (!env_has_inited(drv->location))
>                          continue;
>   
> +               has_get_char = true;
>                  ret = drv->get_char(index);
>                  if (!ret)
>                          return 0;
> @@ -174,6 +176,9 @@ int env_get_char(int index)
>                        drv->name, ret);
>          }
>   
> +       if (!has_get_char)
> +               return *(uchar *)(gd->env_addr + index);
> +
>          return -ENODEV;
>   }
> ------ 8< -------
>
> Thanks!
> Maxime
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot