[RFC 0/5] fix data+OOB writes, add ioctl

[RFC 0/5] fix data+OOB writes, add ioctl

[Brian Norris]

Hello all,

Warning: This patch series is not a finished product yet; it is merely a
"Request For Comments" on my goals, methods, etc. Please comment on
this, and in the near future, there may be a full update to this series.

First off, these fixes are motivated by a number of things:

1) Broken "noecc" writes; `nandwrite -n -o' does not work on my
   hardware, at least, without these patches

2) We cannot write twice to the same page on MLC NAND flash. Thus, the
   current implementation for nandwrite, where we write OOB and page
   data in two separate ioctl operations, will not work. We need a new
   ioctl. See this thread:

   http://lists.infradead.org/pipermail/linux-mtd/2011-August/037316.html

3) It is beneficial to expose the mtd_oob_mode_t options to the
   user-space, since we often want to use MTD_OOB_RAW, MTD_OOB_AUTO,
   etc. on a per-operation basis. This naturally fits in with a new
   ioctl that can handle different combinations of page data and OOB as
   well as different OOB modes. See some discussion:

   http://lists.infradead.org/pipermail/linux-mtd/2011-August/037511.html

Now, there are certainly some things that this patch series does NOT yet
do. I will list some now:

4) Base the old ioctls (MEMWRITEOOB, MEMWRITEOOB64) on the new one
   (MEMWRITEDATAOOB). In fact, the first two patches update support for
   the old ioctls, whereas they could simply be "merged" with the new
   one instead.

5) Gracefully handle cases where mtd->write_oob doesn't exist. It simply
   quits with -EOPNOTSUPP. I test all of this code on NAND flash only,
   and the NAND subsystem always supplies this function. If there is a
   need for this ioctl without OOB writes, then perhaps this can be
   fixed.

Other thoughts:

Patch 3/5 is a weird one. I know code that the patch targets is wrong
(since it's obviously assuming oobsize is a power of 2), but I'm not
sure the *exact* intention of the wrong code - so it's hard to be sure
that my fix is valid. Please look it over.

The name "MEMWRITEDATAOOB" is totally up for debate. This was the best I
could come up with at the moment.

Perhaps something should be done about the MTDFILEMODE ioctl, since it
seems to cause some confusing overlap, at least to me. I see two
different RAW modes:

* MTD_MODE_RAW, which belongs to the mode field in `struct
  mtd_file_info'. It can be set by the ioctl MTDFILEMODE
* MTD_OOB_RAW, which belongs to the mode field in `struct
  mtd_oob_ops'. It is set indirectly by other operations.

Does MTD_MODE_RAW imply MTD_OOB_RAW when OOB operations are involved?
Does MTD_MODE_RAW imply that no ECC is applied to data?
Is the MTD file mode persistent? If so, then it may conflict with a
"per-operation" mode in our new MEMWRITEDATAOOB

Maybe the "something" to be done would just be some better
documentation. Or something more drastic if there's an actual conflict.

Speaking of documentation, what is this supposed to mean (from struct
nand_chip, in include/linux/mtd/nand.h)?

	@oob_poi:            poison value buffer

It seems like the oob_poi buffer is meant to be an intermediary buffer
for storing the laid-out OOB data; it's used by the NAND subsystem,
especially in mode MTD_OOB_AUTO, to layout data to be written. But this
"documentation" means absolutely nothing to me. I'll edit that comment
in nand.h and include it in my revised patch series if I'm on the right
track.

OK, that's a lot to read :) Let me know if you have questions, and I'll
follow up with more info if there's something I left out.

Brian

Brian Norris (5):
  mtd: support MTD_MODE_RAW for writing OOB
  mtd: support MTD_MODE_RAW for reading OOB
  mtd: do not assume oobsize is power of 2
  mtd: move mtd_oob_mode_t to shared kernel/user space
  mtd: add MEMWRITEDATAOOB ioctl

 drivers/mtd/mtdchar.c        |   54 +++++++++++++++++++++++++++++++++--------
 drivers/mtd/nand/nand_base.c |   16 +++++++++++-
 include/linux/mtd/mtd.h      |   14 -----------
 include/mtd/mtd-abi.h        |   23 ++++++++++++++++++
 4 files changed, 80 insertions(+), 27 deletions(-)

[RFC 1/5] mtd: support MTD_MODE_RAW for writing OOB

[Brian Norris]

mtd_do_write_oob does not pass information about whether to write in RAW
mode to the lower layers (e.g., NAND). We should pass MTD_OOB_RAW or
MTD_OOB_PLACE based on the MTD file mode.

This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdchar.c        |    3 ++-
 drivers/mtd/nand/nand_base.c |    9 ++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index a75d555..7ca4361 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -391,6 +391,7 @@ static int mtd_do_writeoob(struct file *file, struct mtd_info *mtd,
 	uint64_t start, uint32_t length, void __user *ptr,
 	uint32_t __user *retp)
 {
+	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_oob_ops ops;
 	uint32_t retlen;
 	int ret = 0;
@@ -412,7 +413,7 @@ static int mtd_do_writeoob(struct file *file, struct mtd_info *mtd,
 	ops.ooblen = length;
 	ops.ooboffs = start & (mtd->oobsize - 1);
 	ops.datbuf = NULL;
-	ops.mode = MTD_OOB_PLACE;
+	ops.mode = (mfi->mode == MTD_MODE_RAW) ? MTD_OOB_RAW : MTD_OOB_PLACE;
 
 	if (ops.ooboffs && ops.ooblen > (mtd->oobsize - ops.ooboffs))
 		return -EINVAL;
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index d2ee68a..e2b1c90 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -2401,7 +2401,14 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 		chip->pagebuf = -1;
 
 	nand_fill_oob(mtd, ops->oobbuf, ops->ooblen, ops);
-	status = chip->ecc.write_oob(mtd, chip, page & chip->pagemask);
+
+	if (ops->mode == MTD_OOB_RAW) {
+		status = 0;
+		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
+		chip->ecc.write_page_raw(mtd, chip, NULL);
+	} else {
+		status = chip->ecc.write_oob(mtd, chip, page & chip->pagemask);
+	}
 
 	if (status)
 		return status;
-- 
1.7.0.4

[RFC 2/5] mtd: support MTD_MODE_RAW for reading OOB

[Brian Norris]

mtd_do_read_oob does not pass information about whether to read in RAW
mode to the lower layers (e.g., NAND). We should pass MTD_OOB_RAW or
MTD_OOB_PLACE based on the MTD file mode.

This fixes issues with `nanddump -n' and the MEMREADOOB[64] ioctls.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdchar.c        |   14 ++++++++------
 drivers/mtd/nand/nand_base.c |    7 ++++++-
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 7ca4361..4488e9a 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -435,9 +435,11 @@ static int mtd_do_writeoob(struct file *file, struct mtd_info *mtd,
 	return ret;
 }
 
