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

Brian Norris
2011-08-23T00:04:03Z

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