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

[Artem Bityutskiy <dedekind1@gmail.com>]

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