-static int mtd_do_readoob(struct mtd_info *mtd, uint64_t start,
-	uint32_t length, void __user *ptr, uint32_t __user *retp)
+static int mtd_do_readoob(struct file *file, struct mtd_info *mtd,
+	uint64_t start, uint32_t length, void __user *ptr,
+	uint32_t __user *retp)
 {
+	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_oob_ops ops;
 	int ret = 0;
 
@@ -455,7 +457,7 @@ static int mtd_do_readoob(struct mtd_info *mtd, uint64_t start,
 	ops.ooblen = length;
 	ops.ooboffs = start & (mtd->oobsize - 1);
 	ops.datbuf = NULL;
-	ops.mode = MTD_OOB_PLACE;
+	ops.mode = (mfi->mode == MTD_MODE_RAW) ? MTD_OOB_RAW : MTD_OOB_PLACE;
 
 	if (ops.ooboffs && ops.ooblen > (mtd->oobsize - ops.ooboffs))
 		return -EINVAL;
@@ -716,7 +718,7 @@ static int mtd_ioctl(struct file *file, u_int cmd, u_long arg)
 		if (copy_from_user(&buf, argp, sizeof(buf)))
 			ret = -EFAULT;
 		else
-			ret = mtd_do_readoob(mtd, buf.start, buf.length,
+			ret = mtd_do_readoob(file, mtd, buf.start, buf.length,
 				buf.ptr, &buf_user->start);
 		break;
 	}
@@ -743,7 +745,7 @@ static int mtd_ioctl(struct file *file, u_int cmd, u_long arg)
 		if (copy_from_user(&buf, argp, sizeof(buf)))
 			ret = -EFAULT;
 		else
-			ret = mtd_do_readoob(mtd, buf.start, buf.length,
+			ret = mtd_do_readoob(file, mtd, buf.start, buf.length,
 				(void __user *)(uintptr_t)buf.usr_ptr,
 				&buf_user->length);
 		break;
@@ -1029,7 +1031,7 @@ static long mtd_compat_ioctl(struct file *file, unsigned int cmd,
 		if (copy_from_user(&buf, argp, sizeof(buf)))
 			ret = -EFAULT;
 		else
-			ret = mtd_do_readoob(mtd, buf.start,
+			ret = mtd_do_readoob(file, mtd, buf.start,
 				buf.length, compat_ptr(buf.ptr),
 				&buf_user->start);
 		break;
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index e2b1c90..9c4df4a 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1787,7 +1787,12 @@ static int nand_do_read_oob(struct mtd_info *mtd, loff_t from,
 	page = realpage & chip->pagemask;
 
 	while (1) {
-		sndcmd = chip->ecc.read_oob(mtd, chip, page, sndcmd);
+		if (ops->mode == MTD_OOB_RAW) {
+			sndcmd = 0;
+			chip->ecc.read_page_raw(mtd, chip, NULL, page);
+		} else {
+			sndcmd = chip->ecc.read_oob(mtd, chip, page, sndcmd);
+		}
 
 		len = min(len, readlen);
 		buf = nand_transfer_oob(chip, buf, ops, len);
-- 
1.7.0.4

[RFC 3/5] mtd: do not assume oobsize is power of 2

[Brian Norris]

Previous generations of MTDs all used OOB sizes that were powers of 2,
(e.g., 64, 128). However, newer generations of flash, especially NAND,
use irregular OOB sizes that are not powers of 2 (e.g., 218, 224, 448).
This means we cannot use masks like "mtd->oobsize - 1" to assume that we
will get a proper bitmask for OOB operations.

As I see it, we don't actually need these masks anyway, so kill them.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdchar.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index 4488e9a..a0c404b 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -411,7 +411,7 @@ static int mtd_do_writeoob(struct file *file, struct mtd_info *mtd,
 		return ret;
 
 	ops.ooblen = length;
-	ops.ooboffs = start & (mtd->oobsize - 1);
+	ops.ooboffs = start;
 	ops.datbuf = NULL;
 	ops.mode = (mfi->mode == MTD_MODE_RAW) ? MTD_OOB_RAW : MTD_OOB_PLACE;
 
@@ -422,7 +422,6 @@ static int mtd_do_writeoob(struct file *file, struct mtd_info *mtd,
 	if (IS_ERR(ops.oobbuf))
 		return PTR_ERR(ops.oobbuf);
 
-	start &= ~((uint64_t)mtd->oobsize - 1);
 	ret = mtd->write_oob(mtd, start, &ops);
 
 	if (ops.oobretlen > 0xFFFFFFFFU)
@@ -455,7 +454,7 @@ static int mtd_do_readoob(struct file *file, struct mtd_info *mtd,
 		return ret;
 
 	ops.ooblen = length;
-	ops.ooboffs = start & (mtd->oobsize - 1);
+	ops.ooboffs = start;
 	ops.datbuf = NULL;
 	ops.mode = (mfi->mode == MTD_MODE_RAW) ? MTD_OOB_RAW : MTD_OOB_PLACE;
 
@@ -466,7 +465,6 @@ static int mtd_do_readoob(struct file *file, struct mtd_info *mtd,
 	if (!ops.oobbuf)
 		return -ENOMEM;
 
-	start &= ~((uint64_t)mtd->oobsize - 1);
 	ret = mtd->read_oob(mtd, start, &ops);
 
 	if (put_user(ops.oobretlen, retp))
-- 
1.7.0.4

[RFC 4/5] mtd: move mtd_oob_mode_t to shared kernel/user space

[Brian Norris]

We will want to use the MTD_OOB_{PLACE,AUTO,RAW} modes in user-space
applications through the introduction of new ioctls, so we should make
this enum a shared type.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 include/linux/mtd/mtd.h |   14 --------------
 include/mtd/mtd-abi.h   |   14 ++++++++++++++
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 46682ac..e089bce 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -68,20 +68,6 @@ struct mtd_erase_region_info {
 	unsigned long *lockmap;		/* If keeping bitmap of locks */
 };
 
-/*
- * oob operation modes
- *
- * MTD_OOB_PLACE:	oob data are placed at the given offset
- * MTD_OOB_AUTO:	oob data are automatically placed at the free areas
- *			which are defined by the ecclayout
- * MTD_OOB_RAW:		mode to read oob and data without doing ECC checking
- */
-typedef enum {
-	MTD_OOB_PLACE,
-	MTD_OOB_AUTO,
-	MTD_OOB_RAW,
-} mtd_oob_mode_t;
-
 /**
  * struct mtd_oob_ops - oob operation operands
  * @mode:	operation mode
diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index 2f7d45b..f850d9a 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -45,6 +45,20 @@ struct mtd_oob_buf64 {
 	__u64 usr_ptr;
 };
 
+/*
+ * oob operation modes
+ *
+ * MTD_OOB_PLACE:       oob data are placed at the given offset
+ * MTD_OOB_AUTO:        oob data are automatically placed at the free areas
+ *                      which are defined by the ecclayout
+ * MTD_OOB_RAW:         mode to read oob and data without doing ECC checking
+ */
+typedef enum {
+	MTD_OOB_PLACE,
+	MTD_OOB_AUTO,
+	MTD_OOB_RAW,
+} mtd_oob_mode_t;
+
 #define MTD_ABSENT		0
 #define MTD_RAM			1
 #define MTD_ROM			2
-- 
1.7.0.4

[RFC 5/5] mtd: add MEMWRITEDATAOOB ioctl

[Brian Norris]

Implement a new ioctl for writing both page data and OOB to flash at the
same time.  This is especially useful for MLC NAND, which cannot be
written twice (i.e., we cannot successfully write the page data and OOB
in two separate operations).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/mtdchar.c |   31 +++++++++++++++++++++++++++++++
 include/mtd/mtd-abi.h |    9 +++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index a0c404b..bef8462 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -749,6 +749,37 @@ static int mtd_ioctl(struct file *file, u_int cmd, u_long arg)
 		break;
 	}
 
+	case MEMWRITEDATAOOB:
+	{
+		struct mtd_data_oob_buf buf;
+
+		if (copy_from_user(&buf, argp, sizeof(buf)) ||
+				!access_ok(VERIFY_READ, buf.usr_ptr, buf.len)) {
+			ret = -EFAULT;
+		} else if (!mtd->write_oob) {
+			ret = -EOPNOTSUPP;
+		} else {
+			struct mtd_oob_ops ops;
+
+			ops.mode = buf.mode;
+			ops.len = (size_t)buf.len;
+			ops.ooblen = (size_t)buf.ooblen;
+			ops.ooboffs = 0;
+
+			ops.datbuf = memdup_user(
+					(void __user *)(uintptr_t)buf.usr_ptr,
+					ops.len + ops.ooblen);
+			if (IS_ERR(ops.datbuf))
+				return PTR_ERR(ops.datbuf);
+
+			ops.oobbuf = ops.datbuf + ops.len;
+
+			ret = mtd->write_oob(mtd, (loff_t)buf.start, &ops);
+			kfree(ops.datbuf);
+		}
+		break;
+	}
+
 	case MEMLOCK:
 	{
 		struct erase_info_user einfo;
diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index f850d9a..20df299 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -59,6 +59,14 @@ typedef enum {
 	MTD_OOB_RAW,
 } mtd_oob_mode_t;
 
+struct mtd_data_oob_buf {
+	__u64 start;
+	__u32 len;
+	__u32 ooblen;
+	__u64 usr_ptr;
+	mtd_oob_mode_t __user mode;
+};
+
 #define MTD_ABSENT		0
 #define MTD_RAM			1
 #define MTD_ROM			2
@@ -141,6 +149,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 MEMWRITEDATAOOB		_IOWR('M', 24, struct mtd_data_oob_buf)
 
 /*
  * Obsolete legacy interface. Keep it in order not to break userspace
-- 
1.7.0.4

Re: [RFC 1/5] mtd: support MTD_MODE_RAW for writing OOB

[Artem Bityutskiy]

On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> mtd_do_write_oob does not pass information about whether to write in RAW
> mode to the lower layers (e.g., NAND). We should pass MTD_OOB_RAW or
> MTD_OOB_PLACE based on the MTD file mode.
> 
> This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

I guess this patch deserves to be non-RFC? Should it be pushed to
l2-mtd-2.6.git? Should it even have "Cc: stable@kernel.org [kernel
version +] ?

> ---
>  drivers/mtd/mtdchar.c        |    3 ++-
>  drivers/mtd/nand/nand_base.c |    9 ++++++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
> index a75d555..7ca4361 100644
> --- a/drivers/mtd/mtdchar.c
> +++ b/drivers/mtd/mtdchar.c
> @@ -391,6 +391,7 @@ static int mtd_do_writeoob(struct file *file, struct mtd_info *mtd,
>  	uint64_t start, uint32_t length, void __user *ptr,
>  	uint32_t __user *retp)
>  {
> +	struct mtd_file_info *mfi = file->private_data;
>  	struct mtd_oob_ops ops;
>  	uint32_t retlen;
>  	int ret = 0;
> @@ -412,7 +413,7 @@ static int mtd_do_writeoob(struct file *file, struct mtd_info *mtd,
>  	ops.ooblen = length;
>  	ops.ooboffs = start & (mtd->oobsize - 1);
>  	ops.datbuf = NULL;
> -	ops.mode = MTD_OOB_PLACE;
> +	ops.mode = (mfi->mode == MTD_MODE_RAW) ? MTD_OOB_RAW : MTD_OOB_PLACE;

Would be really great to document things while you are in it - e.g.,
putting a little note in include/linux/mtd/mtd.h that MTD_OOB_PLACE is
the default mode. And of course documenting the modes in this file a bit
more would be also great. I encourage you to store the wisdom you gain
in the comments :-)

>  
>  	if (ops.ooboffs && ops.ooblen > (mtd->oobsize - ops.ooboffs))
>  		return -EINVAL;
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index d2ee68a..e2b1c90 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -2401,7 +2401,14 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
>  		chip->pagebuf = -1;
>  
>  	nand_fill_oob(mtd, ops->oobbuf, ops->ooblen, ops);
> -	status = chip->ecc.write_oob(mtd, chip, page & chip->pagemask);
> +
> +	if (ops->mode == MTD_OOB_RAW) {
> +		status = 0;
> +		chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
> +		chip->ecc.write_page_raw(mtd, chip, NULL);

Strange that write_page_raw does not return an error code - it cannot
fail? But this is a separate issue anyway.

> +	} else {
> +		status = chip->ecc.write_oob(mtd, chip, page & chip->pagemask);
> +	}
>  
>  	if (status)
>  		return status;


-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 2/5] mtd: support MTD_MODE_RAW for reading OOB

[Artem Bityutskiy]

On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> mtd_do_read_oob does not pass information about whether to read in RAW
> mode to the lower layers (e.g., NAND). We should pass MTD_OOB_RAW or
> MTD_OOB_PLACE based on the MTD file mode.
> 
> This fixes issues with `nanddump -n' and the MEMREADOOB[64] ioctls.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

I guess similar question applies to this patch - why is this RFC while
it looks like a candidate for 'stable@kernel.org'?

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 3/5] mtd: do not assume oobsize is power of 2

[Artem Bityutskiy]

On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> Previous generations of MTDs all used OOB sizes that were powers of 2,
> (e.g., 64, 128). However, newer generations of flash, especially NAND,
> use irregular OOB sizes that are not powers of 2 (e.g., 218, 224, 448).
> This means we cannot use masks like "mtd->oobsize - 1" to assume that we
> will get a proper bitmask for OOB operations.
> 
> As I see it, we don't actually need these masks anyway, so kill them.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Once you double checked that you caught all the places which assume
power-of-2 and do things like "x & (mtd->oobsize - 1", and once you
confirmed that you did some basic test by running few nandwrite commands
which involve OOB operations, I think this patch can go in
independently.

IOW, this does need to be RFC, IMO.

But yes, if there is a buggy piece of user-space code which uses
non-aligned offsets, and kernel alignes them - this change will break
it. IOW, this patch may expose bugs in user-space code. This is kind of
ABI break, but I think for MTD which is not very widely used, we can do
things like this.

IOW, I understand that this may break some user-space program, but
working with MTD for many years I thing there are very very little of
such programs and I think it is not a huge issue.

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 4/5] mtd: move mtd_oob_mode_t to shared kernel/user space

[Artem Bityutskiy]

On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> +/*
> + * oob operation modes
> + *
> + * MTD_OOB_PLACE:       oob data are placed at the given offset
> + * MTD_OOB_AUTO:        oob data are automatically placed at the free areas
> + *                      which are defined by the ecclayout
> + * MTD_OOB_RAW:         mode to read oob and data without doing ECC checking
> + */
> +typedef enum {
> +	MTD_OOB_PLACE,
> +	MTD_OOB_AUTO,
> +	MTD_OOB_RAW,
> +} mtd_oob_mode_t;

Could we get rid of this typedef and use anonymous enum instead:

enum {
	A,
	B,
};

Indeed, we do not need it, and we do not want to pollute the user-space
namespaces unnecessarily.

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 5/5] mtd: add MEMWRITEDATAOOB ioctl

[Artem Bityutskiy]

On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> Implement a new ioctl for writing both page data and OOB to flash at the
> same time.  This is especially useful for MLC NAND, which cannot be
> written twice (i.e., we cannot successfully write the page data and OOB
> in two separate operations).
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Looks good except few nit-picks.

> +			ops.mode = buf.mode;
> +			ops.len = (size_t)buf.len;
> +			ops.ooblen = (size_t)buf.ooblen;
> +			ops.ooboffs = 0;
> +
> +			ops.datbuf = memdup_user(
> +					(void __user *)(uintptr_t)buf.usr_ptr,
> +					ops.len + ops.ooblen);

I'd introduced a local variable for buf.usr_ptr and made the formatting
nicer.

> +			if (IS_ERR(ops.datbuf))
> +				return PTR_ERR(ops.datbuf);
> +
> +			ops.oobbuf = ops.datbuf + ops.len;
> +
> +			ret = mtd->write_oob(mtd, (loff_t)buf.start, &ops);
> +			kfree(ops.datbuf);
> +		}
> +		break;
> +	}
> +
>  	case MEMLOCK:
>  	{
>  		struct erase_info_user einfo;
> diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
> index f850d9a..20df299 100644
> --- a/include/mtd/mtd-abi.h
> +++ b/include/mtd/mtd-abi.h
> @@ -59,6 +59,14 @@ typedef enum {
>  	MTD_OOB_RAW,
>  } mtd_oob_mode_t;
>  
> +struct mtd_data_oob_buf {
> +	__u64 start;
> +	__u32 len;
> +	__u32 ooblen;
> +	__u64 usr_ptr;
> +	mtd_oob_mode_t __user mode;
> +};

Let's add some reserved space for future improvements - I think it is
always a good idea to do for ioctls:

+ __u8 padding[8]

> +
>  #define MTD_ABSENT		0
>  #define MTD_RAM			1
>  #define MTD_ROM			2
> @@ -141,6 +149,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 MEMWRITEDATAOOB		_IOWR('M', 24, struct mtd_data_oob_buf)

Would be greate to add a short comment about what this ioctl does. Of
course you can optionally do this for all of them, but at least for the
new one it does not hurt to have.

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Artem Bityutskiy]

Hi,

On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> First off, these fixes are motivated by a number of things:
> 
> 1) Broken "noecc" writes; `nandwrite -n -o' does not work on my
>    hardware, at least, without these patches

I think Ivan complained about this at some point - he said he has images
with crafted ECC codes to test his BCH library, but he could not flash
them properly. So yes, this is worth fixing.

> Perhaps something should be done about the MTDFILEMODE ioctl, since it
> seems to cause some confusing overlap, at least to me. I see two
> different RAW modes:
> 
> * MTD_MODE_RAW, which belongs to the mode field in `struct
>   mtd_file_info'. It can be set by the ioctl MTDFILEMODE
> * MTD_OOB_RAW, which belongs to the mode field in `struct
>   mtd_oob_ops'. It is set indirectly by other operations.
> 
> Does MTD_MODE_RAW imply MTD_OOB_RAW when OOB operations are involved?

I would make sure these 2 set of macros have different prefixes.
Probably re-naming the internal ones is easier, but I'd anyway re-named
the external ones - indeed they are about access mode for a specific
file descriptor, so I'd use the "MTD_FILE_MODE_" prefix. But not sure,
you can invent something else. In any case, it is cleaner to have
different prefixes for different name-spaces.

> Does MTD_MODE_RAW imply that no ECC is applied to data?

Not sure, but judging from how it is used - no.

> Is the MTD file mode persistent?

It is stored in the file descriptor (file->private_data), so the
life-time of the mode is equivalent to the life-time for the file
descriptor, AFAICS.

>  If so, then it may conflict with a
> "per-operation" mode in our new MEMWRITEDATAOOB

Good point. Frankly, I find this stateful model with modes horrible. But
we have what we have. I guess we will need to carefully document which
ioctl's are affected by the MTD mode, and which are not (in the header
file which we expose to the user-space).

I think MEMWRITEDATAOOB should ignore the mode.

My quick look at the code suggests me that the RAW/NORMAL file modes is
about read/write operations - in normal mode we read/write from/to only
the main area, in raw mode we consider the main area and OOB as one
continuous region and read/write from/to this region.

And MEMWRITEDATAOOB is about accession only OOB, so the modes should not
affect it.

> Maybe the "something" to be done would just be some better
> documentation. Or something more drastic if there's an actual conflict.

To me it looks like (a) re-naming and (be) some more comments in the
header files is enough.

> Speaking of documentation, what is this supposed to mean (from struct
> nand_chip, in include/linux/mtd/nand.h)?

Sorry, do not know off the top of my head.

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Ivan Djelic]

On Mon, Aug 22, 2011 at 11:02:40AM +0100, Artem Bityutskiy wrote:
> Hi,
> 
> On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> > First off, these fixes are motivated by a number of things:
> > 
> > 1) Broken "noecc" writes; `nandwrite -n -o' does not work on my
> >    hardware, at least, without these patches
> 
> I think Ivan complained about this at some point - he said he has images
> with crafted ECC codes to test his BCH library, but he could not flash
> them properly. So yes, this is worth fixing.

Actually it was Ricard Wanderlof, but indeed he was testing the BCH library
(http://lists.infradead.org/pipermail/linux-mtd/2011-March/034516.html).
It would be nice if he could ack the fix ?

--
Best Regards,

Ivan

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Artem Bityutskiy]

On Mon, 2011-08-22 at 14:04 +0200, Ivan Djelic wrote:
> On Mon, Aug 22, 2011 at 11:02:40AM +0100, Artem Bityutskiy wrote:
> > Hi,
> > 
> > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> > > First off, these fixes are motivated by a number of things:
> > > 
> > > 1) Broken "noecc" writes; `nandwrite -n -o' does not work on my
> > >    hardware, at least, without these patches
> > 
> > I think Ivan complained about this at some point - he said he has images
> > with crafted ECC codes to test his BCH library, but he could not flash
> > them properly. So yes, this is worth fixing.
> 
> Actually it was Ricard Wanderlof, but indeed he was testing the BCH library
> (http://lists.infradead.org/pipermail/linux-mtd/2011-March/034516.html).
> It would be nice if he could ack the fix ?

Sure, CCed.

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 1/5] mtd: support MTD_MODE_RAW for writing OOB

[Brian Norris]

I forgot to CC a contributor on this (and the complementary patch for
"read OOB")

Cc: Jim Quinlan <jim2101024@gmail.com>

On Mon, Aug 22, 2011 at 1:35 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
>> This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls.
>
> I guess this patch deserves to be non-RFC? Should it be pushed to
> l2-mtd-2.6.git? Should it even have "Cc: stable@kernel.org [kernel
> version +] ?

I'm not actually sure where this stands (RFC vs. patch) since I really
wanted some outside opinion on the methods used here. I have a feeling
that some of this is only working on my hardware. For instance,
somehow (I'm really not sure how!) `nandwrite -n -o' is working in
nandsim without my fix. Perhaps this is due to a different set of
nand_ecc_ctrl functions (soft ECC vs. HW ECC).

With this patch applied, however, I get some strange kernel oopses
with nandsim. I've identified at least one issue, I think, but I
haven't completely resolved this discrepancy between nandsim and my
driver. See sample commands:

# insmod nandsim.ko
# nandwrite /dev/mtdX <data.bin> -n -o

So for this "fix" (and its coming updates), I would appreciate some
outside testing on other systems, especially before we send this to
stable or even before including it upstream at all.

And any comments on the current status of noecc and MTD_OOB_RAW from
others would be highly valuable to me; a bit of system information and
a "working" or "not working since commit [XXX]" would be a start. In
the meantime, I'm trying to get a hold of a wider variety of test
systems for myself for these kind of issues...

>> -     ops.mode = MTD_OOB_PLACE;
>> +     ops.mode = (mfi->mode == MTD_MODE_RAW) ? MTD_OOB_RAW : MTD_OOB_PLACE;
>
> Would be really great to document things while you are in it - e.g.,
> putting a little note in include/linux/mtd/mtd.h that MTD_OOB_PLACE is
> the default mode. And of course documenting the modes in this file a bit
> more would be also great. I encourage you to store the wisdom you gain
> in the comments :-)

I'll see what I can do. Probably a task for after the dust settles on
some of the changes I'm working on.

Brian

Re: [RFC 3/5] mtd: do not assume oobsize is power of 2

[Brian Norris]

On Mon, Aug 22, 2011 at 1:46 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> Once you double checked that you caught all the places which assume
> power-of-2 and do things like "x & (mtd->oobsize - 1", and once you
> confirmed that you did some basic test by running few nandwrite commands
> which involve OOB operations, I think this patch can go in
> independently.

I'll do a little more combing through the code and perform some
independent tests for this patch, then either resend an amended patch
or comment back here.

> IOW, this does need to be RFC, IMO.

I agree. I think I lumped it in with the others because I discovered
it as a byproduct of my work on the other patches in this series.

Brian

Re: [RFC 4/5] mtd: move mtd_oob_mode_t to shared kernel/user space

[Brian Norris]

On Mon, Aug 22, 2011 at 1:50 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
>> +typedef enum {
>> +     MTD_OOB_PLACE,
>> +     MTD_OOB_AUTO,
>> +     MTD_OOB_RAW,
>> +} mtd_oob_mode_t;
>
> Could we get rid of this typedef and use anonymous enum instead:
>
> enum {
>        A,
>        B,
> };
>
> Indeed, we do not need it, and we do not want to pollute the user-space
> namespaces unnecessarily.

Well, we do *use* the typedef in a few kernel structs, and we will use
it in user space as well. So we may run into issues with 32-bit vs.
64-bit integers if we just stick with an int-based type, right?

Also, I'm pondering the question: do we need to worry about the
underlying numbering for such an enum? I believe there it is
theoretically possible for different compilers to choose a different
numbering scheme, potentially making user-compiled software
incompatible with the internal kernel binary. Perhaps to be safe we
could just do:

enum {
     MTD_OOB_PLACE = 0,
     MTD_OOB_AUTO = 1,
     MTD_OOB_RAW = 2,
};

Or instead, maybe we should just switch to a __u32 type and some #defines...

Sorry if this is a lot of thinking out loud here :)

Brian

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Brian Norris]

On Mon, Aug 22, 2011 at 3:02 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
>> Does MTD_MODE_RAW imply MTD_OOB_RAW when OOB operations are involved?
>
> I would make sure these 2 set of macros have different prefixes.
> Probably re-naming the internal ones is easier, but I'd anyway re-named
> the external ones - indeed they are about access mode for a specific
> file descriptor, so I'd use the "MTD_FILE_MODE_" prefix.

To be sure, are you categorizing the internal/external modes as follows?

External = MTD_MODE_NORMAL, MTD_MODE_RAW, etc.
Internal = MTD_OOB_PLACE, MTD_OOB_AUTO, MTD_OOB_RAW

Because my proposal here is to move those Internal ones to becoming a
new set of External modes for the purpose of the new MEMWRITEDATAOOB.
Finding good names for them makes more sense now than after they
become External. But as you say, renaming the *current* External modes
could help clarify things as well.

>> Does MTD_MODE_RAW imply that no ECC is applied to data?
>
> Not sure, but judging from how it is used - no.

I see at least one instance of code that seems to assume the answer is
"yes"; see the "noecc" code in nandwrite.c. Where do you find evidence
that says "no"?

>> Is the MTD file mode persistent?
>
> It is stored in the file descriptor (file->private_data), so the
> life-time of the mode is equivalent to the life-time for the file
> descriptor, AFAICS.

OK.

> I think MEMWRITEDATAOOB should ignore the mode.

Sure. May be easier said than done though. It looks like there are too
many entry points into the MTD layer, with different ioctls plus the
standard read/write controls. I'll see what I can do about keeping
this under control.

> My quick look at the code suggests me that the RAW/NORMAL file modes is
> about read/write operations - in normal mode we read/write from/to only
> the main area, in raw mode we consider the main area and OOB as one
> continuous region and read/write from/to this region.

I'm not sure I understand where the part about continuous region comes
from, but I think I understand some more here... MTDFILEMODE only
affects calls to mtd_write and mtd_read. It does not currently do
anything for the MEM{READ,WRITE}OOB[64] ioctls. This has been a point
of confusion for me and for others, I think.

It looks like going forward, we should agree on something like the
following, although the names still may change:

1) the generic term "raw" mode means that we are writing or reading
data exactly as-is to/from the flash, without error correction applied
to it.
2) MTD_OOB_RAW means means that we read or write in raw mode. This can
be directly user-controlled from the new MEMWRITEDATAOOB ioctl.
3) MTD_MODE_RAW is used only with the MTDFILEMODE ioctl, and it *only*
affects error correction for read() and write() calls to the
corresponding file descriptor. Its sole purpose is to enable
MTD_OOB_RAW during standard read/write operations (no OOB
interaction).

