[PATCH 0/2] always use partition wrappers; drivers return bitflips

[PATCH 0/2] always use partition wrappers; drivers return bitflips

[Mike Dunn]

Hi,

These two patches accomplish the goal of returning bitflip info from the drivers
to mtd without requiring the huge patch that results from changing the prototype
of the read() method.

The first patch ensures that driver methods always go through the wrappers in
the partitioning code by creating a "partition" that spans the entire device on
otherwise unpartitioned devices.

Now that the methods always pass through the mtd code via the partitioning
wrappers, the second patch tweaks the meaning of the return code from the
driver's read() and read_oob() methods.  Where previously, in the absence of a
hard error, the driver returned either -EUCLEAN (one or more bitflips were
corrected) or 0 (no bitflips), the driver now returns, absent an error, the
maximum number of bitflips corrected on any single page.  The original set of
possible values (and their meanings) returned from mtd to the higher layer
remains unchanged for now.

This bifurcation of the driver <-> mtd and mtd <-> higher layer is admitedly a
little kludgy and potentially confusing for driver writers, but has the
advantage of touching few files.  With two exceptions, all drivers for devices
with ecc capability go through the nand or onenand interfaces, limiting the
changes to the nand and onenand infrastructure code.  Absent an error, devices
without ecc already return 0 always, so they comply with the modified api by
default.

If this flies, I'll follow up with patches to implement the change in the
meaning of -EUCLEAN returned from mtd that was previously discussed. Otherwise,
I can resubmit the large patch with the change to the read() prototype in lieu
of patch 2/2.

Thanks,
Mike

Mike Dunn (2):
  MTD: pass driver methods through partition wrappers on unpartitioned
    devices
  MTD: read(), read_oob() driver methods return num bitflips

 drivers/mtd/devices/docg3.c        |    5 +++-
 drivers/mtd/mtdcore.c              |   17 ++++++++++++--
 drivers/mtd/mtdpart.c              |   39 +++++++++++++++++++++++++++--------
 drivers/mtd/nand/alauda.c          |    4 +-
 drivers/mtd/nand/nand_base.c       |   10 +++++++-
 drivers/mtd/onenand/onenand_base.c |    6 +++-
 6 files changed, 62 insertions(+), 19 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] MTD: pass driver methods through partition wrappers on unpartitioned devices

[Mike Dunn]

Ensure driver methods always go through the wrapper functions in the
partitioning code by creating a single "partition" on otherwise unpartitioned
devices.

Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently
out-of-tree).

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/mtdcore.c |   17 ++++++++++++++---
 drivers/mtd/mtdpart.c |   10 +++++++---
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index b01993e..4ac1962 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -477,10 +477,21 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char **types,
 	if (err > 0) {
 		err = add_mtd_partitions(mtd, real_parts, err);
 		kfree(real_parts);
+
 	} else if (err == 0) {
-		err = add_mtd_device(mtd);
-		if (err == 1)
-			err = -ENODEV;
+		/*
+		 * For unpartitioned devices, create a single "partition" that
+		 * spans the entire device, so that driver methods go through
+		 * partition wrappers in all cases.
+		 */
+		struct mtd_partition single_part = {
+			.name = (char *)mtd->name,
+			.offset = 0,
+			.size = mtd->size,
+			.mask_flags = 0,
+			.ecclayout = mtd->ecclayout,
+		};
+		err = add_mtd_partitions(mtd, &single_part, 1);
 	}
 
 	return err;
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index a0bd2de..81975a5 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -665,9 +665,11 @@ int add_mtd_partitions(struct mtd_info *master,
 {
 	struct mtd_part *slave;
 	uint64_t cur_offset = 0;
-	int i;
+	int i, ret;
 
-	printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name);
+	if (nbparts > 1)
+		printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n",
+		       nbparts, master->name);
 
 	for (i = 0; i < nbparts; i++) {
 		slave = allocate_partition(master, parts + i, i, cur_offset);
@@ -678,7 +680,9 @@ int add_mtd_partitions(struct mtd_info *master,
 		list_add(&slave->list, &mtd_partitions);
 		mutex_unlock(&mtd_partitions_mutex);
 
-		add_mtd_device(&slave->mtd);
+		ret = add_mtd_device(&slave->mtd);
+		if (ret == 1)
+			return -ENODEV;
 
 		cur_offset = slave->offset + slave->mtd.size;
 	}
-- 
1.7.3.4

Re: [PATCH 1/2] MTD: pass driver methods through partition wrappers on unpartitioned devices

[Artem Bityutskiy]

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

On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote:
> Ensure driver methods always go through the wrapper functions in the
> partitioning code by creating a single "partition" on otherwise unpartitioned
> devices.
> 
> Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently
> out-of-tree).
> 
> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>

