Patchwork [v4,1/2] mtd: nand: move SCANLASTPAGE handling to the correct code block

login
register
mail settings
Submitter Brian Norris
Date Jan. 21, 2012, 4:38 a.m.
Message ID <1327120684-7066-2-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/137153/
State Accepted
Commit df698621a5b39ced5ce527444cafd50c6fdc186d
Headers show

Comments

Brian Norris - Jan. 21, 2012, 4:38 a.m.
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(-)
Bityutskiy, Artem - Jan. 27, 2012, 2:56 p.m.
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.
Brian Norris - Jan. 28, 2012, 8:09 p.m.
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

Patch

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