Re: [PATCHv4] MTD: New ioctl calls for >4GiB device support

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 31 March 2009, Kevin Cernekee wrote:
> On Tue, Mar 31, 2009 at 3:51 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Tuesday 31 March 2009, Kevin Cernekee wrote:
> >> +struct mtd_oob_buf64 {
> >> +       uint64_t start;
> >> +       uint32_t res0;
> >> +       uint32_t length;
> >> +       unsigned char __user *ptr;
> >> +       uint32_t res1[8];
> >> +};
> >
> > Does this have to use an indirect pointer? We normally try to avoid
> > ioctl interfaces like this, because they are hard to trace and you
> > need a compat wrapper. You might be able to at least avoid the wrapper
> > by defining the data structure with a __u64 to take the pointer.
> 
> Could you please point out another ioctl that is set up this way, so
> that I can follow the same conventions?

struct signalfd_siginfo uses __u64 to store pointers, and so does
the sg_io_v4 ioctl. Some of the ioctls in kvm.h also use __u64
for addresses.

> Are we ever worried about pointers that are larger than 64 bits, or
> ints that are larger than 32 bits?  Or is it generally OK to assume
> this will never happen?

None of these is a worry, as we already rely on the size of int, long
and pointer in a lot of ways.

> > If you leave the data structure the way it is, you should at least
> > move the compat_ioctl handling into mtdchar.c from compat_ioctl.c.
> > It will simplify your code and help reduce the size of the common
> > ioctl handling.
> 
> Is this what you are recommending?
> 
> 1) Leave existing MTD COMPATIBLE_IOCTLs in fs/compat_ioctl.c
> 
> 2) Implement compat_ioctl handler in mtdchar.c for MEMREADOOB64_32 and
> MEMWRITEOOB64_32
> 
> 3) For all other commands, the new handler should return -ENOIOCTLCMD
> and let fs/compat_ioctl.c handle them

Yes, that would be good.

> Would it be a good idea to move MTDREADOOB32 / MEMWRITEOOB32 out of
> fs/compat_ioctl.c at the same time, so that everything is in one
> place?

Yes, I'd like to see this as a separate patch either before or
after the other one. I have the long-term goal of getting rid of
all the wrapper functions in fs/compat_ioctl.c, but it's stalled
for some time.

> If the compat wrappers are moved to mtdchar.c , does that imply that
> they should be reimplemented "natively" instead of using
> compat_alloc_user_space(), copy_in_user(), and sys_ioctl() to cause
> them to reinvoke the 64-bit versions?

Right, that is what I mean with 'simplify your code'.

Thanks,

	Arnd <><