| Message ID | 1353502761-12640-2-git-send-email-phil.sutter@viprinet.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
On 11/21/2012 06:59:19 AM, Phil Sutter wrote: > Without this patch, when the currently chosen environment to be > written > has bad blocks, saveenv fails completely. Instead, when there is > redundant environment fall back to the other copy. Environment reading > needs no adjustment, as the fallback logic for incomplete writes > applies > to this case as well. > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com> Isn't this what CONFIG_ENV_RANGE is supposed to deal with? Redundant environment is to deal with other problems such as a power failure during saveenv. If you just fall back to the other copy, you're silently losing the redundancy. -Scott
Hi, On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote: > On 11/21/2012 06:59:19 AM, Phil Sutter wrote: > > Without this patch, when the currently chosen environment to be > > written > > has bad blocks, saveenv fails completely. Instead, when there is > > redundant environment fall back to the other copy. Environment reading > > needs no adjustment, as the fallback logic for incomplete writes > > applies > > to this case as well. > > > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com> > > Isn't this what CONFIG_ENV_RANGE is supposed to deal with? Yes, that is more or less what is supposed to help for cases like this. But given the fact that CONFIG_ENV_RANGE needs to span multiple erase pages which in our case are 128k in size, this is quite a deal. Especially since one needs to have multiple pages for both normal and redundant environment to be really sure. But, we already have a fixed hashmap in field, so using CONFIG_ENV_RANGE is simply no option. > Redundant environment is to deal with other problems such as a power > failure during saveenv. If you just fall back to the other copy, > you're silently losing the redundancy. The alternative to silently losing redundancy in case one of the blocks in either normal or redundant env areas turns bad is to not being able to save the environment at all anymore. I'd prefer dropping the redundancy but still having a working system then. ;) Best wishes and thanks for the review, Phil Sutter Software Engineer
Scott, On Thu, Dec 06, 2012 at 12:18:39PM -0600, Scott Wood wrote: > On 11/28/2012 03:06:00 PM, Phil Sutter wrote: > > Hi, > > > > On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote: > > > On 11/21/2012 06:59:19 AM, Phil Sutter wrote: > > > > Without this patch, when the currently chosen environment to be > > > > written > > > > has bad blocks, saveenv fails completely. Instead, when there is > > > > redundant environment fall back to the other copy. Environment > > reading > > > > needs no adjustment, as the fallback logic for incomplete writes > > > > applies > > > > to this case as well. > > > > > > > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com> > > > > > > Isn't this what CONFIG_ENV_RANGE is supposed to deal with? > > > > Yes, that is more or less what is supposed to help for cases like > > this. > > But given the fact that CONFIG_ENV_RANGE needs to span multiple erase > > pages which in our case are 128k in size, this is quite a deal. > > Especially since one needs to have multiple pages for both normal and > > redundant environment to be really sure. > > And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal with. :-) Good to know, I already wondered what exactly this option is there for. > Though at the moment redundant environment is not supported with > CONFIG_ENV_OFFSET_OOB. I will have a look at it now, because that could be an elegant solution for both of us I think. > > But, we already have a fixed hashmap in field, so using > > CONFIG_ENV_RANGE > > is simply no option. > > That's relevant to what you put in your product, but it shouldn't be > the basis of how mainline U-Boot does things for all boards. Yes, of course. That was merely me explaining myself, not so much of a real point in matters of what should go mainline and what not. > > > Redundant environment is to deal with other problems such as a power > > > failure during saveenv. If you just fall back to the other copy, > > > you're silently losing the redundancy. > > > > The alternative to silently losing redundancy in case one of the > > blocks > > in either normal or redundant env areas turns bad is to not being able > > to save the environment at all anymore. I'd prefer dropping the > > redundancy but still having a working system then. ;) > > Another alternative is to noisily lose redundancy. Would that be an acceptable alternative from your point of view? Having CONFIG_ENV_OFFSET_OOB in mind, there may be situations where all blocks of either one of the redundant environments get bad (quite artificial, I know). Losing redundancy in exchange for keeping env writeability could still be an option then. What do you think? Best wishes, Phil Sutter Software Engineer
Scott, On Fri, Dec 07, 2012 at 12:53:34PM +0100, Phil Sutter wrote: > On Thu, Dec 06, 2012 at 12:18:39PM -0600, Scott Wood wrote: > > On 11/28/2012 03:06:00 PM, Phil Sutter wrote: > > > Hi, > > > > > > On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote: > > > > On 11/21/2012 06:59:19 AM, Phil Sutter wrote: > > > > > Without this patch, when the currently chosen environment to be > > > > > written > > > > > has bad blocks, saveenv fails completely. Instead, when there is > > > > > redundant environment fall back to the other copy. Environment > > > reading > > > > > needs no adjustment, as the fallback logic for incomplete writes > > > > > applies > > > > > to this case as well. > > > > > > > > > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com> > > > > > > > > Isn't this what CONFIG_ENV_RANGE is supposed to deal with? > > > > > > Yes, that is more or less what is supposed to help for cases like > > > this. > > > But given the fact that CONFIG_ENV_RANGE needs to span multiple erase > > > pages which in our case are 128k in size, this is quite a deal. > > > Especially since one needs to have multiple pages for both normal and > > > redundant environment to be really sure. > > > > And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal with. :-) > > Good to know, I already wondered what exactly this option is there for. Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select the block(s) within the erase page to save the environment. Looking at common/env_nand.c:318, the environment offset saved in the OOB seems to be in erase page unit. On the other hand, I could not find code that alters this setting based on bad blocks found or whatever. This seems to simply be an alternative to setting CONFIG_ENV_OFFSET at build-time. Best wishes, Phil Sutter Software Engineer
On 12/07/2012 10:58:53 AM, Phil Sutter wrote: > Scott, > > On Fri, Dec 07, 2012 at 12:53:34PM +0100, Phil Sutter wrote: > > On Thu, Dec 06, 2012 at 12:18:39PM -0600, Scott Wood wrote: > > > On 11/28/2012 03:06:00 PM, Phil Sutter wrote: > > > > Hi, > > > > > > > > On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote: > > > > > On 11/21/2012 06:59:19 AM, Phil Sutter wrote: > > > > > > Without this patch, when the currently chosen environment > to be > > > > > > written > > > > > > has bad blocks, saveenv fails completely. Instead, when > there is > > > > > > redundant environment fall back to the other copy. > Environment > > > > reading > > > > > > needs no adjustment, as the fallback logic for incomplete > writes > > > > > > applies > > > > > > to this case as well. > > > > > > > > > > > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com> > > > > > > > > > > Isn't this what CONFIG_ENV_RANGE is supposed to deal with? > > > > > > > > Yes, that is more or less what is supposed to help for cases > like > > > > this. > > > > But given the fact that CONFIG_ENV_RANGE needs to span multiple > erase > > > > pages which in our case are 128k in size, this is quite a deal. > > > > Especially since one needs to have multiple pages for both > normal and > > > > redundant environment to be really sure. > > > > > > And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal > with. :-) > > > > Good to know, I already wondered what exactly this option is there > for. > > Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select the > block(s) within the erase page to save the environment. Looking at > common/env_nand.c:318, the environment offset saved in the OOB seems > to > be in erase page unit. I'm not sure what you mean by "block(s) within the erase page" -- blocks are the unit of erasing, and of bad block marking. The block to hold the environment is stored in the OOB of block zero, which is usually guaranteed to not be bad. > On the other hand, I could not find code that alters this setting > based > on bad blocks found or whatever. This seems to simply be an > alternative > to setting CONFIG_ENV_OFFSET at build-time. It is set by the "nand env.oob" command. It is currently a manual process (or rather, automation is left to the user's board preparation process rather than being built into U-Boot), as U-Boot wouldn't know how to give back unused blocks to other purposes. -Scott
On Fri, Dec 07, 2012 at 11:38:11AM -0600, Scott Wood wrote: > On 12/07/2012 10:58:53 AM, Phil Sutter wrote: > > Scott, > > > > On Fri, Dec 07, 2012 at 12:53:34PM +0100, Phil Sutter wrote: > > > On Thu, Dec 06, 2012 at 12:18:39PM -0600, Scott Wood wrote: > > > > On 11/28/2012 03:06:00 PM, Phil Sutter wrote: > > > > > Hi, > > > > > > > > > > On Tue, Nov 27, 2012 at 04:04:15PM -0600, Scott Wood wrote: > > > > > > On 11/21/2012 06:59:19 AM, Phil Sutter wrote: > > > > > > > Without this patch, when the currently chosen environment > > to be > > > > > > > written > > > > > > > has bad blocks, saveenv fails completely. Instead, when > > there is > > > > > > > redundant environment fall back to the other copy. > > Environment > > > > > reading > > > > > > > needs no adjustment, as the fallback logic for incomplete > > writes > > > > > > > applies > > > > > > > to this case as well. > > > > > > > > > > > > > > Signed-off-by: Phil Sutter <phil.sutter@viprinet.com> > > > > > > > > > > > > Isn't this what CONFIG_ENV_RANGE is supposed to deal with? > > > > > > > > > > Yes, that is more or less what is supposed to help for cases > > like > > > > > this. > > > > > But given the fact that CONFIG_ENV_RANGE needs to span multiple > > erase > > > > > pages which in our case are 128k in size, this is quite a deal. > > > > > Especially since one needs to have multiple pages for both > > normal and > > > > > redundant environment to be really sure. > > > > > > > > And *that* is what CONFIG_ENV_OFFSET_OOB is supposed to deal > > with. :-) > > > > > > Good to know, I already wondered what exactly this option is there > > for. > > > > Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select the > > block(s) within the erase page to save the environment. Looking at > > common/env_nand.c:318, the environment offset saved in the OOB seems > > to > > be in erase page unit. > > I'm not sure what you mean by "block(s) within the erase page" -- > blocks are the unit of erasing, and of bad block marking. Not always, at least not with NAND flash. Erase pages are mostly bigger than write pages (or "blocks"). In my case, flash consists of 0x800 bytes write pages and 0x2000 bytes erase pages. > The block to hold the environment is stored in the OOB of block zero, > which is usually guaranteed to not be bad. Erase or write block? Note that every write block has it's own OOB. > > On the other hand, I could not find code that alters this setting > > based > > on bad blocks found or whatever. This seems to simply be an > > alternative > > to setting CONFIG_ENV_OFFSET at build-time. > > It is set by the "nand env.oob" command. It is currently a manual > process (or rather, automation is left to the user's board preparation > process rather than being built into U-Boot), as U-Boot wouldn't know > how to give back unused blocks to other purposes. So that assumes that any block initially identified 'good' will ever turn 'bad' later on? Best wishes, Phil Sutter Software Engineer
On Tue, Dec 11, 2012 at 05:12:32PM -0600, Scott Wood wrote: > On 12/10/2012 07:41:43 AM, Phil Sutter wrote: > > On Fri, Dec 07, 2012 at 11:38:11AM -0600, Scott Wood wrote: > > > On 12/07/2012 10:58:53 AM, Phil Sutter wrote: > > > > Hmm. Does not look like CONFIG_ENV_OFFSET_OOB is used to select > > the > > > > block(s) within the erase page to save the environment. Looking at > > > > common/env_nand.c:318, the environment offset saved in the OOB > > seems > > > > to > > > > be in erase page unit. > > > > > > I'm not sure what you mean by "block(s) within the erase page" -- > > > blocks are the unit of erasing, and of bad block marking. > > > > Not always, at least not with NAND flash. Erase pages are mostly > > bigger > > than write pages (or "blocks"). In my case, flash consists of 0x800 > > bytes write pages and 0x2000 bytes erase pages. > > Erase blocks are larger than write pages, yes. I've never heard erase > blocks called "pages" or write pages called "blocks" -- but my main > point is that the unit of erasing and the unit of badness are the same. Ah, OK. Please excuse my humble nomenclature, I never cared enough to sort out what is called what. Of course, this is not the best basis for a discussion about these things. But getting back to the topic: The assumption of blocks getting bad, not pages within a block means that for any kind of bad block prevention, multiple blocks need to be used. Although I'm honestly speaking not really sure why this needs to be like that. Maybe the bad page marking would disappear when erasing the block it belongs to? > > > The block to hold the environment is stored in the OOB of block > > zero, > > > which is usually guaranteed to not be bad. > > > > Erase or write block? Note that every write block has it's own OOB. > > "block" means "erase block". > > Every write page has its own OOB, but it is erase blocks that are > marked bad. Typically the block can be marked bad in either the first > or the second page of the erase block. Interesting. I had the impression of pages being marked bad and the block's badness being taken from whether it contains bad pages. Probably the 'nand markbad' command tricked me. > > > > On the other hand, I could not find code that alters this setting > > > > based > > > > on bad blocks found or whatever. This seems to simply be an > > > > alternative > > > > to setting CONFIG_ENV_OFFSET at build-time. > > > > > > It is set by the "nand env.oob" command. It is currently a manual > > > process (or rather, automation is left to the user's board > > preparation > > > process rather than being built into U-Boot), as U-Boot wouldn't > > know > > > how to give back unused blocks to other purposes. > > > > So that assumes that any block initially identified 'good' will ever > > turn 'bad' later on? > > We don't currently have any mechanism for that to happen with the > environment -- which could be another good reason to have real > redundancy that doesn't get crippled from day one by having one copy > land on a factory bad block. Of course, that requires someone to > implement support for redundant environment combined with > CONFIG_ENV_OFFSET_OOB. Well, as long as CONFIG_ENV_OFFSET_REDUND supported falling back to the other copy in case of error there would be a working system in three of four cases instead of only one. > Maybe a better option is to implement support for storing the > environment in ubi, although usually if your environment is in NAND > that means your U-Boot image is in NAND, so you have the same problem > there. Maybe you could have an SPL that contains ubi support, that > fits in the guaranteed-good first block. > > Do you have any data on how often a block might go bad that wasn't > factory-bad, to what extent reads versus writes matter, and whether > there is anything special about block zero beyond not being factory-bad? No, sadly not. I'd guess this information depends on what hardware being used specifically. But I suppose block zero being prone to becoming worn just like any other block, although it not being erased as often should help a lot. Assuming a certain number of erase cycles after each block is worn out and given the fact that CONFIG_ENV_OFFSET_REDUND has always both blocks written (unless power failure occurs), they would turn bad at the same time and therefore rendering the environment useless with or without fallback. :) Best wishes, Phil
On Thu, Dec 20, 2012 at 03:41:37PM -0600, Scott Wood wrote: > On 12/20/2012 03:28:39 PM, Phil Sutter wrote: > > On Tue, Dec 11, 2012 at 05:12:32PM -0600, Scott Wood wrote: > > > Erase blocks are larger than write pages, yes. I've never heard > > erase > > > blocks called "pages" or write pages called "blocks" -- but my main > > > point is that the unit of erasing and the unit of badness are the > > same. > > > > Ah, OK. Please excuse my humble nomenclature, I never cared enough to > > sort out what is called what. Of course, this is not the best basis > > for > > a discussion about these things. > > > > But getting back to the topic: The assumption of blocks getting bad, > > not > > pages within a block means that for any kind of bad block prevention, > > multiple blocks need to be used. Although I'm honestly speaking not > > really sure why this needs to be like that. Maybe the bad page marking > > would disappear when erasing the block it belongs to? > > Yes, it would disappear. This is why erase operations skip bad blocks, > unless the scrub option is uesd. Which is apparently preventing good pages in a block with a bad page from being used, isn't it? > > > > > The block to hold the environment is stored in the OOB of block > > > > zero, > > > > > which is usually guaranteed to not be bad. > > > > > > > > Erase or write block? Note that every write block has it's own > > OOB. > > > > > > "block" means "erase block". > > > > > > Every write page has its own OOB, but it is erase blocks that are > > > marked bad. Typically the block can be marked bad in either the > > first > > > or the second page of the erase block. > > > > Interesting. I had the impression of pages being marked bad and the > > block's badness being taken from whether it contains bad pages. > > Probably > > the 'nand markbad' command tricked me. > > Do you mean the lack of error checking if you pass a non-block-aligned > offset into "nand markbad"? I think the bigger "problem" is 'nand markbad' updating the bad block table along the go. So no real bad block detection occurs as far as I can tell. > > > > So that assumes that any block initially identified 'good' will > > ever > > > > turn 'bad' later on? > > > > > > We don't currently have any mechanism for that to happen with the > > > environment -- which could be another good reason to have real > > > redundancy that doesn't get crippled from day one by having one copy > > > land on a factory bad block. Of course, that requires someone to > > > implement support for redundant environment combined with > > > CONFIG_ENV_OFFSET_OOB. > > > > Well, as long as CONFIG_ENV_OFFSET_REDUND supported falling back to > > the > > other copy in case of error there would be a working system in three > > of > > four cases instead of only one. > > I'm not sure what you mean here -- where do "three", "four", and "one" > come from? Just some quantitative approach: given the environment residing at block A and it's redundant copy at block B, four situations may occur: both blocks good, block A bad, block B bad or both blocks bad. Upstream would fail in all cases but both blocks good. My patch would turn that into failing only if both blocks bad. So working in three of four cases instead of in only one of four. > > > Maybe a better option is to implement support for storing the > > > environment in ubi, although usually if your environment is in NAND > > > that means your U-Boot image is in NAND, so you have the same > > problem > > > there. Maybe you could have an SPL that contains ubi support, that > > > fits in the guaranteed-good first block. > > > > > > Do you have any data on how often a block might go bad that wasn't > > > factory-bad, to what extent reads versus writes matter, and whether > > > there is anything special about block zero beyond not being > > factory-bad? > > > > No, sadly not. I'd guess this information depends on what hardware > > being > > used specifically. But I suppose block zero being prone to becoming > > worn just like any other block, although it not being erased as often > > should help a lot. > > > > Assuming a certain number of erase cycles after each block is worn out > > and given the fact that CONFIG_ENV_OFFSET_REDUND has always both > > blocks > > written (unless power failure occurs), they would turn bad at the same > > time and therefore rendering the environment useless with or without > > fallback. :) > > That depends on whether the specified number of erase cycles per block > is a minimum for any block not marked factory-bad, or whether some > fraction of non-factory-bad blocks may fail early. Sure. Also I'm not sure how "wear" happens, so if blocks get worse or their probability of failure increases from erase to erase. Although the later case would make it hard to guarantee a certain number of erase cycles. Best wishes, Phil
diff --git a/common/env_nand.c b/common/env_nand.c index 79e8033..895532b 100644 --- a/common/env_nand.c +++ b/common/env_nand.c @@ -168,72 +168,53 @@ int writeenv(size_t offset, u_char *buf) return 0; } -#ifdef CONFIG_ENV_OFFSET_REDUND -static unsigned char env_flags; +typedef struct env_location { + const char *name; + nand_erase_options_t *erase_opts; + loff_t offset; +} env_location_t; -int saveenv(void) +static int erase_and_write_env(env_location_t *location, u_char *env_new) { - env_t env_new; - ssize_t len; - char *res; - int ret = 0; - nand_erase_options_t nand_erase_options; - - memset(&nand_erase_options, 0, sizeof(nand_erase_options)); - nand_erase_options.length = CONFIG_ENV_RANGE; + int ret = 0; - if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE) + printf("Erasing %s...\n", location->name); + location->erase_opts->offset = location->offset; + if (nand_erase_opts(&nand_info[0], location->erase_opts)) return 1; - res = (char *)&env_new.data; - len = hexport_r(&env_htab, '\0', &res, ENV_SIZE, 0, NULL); - if (len < 0) { - error("Cannot export environment: errno = %d\n", errno); - return 1; - } - env_new.crc = crc32(0, env_new.data, ENV_SIZE); - env_new.flags = ++env_flags; /* increase the serial */ - - if (gd->env_valid == 1) { - puts("Erasing redundant NAND...\n"); - nand_erase_options.offset = CONFIG_ENV_OFFSET_REDUND; - if (nand_erase_opts(&nand_info[0], &nand_erase_options)) - return 1; - - puts("Writing to redundant NAND... "); - ret = writeenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)&env_new); - } else { - puts("Erasing NAND...\n"); - nand_erase_options.offset = CONFIG_ENV_OFFSET; - if (nand_erase_opts(&nand_info[0], &nand_erase_options)) - return 1; - - puts("Writing to NAND... "); - ret = writeenv(CONFIG_ENV_OFFSET, (u_char *)&env_new); - } - if (ret) { + printf("Writing to %s... ", location->name); + if ((ret = writeenv(location->offset, env_new))) puts("FAILED!\n"); - return 1; - } - - puts("done\n"); - - gd->env_valid = gd->env_valid == 2 ? 1 : 2; return ret; } -#else /* ! CONFIG_ENV_OFFSET_REDUND */ + +static unsigned char env_flags; + int saveenv(void) { int ret = 0; ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1); ssize_t len; char *res; + int env_idx; nand_erase_options_t nand_erase_options; + env_location_t location[] = {{ + .name = "NAND", + .erase_opts = &nand_erase_options, + .offset = CONFIG_ENV_OFFSET, +#ifdef CONFIG_ENV_OFFSET_REDUND + }, { + .name = "redundant NAND", + .erase_opts = &nand_erase_options, + .offset = CONFIG_ENV_OFFSET_REDUND, +#endif + }}; + memset(&nand_erase_options, 0, sizeof(nand_erase_options)); nand_erase_options.length = CONFIG_ENV_RANGE; - nand_erase_options.offset = CONFIG_ENV_OFFSET; if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE) return 1; @@ -244,22 +225,25 @@ int saveenv(void) error("Cannot export environment: errno = %d\n", errno); return 1; } - env_new->crc = crc32(0, env_new->data, ENV_SIZE); - - puts("Erasing Nand...\n"); - if (nand_erase_opts(&nand_info[0], &nand_erase_options)) - return 1; + env_new->crc = crc32(0, env_new->data, ENV_SIZE); +#ifdef CONFIG_ENV_OFFSET_REDUND + env_new->flags = ++env_flags; /* increase the serial */ + env_idx = (gd->env_valid == 1); +#endif - puts("Writing to Nand... "); - if (writeenv(CONFIG_ENV_OFFSET, (u_char *)env_new)) { - puts("FAILED!\n"); - return 1; - } + ret = erase_and_write_env(&location[env_idx], (u_char *)env_new); +#ifdef CONFIG_ENV_OFFSET_REDUND + if (ret) { + env_idx = (env_idx + 1) & 1; + ret = erase_and_write_env(&location[env_idx], + (u_char *)env_new); + } else + gd->env_valid = gd->env_valid == 2 ? 1 : 2; +#endif puts("done\n"); return ret; } -#endif /* CONFIG_ENV_OFFSET_REDUND */ #endif /* CMD_SAVEENV */ int readenv(size_t offset, u_char *buf)
Without this patch, when the currently chosen environment to be written has bad blocks, saveenv fails completely. Instead, when there is redundant environment fall back to the other copy. Environment reading needs no adjustment, as the fallback logic for incomplete writes applies to this case as well. Signed-off-by: Phil Sutter <phil.sutter@viprinet.com> --- common/env_nand.c | 100 ++++++++++++++++++++++------------------------------ 1 files changed, 42 insertions(+), 58 deletions(-)