[RESEND PATCH v2 0/3] mtd: use ONFI bad blocks per LUN to calculate UBI bad PEB limit

[RESEND PATCH v2 0/3] mtd: use ONFI bad blocks per LUN to calculate UBI bad PEB limit

[Zach Brown]

For ONFI-compliant NAND devices, the ONFI parameters report the maximum number
of bad blocks per LUN that will be encountered over the lifetime of the device,
so we can use that information to get a more accurate (and smaller) value for
the UBI bad PEB limit.

The ONFI parameter "maxiumum number of bad blocks per LUN" is the max number of
bad blocks that each individual LUN will ever ecounter. It is not the number of
bad blocks to reserve for the nand device per LUN in the device.

This means that in the worst case a UBI device spanning X LUNs will encounter
"maximum number of bad blocks per LUN" * X bad blocks. The implementation in
this patch assumes this worst case and allocates bad block accordingly.

These patches are ordered in terms of their dependencies, but ideally, all 3
would need to be applied for this to work as intended.

v1:
 * Changed commit message to address concerns from v1[1] about this patch set
   making best case assumptions.

[1]
http://lkml.iu.edu/hypermail/linux/kernel/1505.1/04822.html

Jeff Westfahl (3):
  mtd: introduce function max_bad_blocks
  mtd: nand: implement 'max_bad_blocks' mtd function
  mtd: ubi: use 'max_bad_blocks' to compute bad_peb_limit if available

 drivers/mtd/mtdpart.c        | 12 ++++++++++++
 drivers/mtd/nand/nand_base.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/mtd/ubi/build.c      |  9 +++++++++
 include/linux/mtd/mtd.h      |  1 +
 4 files changed, 56 insertions(+)

-- 
2.7.4

[RESEND PATCH v2 1/3] mtd: introduce function max_bad_blocks

[Zach Brown]

From: Jeff Westfahl <jeff.westfahl@ni.com>

If implemented, 'max_bad_blocks' returns the maximum number of bad
blocks to reserve for an MTD. An implementation for NAND is coming soon.

Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
Signed-off-by: Zach Brown <zach.brown@ni.com>
---
 drivers/mtd/mtdpart.c   | 12 ++++++++++++
 include/linux/mtd/mtd.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index fccdd49..565f0dd 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -349,6 +349,16 @@ static const struct mtd_ooblayout_ops part_ooblayout_ops = {
 	.free = part_ooblayout_free,
 };
 
