Re: [RESEND RESEND RESEND PATCH v2] mtd: nand_bbt: scan for next free bbt block if writing bbt fails

[Brian Norris]

+ Bean, who is looking at refactoring this driver. Perhaps he can review
this.

On Tue, Aug 25, 2015 at 12:49:26PM -0500, Xander Huff wrote:
> From: Ben Shelton <ben.shelton@xxxxxx>
> 
> 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.
> 
> Based on original code implemented by Jeff Westfahl
> <jeff.westfahl@xxxxxx>.
> 
> Signed-off-by: Ben Shelton <ben.shelton@xxxxxx>
> Reviewed-by: Jaeden Amero <jaeden.amero@xxxxxx>
> Suggested-by: Jeff Westfahl <jeff.westfahl@xxxxxx>
> Signed-off-by: Xander Huff <xander.huff@xxxxxx>
> ---
> This v2 patch is in reply to comments from Brian Norris on 7/22/13. See
> the following links for context:
> http://lists.infradead.org/pipermail/linux-mtd/2013-July/047596.html
> http://patchwork.ozlabs.org/patch/244324/

Wow, a blast from the past...

> ---
>  drivers/mtd/nand/nand_base.c |  4 ++++
>  drivers/mtd/nand/nand_bbt.c  | 34 ++++++++++++++++++++++++++++++++--
>  include/linux/mtd/nand.h     |  7 +++++++
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index ceb68ca..48299dc 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2761,6 +2761,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
>  		pr_debug("%s: device is write protected!\n",
>  				__func__);
>  		instr->state = MTD_ERASE_FAILED;
> +		instr->priv = NAND_ERASE_WRITE_PROTECTED;

I'm pretty sure this is an illegal overloading of the 'priv' field.
Notice how ioctl(MEMERASE64) uses this field in mtdchard.c. So I suspect
you've broken char access to /dev/mtdX. Try 'flash_erase' from
mtd-utils, for instance.

>  		goto erase_exit;
>  	}
>  
> @@ -2776,6 +2777,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
>  			pr_warn("%s: attempt to erase a bad block at page 0x%08x\n",
>  				    __func__, page);
>  			instr->state = MTD_ERASE_FAILED;
> +			instr->priv = NAND_ERASE_BAD_BLOCK;
>  			goto erase_exit;
>  		}
>  
> @@ -2802,6 +2804,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
>  			pr_debug("%s: failed erase, page 0x%08x\n",
>  					__func__, page);
>  			instr->state = MTD_ERASE_FAILED;
> +			instr->priv = NAND_ERASE_BLOCK_ERASE_FAILED;
>  			instr->fail_addr =
>  				((loff_t)page << chip->page_shift);
>  			goto erase_exit;
> @@ -2819,6 +2822,7 @@ int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
>  		}
>  	}
>  	instr->state = MTD_ERASE_DONE;
> +	instr->priv = NAND_ERASE_OK;
>  
>  erase_exit:
>  
> diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
> index 63a1a36..09f9e62 100644
> --- a/drivers/mtd/nand/nand_bbt.c
> +++ b/drivers/mtd/nand/nand_bbt.c
> @@ -662,6 +662,7 @@ static int write_bbt(struct mtd_info *mtd, uint8_t *buf,
>  			page = td->pages[chip];
>  			goto write;
>  		}
> +next:
>  
>  		/*
>  		 * Automatic placement of the bad block table. Search direction
> @@ -787,13 +788,42 @@ 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)
> +		if (res == -EIO && einfo.state == MTD_ERASE_FAILED
> +		    && einfo.priv == NAND_ERASE_BLOCK_ERASE_FAILED) {

Do you actually need that last condition? What's wrong with the first
two?

> +			/* This block is bad. Mark it as such and see if
> +			 * there's another block available in the BBT area. */
> +			int block = page >>
> +				(this->bbt_erase_shift - this->page_shift);
> +			pr_info("nand_bbt: failed to erase block %d when writing BBT\n",
> +				block);
> +			bbt_mark_entry(this, block, BBT_BLOCK_WORN);
> +
> +			res = this->block_markbad(mtd, block);
> +			if (res)
> +				pr_warn("nand_bbt: error %d while marking block %d bad\n",
> +					res, block);
> +			goto next;
> +		} else if (res < 0)
>  			goto outerr;
>  
>  		res = scan_write_bbt(mtd, to, len, buf,
>  				td->options & NAND_BBT_NO_OOB ? NULL :
>  				&buf[len]);
> -		if (res < 0)
> +		if (res == -EIO) {
> +			/* This block is bad. Mark it as such and see if
> +			 * there's another block available in the BBT area. */
> +			int block = page >>
> +				(this->bbt_erase_shift - this->page_shift);
> +			pr_info("nand_bbt: failed to erase block %d when writing BBT\n",
> +				block);
> +			bbt_mark_entry(this, block, BBT_BLOCK_WORN);
> +
> +			res = this->block_markbad(mtd, block);
> +			if (res)
> +				pr_warn("nand_bbt: error %d while marking block %d bad\n",
> +					res, block);
> +			goto next;
> +		} else if (res < 0)
>  			goto outerr;
>  
>  		pr_info("Bad block table written to 0x%012llx, version 0x%02X\n",
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 272f429..86e11f6 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -1030,4 +1030,11 @@ struct nand_sdr_timings {
>  
>  /* get timing characteristics from ONFI timing mode. */
>  const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
> +
> +/* reasons for erase failures */
> +#define NAND_ERASE_OK			0
> +#define NAND_ERASE_WRITE_PROTECTED	1
> +#define NAND_ERASE_BAD_BLOCK		2
> +#define NAND_ERASE_BLOCK_ERASE_FAILED	3

Why exactly do you need these statuses? I thought the existing error
codes were sufficient..

> +
>  #endif /* __LINUX_MTD_NAND_H */

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/