Re: [PATCH v6] mmc: Add mmc CMD+ACMD passthrough ioctl

[Arnd Bergmann <arnd@arndb.de>]

On Wednesday 20 April 2011 21:06:49 John Calixto wrote:
> On Wed, 20 Apr 2011, Michał Mirosław wrote:
> > Hmm. This might be even better:
> > 
> > static int mmc_blk_ioctl(struct block_device *bdev, fmode_t mode,
> > unsigned int cmd, unsigned long arg)
> > {
> >   struct mmc_ioc_cmd blk;
> > 
> >   if (cmd != MMC_IOC_CMD)
> >     return -EINVAL;
> > 
> >   copy_from_user((void __user *)arg, &blk) ...
> > 
> > #ifdef CONFIG_COMPAT
> >   if (is_compat_task())
> >     blk.data_ptr = compat_ptr(*(u32 *)&blk.data_ptr);
> > #endif
> > 
> >   return mmc_blk_ioctl_cmd(bdev, &blk);
> > }

No, please don't try to invent random new ways of doing this.
Your example relies on the assumption that the task is calling
the entry point for its native word size. Some architectures
intentionally allow calling the 32 bit entry point from 64 bit
tasks and vice versa, e.g. for user space emulators converting
to a different ABI, and in that case is_compat_task() produces
the wrong result. Don't ever rely on that.

> I'm okay with the anon union + ``compat_ptr(*(u32 *))`` part of your
> solution.  If everyone else thinks it is reasonable, I'll submit a v7
> with it.

No need for a union or a ptr_size member in the struct. Just use
a single __u64 and let the user cast the pointer to that. This
will work on all architectures.
 
> However, I still think it should be implemented in compat_ioctl()
> because compat_blkdev_ioctl() expects it.  Either that, or I add to the
> big switch in compat_blkdev_driver_ioctl(), and spreading this change
> out to block/compat_ioctl.c does not seem like The Right Thing to me.

Yes, you definitely need to fill the .compat_ioctl member. We don't want
new entries in the switch statement, in particular none that are specific
to a single driver.

	Arnd