+static int part_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
+{
+	struct mtd_part *part = mtd_to_part(mtd);
+
+	if ((len + ofs) > mtd->size)
+		return -EINVAL;
+	return part->master->_max_bad_blocks(part->master,
+					     ofs + part->offset, len);
+}
+
 static inline void free_partition(struct mtd_part *p)
 {
 	kfree(p->mtd.name);
@@ -481,6 +491,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
 	if (master->_put_device)
 		slave->mtd._put_device = part_put_device;
 
+	if (master->_max_bad_blocks)
+		slave->mtd._max_bad_blocks = part_max_bad_blocks;
 	slave->mtd._erase = part_erase;
 	slave->master = master;
 	slave->offset = part->offset;
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 13f8052..bd277eb 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -322,6 +322,7 @@ struct mtd_info {
 	int (*_block_isreserved) (struct mtd_info *mtd, loff_t ofs);
 	int (*_block_isbad) (struct mtd_info *mtd, loff_t ofs);
 	int (*_block_markbad) (struct mtd_info *mtd, loff_t ofs);
+	int (*_max_bad_blocks) (struct mtd_info *mtd, loff_t ofs, size_t len);
 	int (*_suspend) (struct mtd_info *mtd);
 	void (*_resume) (struct mtd_info *mtd);
 	void (*_reboot) (struct mtd_info *mtd);
-- 
2.7.4

Re: [RESEND PATCH v2 1/3] mtd: introduce function max_bad_blocks

[Boris Brezillon]

On Thu, 27 Oct 2016 14:14:00 -0500
Zach Brown <zach.brown@ni.com> wrote:

> From: Jeff Westfahl <jeff.westfahl@ni.com>
> 
> If implemented, 'max_bad_blocks' returns the maximum number of bad
> blocks to reserve for an MTD. An implementation for NAND is coming soon.
> 
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> Signed-off-by: Zach Brown <zach.brown@ni.com>
> ---
>  drivers/mtd/mtdpart.c   | 12 ++++++++++++
>  include/linux/mtd/mtd.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index fccdd49..565f0dd 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -349,6 +349,16 @@ static const struct mtd_ooblayout_ops part_ooblayout_ops = {
>  	.free = part_ooblayout_free,
>  };
>  
> +static int part_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
> +{
> +	struct mtd_part *part = mtd_to_part(mtd);
> +
> +	if ((len + ofs) > mtd->size)
> +		return -EINVAL;
> +	return part->master->_max_bad_blocks(part->master,
> +					     ofs + part->offset, len);
> +}
> +
>  static inline void free_partition(struct mtd_part *p)
>  {
>  	kfree(p->mtd.name);
> @@ -481,6 +491,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
>  	if (master->_put_device)
>  		slave->mtd._put_device = part_put_device;
>  
> +	if (master->_max_bad_blocks)
> +		slave->mtd._max_bad_blocks = part_max_bad_blocks;
>  	slave->mtd._erase = part_erase;
>  	slave->master = master;
>  	slave->offset = part->offset;
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index 13f8052..bd277eb 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -322,6 +322,7 @@ struct mtd_info {
>  	int (*_block_isreserved) (struct mtd_info *mtd, loff_t ofs);
>  	int (*_block_isbad) (struct mtd_info *mtd, loff_t ofs);
>  	int (*_block_markbad) (struct mtd_info *mtd, loff_t ofs);
> +	int (*_max_bad_blocks) (struct mtd_info *mtd, loff_t ofs, size_t len);

Please provide an helper function around this hook, so that you can
call the helper in patch 3 and won't have to test the ->_max_bad_blocks
field directly.

This helper should return -ENOTSUPP if ->_max_bad_blocks is not
implemented.

>  	int (*_suspend) (struct mtd_info *mtd);
>  	void (*_resume) (struct mtd_info *mtd);
>  	void (*_reboot) (struct mtd_info *mtd);

[RESEND PATCH v2 2/3] mtd: nand: implement 'max_bad_blocks' mtd function

[Zach Brown]

From: Jeff Westfahl <jeff.westfahl@ni.com>

Implement the new mtd function 'max_bad_blocks'. Use the ONFI parameter
page to find the maximum bad blocks to reserve for an MTD, taking into
account how many LUNs the MTD spans.

Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
Signed-off-by: Zach Brown <zach.brown@ni.com>
---
 drivers/mtd/nand/nand_base.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index e5718e5..ac08224 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3226,6 +3226,39 @@ static int nand_block_markbad(struct mtd_info *mtd, loff_t ofs)
 }
 
 /**
+ * nand_max_bad_blocks - [MTD Interface] Max number of bad blocks for an mtd
+ * @mtd: MTD device structure
+ * @ofs: offset relative to mtd start
+ * @len: length of mtd
+ */
+static int nand_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	uint32_t part_start_block;
+	uint32_t part_end_block;
+	uint32_t part_start_lun;
+	uint32_t part_end_lun;
+
+	/* ONFI is used to determine the maximum bad block count. */
+	if (!chip->onfi_version)
+		return -ENOTSUPP;
+
+	/* Get the start and end of the partition in erase blocks. */
+	part_start_block = mtd_div_by_eb(ofs, mtd);
+	part_end_block = mtd_div_by_eb(len, mtd) + part_start_block - 1;
+
+	/* Get the start and end LUNs of the partition. */
+	part_start_lun = part_start_block / chip->onfi_params.blocks_per_lun;
+	part_end_lun = part_end_block / chip->onfi_params.blocks_per_lun;
+
+	/* Look up the bad blocks per unit and multiply by the number of units
+	 * that the partition spans.
+	 */
+	return chip->onfi_params.bb_per_lun *
+		(part_end_lun - part_start_lun + 1);
+}
+
+/**
  * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
  * @mtd: MTD device structure
  * @chip: nand chip info structure
@@ -4743,6 +4776,7 @@ int nand_scan_tail(struct mtd_info *mtd)
 	mtd->_block_isreserved = nand_block_isreserved;
 	mtd->_block_isbad = nand_block_isbad;
 	mtd->_block_markbad = nand_block_markbad;
+	mtd->_max_bad_blocks = nand_max_bad_blocks;
 	mtd->writebufsize = mtd->writesize;
 
 	/*
-- 
2.7.4

Re: [RESEND PATCH v2 2/3] mtd: nand: implement 'max_bad_blocks' mtd function

[Boris Brezillon]

On Thu, 27 Oct 2016 14:14:01 -0500
Zach Brown <zach.brown@ni.com> wrote:

> From: Jeff Westfahl <jeff.westfahl@ni.com>
> 
> Implement the new mtd function 'max_bad_blocks'. Use the ONFI parameter
> page to find the maximum bad blocks to reserve for an MTD, taking into
> account how many LUNs the MTD spans.
> 
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> Signed-off-by: Zach Brown <zach.brown@ni.com>
> ---
>  drivers/mtd/nand/nand_base.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index e5718e5..ac08224 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3226,6 +3226,39 @@ static int nand_block_markbad(struct mtd_info *mtd, loff_t ofs)
>  }
>  
>  /**
> + * nand_max_bad_blocks - [MTD Interface] Max number of bad blocks for an mtd
> + * @mtd: MTD device structure
> + * @ofs: offset relative to mtd start
> + * @len: length of mtd
> + */
> +static int nand_max_bad_blocks(struct mtd_info *mtd, loff_t ofs, size_t len)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	uint32_t part_start_block;
> +	uint32_t part_end_block;
> +	uint32_t part_start_lun;
> +	uint32_t part_end_lun;
> +
> +	/* ONFI is used to determine the maximum bad block count. */
> +	if (!chip->onfi_version)
> +		return -ENOTSUPP;
> +
> +	/* Get the start and end of the partition in erase blocks. */
> +	part_start_block = mtd_div_by_eb(ofs, mtd);
> +	part_end_block = mtd_div_by_eb(len, mtd) + part_start_block - 1;
> +
> +	/* Get the start and end LUNs of the partition. */
> +	part_start_lun = part_start_block / chip->onfi_params.blocks_per_lun;
> +	part_end_lun = part_end_block / chip->onfi_params.blocks_per_lun;
> +
> +	/* Look up the bad blocks per unit and multiply by the number of units
> +	 * that the partition spans.
> +	 */
> +	return chip->onfi_params.bb_per_lun *
> +		(part_end_lun - part_start_lun + 1);

Well, it's a good start, but I'd like to have something that works even
for non-ONFI chips. How about adding a field in nand_chip and filling
this field when the ONFI param page is retrieved/parsed. This way we
can easily extend the implementation for full-id entries in the
nand_ids table or for JEDEC compliant chips (assuming the JEDEC
standard provides such information).

> +}
> +
> +/**
>   * nand_onfi_set_features- [REPLACEABLE] set features for ONFI nand
>   * @mtd: MTD device structure
>   * @chip: nand chip info structure
> @@ -4743,6 +4776,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>  	mtd->_block_isreserved = nand_block_isreserved;
>  	mtd->_block_isbad = nand_block_isbad;
>  	mtd->_block_markbad = nand_block_markbad;
> +	mtd->_max_bad_blocks = nand_max_bad_blocks;
>  	mtd->writebufsize = mtd->writesize;
>  
>  	/*

[RESEND PATCH v2 3/3] mtd: ubi: use 'max_bad_blocks' to compute bad_peb_limit if available

[Zach Brown]

From: Jeff Westfahl <jeff.westfahl@ni.com>

Use the MTD function 'max_bad_blocks' to compute the UBI bad_peb_limit,
if the function is implemented for an MTD and doesn't return an error.

Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
Signed-off-by: Zach Brown <zach.brown@ni.com>
---
 drivers/mtd/ubi/build.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 85d54f3..0648863 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -584,6 +584,15 @@ static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024)
 	int limit, device_pebs;
 	uint64_t device_size;
 
+	/* If the MTD provides a max_bad_blocks function, use that value. Fall
+	 * back to max_beb_per1024 if that function returns an error.
+	 */
+	if (ubi->mtd->_max_bad_blocks) {
+		limit = ubi->mtd->_max_bad_blocks(ubi->mtd, 0, ubi->mtd->size);
+		if (limit > 0)
+			return limit;
+	}
+
 	if (!max_beb_per1024)
 		return 0;
 
-- 
2.7.4

Re: [RESEND PATCH v2 3/3] mtd: ubi: use 'max_bad_blocks' to compute bad_peb_limit if available

[Boris Brezillon]

On Thu, 27 Oct 2016 14:14:02 -0500
Zach Brown <zach.brown@ni.com> wrote:

> From: Jeff Westfahl <jeff.westfahl@ni.com>
> 
> Use the MTD function 'max_bad_blocks' to compute the UBI bad_peb_limit,
> if the function is implemented for an MTD and doesn't return an error.
> 
> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> Signed-off-by: Zach Brown <zach.brown@ni.com>
> ---
>  drivers/mtd/ubi/build.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 85d54f3..0648863 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -584,6 +584,15 @@ static int get_bad_peb_limit(const struct ubi_device *ubi, int max_beb_per1024)
>  	int limit, device_pebs;
>  	uint64_t device_size;
>  
> +	/* If the MTD provides a max_bad_blocks function, use that value. Fall
> +	 * back to max_beb_per1024 if that function returns an error.
> +	 */
> +	if (ubi->mtd->_max_bad_blocks) {
> +		limit = ubi->mtd->_max_bad_blocks(ubi->mtd, 0, ubi->mtd->size);
> +		if (limit > 0)
> +			return limit;
> +	}

Please use the helper I was suggesting in patch 1.

> +
>  	if (!max_beb_per1024)
>  		return 0;
>  

Re: [RESEND PATCH v2 0/3] mtd: use ONFI bad blocks per LUN to calculate UBI bad PEB limit

[Boris Brezillon]

Hi Zach,

Please do not resend after only one week. Reviewing this series was on
my TODO list ;).