Looks good to me in general. Does it change anything from the user-space
POW (API/ABI)?

One thing to be aware of: some drivers do like this:

drivers/mtd/maps/plat-ram.c:

        /* check to see if there are any available partitions, or wether
         * to add this device whole */

        err = mtd_device_parse_register(info->mtd, pdata->probes, 0,
                        pdata->partitions, pdata->nr_partitions);
        if (!err)
                dev_info(&pdev->dev, "registered mtd device\n");

        if (pdata->nr_partitions) {
                /* add the whole device. */
                err = mtd_device_register(info->mtd, NULL, 0);
                if (err) {
                        dev_err(&pdev->dev,
                                "failed to register the entire device\n");
                }
        }

Could you please:
1. Try to find drivers like this and kill the redundant mtd_device_register()
   call.
2. There is no guarantee you will find all. So you could add a check in
   mtd_device_parse_register() which would print a warning if the device is
   added for the second time and return. So the code would work and people
   would be able to notice the warning and fix up the driver.

> -		add_mtd_device(&slave->mtd);
> +		ret = add_mtd_device(&slave->mtd);
> +		if (ret == 1)
> +			return -ENODEV;

Is this an unrelated change? Would you separate it out?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] MTD: pass driver methods through partition wrappers on unpartitioned devices

[Mike Dunn]

On 12/22/2011 04:34 AM, Artem Bityutskiy wrote:
> On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote:
>> Ensure driver methods always go through the wrapper functions in the
>> partitioning code by creating a single "partition" on otherwise unpartitioned
>> devices.
>>
>> Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently
>> out-of-tree).
>>
>> Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
> Looks good to me in general. Does it change anything from the user-space
> POW (API/ABI)?


No, should be completely transparent (at least that's my intention).  The only
new behavior is that the partition code puts a string in the log with the
partition boundaries and partition name, which in the single partition case is
mtd->name, which should avoid any confusion.


> One thing to be aware of: some drivers do like this:
>
> drivers/mtd/maps/plat-ram.c:
>
>         /* check to see if there are any available partitions, or wether
>          * to add this device whole */
>
>         err = mtd_device_parse_register(info->mtd, pdata->probes, 0,
>                         pdata->partitions, pdata->nr_partitions);
>         if (!err)
>                 dev_info(&pdev->dev, "registered mtd device\n");
>
>         if (pdata->nr_partitions) {
>                 /* add the whole device. */
>                 err = mtd_device_register(info->mtd, NULL, 0);
>                 if (err) {
>                         dev_err(&pdev->dev,
>                                 "failed to register the entire device\n");
>                 }
>         }
>
> Could you please:
> 1. Try to find drivers like this and kill the redundant mtd_device_register()
>    call.
> 2. There is no guarantee you will find all. So you could add a check in
>    mtd_device_parse_register() which would print a warning if the device is
>    added for the second time and return. So the code would work and people
>    would be able to notice the warning and fix up the driver.


OK.  Sorry, missed that.


>> -		add_mtd_device(&slave->mtd);
>> +		ret = add_mtd_device(&slave->mtd);
>> +		if (ret == 1)
>> +			return -ENODEV;
> Is this an unrelated change? Would you separate it out?


No, it's related.  Currently, mtd_device_parse_register() does the above if no
partitions are created, whereas here in add_mtd_partitions() the return code of
add_mtd_device() is not checked, so I added this to be consistent with the
previous behavior.  mtd_device_parse_register() propagates this return code back
to the caller, so the result is the same.

Thanks,
Mike

Re: [PATCH 1/2] MTD: pass driver methods through partition wrappers on unpartitioned devices

[Artem Bityutskiy]

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

On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote:
> +		/*
> +		 * For unpartitioned devices, create a single "partition" that
> +		 * spans the entire device, so that driver methods go through
> +		 * partition wrappers in all cases.
> +		 */
> +		struct mtd_partition single_part = {
> +			.name = (char *)mtd->name,
> +			.offset = 0,
> +			.size = mtd->size,
> +			.mask_flags = 0,
> +			.ecclayout = mtd->ecclayout,
> +		};
> +		err = add_mtd_partitions(mtd, &single_part, 1);
>  	}