Unfortunately, item 3 showcases our naming inconsistency, since
MTD_OOB_RAW is actually utilized for a non-OOB operation. Perhaps this
should be considered for renaming?

[Regarding MTD_MODE_RAW:]
>> Maybe the "something" to be done would just be some better
>> documentation. Or something more drastic if there's an actual conflict.
>
> To me it looks like (a) re-naming and (be) some more comments in the
> header files is enough.

OK, I may rename, and I'll try to add more comments when we've got the code set.

Speaking of renaming, I'm not so sure about the name I gave this
ioctl; which of the following makes the most sense for naming the new
ioctl? Speak soon or forever hold your peace.
1) MEMWRITEDATAOOB
2) MEMWRITE
3) MEMWRITEOPS
4) ...something else?

Brian

Re: [RFC 5/5] mtd: add MEMWRITEDATAOOB ioctl

[Brian Norris]

On Mon, Aug 22, 2011 at 1:56 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
>> +                     ops.datbuf = memdup_user(
>> +                                     (void __user *)(uintptr_t)buf.usr_ptr,
>> +                                     ops.len + ops.ooblen);
>
> I'd introduced a local variable for buf.usr_ptr and made the formatting
> nicer.

Sure. Will incorporate into v2.

>> +struct mtd_data_oob_buf {
>
> Let's add some reserved space for future improvements - I think it is
> always a good idea to do for ioctls:
>
> + __u8 padding[8]

OK. Will consider for v2.

>> +#define MEMWRITEDATAOOB              _IOWR('M', 24, struct mtd_data_oob_buf)
>
> Would be greate to add a short comment about what this ioctl does. Of
> course you can optionally do this for all of them, but at least for the
> new one it does not hurt to have.

Sure, will do. I'll probably add at least a few comments in the header
for all the relevant ioctls we've been discussing, but that'll wait
until we've finished the code.

BTW, I'm considering splitting the usr_ptr into separate buffers for
data and oob. This will probably be a little easier for the user
interface as well as for internal kernel operations. Anyway, the
resulting struct is looking more and more like the existing `struct
mtd_oob_ops' (this is kind of by design); is it still a good idea to
keep the user-facing struct independent of the internal mtd_oob_ops
struct? I'm thinking it would leave some flexibility for the future.

Brian

Re: [RFC 1/5] mtd: support MTD_MODE_RAW for writing OOB

[Artem Bityutskiy]

On Mon, 2011-08-22 at 13:08 -0700, Brian Norris wrote:
> I forgot to CC a contributor on this (and the complementary patch for
> "read OOB")
> 
> Cc: Jim Quinlan <jim2101024@gmail.com>
> 
> On Mon, Aug 22, 2011 at 1:35 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> >> This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls.
> >
> > I guess this patch deserves to be non-RFC? Should it be pushed to
> > l2-mtd-2.6.git? Should it even have "Cc: stable@kernel.org [kernel
> > version +] ?
> 
> I'm not actually sure where this stands (RFC vs. patch) since I really
> wanted some outside opinion on the methods used here. I have a feeling
> that some of this is only working on my hardware. For instance,
> somehow (I'm really not sure how!) `nandwrite -n -o' is working in
> nandsim without my fix. Perhaps this is due to a different set of
> nand_ecc_ctrl functions (soft ECC vs. HW ECC).

May be one of the reasons is that nandsim just _copies_ data to the
internal RAM buffer, instead of doing binary "&", so you can re-write in
some cases, but not sure.

> With this patch applied, however, I get some strange kernel oopses
> with nandsim. I've identified at least one issue, I think, but I
> haven't completely resolved this discrepancy between nandsim and my
> driver. See sample commands:
> 
> # insmod nandsim.ko
> # nandwrite /dev/mtdX <data.bin> -n -o
> 
> So for this "fix" (and its coming updates), I would appreciate some
> outside testing on other systems, especially before we send this to
> stable or even before including it upstream at all.
> 
> And any comments on the current status of noecc and MTD_OOB_RAW from
> others would be highly valuable to me; a bit of system information and
> a "working" or "not working since commit [XXX]" would be a start. In
> the meantime, I'm trying to get a hold of a wider variety of test
> systems for myself for these kind of issues...

Yes, would be great to have more people to test. I always encourage
people to look at patches _before_ they get in and break their systems,
not afterwards. But even if nobody cares, we can merge your stuff after
you gave it some more testing - absence of caring people should not stop
the progress in MTD :-) 

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 1/5] mtd: support MTD_MODE_RAW for writing OOB

