[U-Boot] [PATCH] Crash in env_relocate_spec() of env_mmc.c

[Steve Sakoman]

On Fri, Oct 8, 2010 at 6:39 AM, Steve Sakoman <sakoman at gmail.com> wrote:
> On Fri, Oct 8, 2010 at 1:03 AM, Stefano Babic <sbabic at denx.de> wrote:
>> mmc_initialize is not called at the startup if the
>> relocation takes place and the environment is stored
>> into a MMC card.
>>
>> Signed-off-by: Stefano Babic <sbabic at denx.de>
>> ---
>>  arch/arm/lib/board.c |   10 +++++-----
>>  common/env_mmc.c     |   11 +++++------
>>  2 files changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
>> index 5f2dfd0..704ddcb 100644
>> --- a/arch/arm/lib/board.c
>> +++ b/arch/arm/lib/board.c
>> @@ -775,6 +775,11 @@ void board_init_r (gd_t *id, ulong dest_addr)
>>        nand_init();            /* go init the NAND */
>>  #endif
>>
>> +#ifdef CONFIG_GENERIC_MMC
>> +       puts ("MMC:   ");
>> +       mmc_initialize (bd);
>> +#endif
>> +
>>  #if defined(CONFIG_CMD_ONENAND)
>>        onenand_init();
>>  #endif
>> @@ -854,11 +859,6 @@ extern void davinci_eth_set_mac_addr (const u_int8_t *addr);
>>        board_late_init ();
>>  #endif
>>
>> -#ifdef CONFIG_GENERIC_MMC
>> -       puts ("MMC:   ");
>> -       mmc_initialize (gd->bd);
>> -#endif
>> -
>>  #ifdef CONFIG_BITBANGMII
>>        bb_miiphy_init();
>>  #endif
>
> I think it might make more sense to put the MMC ifdef after the
> onenand code so that it doesn't split the nand/onenand grouping.
> Otherwise it matches what I did.
>
>> diff --git a/common/env_mmc.c b/common/env_mmc.c
>> index 14203b6..cb7c448 100644
>> --- a/common/env_mmc.c
>> +++ b/common/env_mmc.c
>> @@ -130,24 +130,23 @@ void env_relocate_spec(void)
>>  {
>>  #if !defined(ENV_IS_EMBEDDED)
>>        struct mmc *mmc = find_mmc_device(CONFIG_SYS_MMC_ENV_DEV);
>> +       char buf[CONFIG_ENV_SIZE];
>>
>>        if (init_mmc_for_env(mmc))
>>                return;
>>
>> -       if (read_env(mmc, CONFIG_ENV_SIZE, CONFIG_ENV_OFFSET, env_ptr))
>> -               return use_default();
>> -
>> -       if (crc32(0, env_ptr->data, ENV_SIZE) != env_ptr->crc)
>> +       if (read_env(mmc, CONFIG_ENV_SIZE, CONFIG_ENV_OFFSET, buf))
>>                return use_default();
>
> This is a void function and shouldn't have a return value.  I fixed
> this in my version.
>
>>
>>        gd->env_valid = 1;
>
> I removed this in my version, probably an error to do that though :-)
>
>> +
>> +       env_import(buf, 1);
>>  #endif
>>  }
>>
>>  #if !defined(ENV_IS_EMBEDDED)
>>  static void use_default()
>>  {
>> -       puts ("*** Warning - bad CRC or MMC, using default environment\n\n");
>> -       set_default_env();
>> +       set_default_env("*** Warning - bad CRC or MMC, using default environment\n\n");
>
> I previously submitted a patch to fix this and Wolfgang sent an email
> saying that it had been applied.
>
>>  }
>>  #endif
>> --
>> 1.6.3.3
>>
>>
>
> I'll revise my patch to add the missing gd->env_valid = 1; and re-test.

Hmm . . . looking at env_nand.c I see that it doesn't do gd->env_valid
= 1 either, which is probably why I removed it last night.

Any idea whether it is actually required?  Seems to work fine without it!

Steve