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

[Bean Huo 霍斌斌 (beanhuo)]

> + 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;
> 
Here will overload the 'priv' field of mtdchar.c 
Please go thought mtdchar_ioctl() function.

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


For my knowledge , we don't directly mark this block be a bad block,
Just like ubi layer,this block also need to further testing and verify if
It is real bad block.right?

> >
> >  		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/