[Jason Liu]

Hi, Brian,

2011/8/18 Brian Norris <computersforpeace@gmail.com>:
> mtd_do_write_oob does not pass information about whether to write in RAW
> mode to the lower layers (e.g., NAND). We should pass MTD_OOB_RAW or
> MTD_OOB_PLACE based on the MTD file mode.
>
> This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls.

It's worthy here stating clear what the issue you meet and how it fix.

Jason

Re: [RFC 4/5] mtd: move mtd_oob_mode_t to shared kernel/user space

[Artem Bityutskiy]

On Mon, 2011-08-22 at 14:43 -0700, Brian Norris wrote:
> On Mon, Aug 22, 2011 at 1:50 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> >> +typedef enum {
> >> +     MTD_OOB_PLACE,
> >> +     MTD_OOB_AUTO,
> >> +     MTD_OOB_RAW,
> >> +} mtd_oob_mode_t;
> >
> > Could we get rid of this typedef and use anonymous enum instead:
> >
> > enum {
> >        A,
> >        B,
> > };
> >
> > Indeed, we do not need it, and we do not want to pollute the user-space
> > namespaces unnecessarily.
> 
> Well, we do *use* the typedef in a few kernel structs, and we will use
> it in user space as well. So we may run into issues with 32-bit vs.
> 64-bit integers if we just stick with an int-based type, right?

I just find typedefs unreadable. Every time you see it - you need to
look at the definition, find it, distract. And C does not do any type
checking anway, so typedefs for enums really make little sense. If you
really do not want to use int, let's use 'enum blah' type, which is at
least readable - when you see it - you know it is just an integer, and
you do not have to find the definition.

Anonymous enums is my preference. They are just like macro definition,
but with assumed type, and potentially debugger-friendly.

So, to conclude:

1. typedefs for enums make no sense a all - C does not do any strict
type-checking anyway, and typedefs only make reading code more
difficult.

2. using 'enum blah' is more sensible - it does not hurt readability too
much at least.

3. IMHO, and I even dare to say that many kernel gurus would agree, if
we are talking about a simple enum with just few constants inside -
anonymous enum is the best - the code is simpler and more readable.

It is not that difficult to remove in-kernel usage of typedefs, I think.
VS user-space - the only user we care about - mtd-utils - has its own
copy of the headers, and if we update the header files there, we can
amend mtd-utils.

32/64 problems - what do you mean? enumeration = 'int' which is always
32 bits in all architectures Linux supports.

References:
1. C99 standard, section 6.4.4.3: enumeration = int
2. C99 section 6.2.5, marker 20: about enum = just definition of a
constant and _not_ a typed form.

So, I'd be happy with 3, can live with 2, and dislike 1 very much :-)

> Also, I'm pondering the question: do we need to worry about the
> underlying numbering for such an enum? I believe there it is
> theoretically possible for different compilers to choose a different
> numbering scheme, potentially making user-compiled software
> incompatible with the internal kernel binary. Perhaps to be safe we
> could just do:
> 
> enum {
>      MTD_OOB_PLACE = 0,
>      MTD_OOB_AUTO = 1,
>      MTD_OOB_RAW = 2,
> };

I cannot refer to a section in a standard, but I am sure unitialized
tags will start with 0 reliably. But yes, it is much less error-prone to
initialize the tags explicitly, because it makes it a bit more difficult
to make a stupid mistake by adding a new constant and change values of
other constants.

> Or instead, maybe we should just switch to a __u32 type and some #defines...

Anonymous enum is almost that, you can just treat it as int32_t.

> Sorry if this is a lot of thinking out loud here :)

Yeah, least significant topics usually cause most discussions - I was
told recently by a colleague that this is called "bike shedding". Notice
how much I wrote comparing to more important posts of yours :-)

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Artem Bityutskiy]

On Mon, 2011-08-22 at 16:42 -0700, Brian Norris wrote:
> On Mon, Aug 22, 2011 at 3:02 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> >> Does MTD_MODE_RAW imply MTD_OOB_RAW when OOB operations are involved?
> >
> > I would make sure these 2 set of macros have different prefixes.
> > Probably re-naming the internal ones is easier, but I'd anyway re-named
> > the external ones - indeed they are about access mode for a specific
> > file descriptor, so I'd use the "MTD_FILE_MODE_" prefix.
> 
> To be sure, are you categorizing the internal/external modes as follows?
> 
> External = MTD_MODE_NORMAL, MTD_MODE_RAW, etc.
> Internal = MTD_OOB_PLACE, MTD_OOB_AUTO, MTD_OOB_RAW

Yes :-)

> Because my proposal here is to move those Internal ones to becoming a
> new set of External modes for the purpose of the new MEMWRITEDATAOOB.

Right, I was talking about what is in mainline at the moment.

> Finding good names for them makes more sense now than after they
> become External. But as you say, renaming the *current* External modes
> could help clarify things as well.

I think you can re-name any of them nicely - we can always amend
mtd-utils.

> 
> >> Does MTD_MODE_RAW imply that no ECC is applied to data?
> >
> > Not sure, but judging from how it is used - no.
> 
> I see at least one instance of code that seems to assume the answer is
> "yes"; see the "noecc" code in nandwrite.c. Where do you find evidence
> that says "no"?

You are right, this looks like "no ECC" actually.

Well, my cscopes find only 2 places where this symbol is used in the
kernel:

   2    220  drivers/mtd/mtdchar.c <<mtd_read>>
             case MTD_MODE_RAW:
   3    316  drivers/mtd/mtdchar.c <<mtd_write>>
             case MTD_MODE_RAW:

So this is only about reading/writing and nothing else.

Then, let's look at one of them, say 3:

drivers/mtd/mtdchar.c: ret = mtd->write_oob(mtd, *ppos, &ops);

Then find out in nand_base.c that this maps to 'nand_write_oob()',
which, in-turn calls then 'nand_do_write_ops()', and which then calls,
_I guess_:

               ret = chip->write_page(mtd, chip, wbuf, page, cached,
                                       (ops->mode == MTD_OOB_RAW));

which then calls:

chip->ecc.write_page_raw(mtd, chip, buf);

which seems to be the "no ECC" write function.

But I guess MEMWRITEDATAOOB should anyway "override" the "global" mode.

> > I think MEMWRITEDATAOOB should ignore the mode.
> 
> Sure. May be easier said than done though. It looks like there are too
> many entry points into the MTD layer, with different ioctls plus the
> standard read/write controls. I'll see what I can do about keeping
> this under control.

Yeah, it is a mess, I agree :-(

> It looks like going forward, we should agree on something like the
> following, although the names still may change:
> 
> 1) the generic term "raw" mode means that we are writing or reading
> data exactly as-is to/from the flash, without error correction applied
> to it.

Right.

> 2) MTD_OOB_RAW means means that we read or write in raw mode. This can
> be directly user-controlled from the new MEMWRITEDATAOOB ioctl.

OK

> 3) MTD_MODE_RAW is used only with the MTDFILEMODE ioctl, and it *only*
> affects error correction for read() and write() calls to the
> corresponding file descriptor. Its sole purpose is to enable
> MTD_OOB_RAW during standard read/write operations (no OOB
> interaction).

OK

> Unfortunately, item 3 showcases our naming inconsistency, since
> MTD_OOB_RAW is actually utilized for a non-OOB operation. Perhaps this
> should be considered for renaming?

Probably the "OOB" part should go away? Give the "per-file" modes some
other name which suggests that these are old-fashioned and then use
MTD_RW_MODE_ or even just MTD_MODE_ for the "internal" constants?

> Speaking of renaming, I'm not so sure about the name I gave this
> ioctl; which of the following makes the most sense for naming the new
> ioctl? Speak soon or forever hold your peace.
> 1) MEMWRITEDATAOOB
> 2) MEMWRITE
> 3) MEMWRITEOPS
> 4) ...something else?

1 or 2 seems OK to me, but I do not have a strong opinion here.

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 5/5] mtd: add MEMWRITEDATAOOB ioctl

[Artem Bityutskiy]

On Mon, 2011-08-22 at 17:04 -0700, Brian Norris wrote:
> BTW, I'm considering splitting the usr_ptr into separate buffers for
> data and oob. This will probably be a little easier for the user
> interface as well as for internal kernel operations. Anyway, the
> resulting struct is looking more and more like the existing `struct
> mtd_oob_ops' (this is kind of by design); is it still a good idea to
> keep the user-facing struct independent of the internal mtd_oob_ops
> struct? I'm thinking it would leave some flexibility for the future. 

Yes, it is good idea, and you anyway cannot use the same struct for
both, because structs for ioctl have more limitation WRT struct size
(best to make it to be multiple of 64-bits), fileds (no pointers should
be used, better types like _u32 should be used), reserve for future (a
pool of bytes which user must set to 0 and which we can use in future).

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 5/5] mtd: add MEMWRITEDATAOOB ioctl

[Artem Bityutskiy]

On Mon, 2011-08-22 at 17:04 -0700, Brian Norris wrote:
> > Let's add some reserved space for future improvements - I think it is
> > always a good idea to do for ioctls:
> >
> > + __u8 padding[8]
> 
> OK. Will consider for v2.

And add a comment that the user has to set them to 0. This way, when in
the future we want to use them, we'll know that 0 means "original"
behavior.

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 5/5] mtd: add MEMWRITEDATAOOB ioctl

[Artem Bityutskiy]

On Mon, 2011-08-22 at 17:04 -0700, Brian Norris wrote:
> On Mon, Aug 22, 2011 at 1:56 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
> >> +                     ops.datbuf = memdup_user(
> >> +                                     (void __user *)(uintptr_t)buf.usr_ptr,
> >> +                                     ops.len + ops.ooblen);
> >
> > I'd introduced a local variable for buf.usr_ptr and made the formatting
> > nicer.
> 
> Sure. Will incorporate into v2.
> 
> >> +struct mtd_data_oob_buf {
> >
> > Let's add some reserved space for future improvements - I think it is
> > always a good idea to do for ioctls:
> >
> > + __u8 padding[8]
> 
> OK. Will consider for v2.
> 
> >> +#define MEMWRITEDATAOOB              _IOWR('M', 24, struct mtd_data_oob_buf)
> >
> > Would be greate to add a short comment about what this ioctl does. Of
> > course you can optionally do this for all of them, but at least for the
> > new one it does not hurt to have.
> 
> Sure, will do. I'll probably add at least a few comments in the header
> for all the relevant ioctls we've been discussing, but that'll wait
> until we've finished the code.
> 
> BTW, I'm considering splitting the usr_ptr into separate buffers for
> data and oob. This will probably be a little easier for the user
> interface as well as for internal kernel operations. Anyway, the
> resulting struct is looking more and more like the existing `struct
> mtd_oob_ops' (this is kind of by design); is it still a good idea to
> keep the user-facing struct independent of the internal mtd_oob_ops
> struct? I'm thinking it would leave some flexibility for the future.

BTW, being consistent and adding an "_user" or "_req" (request) or other
suffix to all the ioctl request structures is a good idea. But this is
again IMHO, and some people may consider this to be too pedantic.
Anyway, for UBI ioctls all the structures end with "_req", you can see
the example: include/mtd/ubi-user.h

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Ricard Wanderlof]

On Mon, 22 Aug 2011, Artem Bityutskiy wrote:

> On Mon, 2011-08-22 at 14:04 +0200, Ivan Djelic wrote:
>> On Mon, Aug 22, 2011 at 11:02:40AM +0100, Artem Bityutskiy wrote:
>>> Hi,
>>>
>>> On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
>>>> First off, these fixes are motivated by a number of things:
>>>>
>>>> 1) Broken "noecc" writes; `nandwrite -n -o' does not work on my
>>>>    hardware, at least, without these patches
>>>
>>> I think Ivan complained about this at some point - he said he has images
>>> with crafted ECC codes to test his BCH library, but he could not flash
>>> them properly. So yes, this is worth fixing.
>>
>> Actually it was Ricard Wanderlof, but indeed he was testing the BCH library
>> (http://lists.infradead.org/pipermail/linux-mtd/2011-March/034516.html).
>> It would be nice if he could ack the fix ?

(Hi guys, just got back, been away for a while over the summer).

This particular issue was fixed in a patch by Peter Wippich on June 6th:

http://lists.infradead.org/pipermail/linux-mtd/2011-June/036016.html

(I subsequently tested the patch and found it to fix the problem: 
http://lists.infradead.org/pipermail/linux-mtd/2011-June/036085.html).

I could test patch 1 of this set to see if it accomplishes the same thing, 
but initially to me it seems that the issue has already been fixed, albeit 
perhaps not pushed yet?

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Brian Norris]

On Mon, Aug 22, 2011 at 11:48 PM, Ricard Wanderlof
<ricard.wanderlof@axis.com> wrote:
>>>> On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
>>>>> 1) Broken "noecc" writes; `nandwrite -n -o' does not work on my
>>>>>   hardware, at least, without these patches
>
> This particular issue was fixed in a patch by Peter Wippich on June 6th:
>
> http://lists.infradead.org/pipermail/linux-mtd/2011-June/036016.html

Right, I actually found this patch already and have it lined up in my
test kernel, but apparently my issue is separate.

I will try to describe my issue further on the thread for patch 1.
Jason Liu has asked about the details there.

> I could test patch 1 of this set to see if it accomplishes the same thing,
> but initially to me it seems that the issue has already been fixed, albeit
> perhaps not pushed yet?

I am now doubting that my fix will solve your problem like Peter
Wippich's patch did. I believe the problem I am having is specifically
related to my NAND controller's characteristics (how it handles ECC).

So for hardware on which Peter's fix solves your problems, I suppose I
would be interested mostly in seeing if my patch *breaks* your build.
FYI, I already see an issue for those who use both the builtin
`nand_write_page_raw()' and `nand_write_buf()' functions. It can be
fixed if you amend my patch with the following. I can resend later,
but test these for breakage if you can!

--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1898,7 +1898,8 @@ out:
 static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
                                const uint8_t *buf)
 {
-       chip->write_buf(mtd, buf, mtd->writesize);
+       if (buf)
+               chip->write_buf(mtd, buf, mtd->writesize);
        chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
 }

Re: [RFC 4/5] mtd: move mtd_oob_mode_t to shared kernel/user space

[Brian Norris]

On Mon, Aug 22, 2011 at 10:30 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> 32/64 problems - what do you mean? enumeration = 'int' which is always
> 32 bits in all architectures Linux supports.

Oops, got things mixed up here (ints and pointers). Thanks for the
correction. My worry was unfounded...

> 3. IMHO, and I even dare to say that many kernel gurus would agree, if
> we are talking about a simple enum with just few constants inside -
> anonymous enum is the best - the code is simpler and more readable.
...
> So, I'd be happy with 3, can live with 2, and dislike 1 very much :-)

3 sounds perfect, now that I've been straightened out.

> I cannot refer to a section in a standard, but I am sure unitialized
> tags will start with 0 reliably. But yes, it is much less error-prone to
> initialize the tags explicitly, because it makes it a bit more difficult
> to make a stupid mistake by adding a new constant and change values of
> other constants.

It's also useful if we have to kill an old option at some point. I'll
probably include initializers.

>> Sorry if this is a lot of thinking out loud here :)
>
> Yeah, least significant topics usually cause most discussions - I was
> told recently by a colleague that this is called "bike shedding". Notice
> how much I wrote comparing to more important posts of yours :-)

I actually learned/recalled a few things here; I had pieces of the
puzzle but not the whole picture. And the "bike shed" is an important
addition to my vocabulary.

Thanks,
Brian

Re: [RFC 1/5] mtd: support MTD_MODE_RAW for writing OOB

[Brian Norris]

On Mon, Aug 22, 2011 at 10:25 PM, Jason Liu <liu.h.jason@gmail.com> wrote:
> 2011/8/18 Brian Norris <computersforpeace@gmail.com>:
>> This fixes issues with `nandwrite -n' and the MEMWRITEOOB[64] ioctls.
>
> It's worthy here stating clear what the issue you meet and how it fix.

The issue is simply that `nandwrite -n -o' does not work on my system;
`nandwrite -n' can write page data to my flash without applying ECC,
but when I use the `-o' option, ECC is applied (incorrectly), contrary
to the `--noecc' option.

I found that this is the case because my hardware computes and writes
ECC data to flash upon either OOB write or page write. Thus, to
support a proper "no ECC" write, my driver must know when we're
performing a raw OOB write vs. a normal ECC OOB write. However, MTD
does not pass any raw mode information to the write_oob functions.
This patch addresses the problems by:
1) Passing MTD_OOB_RAW down to lower layers, instead of just
defaulting to MTD_OOB_PLACE
2) Handling MTD_OOB_RAW within the NAND layer's `nand_do_write_oob'

Is this a better description? I'll include this description for v2.

Note that I have yet to update the appropriate `nand_write_page_raw'
function to properly handle an OOB-only write. I've fixed my own
driver, but there's an issue in the nand_base version. Will include in
v2.

Thanks,
Brian

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Ricard Wanderlof]

On Tue, 23 Aug 2011, Brian Norris wrote:

> On Mon, Aug 22, 2011 at 11:48 PM, Ricard Wanderlof
> <ricard.wanderlof@axis.com> wrote:
>>>>> On Wed, 2011-08-17 at 16:50 -0700, Brian Norris wrote:
>>>>>> 1) Broken "noecc" writes; `nandwrite -n -o' does not work on my
>>>>>>   hardware, at least, without these patches
>>
>> This particular issue was fixed in a patch by Peter Wippich on June 6th:
>>
>> http://lists.infradead.org/pipermail/linux-mtd/2011-June/036016.html
>
> Right, I actually found this patch already and have it lined up in my
> test kernel, but apparently my issue is separate.
>
>> I could test patch 1 of this set to see if it accomplishes the same thing,
>> but initially to me it seems that the issue has already been fixed, albeit
>> perhaps not pushed yet?
>
> I am now doubting that my fix will solve your problem like Peter
> Wippich's patch did. I believe the problem I am having is specifically
> related to my NAND controller's characteristics (how it handles ECC).
>
> So for hardware on which Peter's fix solves your problems, I suppose I
> would be interested mostly in seeing if my patch *breaks* your build.
> FYI, I already see an issue for those who use both the builtin
> `nand_write_page_raw()' and `nand_write_buf()' functions. It can be
> fixed if you amend my patch with the following. I can resend later,
> but test these for breakage if you can!

Ok. I tried applying patch #1 of your patch set, to a tree in which 
Peter's patch has already been applying, to see what would happen. I had a 
problem in that in the mtdchar.c I have it looks like this:

      memset(chip->oob_poi, 0xff, mtd->oobsize);
      nand_fill_oob(chip, ops->oobbuf, ops->ooblen, ops);
      status = chip->ecc.write_oob(mtd, chip, page & chip->pagemask);
      memset(chip->oob_poi, 0xff, mtd->oobsize);

whereas your patch looks like it was made against a version which lacks 
the memsets. First I thought it was because I was running an older kernel 
(2.6.35), but I looked at HEAD of the linux-2.6 and mtd-2.6 trees at 
git.infradead.org, and it's the same there. So I'm not sure exactly which 
version your patch was made against. Perhaps it's obvious to someone but 
not me right now.

Nevertheless, after applying the patch, as you suspected, using 'nandwrite 
-o -n' fails, in this case the application hangs after outputting

Writing data to block 0

After applying the following patch from your email, 'nandwrite -o -n' 
initially appears to work, however upon closer inspection the oob's of all 
the written pages are all-ff, i.e. the oob's are never written.

> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1898,7 +1898,8 @@ out:
> static void nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
>                                const uint8_t *buf)
> {
> -       chip->write_buf(mtd, buf, mtd->writesize);
> +       if (buf)
> +               chip->write_buf(mtd, buf, mtd->writesize);
>        chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> }
>

Now, this may all be nonsense as as I mentioned I'm not sure that I've had 
the right version to start with. The memsets bug me, as they would explain 
the all-ff's stuff, but I don't really feel like just experimenting.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Brian Norris]

On Wed, Aug 24, 2011 at 8:36 AM, Ricard Wanderlof
<ricard.wanderlof@axis.com> wrote:
...
> I had a problem
> in that in the mtdchar.c I have it looks like this:

Did you mean nand_base.c?

> whereas your patch looks like it was made against a version which lacks the
> memsets. First I thought it was because I was running an older kernel
> (2.6.35), but I looked at HEAD of the linux-2.6 and mtd-2.6 trees at
> git.infradead.org, and it's the same there. So I'm not sure exactly which
> version your patch was made against. Perhaps it's obvious to someone but not
> me right now.

My patches were based on l2-mtd-2.6.git, actually. David Woodhouse
rarely pulls patches into his mtd-2.6 tree, so I have moved to working
with Artem's l2-mtd-2.6 tree, where all the MTD work that's waiting
for upstream sits (some stuff's been there since May). This is not
obvious, and usually when it matters, I try to mention it in the patch
summaries.

Artem: is there any official change in policy on patch submission? I
see documentation that says to base off mtd-2.6.git, but I've been
using l2-mtd-2.6 to help you avoid merge conflicts:
http://www.linux-mtd.infradead.org/source.html

Anyway, I believe the relevant patch dependency (from l2-mtd-2.6.git) is:

   commit a8ee364bbf14861d5d0af39c4da06c30441895fb
   Author: THOMSON, Adam (Adam) <adam.thomson@alcatel-lucent.com>
   mtd: nand_base: always initialise oob_poi before writing OOB data

> Nevertheless, after applying the patch, as you suspected, using 'nandwrite
> -o -n' fails, in this case the application hangs after outputting
>
> Writing data to block 0

I guessed you would be more likely to get a segmentation fault on a
NULL pointer, but I may be wrong.

> Now, this may all be nonsense as as I mentioned I'm not sure that I've had
> the right version to start with. The memsets bug me, as they would explain
> the all-ff's stuff, but I don't really feel like just experimenting.

Right, that's fair enough. I still think this patch is not 100% ready,
anyway. Thanks for the help though!

If you're still up for trying, you can try applying Adam Thomson's
patch, then my RFC, then the inlined amendment I sent. With your
feedback, I will

I need to figure out structurally how to handle the differences
between hardware that uses the standard functions (and works fine
without my patch) and hardware like mine that needs more information
passed to it. Perhaps there needs to be a replaceable
`chip->ecc.write_oob_raw' function? Or at least some modifications to
the other "replaceable" raw functions.

Brian

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Artem Bityutskiy]

On Wed, 2011-08-24 at 11:01 -0700, Brian Norris wrote:
> On Wed, Aug 24, 2011 at 8:36 AM, Ricard Wanderlof
> <ricard.wanderlof@axis.com> wrote:
> ...
> > I had a problem
> > in that in the mtdchar.c I have it looks like this:
> 
> Did you mean nand_base.c?
> 
> > whereas your patch looks like it was made against a version which lacks the
> > memsets. First I thought it was because I was running an older kernel
> > (2.6.35), but I looked at HEAD of the linux-2.6 and mtd-2.6 trees at
> > git.infradead.org, and it's the same there. So I'm not sure exactly which
> > version your patch was made against. Perhaps it's obvious to someone but not
> > me right now.
> 
> My patches were based on l2-mtd-2.6.git, actually. David Woodhouse
> rarely pulls patches into his mtd-2.6 tree, so I have moved to working
> with Artem's l2-mtd-2.6 tree, where all the MTD work that's waiting
> for upstream sits (some stuff's been there since May). This is not
> obvious, and usually when it matters, I try to mention it in the patch
> summaries.

David's tree is desperately out-of-date now, I did not talk to him
lately, he is not very reachable now.

There are patches from May because David did merge anything this merge
window, probably he had some issues/etc, let's hope he'll merge
everything next merge window. May be he wanted to ask me to merge it,
but I have been having vacation and was not available at the IRC chat.

> Artem: is there any official change in policy on patch submission? I
> see documentation that says to base off mtd-2.6.git, but I've been
> using l2-mtd-2.6 to help you avoid merge conflicts:
> http://www.linux-mtd.infradead.org/source.html

There is no official policy, this all works because enthusiasts who just
like MTD stuff and keep it alive. When I noticed that dwmw2 does not
give MTD ML enough attention, I just started my l2 tree to help him - it
was faster/easier for him to look with reviewed patches in my tree
rather than look through whole MTD ML, find out which acks/reviewed-by
to add and where, which patch versions are out of date, etc. 

At this point I think, that you have to use the l2 tree, because David's
tree is very out-of-date. Also, beware that the l2 tree is currently in
linux-next.

-- 
Best Regards,
Artem Bityutskiy

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Ricard Wanderlof]

On Wed, 24 Aug 2011, Brian Norris wrote:

> On Wed, Aug 24, 2011 at 8:36 AM, Ricard Wanderlof
> <ricard.wanderlof@axis.com> wrote:
> ...
>> I had a problem
>> in that in the mtdchar.c I have it looks like this:
>
> Did you mean nand_base.c?

Yes, sorry about that.

>> whereas your patch looks like it was made against a version which lacks the
>> memsets. First I thought it was because I was running an older kernel
>> ...
> My patches were based on l2-mtd-2.6.git, actually. David Woodhouse
> rarely pulls patches into his mtd-2.6 tree, so I have moved to working
> with Artem's l2-mtd-2.6 tree, where all the MTD work that's waiting
> for upstream sits (some stuff's been there since May). This is not
> obvious, and usually when it matters, I try to mention it in the patch
> summaries.

I guess I should have thought of it but never thought to look there.

> Anyway, I believe the relevant patch dependency (from l2-mtd-2.6.git) is:
>
>   commit a8ee364bbf14861d5d0af39c4da06c30441895fb
>   Author: THOMSON, Adam (Adam) <adam.thomson@alcatel-lucent.com>
>   mtd: nand_base: always initialise oob_poi before writing OOB data

That seems right.

>> Nevertheless, after applying the patch, as you suspected, using 'nandwrite
>> -o -n' fails, in this case the application hangs after outputting
>>
>> Writing data to block 0
>
> I guessed you would be more likely to get a segmentation fault on a
> NULL pointer, but I may be wrong.

Something seems to hang at a lower level, because the system becomes 
unresponsive at this point (I can telnet in, but trying to access the 
flash with ls for instance causes the shell to hang).

> If you're still up for trying, you can try applying Adam Thomson's
> patch, then my RFC, then the inlined amendment I sent. With your
> feedback, I will

(something seems missing here ... nevertheless)

I applied the patches as you mentioned, which brought success. Dumping a 
partition with nanddump, then writing it back with nandwrite -o -n results 
in the correct data being written both to the main and oob areas.

(Without your inlinend patch, the nandwrite application still hangs after 
Writing to data block 0).

So the conclusion would be that this combination of patches does not break 
Peter Wippich's patch.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Brian Norris]

Hi Ricard,

On Thu, Aug 25, 2011 at 2:33 AM, Ricard Wanderlof
<ricard.wanderlof@axis.com> wrote:
> On Wed, 24 Aug 2011, Brian Norris wrote:
>> On Wed, Aug 24, 2011 at 8:36 AM, Ricard Wanderlof <ricard.wanderlof@axis.com> wrote:
>> Anyway, I believe the relevant patch dependency (from l2-mtd-2.6.git) is:
>>
>>  commit a8ee364bbf14861d5d0af39c4da06c30441895fb
>>  Author: THOMSON, Adam (Adam) <adam.thomson@alcatel-lucent.com>
>>  mtd: nand_base: always initialise oob_poi before writing OOB data
>
> That seems right.

BTW, that patch seems a bit broken to me; I sent a fixup for it:
http://lists.infradead.org/pipermail/linux-mtd/2011-August/037698.html

>> I guessed you would be more likely to get a segmentation fault on a
>> NULL pointer, but I may be wrong.
>
> Something seems to hang at a lower level, because the system becomes
> unresponsive at this point (I can telnet in, but trying to access the flash
> with ls for instance causes the shell to hang).

I think that I had a little bit of the wrong approach. I was doing
some ill advised hacking to the existing write functions. I spun off
my first two patches here as a different series (I believe I CC'd
you); I used a different approach that should make as little impact on
currently working hardware as possible:
http://lists.infradead.org/pipermail/linux-mtd/2011-August/037695.html

> (something seems missing here ...

Acknowledged :)

> I applied the patches as you mentioned, which brought success. Dumping a
> partition with nanddump, then writing it back with nandwrite -o -n results
> in the correct data being written both to the main and oob areas.

Great!

> (Without your inlinend patch, the nandwrite application still hangs after
> Writing to data block 0).

Not quite sure what the hanging is all about - may be related to my
improper use of CMD_SEQIN, then a nand wait function being called
later? - but that's why I've changed my approach.

> So the conclusion would be that this combination of patches does not break
> Peter Wippich's patch.

Thanks a lot for the testing. I think that my first approach still may
easily have unintended consequences, though. I welcome any testing on
my new patch series, and any more systems with broken "noecc" should
be handled through that thread.

Brian

Re: [RFC 0/5] fix data+OOB writes, add ioctl

[Ricard Wanderlof]

On Thu, 25 Aug 2011, Brian Norris wrote:

>>>  Author: THOMSON, Adam (Adam) <adam.thomson@alcatel-lucent.com>
>>>  mtd: nand_base: always initialise oob_poi before writing OOB data
>>
>> That seems right.
>
> BTW, that patch seems a bit broken to me; I sent a fixup for it: 
> http://lists.infradead.org/pipermail/linux-mtd/2011-August/037698.html

Thanks, I did see it but it failed to register. I've applied it to my tree 
now.

> I think that I had a little bit of the wrong approach. I was doing
> some ill advised hacking to the existing write functions. I spun off
> my first two patches here as a different series (I believe I CC'd
> you); I used a different approach that should make as little impact on
> currently working hardware as possible:
> http://lists.infradead.org/pipermail/linux-mtd/2011-August/037695.html
>
> Thanks a lot for the testing. I think that my first approach still may 
> easily have unintended consequences, though. I welcome any testing on my 
> new patch series, and any more systems with broken "noecc" should be 
> handled through that thread.

I applied the two patches in the above mentioned patch series to my tree 
(after applying Peter's patch, Adam's, and your amendment to it). 
Conclusion: nandwrite -o -n still works for me, i.e. the patch doesn't 
break it.

One little caveat, the tree I'm using is 2.6.35 based. The patches don't 
apply cleanly, but it just takes a little effort. So I think the results 
are relevant, just wanted to mention it.

/Ricard
-- 
Ricard Wolf Wanderlöf                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30