[U-Boot] [PATCH] Crash in env_relocate_spec() of env_mmc.c
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
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 Babicsbabic@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
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
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
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
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
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
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
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
participants (3)
-
Sergei Shtylyov -
Stefano Babic -
Steve Sakoman