[PATCH 1/5] mtd: do not assume oobsize is power of 2

[PATCH 1/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.

These masks are really only intended to hide the "page" portion of the
offset, leaving any OOB offset intact, so a masking with the writesize
(which *is* always a power of 2) is valid and makes more sense.

This has been tested for read/write of NAND devices (nanddump/nandwrite)
using nandsim and actual NAND flash.

Cc: stable@kernel.org [2.6.30+]
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
Changes since v1 as "RFC": instead of dropping the mask, we use
    writesize instead.

Stable: this error actually appeared way back in 2.6.17, but it was
    cut-and-pasted in the 2.6.30-rcX cycle, so it won't apply cleanly.
    Anyway, I don't think we don't maintain -stable that far back.

 drivers/mtd/mtdchar.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c
index a75d555..b206254 100644
--- a/drivers/mtd/mtdchar.c
+++ b/drivers/mtd/mtdchar.c
@@ -410,7 +410,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 & (mtd->writesize - 1);
 	ops.datbuf = NULL;
 	ops.mode = MTD_OOB_PLACE;
 
@@ -421,7 +421,7 @@ 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);
+	start &= ~((uint64_t)mtd->writesize - 1);
 	ret = mtd->write_oob(mtd, start, &ops);
 
 	if (ops.oobretlen > 0xFFFFFFFFU)
