| Message ID | 1313625029-19546-6-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | RFC |
| Headers | show |
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
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).
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.
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
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
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(-)