There is a problem with this approach :-(

Look at the 'mtd_blkpg_ioctl()' function which is used to re-partition
MTD device from user-space. It is a bit strange, but we use block device
for re-partition in order to not invent new ioctls.

This function has this code:

                /* Only master mtd device must be used to add partitions */
                if (mtd_is_partition(mtd))
                        return -EINVAL;

and your patch  will brake re-partitioning.

So I guess we should go back to your first patch where you just change
the ->read() / ->read_oob() interface.

Or:

1. Rework whole MTD interface from ->read()/->write()/etc (pointers to
functions) to mtd_read(mtd, ...)/mtd_write(mtd, ...)/etc.

2. In the new functions you may do the stuff you do at patch 2.

For me the second approach is more plausible.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] MTD: pass driver methods through partition wrappers on unpartitioned devices

[Artem Bityutskiy]

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

On Thu, 2011-12-22 at 15:05 +0200, Artem Bityutskiy wrote:
> 1. Rework whole MTD interface from ->read()/->write()/etc (pointers to
> functions) to mtd_read(mtd, ...)/mtd_write(mtd, ...)/etc.

I will send a patch-set for this change tomorrow.

> 2. In the new functions you may do the stuff you do at patch 2.

Then you will be able to do this change easily.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] MTD: pass driver methods through partition wrappers on unpartitioned devices

[Mike Dunn]

On 12/22/2011 10:03 AM, Artem Bityutskiy wrote:
> On Thu, 2011-12-22 at 15:05 +0200, Artem Bityutskiy wrote:
>> 1. Rework whole MTD interface from ->read()/->write()/etc (pointers to
>> functions) to mtd_read(mtd, ...)/mtd_write(mtd, ...)/etc.
> I will send a patch-set for this change tomorrow.
>
>> 2. In the new functions you may do the stuff you do at patch 2.
> Then you will be able to do this change easily.
>



OK, thank you Artem.  I'll watch for it and follow up with a reworked patch 2.

Mike

Re: [PATCH 1/2] MTD: pass driver methods through partition wrappers on unpartitioned devices

[Artem Bityutskiy]

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

On Thu, 2011-12-22 at 15:28 -0800, Mike Dunn wrote:
> OK, thank you Artem.  I'll watch for it and follow up with a reworked patch 2.

I'm on it, but I think I've under-estimated the amount of work
and it won't be ready today.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] MTD: pass driver methods through partition wrappers on unpartitioned devices

[Mike Dunn]

