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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.