| Message ID | 1314755147-17756-5-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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.
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!
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
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
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(-)