On 12/22/2011 05:05 AM, Artem Bityutskiy wrote:
> On Tue, 2011-12-20 at 10:42 -0800, Mike Dunn wrote:
>> +		/*
>> +		 * For unpartitioned devices, create a single "partition" that
>> +		 * spans the entire device, so that driver methods go through
>> +		 * partition wrappers in all cases.
>> +		 */
>> +		struct mtd_partition single_part = {
>> +			.name = (char *)mtd->name,
>> +			.offset = 0,
>> +			.size = mtd->size,
>> +			.mask_flags = 0,
>> +			.ecclayout = mtd->ecclayout,
>> +		};
>> +		err = add_mtd_partitions(mtd, &single_part, 1);
>>  	}
> There is a problem with this approach :-(
>
> Look at the 'mtd_blkpg_ioctl()' function which is used to re-partition
> MTD device from user-space. It is a bit strange, but we use block device
> for re-partition in order to not invent new ioctls.


Sorry, missed that too.  So many perils...

Thanks,
Mike

[PATCH 2/2] MTD: read(), read_oob() driver methods return num bitflips

[Mike Dunn]

This patch changes the meaning of the value returned by the read() and
read_oob() mtd driver methods.  Previously, absent a hard error, these functions
returned either -EUCLEAN (one or more bitflips were corrected) or 0 (no
bitflips).  Drivers now return, absent a hard error, the maximum number of
bitflips that were corrected on any single page.  

Note that all calls to thr driver methods now go through the mtd partitioning
wrappers, and the values returned by those wrapper functions have not changed,
nor have their meaning.  Only the values returned to the mtd wrappers by the
driver have changed.

Tested with nandsim, onenand_sim, and the diskonchip g4 nand driver (currently
out-of-tree).  The two drivers that were modified were compile-tested only.

Signed-off-by: Mike Dunn <mikedunn@newsguy.com>
---
 drivers/mtd/devices/docg3.c        |    5 ++++-
 drivers/mtd/mtdpart.c              |   29 +++++++++++++++++++++++------
 drivers/mtd/nand/alauda.c          |    4 ++--
 drivers/mtd/nand/nand_base.c       |   10 ++++++++--
 drivers/mtd/onenand/onenand_base.c |    6 ++++--
 5 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 22d5099..73c7574 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -854,6 +854,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 	u8 *buf = ops->datbuf;
 	size_t len, ooblen, nbdata, nboob;
 	u8 hwecc[DOC_ECC_BCH_SIZE], eccconf1;
+	unsigned int max_bitflips = 0;
 
 	if (buf)
 		len = ops->len;
@@ -938,7 +939,9 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 			}
 			if (ret > 0) {
 				mtd->ecc_stats.corrected += ret;
-				ret = -EUCLEAN;
+				max_bitflips = max(max_bitflips,
+						   (unsigned int)ret);
+				ret = max_bitflips;
 			}
 		}
 
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 81975a5..9dfb2ad 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -73,9 +73,15 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
 	res = part->master->read(part->master, from + part->offset,
 				   len, retlen, buf);
 	if (unlikely(res)) {
-		if (mtd_is_bitflip(res))
+
+		/*
+		 * In the absence of errors, drivers return max_bitflips.
+		 * We return -EUCLEAN (bitflips corrected) or 0 (no bitflips).
+		 */
+		if (res > 0) {
 			mtd->ecc_stats.corrected += part->master->ecc_stats.corrected - stats.corrected;
-		if (mtd_is_eccerr(res))
+			res = -EUCLEAN;
+		} else if (mtd_is_eccerr(res))
 			mtd->ecc_stats.failed += part->master->ecc_stats.failed - stats.failed;
 	}
 	return res;
@@ -116,6 +122,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 		struct mtd_oob_ops *ops)
 {
 	struct mtd_part *part = PART(mtd);
+	struct mtd_ecc_stats stats = part->master->ecc_stats;
 	int res;
 
 	if (from >= mtd->size)
@@ -142,10 +149,20 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
 
 	res = part->master->read_oob(part->master, from + part->offset, ops);
 	if (unlikely(res)) {
-		if (mtd_is_bitflip(res))
-			mtd->ecc_stats.corrected++;
-		if (mtd_is_eccerr(res))
-			mtd->ecc_stats.failed++;
+
+		/*
+		 * In the absence of errors, drivers return max_bitflips.
+		 * We return -EUCLEAN (bitflips corrected) or 0 (no bitflips).
+		 */
+		if (res > 0) {
+			mtd->ecc_stats.corrected +=
+				part->master->ecc_stats.corrected -
+				stats.corrected;
+			res = -EUCLEAN;
+		} else if (mtd_is_eccerr(res))
+			mtd->ecc_stats.failed +=
+				part->master->ecc_stats.failed -
+				stats.failed;
 	}
 	return res;
 }
