From: Mike Frysinger <vapier@gentoo.org>
To: u-boot@lists.denx.de
Subject: [U-Boot] env_flash.c:saveenv() broken when env is smaller than a sector
Date: Wed, 29 Dec 2010 20:53:17 -0500 [thread overview]
Message-ID: <201012292053.18557.vapier@gentoo.org> (raw)
In-Reply-To: <1279395948-25864-6-git-send-email-wd@denx.de>
On Saturday, July 17, 2010 15:45:48 Wolfgang Denk wrote:
> --- a/common/env_flash.c
> +++ b/common/env_flash.c
> #ifdef CMD_SAVEENV
> int saveenv(void)
> {
> - char *saved_data = NULL;
> - int rc = 1;
> - char flag = OBSOLETE_FLAG, new_flag = ACTIVE_FLAG;
> + env_t env_new;
> + ssize_t len;
> + char *saved_data = NULL;
> + char *res;
> + int rc = 1;
> + char flag = OBSOLETE_FLAG, new_flag = ACTIVE_FLAG;
> #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
> - ulong up_data = 0;
> + ulong up_data = 0;
> #endif
>
> - debug ("Protect off %08lX ... %08lX\n",
> + debug("Protect off %08lX ... %08lX\n",
> (ulong)flash_addr, end_addr);
>
> - if (flash_sect_protect (0, (ulong)flash_addr, end_addr)) {
> - goto Done;
> + if (flash_sect_protect(0, (ulong)flash_addr, end_addr)) {
> + goto done;
> }
>
> - debug ("Protect off %08lX ... %08lX\n",
> + debug("Protect off %08lX ... %08lX\n",
> (ulong)flash_addr_new, end_addr_new);
>
> - if (flash_sect_protect (0, (ulong)flash_addr_new, end_addr_new)) {
> - goto Done;
> + if (flash_sect_protect(0, (ulong)flash_addr_new, end_addr_new)) {
> + goto done;
> + }
> +
> + res = (char *)&env_new.data;
> + len = hexport('\0', &res, ENV_SIZE);
> + if (len < 0) {
> + error("Cannot export environment: errno = %d\n", errno);
> + goto done;
> }
> + env_new.crc = crc32(0, env_new.data, ENV_SIZE);
> + env_new.flags = new_flag;
>
> #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
> up_data = (end_addr_new + 1 - ((long)flash_addr_new + CONFIG_ENV_SIZE));
> - debug ("Data to save 0x%x\n", up_data);
> + debug("Data to save 0x%lX\n", up_data);
> if (up_data) {
> if ((saved_data = malloc(up_data)) == NULL) {
> printf("Unable to save the rest of sector (%ld)\n",
> up_data);
> - goto Done;
> + goto done;
> }
> memcpy(saved_data,
> (void *)((long)flash_addr_new + CONFIG_ENV_SIZE), up_data);
> - debug ("Data (start 0x%x, len 0x%x) saved at 0x%x\n",
> - (long)flash_addr_new + CONFIG_ENV_SIZE,
> - up_data, saved_data);
> + debug("Data (start 0x%lX, len 0x%lX) saved at 0x%p\n",
> + (long)flash_addr_new + CONFIG_ENV_SIZE,
> + up_data, saved_data);
> }
> #endif
> - puts ("Erasing Flash...");
> - debug (" %08lX ... %08lX ...",
> + puts("Erasing Flash...");
> + debug(" %08lX ... %08lX ...",
> (ulong)flash_addr_new, end_addr_new);
>
> - if (flash_sect_erase ((ulong)flash_addr_new, end_addr_new)) {
> - goto Done;
> + if (flash_sect_erase((ulong)flash_addr_new, end_addr_new)) {
> + goto done;
> }
>
> - puts ("Writing to Flash... ");
> - debug (" %08lX ... %08lX ...",
> + puts("Writing to Flash... ");
> + debug(" %08lX ... %08lX ...",
> (ulong)&(flash_addr_new->data),
> sizeof(env_ptr->data)+(ulong)&(flash_addr_new->data));
> - if ((rc = flash_write((char *)env_ptr->data,
> - (ulong)&(flash_addr_new->data),
> - sizeof(env_ptr->data))) ||
> - (rc = flash_write((char *)&(env_ptr->crc),
> - (ulong)&(flash_addr_new->crc),
> - sizeof(env_ptr->crc))) ||
> + if ((rc = flash_write((char *)&env_new,
> + (ulong)flash_addr_new,
> + sizeof(env_new))) ||
> (rc = flash_write(&flag,
> (ulong)&(flash_addr->flags),
> - sizeof(flash_addr->flags))) ||
> - (rc = flash_write(&new_flag,
> - (ulong)&(flash_addr_new->flags),
> - sizeof(flash_addr_new->flags))))
> - {
> - flash_perror (rc);
> - goto Done;
> + sizeof(flash_addr->flags))) ) {
> + flash_perror(rc);
> + goto done;
> }
> - puts ("done\n");
>
> #if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
> if (up_data) { /* restore the rest of sector */
> - debug ("Restoring the rest of data to 0x%x len 0x%x\n",
> - (long)flash_addr_new + CONFIG_ENV_SIZE, up_data);
> + debug("Restoring the rest of data to 0x%lX len 0x%lX\n",
> + (long)flash_addr_new + CONFIG_ENV_SIZE, up_data);
> if (flash_write(saved_data,
> (long)flash_addr_new + CONFIG_ENV_SIZE,
> up_data)) {
> flash_perror(rc);
> - goto Done;
> + goto done;
> }
> }
> #endif
> + puts("done\n");
> +
> {
> env_t * etmp = flash_addr;
> ulong ltmp = end_addr;
> @@ -220,13 +230,12 @@ int saveenv(void)
> }
>
> rc = 0;
> -Done:
> -
> +done:
> if (saved_data)
> - free (saved_data);
> + free(saved_data);
> /* try to re-protect */
> - (void) flash_sect_protect (1, (ulong)flash_addr, end_addr);
> - (void) flash_sect_protect (1, (ulong)flash_addr_new, end_addr_new);
> + (void) flash_sect_protect(1, (ulong)flash_addr, end_addr);
> + (void) flash_sect_protect(1, (ulong)flash_addr_new, end_addr_new);
>
> return rc;
> }
> @@ -244,83 +253,93 @@ int env_init(void)
>
> gd->env_addr = (ulong)&default_environment[0];
> gd->env_valid = 0;
> - return (0);
> + return 0;
> }
>
> #ifdef CMD_SAVEENV
>
> int saveenv(void)
> {
> - int len, rc;
> - ulong end_addr;
> - ulong flash_sect_addr;
> -#if defined(CONFIG_ENV_SECT_SIZE) && (CONFIG_ENV_SECT_SIZE >
> CONFIG_ENV_SIZE) - ulong flash_offset;
> - uchar env_buffer[CONFIG_ENV_SECT_SIZE];
> -#else
> - uchar *env_buffer = (uchar *)env_ptr;
> -#endif /* CONFIG_ENV_SECT_SIZE */
> - int rcode = 0;
> -
> -#if defined(CONFIG_ENV_SECT_SIZE) && (CONFIG_ENV_SECT_SIZE >
> CONFIG_ENV_SIZE) -
> - flash_offset = ((ulong)flash_addr) & (CONFIG_ENV_SECT_SIZE-1);
> - flash_sect_addr = ((ulong)flash_addr) & ~(CONFIG_ENV_SECT_SIZE-1);
> -
> - debug ( "copy old content: "
> - "sect_addr: %08lX env_addr: %08lX offset: %08lX\n",
> - flash_sect_addr, (ulong)flash_addr, flash_offset);
> -
> - /* copy old contents to temporary buffer */
> - memcpy (env_buffer, (void *)flash_sect_addr, CONFIG_ENV_SECT_SIZE);
> -
> - /* copy current environment to temporary buffer */
> - memcpy ((uchar *)((unsigned long)env_buffer + flash_offset),
> - env_ptr,
> - CONFIG_ENV_SIZE);
> + env_t env_new;
> + ssize_t len;
> + int rc = 1;
> + char *res;
> + char *saved_data = NULL;
> +#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
> + ulong up_data = 0;
>
> - len = CONFIG_ENV_SECT_SIZE;
> -#else
> - flash_sect_addr = (ulong)flash_addr;
> - len = CONFIG_ENV_SIZE;
> + up_data = (end_addr + 1 - ((long)flash_addr + CONFIG_ENV_SIZE));
> + debug("Data to save 0x%lx\n", up_data);
> + if (up_data) {
> + if ((saved_data = malloc(up_data)) == NULL) {
> + printf("Unable to save the rest of sector (%ld)\n",
> + up_data);
> + goto done;
> + }
> + memcpy(saved_data,
> + (void *)((long)flash_addr + CONFIG_ENV_SIZE), up_data);
> + debug("Data (start 0x%lx, len 0x%lx) saved at 0x%lx\n",
> + (ulong)flash_addr + CONFIG_ENV_SIZE,
> + up_data,
> + (ulong)saved_data);
> + }
> #endif /* CONFIG_ENV_SECT_SIZE */
>
> - end_addr = flash_sect_addr + len - 1;
> + debug("Protect off %08lX ... %08lX\n",
> + (ulong)flash_addr, end_addr);
>
> - debug ("Protect off %08lX ... %08lX\n",
> - (ulong)flash_sect_addr, end_addr);
> + if (flash_sect_protect(0, (long)flash_addr, end_addr))
> + goto done;
>
> - if (flash_sect_protect (0, flash_sect_addr, end_addr))
> - return 1;
> + res = (char *)&env_new.data;
> + len = hexport('\0', &res, ENV_SIZE);
> + if (len < 0) {
> + error("Cannot export environment: errno = %d\n", errno);
> + goto done;
> + }
> + env_new.crc = crc32(0, env_new.data, ENV_SIZE);
>
> - puts ("Erasing Flash...");
> - if (flash_sect_erase (flash_sect_addr, end_addr))
> - return 1;
> + puts("Erasing Flash...");
> + if (flash_sect_erase((long)flash_addr, end_addr))
> + goto done;
>
> - puts ("Writing to Flash... ");
> - rc = flash_write((char *)env_buffer, flash_sect_addr, len);
> + puts("Writing to Flash... ");
> + rc = flash_write((char *)&env_new, (long)flash_addr, CONFIG_ENV_SIZE);
> if (rc != 0) {
> - flash_perror (rc);
> - rcode = 1;
> - } else {
> - puts ("done\n");
> + flash_perror(rc);
> + goto done;
> }
> -
> +#if CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE
> + if (up_data) { /* restore the rest of sector */
> + debug("Restoring the rest of data to 0x%lx len 0x%lx\n",
> + (ulong)flash_addr + CONFIG_ENV_SIZE, up_data);
> + if (flash_write(saved_data,
> + (long)flash_addr + CONFIG_ENV_SIZE,
> + up_data)) {
> + flash_perror(rc);
> + goto done;
> + }
> + }
> +#endif
> + puts("done\n");
> + rc = 0;
> +done:
> + if (saved_data)
> + free(saved_data);
> /* try to re-protect */
> - (void) flash_sect_protect (1, flash_sect_addr, end_addr);
> - return rcode;
> + (void) flash_sect_protect(1, (long)flash_addr, end_addr);
> + return rc;
> }
it would seem that somewhere nestled in this rewrite, support for embedded env
was broken (CONFIG_ENV_SIZE < CONFIG_ENV_SECT_SIZE). on a bf548-ezkit for
example, i see this behavior:
(v2010.09 / before this commit):
bfin> save
Saving Environment to Flash...
. done
Un-Protected 1 sectors
Erasing Flash...
. done
Erased 1 sectors
Writing to Flash... done
. done
Protected 1 sectors
(after this commit / v2010.12):
bfin> save
Saving Environment to Flash...
Error: start address not on sector boundary
Error: start address not on sector boundary
not sure if anyone has noticed/fixed this yet and would save me from the
trouble of finding/fixing the problem ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20101229/14330289/attachment.pgp
next prev parent reply other threads:[~2010-12-30 1:53 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-07-17 19:45 [U-Boot] [PATCH 0/5] New environment code Wolfgang Denk
2010-07-17 19:45 ` [U-Boot] [PATCH 1/5] Add basic errno support Wolfgang Denk
2010-07-17 21:17 ` Mike Frysinger
2010-07-17 21:34 ` Wolfgang Denk
2010-07-18 12:51 ` Jerry Van Baren
2010-07-18 13:03 ` Wolfgang Denk
2010-09-12 19:16 ` Wolfgang Denk
2010-07-17 19:45 ` [U-Boot] [PATCH 2/5] Add qsort - add support for sorting data arrays Wolfgang Denk
2010-09-12 19:16 ` Wolfgang Denk
2010-07-17 19:45 ` [U-Boot] [PATCH 3/5] Add hash table support as base for new environment code Wolfgang Denk
2010-09-12 19:16 ` Wolfgang Denk
2010-12-08 9:44 ` Mike Frysinger
2010-12-08 10:02 ` Wolfgang Denk
2010-12-08 10:52 ` Mike Frysinger
2010-07-17 19:45 ` [U-Boot] [PATCH 4/5] Remove support for CONFIG_HAS_UID and "forceenv" command Wolfgang Denk
2010-07-17 22:12 ` Sergey Kubushyn
2010-09-12 19:18 ` Wolfgang Denk
2010-07-17 19:45 ` [U-Boot] [PATCH 5/5] New implementation for internal handling of environment variables Wolfgang Denk
2010-07-17 22:48 ` [U-Boot] [PATCH] new env: fix off-by-one error in setenv command Wolfgang Denk
2010-07-20 0:38 ` [U-Boot] [PATCH 5/5] New implementation for internal handling of environment variables Kim Phillips
2010-07-20 9:40 ` Wolfgang Denk
2010-07-20 18:36 ` Kim Phillips
2010-07-20 19:01 ` Wolfgang Denk
2010-07-20 20:09 ` Wolfgang Denk
[not found] ` <1279658019.5685.125.camel@thunk>
2010-07-20 20:35 ` Wolfgang Denk
2010-07-20 21:08 ` Kim Phillips
2010-07-20 21:43 ` Wolfgang Denk
2010-07-20 22:00 ` Kim Phillips
2010-07-25 21:45 ` Wolfgang Denk
2010-07-26 23:18 ` Kim Phillips
2010-07-20 19:11 ` Wolfgang Denk
2010-07-26 14:52 ` Matthias Fuchs
2010-07-28 21:17 ` Wolfgang Denk
2010-07-29 9:16 ` Matthias Fuchs
2010-08-03 22:48 ` Wolfgang Denk
2010-09-12 19:19 ` Wolfgang Denk
2010-12-30 1:53 ` Mike Frysinger [this message]
2010-12-30 2:39 ` [U-Boot] env_flash.c:saveenv() broken when env is smaller than a sector Mike Frysinger
2011-01-17 20:23 ` Wolfgang Denk
2010-07-17 19:49 ` [U-Boot] [PATCH 0/5] New environment code Wolfgang Denk
2010-07-17 20:56 ` Reinhard Meyer
2010-07-17 21:28 ` Mike Frysinger
2010-07-17 21:41 ` Wolfgang Denk
2010-07-18 2:18 ` Mike Frysinger
2010-07-17 21:31 ` Wolfgang Denk
2010-07-17 21:55 ` Wolfgang Denk
2010-07-18 5:32 ` Reinhard Meyer
2010-07-18 10:26 ` Wolfgang Denk
2010-10-20 8:08 ` Mike Frysinger
2010-10-20 8:19 ` Wolfgang Denk
2010-12-08 9:56 ` Mike Frysinger
2010-12-08 10:04 ` Wolfgang Denk
2010-12-08 10:19 ` Mike Frysinger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201012292053.18557.vapier@gentoo.org \
--to=vapier@gentoo.org \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox