[PATCH v6 1/2] mtd: nand_bbt: Move BBT block selection logic out of write_bbt()

[PATCH v6 1/2] mtd: nand_bbt: Move BBT block selection logic out of write_bbt()

[Kyle Roeschley]

From: Boris Brezillon <boris.brezillon@free-electrons.com>

This clarifies the write_bbt() by removing the write label and clarifying
the error/exit path.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
v6: Split functionality of write_bbt out into new functions

v5: De-duplicate bad block handling

v4: Don't ignore write protection while marking bad BBT blocks
    Correctly call block_markbad
    Minor cleanups

v3: Don't overload mtd->priv
    Keep nand_erase_nand from erroring on protected BBT blocks

v2: Mark OOB area in each block as well as BBT
    Avoid marking read-only, bad address, or known bad blocks as bad

 drivers/mtd/nand/nand_bbt.c | 108 ++++++++++++++++++++++++++++----------------
 1 file changed, 70 insertions(+), 38 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 2fbb523..19f97e9 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -605,6 +605,69 @@ static void search_read_bbts(struct mtd_info *mtd, uint8_t *buf,
 }
 
 /**
+ * get_bbt_block - Get the first valid eraseblock suitable to store a BBT
+ * @this: the NAND device
+ * @td: the BBT description
+ * @md: the mirror BBT descriptor
+ * @chip: the CHIP selector
+ *
+ * This functions returns a positive block number pointing a valid eraseblock
+ * suitable to store a BBT (i.e. in the range reserved for BBT), or -ENOSPC if
+ * all blocks are already used of marked bad. If td->pages[chip] was already
+ * pointing to a valid block we re-use it, otherwise we search for the next
+ * valid one.
+ */
+static int get_bbt_block(struct nand_chip *this, struct nand_bbt_descr *td,
+			 struct nand_bbt_descr *md, int chip)
+{
+	int startblock, dir, page, numblocks, i;
+
+	/*
+	 * There was already a version of the table, reuse the page. This
+	 * applies for absolute placement too, as we have the page number in
+	 * td->pages.
+	 */
+	if (td->pages[chip] != -1)
+		return td->pages[chip] >>
+				(this->bbt_erase_shift - this->page_shift);
+
+	numblocks = (int)(this->chipsize >> this->bbt_erase_shift);
+	if (!(td->options & NAND_BBT_PERCHIP))
+		numblocks *= this->numchips;
+
+	/*
+	 * Automatic placement of the bad block table. Search direction
+	 * top -> down?
+	 */
+	if (td->options & NAND_BBT_LASTBLOCK) {
+		startblock = numblocks * (chip + 1) - 1;
+		dir = -1;
+	} else {
+		startblock = chip * numblocks;
+		dir = 1;
+	}
+
+	for (i = 0; i < td->maxblocks; i++) {
+		int block = startblock + dir * i;
+
+		/* Check, if the block is bad */
+		switch (bbt_get_entry(this, block)) {
+		case BBT_BLOCK_WORN:
+		case BBT_BLOCK_FACTORY_BAD:
+			continue;
+		}
+
+		page = block <<	(this->bbt_erase_shift - this->page_shift);
+
+		/* Check, if the block is used by the mirror table */
+		if (!md || md->pages[chip] != page)
+			return block;
+	}
+
+	return -ENOSPC;
+}
+
+/**
  * write_bbt - [GENERIC] (Re)write the bad block table
  * @mtd: MTD device structure
  * @buf: temporary buffer
@@ -621,7 +684,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 	struct nand_chip *this = mtd_to_nand(mtd);
 	struct erase_info einfo;
 	int i, res, chip = 0;
-	int bits, startblock, dir, page, offs, numblocks, sft, sftmsk;
+	int bits, page, offs, numblocks, sft, sftmsk;
 	int nrchips, pageoffs, ooboffs;
 	uint8_t msk[4];
 	uint8_t rcode = td->reserved_block_code;
@@ -653,45 +716,14 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 
 	/* Loop through the chips */
 	for (; chip < nrchips; chip++) {
-		/*
-		 * There was already a version of the table, reuse the page
-		 * This applies for absolute placement too, as we have the
-		 * page nr. in td->pages.
-		 */
-		if (td->pages[chip] != -1) {
-			page = td->pages[chip];
-			goto write;
-		}
-
-		/*
-		 * Automatic placement of the bad block table. Search direction
-		 * top -> down?
-		 */
-		if (td->options & NAND_BBT_LASTBLOCK) {
-			startblock = numblocks * (chip + 1) - 1;
-			dir = -1;
-		} else {
-			startblock = chip * numblocks;
-			dir = 1;
-		}
+		int block;
 
-		for (i = 0; i < td->maxblocks; i++) {
-			int block = startblock + dir * i;
-			/* Check, if the block is bad */
-			switch (bbt_get_entry(this, block)) {
-			case BBT_BLOCK_WORN:
-			case BBT_BLOCK_FACTORY_BAD:
-				continue;
-			}
-			page = block <<
-				(this->bbt_erase_shift - this->page_shift);
-			/* Check, if the block is used by the mirror table */
-			if (!md || md->pages[chip] != page)
-				goto write;
+		block = get_bbt_block(this, td, md, chip);
+		if (block < 0) {
+			pr_err("No space left to write bad block table\n");
+			res = block;
+			goto outerr;
 		}
-		pr_err("No space left to write bad block table\n");
-		return -ENOSPC;
-	write:
 
 		/* Set up shift count and masks for the flash table */
 		bits = td->options & NAND_BBT_NRBITS_MSK;
-- 
2.8.1

[PATCH v6 2/2] mtd: nand_bbt: scan for next free bbt block if writing bbt fails

[Kyle Roeschley]

If erasing or writing the BBT fails, we should mark the current BBT
block as bad and use the BBT descriptor to scan for the next available
unused block in the BBT. We should only return a failure if there isn't
any space left.

Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Suggested-by: Jeff Westfahl <jeff.westfahl@ni.com>
---
 drivers/mtd/nand/nand_bbt.c | 57 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
index 19f97e9..fdf9d90f 100644
--- a/drivers/mtd/nand/nand_bbt.c
+++ b/drivers/mtd/nand/nand_bbt.c
@@ -668,6 +668,37 @@ static int get_bbt_block(struct nand_chip *this, struct nand_bbt_descr *td,
 }
 
 /**
+ * mark_bbt_block_bad - Mark one of the block reserved for BBT bad
+ * @this: the NAND device
+ * @td: the BBT description
+ * @md: the mirror BBT descriptor
+ * @chip: the CHIP selector
+ *
+ * Blocks reserved for BBT can become bad. This functions is an helper to mark
+ * such blocks as bad. It takes care of updating the in-memory BBT, marking the
+ * block as bad using a bad block marker and invalidating the associated
+ * td->pages[] entry.
+ */
+static void mark_bbt_block_bad(struct nand_chip *this,
+			       struct nand_bbt_descr *td,
+			       int chip, int block)
+{
+	struct mtd_info *mtd = nand_to_mtd(this);
+	loff_t to;
+	int res;
+
+	bbt_mark_entry(this, block, BBT_BLOCK_WORN);
+
+	to = (loff_t)block << this->bbt_erase_shift;
+	res = this->block_markbad(mtd, to);
+	if (res)
+		pr_warn("nand_bbt: error %d while marking block %d bad\n",
+			res, block);
+
+	td->pages[chip] = -1;
+}
+
+/**
  * write_bbt - [GENERIC] (Re)write the bad block table
  * @mtd: MTD device structure
  * @buf: temporary buffer
@@ -715,7 +746,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 	}
 
 	/* Loop through the chips */
-	for (; chip < nrchips; chip++) {
+	while (chip < nrchips) {
 		int block;
 
 		block = get_bbt_block(this, td, md, chip);
@@ -725,6 +756,12 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 			goto outerr;
 		}
 
+		/*
+		 * get_bbt_block() returns a block number, shift the value to
+		 * get a page number.
+		 */
+		page = block << (this->bbt_erase_shift - this->page_shift);
+
 		/* Set up shift count and masks for the flash table */
 		bits = td->options & NAND_BBT_NRBITS_MSK;
 		msk[2] = ~rcode;
@@ -819,20 +856,28 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
 		einfo.addr = to;
 		einfo.len = 1 << this->bbt_erase_shift;
 		res = nand_erase_nand(mtd, &einfo, 1);
-		if (res < 0)
-			goto outerr;
+		if (res < 0) {
+			pr_warn("nand_bbt: error while erasing BBT block %d\n",
+				res);
+			mark_bbt_block_bad(this, td, chip, block);
+			continue;
+		}
 
 		res = scan_write_bbt(mtd, to, len, buf,
 				td->options & NAND_BBT_NO_OOB ? NULL :
 				&buf[len]);
-		if (res < 0)
-			goto outerr;
+		if (res < 0) {
+			pr_warn("nand_bbt: error while writing bad block table %d\n",
+				res);
+			mark_bbt_block_bad(this, td, chip, block);
+			continue;
+		}
 
 		pr_info("Bad block table written to 0x%012llx, version 0x%02X\n",
 			 (unsigned long long)to, td->version[chip]);
 
 		/* Mark it as used */
-		td->pages[chip] = page;
+		td->pages[chip++] = page;
 	}
 	return 0;
 
-- 
2.8.1

Re: [PATCH v6 1/2] mtd: nand_bbt: Move BBT block selection logic out of write_bbt()

[Boris Brezillon]

Hi Kyle,

On Fri, 12 Aug 2016 12:54:49 -0500
Kyle Roeschley <kyle.roeschley@ni.com> wrote:

> From: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> This clarifies the write_bbt() by removing the write label and clarifying
> the error/exit path.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Just want to make sure you actually tested those patches, because I
didn't :).

Can you add your Tested-by on this one and confirm you've tested patch
2 as well.

Thanks,

Boris

> ---
> v6: Split functionality of write_bbt out into new functions
> 
> v5: De-duplicate bad block handling
> 
> v4: Don't ignore write protection while marking bad BBT blocks
>     Correctly call block_markbad
>     Minor cleanups
> 
> v3: Don't overload mtd->priv
>     Keep nand_erase_nand from erroring on protected BBT blocks
> 
> v2: Mark OOB area in each block as well as BBT
>     Avoid marking read-only, bad address, or known bad blocks as bad
> 
>  drivers/mtd/nand/nand_bbt.c | 108 ++++++++++++++++++++++++++++----------------
>  1 file changed, 70 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
> index 2fbb523..19f97e9 100644
> --- a/drivers/mtd/nand/nand_bbt.c
> +++ b/drivers/mtd/nand/nand_bbt.c
> @@ -605,6 +605,69 @@ static void search_read_bbts(struct mtd_info *mtd, uint8_t *buf,
>  }
>  
>  /**
> + * get_bbt_block - Get the first valid eraseblock suitable to store a BBT
> + * @this: the NAND device
> + * @td: the BBT description
> + * @md: the mirror BBT descriptor
> + * @chip: the CHIP selector
> + *
> + * This functions returns a positive block number pointing a valid eraseblock
> + * suitable to store a BBT (i.e. in the range reserved for BBT), or -ENOSPC if
> + * all blocks are already used of marked bad. If td->pages[chip] was already
> + * pointing to a valid block we re-use it, otherwise we search for the next
> + * valid one.
> + */
> +static int get_bbt_block(struct nand_chip *this, struct nand_bbt_descr *td,
> +			 struct nand_bbt_descr *md, int chip)
> +{
> +	int startblock, dir, page, numblocks, i;
> +
> +	/*
> +	 * There was already a version of the table, reuse the page. This
> +	 * applies for absolute placement too, as we have the page number in
> +	 * td->pages.
> +	 */
> +	if (td->pages[chip] != -1)
> +		return td->pages[chip] >>
> +				(this->bbt_erase_shift - this->page_shift);
> +
> +	numblocks = (int)(this->chipsize >> this->bbt_erase_shift);
> +	if (!(td->options & NAND_BBT_PERCHIP))
> +		numblocks *= this->numchips;
> +
> +	/*
> +	 * Automatic placement of the bad block table. Search direction
> +	 * top -> down?
> +	 */
> +	if (td->options & NAND_BBT_LASTBLOCK) {
> +		startblock = numblocks * (chip + 1) - 1;
> +		dir = -1;
> +	} else {
> +		startblock = chip * numblocks;
> +		dir = 1;
> +	}
> +
> +	for (i = 0; i < td->maxblocks; i++) {
> +		int block = startblock + dir * i;
> +
> +		/* Check, if the block is bad */
> +		switch (bbt_get_entry(this, block)) {
> +		case BBT_BLOCK_WORN:
> +		case BBT_BLOCK_FACTORY_BAD:
> +			continue;
> +		}
> +
> +		page = block <<	(this->bbt_erase_shift - this->page_shift);
> +
> +		/* Check, if the block is used by the mirror table */
> +		if (!md || md->pages[chip] != page)
> +			return block;
> +	}
> +
> +	return -ENOSPC;
> +}
> +
> +/**
>   * write_bbt - [GENERIC] (Re)write the bad block table
>   * @mtd: MTD device structure
>   * @buf: temporary buffer
> @@ -621,7 +684,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
>  	struct nand_chip *this = mtd_to_nand(mtd);
>  	struct erase_info einfo;
>  	int i, res, chip = 0;
> -	int bits, startblock, dir, page, offs, numblocks, sft, sftmsk;
> +	int bits, page, offs, numblocks, sft, sftmsk;
>  	int nrchips, pageoffs, ooboffs;
>  	uint8_t msk[4];
>  	uint8_t rcode = td->reserved_block_code;
> @@ -653,45 +716,14 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
>  
>  	/* Loop through the chips */
>  	for (; chip < nrchips; chip++) {
> -		/*
> -		 * There was already a version of the table, reuse the page
> -		 * This applies for absolute placement too, as we have the
> -		 * page nr. in td->pages.
> -		 */
> -		if (td->pages[chip] != -1) {
> -			page = td->pages[chip];
> -			goto write;
> -		}
> -
> -		/*
> -		 * Automatic placement of the bad block table. Search direction
> -		 * top -> down?
> -		 */
> -		if (td->options & NAND_BBT_LASTBLOCK) {
> -			startblock = numblocks * (chip + 1) - 1;
> -			dir = -1;
> -		} else {
> -			startblock = chip * numblocks;
> -			dir = 1;
> -		}
> +		int block;
>  
> -		for (i = 0; i < td->maxblocks; i++) {
> -			int block = startblock + dir * i;
> -			/* Check, if the block is bad */
> -			switch (bbt_get_entry(this, block)) {
> -			case BBT_BLOCK_WORN:
> -			case BBT_BLOCK_FACTORY_BAD:
> -				continue;
> -			}
> -			page = block <<
> -				(this->bbt_erase_shift - this->page_shift);
> -			/* Check, if the block is used by the mirror table */
> -			if (!md || md->pages[chip] != page)
> -				goto write;
> +		block = get_bbt_block(this, td, md, chip);
> +		if (block < 0) {
> +			pr_err("No space left to write bad block table\n");
> +			res = block;
> +			goto outerr;
>  		}
> -		pr_err("No space left to write bad block table\n");
> -		return -ENOSPC;
> -	write:
>  
>  		/* Set up shift count and masks for the flash table */
>  		bits = td->options & NAND_BBT_NRBITS_MSK;

Re: [PATCH v6 2/2] mtd: nand_bbt: scan for next free bbt block if writing bbt fails

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

Hi Kyle,

[auto build test WARNING on mtd/master]
[also build test WARNING on v4.8-rc1 next-20160812]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kyle-Roeschley/mtd-nand_bbt-Move-BBT-block-selection-logic-out-of-write_bbt/20160813-015713
base:   git://git.infradead.org/linux-mtd.git master
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

>> drivers/mtd/nand/nand_bbt.c:685: warning: No description found for parameter 'block'
>> drivers/mtd/nand/nand_bbt.c:685: warning: Excess function parameter 'md' description in 'mark_bbt_block_bad'
   drivers/mtd/nand/nand_bbt.c:1: warning: no structured comments found
>> drivers/mtd/nand/nand_bbt.c:685: warning: No description found for parameter 'block'
>> drivers/mtd/nand/nand_bbt.c:685: warning: Excess function parameter 'md' description in 'mark_bbt_block_bad'

vim +/block +685 drivers/mtd/nand/nand_bbt.c

   669	
   670	/**
   671	 * mark_bbt_block_bad - Mark one of the block reserved for BBT bad
   672	 * @this: the NAND device
   673	 * @td: the BBT description
   674	 * @md: the mirror BBT descriptor
   675	 * @chip: the CHIP selector
   676	 *
   677	 * Blocks reserved for BBT can become bad. This functions is an helper to mark
   678	 * such blocks as bad. It takes care of updating the in-memory BBT, marking the
   679	 * block as bad using a bad block marker and invalidating the associated
   680	 * td->pages[] entry.
   681	 */
   682	static void mark_bbt_block_bad(struct nand_chip *this,
   683				       struct nand_bbt_descr *td,
   684				       int chip, int block)
 > 685	{
   686		struct mtd_info *mtd = nand_to_mtd(this);
   687		loff_t to;
   688		int res;
   689	
   690		bbt_mark_entry(this, block, BBT_BLOCK_WORN);
   691	
   692		to = (loff_t)block << this->bbt_erase_shift;
   693		res = this->block_markbad(mtd, to);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6366 bytes --]

Re: [PATCH v6 1/2] mtd: nand_bbt: Move BBT block selection logic out of write_bbt()

[Kyle Roeschley]

Hi Boris,

On Fri, Aug 12, 2016 at 09:15:03PM +0200, Boris Brezillon wrote:
> Hi Kyle,
> 
> On Fri, 12 Aug 2016 12:54:49 -0500
> Kyle Roeschley <kyle.roeschley@ni.com> wrote:
> 
> > From: Boris Brezillon <boris.brezillon@free-electrons.com>
> > 
> > This clarifies the write_bbt() by removing the write label and clarifying
> > the error/exit path.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> Just want to make sure you actually tested those patches, because I
> didn't :).
> 
> Can you add your Tested-by on this one and confirm you've tested patch
> 2 as well.

Whoops, I goofed and only tested with both patches applied. Thanks for the
catch. I'll go ahead and test the first alone and submit a v7.

Regards,

-- 
Kyle Roeschley
Software Engineer
National Instruments