[RFC] mtd: add MEMSETGOODBLOCK ioctl

[RFC] mtd: add MEMSETGOODBLOCK ioctl

[Jon Povey]

Adds the MEMSETGOODBLOCK ioctl, inverse operation of MEMSETBADBLOCK for
times when you really are sure that a block is good, but was marked bad
in error.

WARNING: This ioctl should not be casually used, you can't magically
fix unreliable blocks, and causing the system to try and use them may
cause you problems. Manufacturer-marked bad blocks may be forgotten
and impossible to find again later. Be careful.

This ioctl is for situations where you know what you are doing, for
example your bootloader has to be written with a different OOB layout
and scanned as bad when the BBT was generated, but you know it's good
and want to rewrite it from Linux.

Signed-off-by: Jon Povey <jon.povey@racelogic.co.uk>
---
This is a request for comment on this approach to bad blocks, this patch
only considers the nand driver and only versions with a BBT on flash.

This works for me on davinci DM355 with 2K page NAND flash.

 drivers/mtd/mtdchar.c        |   13 +++++++++
 drivers/mtd/mtdpart.c        |   18 ++++++++++++
 drivers/mtd/nand/nand_base.c |   60 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h      |    1 +
 include/linux/mtd/nand.h     |    2 +
 include/mtd/mtd-abi.h        |    1 +
 6 files changed, 95 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index a825002..20250ae 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -750,6 +750,19 @@ static int mtd_ioctl(struct file *file, u_int cmd, u_long arg)
 		break;
 	}
 
+	case MEMSETGOODBLOCK:
+	{
+		loff_t offs;
+
+		if (copy_from_user(&offs, argp, sizeof(loff_t)))
+			return -EFAULT;
+		if (!mtd->block_markgood)
+			ret = -EOPNOTSUPP;
+		else
+			return mtd->block_markgood(mtd, offs);
+		break;
+	}
+
 #ifdef CONFIG_HAVE_MTD_OTP
 	case OTPSELECT:
 	{
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 0090690..cd03c29 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -327,6 +327,22 @@ static int part_block_markbad(struct mtd_info *mtd, loff_t ofs)
 	return res;
 }
 
+static int part_block_markgood(struct mtd_info *mtd, loff_t ofs)
+{
+	struct mtd_part *part = PART(mtd);
+	int res;
+
+	if (!(mtd->flags & MTD_WRITEABLE))
+		return -EROFS;
+	if (ofs >= mtd->size)
+		return -EINVAL;
+	ofs += part->offset;
+	res = part->master->block_markgood(part->master, ofs);
+	if (!res)
+		mtd->ecc_stats.badblocks--;
+	return res;
+}
+
 /*
  * This function unregisters and destroy all slave MTD objects which are
  * attached to the given master MTD object.
@@ -464,6 +480,8 @@ static struct mtd_part *add_one_partition(struct mtd_info *master,
 		slave->mtd.block_isbad = part_block_isbad;
 	if (master->block_markbad)
 		slave->mtd.block_markbad = part_block_markbad;
+	if (master->block_markgood)
+		slave->mtd.block_markgood = part_block_markgood;
 	slave->mtd.erase = part_erase;
 	slave->master = master;
 	slave->offset = part->offset;
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index b3469a7..c2c6211 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -444,6 +444,40 @@ static int nand_default_block_markbad(struct mtd_info *mtd, loff_t ofs)
 }
 
 /**
+ * nand_default_block_markgood - [DEFAULT] mark a block good
+ * @mtd:	MTD device structure
+ * @ofs:	offset from device start
+ *
+ * This is the default implementation, which can be overridden by
+ * a hardware specific driver.
+*/
+static int nand_default_block_markgood(struct mtd_info *mtd, loff_t ofs)
+{
+	struct nand_chip *chip = mtd->priv;
+	int block, ret;
+
+	if (chip->options & NAND_BBT_SCANLASTPAGE)
+		ofs += mtd->erasesize - mtd->writesize;
+
+	/* Get block number */
+	block = (int)(ofs >> chip->bbt_erase_shift);
+	if (chip->bbt)
+		chip->bbt[block >> 2] &= ~(0x03 << ((block & 0x03) << 1));
+
+	/* Do we have a flash based bad block table ? */
+	if (chip->options & NAND_USE_FLASH_BBT) {
+		ret = nand_update_bbt(mtd, ofs);
+	} else {
+		WARN_ONCE(1, "Attempt to NAND markgood without flash BBT");
+		ret = -EOPNOTSUPP;
+	}
+	if (!ret)
+		mtd->ecc_stats.badblocks--;
+
+	return ret;
+}
+
+/**
  * nand_check_wp - [GENERIC] check if the chip is write protected
  * @mtd:	MTD device structure
  * Check, if the device is write protected
@@ -2736,6 +2770,29 @@ static int nand_block_markbad(struct mtd_info *mtd, loff_t ofs)
 }
 
 /**
+ * nand_block_markgood - [MTD Interface] Mark block at the given offset as good
+ * @mtd:	MTD device structure
+ * @ofs:	offset relative to mtd start
+ */
+static int nand_block_markgood(struct mtd_info *mtd, loff_t ofs)
+{
+	struct nand_chip *chip = mtd->priv;
+	int ret;
+
+	printk(KERN_INFO "markgood 0x%llx\n", ofs);
+	ret = nand_block_isbad(mtd, ofs);
+	if (ret < 0)
+		return ret;
+	else if (ret == 0) {
+		printk(KERN_INFO "is good, do nothing\n");
+		/* Good already, return success and do nothing */
+		return 0;
+	}
+
+	return chip->block_markgood(mtd, ofs);
+}
+
+/**
  * nand_suspend - [MTD Interface] Suspend the NAND flash
  * @mtd:	MTD device structure
  */