diff --git a/drivers/mtd/nand/alauda.c b/drivers/mtd/nand/alauda.c
index eb40ea8..60db9f2 100644
--- a/drivers/mtd/nand/alauda.c
+++ b/drivers/mtd/nand/alauda.c
@@ -414,7 +414,7 @@ static int alauda_bounce_read(struct mtd_info *mtd, loff_t from, size_t len,
 	}
 	err = 0;
 	if (corrected)
-		err = -EUCLEAN;
+		err = 1;	/* return max bitflips per page */
 	if (uncorrected)
 		err = -EBADMSG;
 out:
@@ -446,7 +446,7 @@ static int alauda_read(struct mtd_info *mtd, loff_t from, size_t len,
 	}
 	err = 0;
 	if (corrected)
-		err = -EUCLEAN;
+		err = 1;	/* return max bitflips per page */
 	if (uncorrected)
 		err = -EBADMSG;
 	return err;
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 35b4565..5709af8 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1441,7 +1441,7 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	uint32_t oobreadlen = ops->ooblen;
 	uint32_t max_oobsize = ops->mode == MTD_OPS_AUTO_OOB ?
 		mtd->oobavail : mtd->oobsize;
-
+	unsigned int max_bitflips = 0;
 	uint8_t *bufpoi, *oob, *buf;
 
 	stats = mtd->ecc_stats;
@@ -1463,6 +1463,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 
 		/* Is the current page in the buffer? */
 		if (realpage != chip->pagebuf || oob) {
+			unsigned int prev_corrected = mtd->ecc_stats.corrected;
+
 			bufpoi = aligned ? buf : chip->buffers->databuf;
 
 			if (likely(sndcmd)) {
@@ -1525,6 +1527,9 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 				else
 					nand_wait_ready(mtd);
 			}
+			max_bitflips = max(max_bitflips,
+					   mtd->ecc_stats.corrected -
+					   prev_corrected);
 		} else {
 			memcpy(buf, chip->buffers->databuf + col, bytes);
 			buf += bytes;
@@ -1566,7 +1571,8 @@ static int nand_do_read_ops(struct mtd_info *mtd, loff_t from,
 	if (mtd->ecc_stats.failed - stats.failed)
 		return -EBADMSG;
 
-	return  mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
+	/* return max bitflips per page to mtd partition wrappers */
+	return max_bitflips;
 }
 
 /**
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index a839473..2bc35d4 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1201,7 +1201,8 @@ static int onenand_mlc_read_ops_nolock(struct mtd_info *mtd, loff_t from,
 	if (mtd->ecc_stats.failed - stats.failed)
 		return -EBADMSG;
 
-	return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
+	/* return max bitflips per page to mtd partition wrappers */
+	return mtd->ecc_stats.corrected - stats.corrected ? 1 : 0;
 }
 
 /**
@@ -1333,7 +1334,8 @@ static int onenand_read_ops_nolock(struct mtd_info *mtd, loff_t from,
 	if (mtd->ecc_stats.failed - stats.failed)
 		return -EBADMSG;
 
-	return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
+	/* return max bitflips per page to mtd partition wrappers */
+	return mtd->ecc_stats.corrected - stats.corrected ? 1 : 0;
 }
 
 /**
-- 
1.7.3.4

Re: [PATCH 0/2] always use partition wrappers; drivers return bitflips

[Thomas Petazzoni]

Hello,

Le Tue, 20 Dec 2011 10:42:14 -0800,
Mike Dunn <mikedunn@newsguy.com> a écrit :

> This bifurcation of the driver <-> mtd and mtd <-> higher layer is admitedly a
> little kludgy and potentially confusing for driver writers, but has the
> advantage of touching few files.

The advantage of touching few files is only a short-term advantage,
while the drawback of being a "little kludgy and confusing for driver
writers" is a long-term, major, drawback. I think it's much better to
spend a bit of time now to touch more files, but get a nice and clean
interface, rather than leaving the kludge of stacking a MTD partition
on top of another.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com