[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM
From: Eric Schumann E.Schumann@phytec.de
On the pcm030 the environment is located in the onboard EEPROM. But we want to handle flash sector protection in a safe manner. So we must read the unlock environment variable from EEPROM instead from flash.
This patch is required as long the evironment is saved into the EEPROM.
Signed-off-by: Eric Schumann E.Schumann@phytec.de Signed-off-by: Sascha Hauer s.hauer@pengutronix.de --- 0 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 391d169..1436131 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2009,7 +2009,9 @@ unsigned long flash_init (void) #endif
#ifdef CONFIG_SYS_FLASH_PROTECTION - char *s = getenv("unlock"); + /* read environment from EEPROM */ + char s[4]; + getenv_r ("unlock", s, sizeof(s)); #endif
#define BANK_BASE(i) (((phys_addr_t [CFI_MAX_FLASH_BANKS])CONFIG_SYS_FLASH_BANKS_LIST)[i])
Dear Jon,
In message 20090321135934.12464.65400.stgit@localhost you wrote:
From: Eric Schumann E.Schumann@phytec.de
On the pcm030 the environment is located in the onboard EEPROM. But we want to handle flash sector protection in a safe manner. So we must read the unlock environment variable from EEPROM instead from flash.
This patch is required as long the evironment is saved into the EEPROM.
Signed-off-by: Eric Schumann E.Schumann@phytec.de Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
0 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 391d169..1436131 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2009,7 +2009,9 @@ unsigned long flash_init (void) #endif
#ifdef CONFIG_SYS_FLASH_PROTECTION
- char *s = getenv("unlock");
- /* read environment from EEPROM */
- char s[4];
- getenv_r ("unlock", s, sizeof(s));
#endif
This patch doesn't really make sense to me. The flash_init() code is only being run after relocation to RAM, so standard getenv() is supposed to work, and there should be no reason why we would have to fall back on getenv_r().
Please explain which issues you are observing.
Best regards,
Wolfgang Denk
On Sat, Mar 21, 2009 at 3:52 PM, Wolfgang Denk wd@denx.de wrote:
Dear Jon,
In message 20090321135934.12464.65400.stgit@localhost you wrote:
From: Eric Schumann E.Schumann@phytec.de
On the pcm030 the environment is located in the onboard EEPROM. But we want to handle flash sector protection in a safe manner. So we must read the unlock environment variable from EEPROM instead from flash.
This patch is required as long the evironment is saved into the EEPROM.
Signed-off-by: Eric Schumann E.Schumann@phytec.de Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
0 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 391d169..1436131 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2009,7 +2009,9 @@ unsigned long flash_init (void) #endif
#ifdef CONFIG_SYS_FLASH_PROTECTION
- char *s = getenv("unlock");
- /* read environment from EEPROM */
- char s[4];
- getenv_r ("unlock", s, sizeof(s));
#endif
This patch doesn't really make sense to me. The flash_init() code is only being run after relocation to RAM, so standard getenv() is supposed to work, and there should be no reason why we would have to fall back on getenv_r().
I didn't write these, these are the Phytex pcm030 board support patches from Pengutronix that have never been submitted to mainline. They're all working on my hardware but of course there may be better ways to do things.
My guess is getenv() returns a pointer to the environment variable, not a copy of the environment variable. getenv_r() returns a copy. How can you return a pointer to the variable if the variable is in something not directly addressable like EEPROM? Does getenv("unlock"); do what you want when the environment is in EEPROM?
Please explain which issues you are observing.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de "Who is the oldest inhabitant of this village?" "We haven't got one; we had one, but he died three weeks ago."
Dear Jon Smirl,
In message 9e4733910903211308v63878fabx19f3327371db56c6@mail.gmail.com you wrote:
My guess is getenv() returns a pointer to the environment variable, not a copy of the environment variable. getenv_r() returns a copy. How can you return a pointer to the variable if the variable is in something not directly addressable like EEPROM? Does
The environment always gets copied to RAM. And it's a perfectly simple thing to return an adress pointing to some memory in RAM :-)
getenv("unlock"); do what you want when the environment is in EEPROM?
getenv() always works that way, no matter which actual media is used for the persistent storage of the environment.
Best regards,
Wolfgang Denk
On Sat, Mar 21, 2009 at 4:24 PM, Wolfgang Denk wd@denx.de wrote:
Dear Jon Smirl,
In message 9e4733910903211308v63878fabx19f3327371db56c6@mail.gmail.com you wrote:
My guess is getenv() returns a pointer to the environment variable, not a copy of the environment variable. getenv_r() returns a copy. How can you return a pointer to the variable if the variable is in something not directly addressable like EEPROM? Does
The environment always gets copied to RAM. And it's a perfectly simple thing to return an adress pointing to some memory in RAM :-)
getenv("unlock"); do what you want when the environment is in EEPROM?
getenv() always works that way, no matter which actual media is used for the persistent storage of the environment.
This one can be dropped. It looks like a misunderstanding in how the API worked.
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de The only person who always got his work done by Friday was Robinson Crusoe.
Hi Wolgang,
On Sat, Mar 21, 2009 at 09:24:05PM +0100, Wolfgang Denk wrote:
Dear Jon Smirl,
In message 9e4733910903211308v63878fabx19f3327371db56c6@mail.gmail.com you wrote:
My guess is getenv() returns a pointer to the environment variable, not a copy of the environment variable. getenv_r() returns a copy. How can you return a pointer to the variable if the variable is in something not directly addressable like EEPROM? Does
The environment always gets copied to RAM. And it's a perfectly simple thing to return an adress pointing to some memory in RAM :-)
getenv("unlock"); do what you want when the environment is in EEPROM?
getenv() always works that way, no matter which actual media is used for the persistent storage of the environment.
This is not quite true. In the PPC init sequence flash_init() is called before env_relocate() and thus getenv is not available in flash_init(). Please note that this was just a quick way for us to make things work and I never considered this a fix for mainline.
Sascha
On Sun, Mar 22, 2009 at 6:59 PM, Sascha Hauer s.hauer@pengutronix.de wrote:
Hi Wolgang,
On Sat, Mar 21, 2009 at 09:24:05PM +0100, Wolfgang Denk wrote:
Dear Jon Smirl,
In message 9e4733910903211308v63878fabx19f3327371db56c6@mail.gmail.com you wrote:
My guess is getenv() returns a pointer to the environment variable, not a copy of the environment variable. getenv_r() returns a copy. How can you return a pointer to the variable if the variable is in something not directly addressable like EEPROM? Does
The environment always gets copied to RAM. And it's a perfectly simple thing to return an adress pointing to some memory in RAM :-)
getenv("unlock"); do what you want when the environment is in EEPROM?
getenv() always works that way, no matter which actual media is used for the persistent storage of the environment.
This is not quite true. In the PPC init sequence flash_init() is called before env_relocate() and thus getenv is not available in flash_init(). Please note that this was just a quick way for us to make things work and I never considered this a fix for mainline.
What is your fix for mainline?
Sascha
-- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Sun, Mar 22, 2009 at 11:01:09PM -0400, Jon Smirl wrote:
On Sun, Mar 22, 2009 at 6:59 PM, Sascha Hauer s.hauer@pengutronix.de wrote:
Hi Wolgang,
On Sat, Mar 21, 2009 at 09:24:05PM +0100, Wolfgang Denk wrote:
Dear Jon Smirl,
In message 9e4733910903211308v63878fabx19f3327371db56c6@mail.gmail.com you wrote:
My guess is getenv() returns a pointer to the environment variable, not a copy of the environment variable. getenv_r() returns a copy. How can you return a pointer to the variable if the variable is in something not directly addressable like EEPROM? Does
The environment always gets copied to RAM. And it's a perfectly simple thing to return an adress pointing to some memory in RAM :-)
getenv("unlock"); do what you want when the environment is in EEPROM?
getenv() always works that way, no matter which actual media is used for the persistent storage of the environment.
This is not quite true. In the PPC init sequence flash_init() is called before env_relocate() and thus getenv is not available in flash_init(). Please note that this was just a quick way for us to make things work and I never considered this a fix for mainline.
What is your fix for mainline?
I have none.
Sascha
Dear Jon,
in message 9e4733910903222001h54568fao2f96dcbdc8925df5@mail.gmail.com you wrote:
getenv("unlock"); do what you want when the environment is in EEPROM?
getenv() always works that way, no matter which actual media is used for the persistent storage of the environment.
This is not quite true. In the PPC init sequence flash_init() is called before env_relocate() and thus getenv is not available in flash_init().
I see. You are right.
Please note that this was just a quick way for us to make things work and I never considered this a fix for mainline.
What is your fix for mainline?
We cannot run env_relocate() before flash_init(), as (with environ- ment stored in flash) we need an initialized flash memory to do so. Consequently, we indeed have to use getenv_r() instead of getenv().
Stefan, will you pick up the original patch, please: http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/56275
Hm, you might want to change "char s[4];" into "char s[256];" or similar, though, to make buffer overflows less likely.
Best regards,
Wolfgang Denk
Dear Stefan,
In message 20090321135934.12464.65400.stgit@localhost Jon Smirl wrote:
From: Eric Schumann E.Schumann@phytec.de
On the pcm030 the environment is located in the onboard EEPROM. But we want to handle flash sector protection in a safe manner. So we must read the unlock environment variable from EEPROM instead from flash.
This patch is required as long the evironment is saved into the EEPROM.
Signed-off-by: Eric Schumann E.Schumann@phytec.de Signed-off-by: Sascha Hauer s.hauer@pengutronix.de
0 files changed, 0 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 391d169..1436131 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -2009,7 +2009,9 @@ unsigned long flash_init (void) #endif
#ifdef CONFIG_SYS_FLASH_PROTECTION
- char *s = getenv("unlock");
- /* read environment from EEPROM */
- char s[4];
- getenv_r ("unlock", s, sizeof(s));
#endif
#define BANK_BASE(i) (((phys_addr_t [CFI_MAX_FLASH_BANKS])CONFIG_SYS_FLASH_BANKS_LIST)[i])
Please change "char s[4];" into "char s[256];", otherwise:
Acked-by: Wolfgang Denk wd@denx.de
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Monday 23 March 2009, Wolfgang Denk wrote:
+++ b/drivers/mtd/cfi_flash.c @@ -2009,7 +2009,9 @@ unsigned long flash_init (void) #endif
#ifdef CONFIG_SYS_FLASH_PROTECTION
- char *s = getenv("unlock");
- /* read environment from EEPROM */
- char s[4];
- getenv_r ("unlock", s, sizeof(s));
#endif
#define BANK_BASE(i) (((phys_addr_t [CFI_MAX_FLASH_BANKS])CONFIG_SYS_FLASH_BANKS_LIST)[i])
Please change "char s[4];" into "char s[256];", otherwise:
Acked-by: Wolfgang Denk wd@denx.de
OK, I changed the array to 64 (256 is a bit high without "real" stack) and will push this patch quickly to the cfi repository.
Best regards, Stefan
===================================================================== 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@denx.de =====================================================================
participants (5)
-
Jon -
Jon Smirl
-
Sascha Hauer -
Stefan Roese -
Wolfgang Denk