On Thu, 27 Oct 2016 14:13:59 -0500
Zach Brown <zach.brown@ni.com> wrote:

> For ONFI-compliant NAND devices, the ONFI parameters report the maximum number
> of bad blocks per LUN that will be encountered over the lifetime of the device,
> so we can use that information to get a more accurate (and smaller) value for
> the UBI bad PEB limit.
> 
> The ONFI parameter "maxiumum number of bad blocks per LUN" is the max number of
> bad blocks that each individual LUN will ever ecounter. It is not the number of
> bad blocks to reserve for the nand device per LUN in the device.
> 
> This means that in the worst case a UBI device spanning X LUNs will encounter
> "maximum number of bad blocks per LUN" * X bad blocks. The implementation in
> this patch assumes this worst case and allocates bad block accordingly.

That's a discussion I had with Richard a few months ago, and I didn't
know someone had already proposed a patch for that. So that's all good
news.
Indeed, I really think we should use information retrieved at flash
detection time rather than asking the user to explicitly tweak the
bad_peb_limit value for its chip.

> 
> These patches are ordered in terms of their dependencies, but ideally, all 3
> would need to be applied for this to work as intended.
> 
> v1:
>  * Changed commit message to address concerns from v1[1] about this patch set
>    making best case assumptions.
> 
> [1]
> http://lkml.iu.edu/hypermail/linux/kernel/1505.1/04822.html
> 
> Jeff Westfahl (3):
>   mtd: introduce function max_bad_blocks
>   mtd: nand: implement 'max_bad_blocks' mtd function
>   mtd: ubi: use 'max_bad_blocks' to compute bad_peb_limit if available
> 
>  drivers/mtd/mtdpart.c        | 12 ++++++++++++
>  drivers/mtd/nand/nand_base.c | 34 ++++++++++++++++++++++++++++++++++
>  drivers/mtd/ubi/build.c      |  9 +++++++++
>  include/linux/mtd/mtd.h      |  1 +
>  4 files changed, 56 insertions(+)
>