Patchwork [04/12] mtd: move mtd_oob_mode_t to shared kernel/user space

login
register
mail settings
Submitter Brian Norris
Date Aug. 31, 2011, 1:45 a.m.
Message ID <1314755147-17756-5-git-send-email-computersforpeace@gmail.com>
Download mbox | patch
Permalink /patch/112421/
State New
Headers show

Comments

Brian Norris - Aug. 31, 2011, 1:45 a.m.
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.

This enum is now anonymous and will be used in 8-bit fields (__u8 or
uint8_t).

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/onenand/onenand_base.c |    4 ++--
 include/linux/mtd/mtd.h            |   16 +---------------
 include/mtd/mtd-abi.h              |   15 +++++++++++++++
 3 files changed, 18 insertions(+), 17 deletions(-)
Artem Bityutskiy - Sept. 11, 2011, 11:57 a.m.
On Tue, 2011-08-30 at 18:45 -0700, Brian Norris wrote:
> -	mtd_oob_mode_t mode = ops->mode;
> +	uint8_t mode = ops->mode;

...
>  struct mtd_oob_ops {
> -	mtd_oob_mode_t	mode;
> +	uint8_t		mode;
>  	size_t		len;
>  	size_t		retlen;
>  	size_t		ooblen;

It is good to use __u8 in ioctls for this field.

But for internal usage there is no need to make it uint8_t, just use
'int' instead. All modern CPUs will anyway reserve 32 bits for this.

And unnecessary usage of the 8-bits restriction only imposes more
unnecessary limitations to the compiler/CPU. Using 'int' is instead
making CPU/compiler use the native integer type.

BTW, you may also re-arrange this data structure and make it 8-bytes
smaller on 64-bit architectures. Indeed, it has 'size_t' and 'uint8_t *'
fields, which are 64-bits, and it has one 'uint32_t        ooboffs;',
which is 32-bits and is also padded. If you put 'int mode' and 'uint32_t
ooboffs' together, you'll save 2 paddings. But this is optional.
Artem Bityutskiy - Sept. 11, 2011, 12:28 p.m.
On Sun, 2011-09-11 at 14:57 +0300, Artem Bityutskiy wrote:
> On Tue, 2011-08-30 at 18:45 -0700, Brian Norris wrote:
> > -	mtd_oob_mode_t mode = ops->mode;
> > +	uint8_t mode = ops->mode;
> 
> ...
> >  struct mtd_oob_ops {
> > -	mtd_oob_mode_t	mode;
> > +	uint8_t		mode;
> >  	size_t		len;
> >  	size_t		retlen;
> >  	size_t		ooblen;
> 
> It is good to use __u8 in ioctls for this field.
> 
> But for internal usage there is no need to make it uint8_t, just use
> 'int' instead. All modern CPUs will anyway reserve 32 bits for this.
> 
> And unnecessary usage of the 8-bits restriction only imposes more
> unnecessary limitations to the compiler/CPU. Using 'int' is instead
> making CPU/compiler use the native integer type.

I've actually amended your patch and used 'unsigned int mode' instead,
and pushed to my three. Please, complain and send new versions if you do
not like that.

> BTW, you may also re-arrange this data structure and make it 8-bytes
> smaller on 64-bit architectures. Indeed, it has 'size_t' and 'uint8_t *'
> fields, which are 64-bits, and it has one 'uint32_t        ooboffs;',
> which is 32-bits and is also padded. If you put 'int mode' and 'uint32_t
> ooboffs' together, you'll save 2 paddings. But this is optional.

Did not do this, though.

Thanks!
Brian Norris - Sept. 13, 2011, 10:29 p.m.
On Sun, Sep 11, 2011 at 8:28 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Sun, 2011-09-11 at 14:57 +0300, Artem Bityutskiy wrote:
>> It is good to use __u8 in ioctls for this field.
>>
>> But for internal usage there is no need to make it uint8_t, just use
>> 'int' instead. All modern CPUs will anyway reserve 32 bits for this.
>>
>> And unnecessary usage of the 8-bits restriction only imposes more
>> unnecessary limitations to the compiler/CPU. Using 'int' is instead
>> making CPU/compiler use the native integer type.

Thanks, good suggestion. I think I considered this issue but not very
thoroughly. My only reason for uint8_t here was just to prevent the
temptation to use all 32 bits of the field. But there's no way we'd
use 32 bits for a enumerated mode, i.e., we'd need to have more than
256 modes to run into this issue... I think we'd be having other more
important issues if we get there :)

> I've actually amended your patch and used 'unsigned int mode' instead,
> and pushed to my three. Please, complain and send new versions if you do
> not like that.

No complaint.

Brian

Patch

diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c
index de98791..9edc8b1 100644
--- a/drivers/mtd/onenand/onenand_base.c
+++ b/drivers/mtd/onenand/onenand_base.c
@@ -1351,7 +1351,7 @@  static int onenand_read_oob_nolock(struct mtd_info *mtd, loff_t from,
 	struct mtd_ecc_stats stats;
 	int read = 0, thislen, column, oobsize;
 	size_t len = ops->ooblen;
-	mtd_oob_mode_t mode = ops->mode;
+	uint8_t mode = ops->mode;
 	u_char *buf = ops->oobbuf;
 	int ret = 0, readcmd;
 
@@ -2074,7 +2074,7 @@  static int onenand_write_oob_nolock(struct mtd_info *mtd, loff_t to,
 	u_char *oobbuf;
 	size_t len = ops->ooblen;
 	const u_char *buf = ops->oobbuf;
-	mtd_oob_mode_t mode = ops->mode;
+	uint8_t mode = ops->mode;
 
 	to += ops->ooboffs;
 
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index ff7bae0..3b2c678 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
@@ -102,7 +88,7 @@  typedef enum {
  * OOB area.
  */
 struct mtd_oob_ops {
-	mtd_oob_mode_t	mode;
+	uint8_t		mode;
 	size_t		len;
 	size_t		retlen;
 	size_t		ooblen;
diff --git a/include/mtd/mtd-abi.h b/include/mtd/mtd-abi.h
index 3bdda5c..af42c7a 100644
--- a/include/mtd/mtd-abi.h
+++ b/include/mtd/mtd-abi.h
@@ -45,6 +45,21 @@  struct mtd_oob_buf64 {
 	__u64 usr_ptr;
 };
 
+/*
+ * oob operation modes
+ *
+ * MTD_OOB_PLACE:       oob data are placed at the given offset (default)
+ * MTD_OOB_AUTO:        oob data are automatically placed at the free areas
+ *                      which are defined by the internal ecclayout
+ * MTD_OOB_RAW:         mode to read or write oob and data without doing ECC
+ *			checking
+ */
+enum {
+	MTD_OOB_PLACE = 0,
+	MTD_OOB_AUTO = 1,
+	MTD_OOB_RAW = 2,
+};
+
 #define MTD_ABSENT		0
 #define MTD_RAM			1
 #define MTD_ROM			2