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

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

[Stefano Babic]

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@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
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();
 
 	gd->env_valid = 1;
+
+	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");
 }
 #endif
-- 
1.6.3.3

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

[Sergei Shtylyov]

Hello.

On 08-10-2010 12:03, Stefano Babic 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@denx.de>
[...]

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

    This wouldn't pass checkpatch.pl. Spaces before ( are not allowed.

WBR, Sergei

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

[Stefano Babic]

Sergei Shtylyov wrote:
> Hello.
> 

Hello Sergei,

>    This wouldn't pass checkpatch.pl. Spaces before ( are not allowed.

Agree, thanks. I will wait until Steve's feedback to understand if the
problem is really solved.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

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

[Steve Sakoman]

On Fri, Oct 8, 2010 at 1:03 AM, Stefano Babic <sbabic@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@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.

Steve

[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@gmail.com> wrote:
> On Fri, Oct 8, 2010 at 1:03 AM, Stefano Babic <sbabic@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@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

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

[Stefano Babic]

Steve Sakoman wrote:
> On Fri, Oct 8, 2010 at 1:03 AM, Stefano Babic <sbabic@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.
>>

Hallo Steve,

> 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.

Yes, it makes more sens - and I do not see any problem to move it

> 
> This is a void function and shouldn't have a return value.  I fixed
> this in my version.

Agree.

> 
>>        gd->env_valid = 1;
> 
> I removed this in my version, probably an error to do that though :-)

Or probably not...env_valid should signalize if the checksum is correct,
and now the checksum is verified by the env_import function, as I have
understood.

> I previously submitted a patch to fix this and Wolfgang sent an email
> saying that it had been applied.

Ah, ok, I have missed it ;-)

> I'll revise my patch to add the missing gd->env_valid = 1; and re-test.

Ok, thanks

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

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

[Stefano Babic]

Steve Sakoman wrote:

>> 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!

I have understood, it is not required if we call env_import and we
require to verify the checksum.

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

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

[Steve Sakoman]

On Fri, Oct 8, 2010 at 9:05 AM, Stefano Babic <sbabic@denx.de> wrote:
> Steve Sakoman wrote:
>
>>> 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!
>
> I have understood, it is not required if we call env_import and we
> require to verify the checksum.

OK, thanks!

Would you like me to submit the patch or would you like to revise & resubmit?

Steve

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

[Stefano Babic]

Steve Sakoman wrote:
> On Fri, Oct 8, 2010 at 9:05 AM, Stefano Babic <sbabic@denx.de> wrote:
>> Steve Sakoman wrote:
>>
>>>> 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!
>> I have understood, it is not required if we call env_import and we
>> require to verify the checksum.
> 
> OK, thanks!
> 
> Would you like me to submit the patch or would you like to revise & resubmit?

No problem at all - you can submit your patch, as it seems to me more
complete as mine.

Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================