[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Jon]

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

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Wolfgang Denk]

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

-- 
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 at denx.de
"Who is the oldest inhabitant of this village?"
"We haven't got one; we had one, but he died three weeks ago."

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Jon Smirl]

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 at denx.de
> "Who is the oldest inhabitant of this village?"
> "We haven't got one; we had one, but he died three weeks ago."
>



-- 
Jon Smirl
jonsmirl at gmail.com

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Wolfgang Denk]

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

-- 
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 at denx.de
The only person who always got his work done by Friday
                                                 was Robinson Crusoe.

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Jon Smirl]

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 at denx.de
> The only person who always got his work done by Friday
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? was Robinson Crusoe.
>



-- 
Jon Smirl
jonsmirl at gmail.com

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Sascha Hauer]

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

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

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Jon Smirl]

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



-- 
Jon Smirl
jonsmirl at gmail.com

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Sascha Hauer]

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

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

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Wolfgang Denk]

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

-- 
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 at denx.de
I used to be indecisive, now I'm not sure.

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[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

-- 
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 at denx.de
I don't see any direct evidence ...  but, then, my crystal ball is in
dire need of an ectoplasmic upgrade. :-)              -- Howard Smith

[U-Boot] [PATCH] Make flash protection work, when the environment is in EEPROM

[Stefan Roese]

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 at denx.de
=====================================================================