| Message ID | 1327120684-7066-2-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Commit | df698621a5b39ced5ce527444cafd50c6fdc186d |
| Headers | show |
On Fri, 2012-01-20 at 20:38 -0800, Brian Norris wrote: > As nand_default_block_markbad() is becoming more complex, it helps to > have code appear only in its relevant codepath(s). Here, the calculation > of `ofs' based on NAND_BBT_SCANLASTPAGE is only useful on paths where we > write bad block markers to OOB. We move the condition/calculation closer > to the `write' operation and update the comment to more correctly > describe the operation. > > The variable `wr_ofs' is also used to help isolate our calculation of > the "write" offset from the usage of `ofs' to represent the eraseblock > offset. This will become useful when we reorder operations in the next > patch. > > This patch should make no functional change. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> Pushed this patch to l2-mtd.git because it seem to be good regardless of whether your second patch gets merged or not, thanks. Not sure about the second patch yet, though.
On Fri, Jan 27, 2012 at 6:56 AM, Bityutskiy, Artem <artem.bityutskiy@intel.com> wrote: > On Fri, 2012-01-20 at 20:38 -0800, Brian Norris wrote: >> As nand_default_block_markbad() is becoming more complex, it helps to >> have code appear only in its relevant codepath(s). Here, the calculation >> of `ofs' based on NAND_BBT_SCANLASTPAGE is only useful on paths where we >> write bad block markers to OOB. We move the condition/calculation closer >> to the `write' operation and update the comment to more correctly >> describe the operation. > > Pushed this patch to l2-mtd.git because it seem to be good regardless of > whether your second patch gets merged or not, thanks. Sure, thanks. > Not sure about the second patch yet, though. OK, what are the remaining concerns? Last I saw, Shmulik was recommending we follow up on your suggestion to check the OOB marker before erasing/writing new bad block marker. I can try implementing this with "chip->block_bad" if this seems necessary. Also, there was mention of "lazy checking" of some sort. I feel like that's not central to this patch and might add unnecessary complexity and overhead. So, please comment which of the above are necessary additions to the second patch. I'm fine as is, but I can easily implement the first. The second would require more discussion and/or somebody else to do it. Brian
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index c6603d4..b902066 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -411,9 +411,6 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs) nand_erase_nand(mtd, &einfo, 0); } - if (chip->bbt_options & NAND_BBT_SCANLASTPAGE) - ofs += mtd->erasesize - mtd->writesize; - /* Get block number */ block = (int)(ofs >> chip->bbt_erase_shift); if (chip->bbt) @@ -424,11 +421,12 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs) ret = nand_update_bbt(mtd, ofs); else { struct mtd_oob_ops ops; + loff_t wr_ofs = ofs; nand_get_device(chip, mtd, FL_WRITING); /* - * Write to first two pages if necessary. If we write to more + * Write to first/last page(s) if necessary. If we write to more * than one location, the first error encountered quits the * procedure. */ @@ -442,11 +440,14 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs) ops.len = ops.ooblen = 1; } ops.mode = MTD_OPS_PLACE_OOB; + + if (chip->bbt_options & NAND_BBT_SCANLASTPAGE) + wr_ofs += mtd->erasesize - mtd->writesize; do { - ret = nand_do_write_oob(mtd, ofs, &ops); + ret = nand_do_write_oob(mtd, wr_ofs, &ops); i++; - ofs += mtd->writesize; + wr_ofs += mtd->writesize; } while (!ret && (chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
As nand_default_block_markbad() is becoming more complex, it helps to have code appear only in its relevant codepath(s). Here, the calculation of `ofs' based on NAND_BBT_SCANLASTPAGE is only useful on paths where we write bad block markers to OOB. We move the condition/calculation closer to the `write' operation and update the comment to more correctly describe the operation. The variable `wr_ofs' is also used to help isolate our calculation of the "write" offset from the usage of `ofs' to represent the eraseblock offset. This will become useful when we reorder operations in the next patch. This patch should make no functional change. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/nand/nand_base.c | 13 +++++++------ 1 files changed, 7 insertions(+), 6 deletions(-)