@@ -452,7 +452,7 @@ static int mtd_do_readoob(struct mtd_info *mtd, uint64_t start,
 		return ret;
 
 	ops.ooblen = length;
-	ops.ooboffs = start & (mtd->oobsize - 1);
+	ops.ooboffs = start & (mtd->writesize - 1);
 	ops.datbuf = NULL;
 	ops.mode = MTD_OOB_PLACE;
 
@@ -463,7 +463,7 @@ static int mtd_do_readoob(struct mtd_info *mtd, uint64_t start,
 	if (!ops.oobbuf)
 		return -ENOMEM;
 
-	start &= ~((uint64_t)mtd->oobsize - 1);
+	start &= ~((uint64_t)mtd->writesize - 1);
 	ret = mtd->read_oob(mtd, start, &ops);
 
 	if (put_user(ops.oobretlen, retp))
-- 
1.7.5.4

[PATCH 2/5] mtd: doc: remove mention of MEMSETOOBSEL

[Brian Norris]

It's been gone for a while.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 Documentation/DocBook/mtdnand.tmpl |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/Documentation/DocBook/mtdnand.tmpl b/Documentation/DocBook/mtdnand.tmpl
index 8c07540..0c674be 100644
--- a/Documentation/DocBook/mtdnand.tmpl
+++ b/Documentation/DocBook/mtdnand.tmpl
@@ -773,20 +773,6 @@ struct nand_oobinfo {
 				done according to the default builtin scheme.
 			</para>
 		</sect2>
-		<sect2 id="User_space_placement_selection">
-			<title>User space placement selection</title>
-		<para>
-			All non ecc functions like mtd->read and mtd->write use an internal 
-			structure, which can be set by an ioctl. This structure is preset 
-			to the autoplacement default.
-	     		<programlisting>
-	ioctl (fd, MEMSETOOBSEL, oobsel);
-	     		</programlisting>
-			oobsel is a pointer to a user supplied structure of type
-			nand_oobconfig. The contents of this structure must match the 
-			criteria of the filesystem, which will be used. See an example in utils/nandwrite.c.
-		</para>
-		</sect2>
 	</sect1>	
 	<sect1 id="Spare_area_autoplacement_default">
 		<title>Spare area autoplacement default schemes</title>
-- 
1.7.5.4

[PATCH 3/5] mtd: remove MEMSETOOBSEL macro definition

[Brian Norris]

MEMSETOOBSEL is completely unused and useless. Remove the definition.

Note: it's probably best not to use this ioctl number in the future for
MTD, since that may cause conflicts between old kernels and new user
software (or new kernels and old user software). This shouldn't be much
problem.

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

diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index 2f7d45b..f0e8027 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -112,7 +112,7 @@ struct otp_info {
 #define MEMUNLOCK		_IOW('M', 6, struct erase_info_user)
 #define MEMGETREGIONCOUNT	_IOR('M', 7, int)
 #define MEMGETREGIONINFO	_IOWR('M', 8, struct region_info_user)
-#define MEMSETOOBSEL		_IOW('M', 9, struct nand_oobinfo)
+
 #define MEMGETOOBSEL		_IOR('M', 10, struct nand_oobinfo)
 #define MEMGETBADBLOCK		_IOW('M', 11, __kernel_loff_t)
 #define MEMSETBADBLOCK		_IOW('M', 12, __kernel_loff_t)
-- 
1.7.5.4

Re: [PATCH 3/5] mtd: remove MEMSETOOBSEL macro definition

[Ricard Wanderlof]

On Wed, 24 Aug 2011, Brian Norris wrote:

> MEMSETOOBSEL is completely unused and useless. Remove the definition.
>
> Note: it's probably best not to use this ioctl number in the future for
> MTD, since that may cause conflicts between old kernels and new user
> software (or new kernels and old user software). This shouldn't be much
> problem.

Perhaps this should be noted as a comment in the code, rather than just 
removing the line. Such information tends to get forgotten sooner or later 
otherwise.

>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
> include/mtd/mtd-abi.h |    2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
> index 2f7d45b..f0e8027 100644
> --- a/include/mtd/mtd-abi.h
> +++ b/include/mtd/mtd-abi.h
> @@ -112,7 +112,7 @@ struct otp_info {
> #define MEMUNLOCK		_IOW('M', 6, struct erase_info_user)
> #define MEMGETREGIONCOUNT	_IOR('M', 7, int)
> #define MEMGETREGIONINFO	_IOWR('M', 8, struct region_info_user)
> -#define MEMSETOOBSEL		_IOW('M', 9, struct nand_oobinfo)
> +
> #define MEMGETOOBSEL		_IOR('M', 10, struct nand_oobinfo)
> #define MEMGETBADBLOCK		_IOW('M', 11, __kernel_loff_t)
> #define MEMSETBADBLOCK		_IOW('M', 12, __kernel_loff_t)
> -- 
> 1.7.5.4

/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: [PATCH 3/5] mtd: remove MEMSETOOBSEL macro definition

[Artem Bityutskiy]

On Wed, 2011-08-24 at 14:28 +0200, Ricard Wanderlof wrote:
> On Wed, 24 Aug 2011, Brian Norris wrote:
> 
> > MEMSETOOBSEL is completely unused and useless. Remove the definition.
> >
> > Note: it's probably best not to use this ioctl number in the future for
> > MTD, since that may cause conflicts between old kernels and new user
> > software (or new kernels and old user software). This shouldn't be much
> > problem.
> 
> Perhaps this should be noted as a comment in the code, rather than just 
> removing the line. Such information tends to get forgotten sooner or later 
> otherwise.

Yeah, I've amended the patch and added a comment:

+/*
+ * Note, the following ioctl existed in the past and was removed:
+ * #define MEMSETOOBSEL           _IOW('M', 9, struct nand_oobinfo)
+ * Try to avoid adding a new ioctl with the same ioctl number.
+ */

-- 
Best Regards,
Artem Bityutskiy

[PATCH 4/5] mtd: nand: fix spelling error (date => data)

[Brian Norris]

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

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 6d56968..85fef68 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -465,7 +465,7 @@ struct nand_buffers {
  * @controller:		[REPLACEABLE] a pointer to a hardware controller
  *			structure which is shared among multiple independent
  *			devices.
- * @priv:		[OPTIONAL] pointer to private chip date
+ * @priv:		[OPTIONAL] pointer to private chip data
  * @errstat:		[OPTIONAL] hardware specific function to perform
  *			additional error status checks (determine if errors are
  *			correctable).
-- 
1.7.5.4

[PATCH 5/5] mtd: style fixups in multi-line comment, indentation

[Brian Norris]

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

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 68ea229..ff7bae0 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -33,17 +33,19 @@
 #define MTD_CHAR_MAJOR 90
 #define MTD_BLOCK_MAJOR 31
 
-#define MTD_ERASE_PENDING      	0x01
+#define MTD_ERASE_PENDING	0x01
 #define MTD_ERASING		0x02
 #define MTD_ERASE_SUSPEND	0x04
-#define MTD_ERASE_DONE          0x08
-#define MTD_ERASE_FAILED        0x10
+#define MTD_ERASE_DONE		0x08
+#define MTD_ERASE_FAILED	0x10
 
 #define MTD_FAIL_ADDR_UNKNOWN -1LL
 
-/* If the erase fails, fail_addr might indicate exactly which block failed.  If
-   fail_addr = MTD_FAIL_ADDR_UNKNOWN, the failure was not at the device level or was not
-   specific to any particular block. */
+/*
+ * If the erase fails, fail_addr might indicate exactly which block failed. If
+ * fail_addr = MTD_FAIL_ADDR_UNKNOWN, the failure was not at the device level
+ * or was not specific to any particular block.
+ */
 struct erase_info {
 	struct mtd_info *mtd;
 	uint64_t addr;
@@ -60,7 +62,7 @@ struct erase_info {
 };
 
 struct mtd_erase_region_info {
-	uint64_t offset;			/* At which this region starts, from the beginning of the MTD */
+	uint64_t offset;		/* At which this region starts, from the beginning of the MTD */
 	uint32_t erasesize;		/* For this region */
 	uint32_t numblocks;		/* Number of blocks of erasesize in this region */
 	unsigned long *lockmap;		/* If keeping bitmap of locks */
-- 
1.7.5.4

Re: [PATCH 1/5] mtd: do not assume oobsize is power of 2

[Artem Bityutskiy]

On Tue, 2011-08-23 at 17:17 -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.
> 
> These masks are really only intended to hide the "page" portion of the
> offset, leaving any OOB offset intact, so a masking with the writesize
> (which *is* always a power of 2) is valid and makes more sense.
> 
> This has been tested for read/write of NAND devices (nanddump/nandwrite)
> using nandsim and actual NAND flash.
> 
> Cc: stable@kernel.org [2.6.30+]
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

I removed -stable Cc, because this is not a bug-fix, and I feel this is
a bit risky for -stable. And pushed, thanks.

Pushed also all the other patches, and amended the MEMOOBSEL removal
patch.

-- 
Best Regards,
Artem Bityutskiy