@@ -2788,6 +2845,8 @@ static void nand_set_defaults(struct nand_chip *chip, int busw)
 		chip->block_bad = nand_block_bad;
 	if (!chip->block_markbad)
 		chip->block_markbad = nand_default_block_markbad;
+	if (!chip->block_markgood)
+		chip->block_markgood = nand_default_block_markgood;
 	if (!chip->write_buf)
 		chip->write_buf = busw ? nand_write_buf16 : nand_write_buf;
 	if (!chip->read_buf)
@@ -3302,6 +3361,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	mtd->resume = nand_resume;
 	mtd->block_isbad = nand_block_isbad;
 	mtd->block_markbad = nand_block_markbad;
+	mtd->block_markgood = nand_block_markgood;
 
 	/* propagate ecc.layout to mtd_info */
 	mtd->ecclayout = chip->ecc.layout;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 8485e42..c71276f 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -237,6 +237,7 @@ struct mtd_info {
 	/* Bad block management functions */
 	int (*block_isbad) (struct mtd_info *mtd, loff_t ofs);
 	int (*block_markbad) (struct mtd_info *mtd, loff_t ofs);
+	int (*block_markgood) (struct mtd_info *mtd, loff_t ofs);
 
 	struct notifier_block reboot_notifier;  /* default mode before reboot */
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index e41d377..9744a51 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -332,6 +332,7 @@ struct nand_buffers {
  * @select_chip:	[REPLACEABLE] select chip nr
  * @block_bad:		[REPLACEABLE] check, if the block is bad
  * @block_markbad:	[REPLACEABLE] mark the block bad
+ * @block_markgood:	[REPLACEABLE] mark the block good
  * @cmd_ctrl:		[BOARDSPECIFIC] hardwarespecific funtion for controlling
  *			ALE/CLE/nCE. Also used to write command and address
  * @dev_ready:		[BOARDSPECIFIC] hardwarespecific function for accesing device ready/busy line
@@ -386,6 +387,7 @@ struct nand_chip {
 	void		(*select_chip)(struct mtd_info *mtd, int chip);
 	int		(*block_bad)(struct mtd_info *mtd, loff_t ofs, int getchip);
 	int		(*block_markbad)(struct mtd_info *mtd, loff_t ofs);
+	int		(*block_markgood)(struct mtd_info *mtd, loff_t ofs);
 	void		(*cmd_ctrl)(struct mtd_info *mtd, int dat,
 				    unsigned int ctrl);
 	int		(*dev_ready)(struct mtd_info *mtd);
diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index 4debb45..d32bea8 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -126,6 +126,7 @@ struct otp_info {
 #define MEMWRITEOOB64		_IOWR('M', 21, struct mtd_oob_buf64)
 #define MEMREADOOB64		_IOWR('M', 22, struct mtd_oob_buf64)
 #define MEMISLOCKED		_IOR('M', 23, struct erase_info_user)
+#define MEMSETGOODBLOCK		_IOW('M', 24, __kernel_loff_t)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace
-- 
1.6.3.3

Re: [RFC] mtd: add MEMSETGOODBLOCK ioctl

[Artem Bityutskiy]

On Wed, 2010-10-06 at 12:21 +0900, Jon Povey wrote:
> Adds the MEMSETGOODBLOCK ioctl, inverse operation of MEMSETBADBLOCK for
> times when you really are sure that a block is good, but was marked bad
> in error.
> 
> WARNING: This ioctl should not be casually used, you can't magically
> fix unreliable blocks, and causing the system to try and use them may
> cause you problems. Manufacturer-marked bad blocks may be forgotten
> and impossible to find again later. Be careful.
> 
> This ioctl is for situations where you know what you are doing, for
> example your bootloader has to be written with a different OOB layout
> and scanned as bad when the BBT was generated, but you know it's good
> and want to rewrite it from Linux.
> 
> Signed-off-by: Jon Povey <jon.povey@racelogic.co.uk>
> ---
> This is a request for comment on this approach to bad blocks, this patch
> only considers the nand driver and only versions with a BBT on flash.
> 
> This works for me on davinci DM355 with 2K page NAND flash.
> 
>  drivers/mtd/mtdchar.c        |   13 +++++++++
>  drivers/mtd/mtdpart.c        |   18 ++++++++++++
>  drivers/mtd/nand/nand_base.c |   60 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/mtd.h      |    1 +
>  include/linux/mtd/nand.h     |    2 +
>  include/mtd/mtd-abi.h        |    1 +
>  6 files changed, 95 insertions(+), 0 deletions(-)

Briefly looked - you do not have any locking. You should have some
nand_get_device() calls.

Also, did you investigate whether it is possible to distinguish between
factory-marked bad eraseblocks and user-marked bad eraseblocks?

Also, if this patch to go in, I'd really like to see some Reviewed-by
and Tested by. Or some good list of setups where you tested this
yourself and how you did this. Is this possible?

Let's CC Mike who was interested in this and Brian who was doing great
job in the BBT area recently and could review this patch.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

RE: [RFC] mtd: add MEMSETGOODBLOCK ioctl

[Jon Povey]

Just to make clear, this patch should NOT go in in the present form.
I am just posting it to see if some discussion starts about how this
should be done.

In the current form, it's just a hack that works for me.

Artem Bityutskiy wrote:
> On Wed, 2010-10-06 at 12:21 +0900, Jon Povey wrote:
>> Adds the MEMSETGOODBLOCK ioctl, inverse operation of MEMSETBADBLOCK
>> for times when you really are sure that a block is good, but was
>> marked bad in error.

> Briefly looked - you do not have any locking. You should have some
> nand_get_device() calls.

I pretty much copy-pasted from the existing SETBADBLOCK support, maybe
I missed something or maybe they are missing locking too.

> Also, did you investigate whether it is possible to
> distinguish between
> factory-marked bad eraseblocks and user-marked bad eraseblocks?

Yes, that can be done. They have different bits in the BBT. This ioctl
ignores them though, for my purposes sometimes MTD thinks blocks are
factory-bad when they are not. This ioctl says "I don't care why you
think this block is bad, I know better".

Thinking about this, to handle well in userland software,
MEMGETBADBLOCK should return more than just boolean BAD or GOOD, maybe
it should return different values for factory bad, reserved, went-bad.

The mtd-abi.h doesn't seem to have any spec, it looks like it comes
from nand_bbt.c:nand_isbad_bbt() which returns only 0 or 1.
Don't know if this could be quietly extended without breaking ABI.

> Also, if this patch to go in, I'd really like to see some Reviewed-by
> and Tested by. Or some good list of setups where you tested this
> yourself and how you did this. Is this possible?

Sure. But this isn't even ready for that kind of testing in my opinion.
Like I say it's a hack and conversation-starter.

> Let's CC Mike who was interested in this and Brian who was doing great
> job in the BBT area recently and could review this patch.



--
Jon Povey
jon.povey@racelogic.co.uk

Racelogic is a limited company registered in England. Registered number 2743719 .
Registered Office Unit 10, Swan Business Centre, Osier Way, Buckingham, Bucks, MK18 1TB .

The information contained in this electronic mail transmission is intended by Racelogic Ltd for the use of the named individual or entity to which it is directed and may contain information that is confidential or privileged. If you have received this electronic mail transmission in error, please delete it from your system without copying or forwarding it, and notify the sender of the error by reply email so that the sender's address records can be corrected. The views expressed by the sender of this communication do not necessarily represent those of Racelogic Ltd. Please note that Racelogic reserves the right to monitor e-mail communications passing through its network