From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933195Ab0E0WDm (ORCPT ); Thu, 27 May 2010 18:03:42 -0400 Received: from mail-wy0-f174.google.com ([74.125.82.174]:40405 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753843Ab0E0WDk (ORCPT ); Thu, 27 May 2010 18:03:40 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=bh+MN8ft/azFcEjCxKgR0NYM70bPyAMYA7Of1rY62ayQirICKZXIgcvWqnomceGt29 DHhJrq6JubcAqlr5ijTXTJkeXjpAZf48Ns/k/5tZuhHRR9N0j/izAtcl71kDwW0UVVkn 2Wac/qA4FQr5gtRxMZcPA1r7PqrJabYF2OljQ= Date: Fri, 28 May 2010 00:03:37 +0200 From: Frederic Weisbecker To: Arnd Bergmann Cc: Linus Torvalds , LKML , John Kacur , Thomas Gleixner , Frederic Lassabe , Jesper Nilsson , Hans Verkuil , Tyler Hicks , "H. Peter Anvin" , =?iso-8859-1?Q?J=F6rn?= Engel Subject: Re: [GIT PULL] Bkl ioctl pushdowns for 2.6.35-rc1 Message-ID: <20100527220334.GC5390@nowhere> References: <1274547426-25937-1-git-send-regression-fweisbec@gmail.com> <201005272338.25049.arnd@arndb.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201005272338.25049.arnd@arndb.de> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 27, 2010 at 11:38:24PM +0200, Arnd Bergmann wrote: > On Saturday 22 May 2010 18:57:05 Frederic Weisbecker wrote: > > Please pull the bkl/ioctl branch that can be found at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git > > bkl/ioctl > > > > There are still some pushdowns left, pending for tests or in discussion. > > I guess ioctl will die for the next merge window. > > Let's take a look at the final patch in the meantime. I would suggest merging > the patch below once all users we care about are done. > > Right now, what remains are these trivial users: > > | arch/cris/arch-v10/drivers/ds1302.c > | arch/cris/arch-v10/drivers/gpio.c > | arch/cris/arch-v10/drivers/i2c.c > | arch/cris/arch-v10/drivers/pcf8563.c > | arch/cris/arch-v10/drivers/sync_serial.c > | arch/cris/arch-v32/drivers/cryptocop.c > | arch/cris/arch-v32/drivers/i2c.c > | arch/cris/arch-v32/drivers/mach-a3/gpio.c > | arch/cris/arch-v32/drivers/mach-fs/gpio.c > | arch/cris/arch-v32/drivers/pcf8563.c > | arch/cris/arch-v32/drivers/sync_serial.c > > All queued in the cris maintainer tree, but no pull request > has been sent for 2.6.35 cris. Not all, I also have a part in my local tree (posted in my last batch): arch/cris/arch-v10/drivers/gpio.c | 30 ++++++++++++++++++--------- arch/cris/arch-v10/drivers/i2c.c | 24 ++++++++++++++++----- arch/cris/arch-v10/drivers/sync_serial.c | 32 +++++++++++++++++++--------- arch/cris/arch-v32/drivers/cryptocop.c | 24 ++++++++++++++++----- arch/cris/arch-v32/drivers/mach-a3/gpio.c | 28 +++++++++++++++++-------- arch/cris/arch-v32/drivers/mach-fs/gpio.c | 29 +++++++++++++++++--------- arch/cris/arch-v32/drivers/sync_serial.c | 30 +++++++++++++++++++-------- I did not include it in the pull request because I hadn't yet the occasion to build-test it. I'll do that and try to get this in the cris tree. > > | arch/ia64/kernel/perfmon.c > > Not sure what happened to this patch. The ioctl function here > is basically empty, but the patch you did was not merged. Same problem here: I need to built test it before pushing for good. > | drivers/media/video/v4l2-dev.c > > At least two patches exist, a simple one from me and a > more complete one from Frederic. Both have been Ack-ed > by Hans Verkuil, so one of them should just go in. Yeah, I guess it's too late to integrate the big v4l pushdown for this merge window. I'll ping Mauro once -rc1 is released. > | fs/autofs/root.c > > A discussion came up on the naming of the function after > the patch, and another discussion on when to finally > kill this driver. But I won't wait for the driver to be removed though, so I'll fix the naming issue and queue it soon. > | fs/logfs/file.c > | fs/logfs/dir.c > > Joern said he had this in his tree, but it was not > part of the pull request. At least it's there: http://git.kernel.org/?p=linux/kernel/git/joern/logfs.git;a=commit;h=ddbb5dd99c40a695d5d75645b5a18bef394acb16 > > | fs/ecryptfs/file.c > > Tyler wanted to take care of this one, it conflicted with > a bug fix he did. The bug fix should go in anyway, so we should > just do it the right way (removing the .ioctl version from there) > > | sound/oss/dmasound/dmasound_core.c > | sound/oss/msnd_pinnacle.c > | sound/oss/au1550_ac97.c > | sound/oss/sh_dac_audio.c > | sound/oss/swarm_cs4297a.c > | sound/oss/vwsnd.c > > The patches for this exist but are untested apparently. > A discussion came up on whether we should just kill oss Yeah. > > Arnd > > --- > [PATCH] Kill ioctl file operation > > All instances of the .ioctl() file operation are gone now, so we can > remove the pointer itself along with the code accessing it. > > Signed-off-by: Arnd Bergmann I keep this close for 2.6.36 Thanks. > --- > > Documentation/filesystems/Locking | 8 +------- > Documentation/filesystems/vfs.txt | 6 +----- > fs/bad_inode.c | 7 ------- > fs/compat_ioctl.c | 3 +-- > fs/ioctl.c | 18 ++++-------------- > fs/proc/inode.c | 17 ++++------------- > include/linux/fs.h | 5 ++--- > 7 files changed, 13 insertions(+), 51 deletions(-) > > diff --git a/Documentation/filesystems/Locking b/Documentation/filesystems/Locking > index 61c98f0..ea8e1d2 100644 > --- a/Documentation/filesystems/Locking > +++ b/Documentation/filesystems/Locking > @@ -372,8 +372,6 @@ prototypes: > ssize_t (*aio_write) (struct kiocb *, const struct iovec *, unsigned long, loff_t); > int (*readdir) (struct file *, void *, filldir_t); > unsigned int (*poll) (struct file *, struct poll_table_struct *); > - int (*ioctl) (struct inode *, struct file *, unsigned int, > - unsigned long); > long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long); > long (*compat_ioctl) (struct file *, unsigned int, unsigned long); > int (*mmap) (struct file *, struct vm_area_struct *); > @@ -407,8 +405,7 @@ write: no > aio_write: no > readdir: no > poll: no > -ioctl: yes (see below) > -unlocked_ioctl: no (see below) > +unlocked_ioctl: no > compat_ioctl: no > mmap: no > open: no > @@ -451,9 +448,6 @@ move ->readdir() to inode_operations and use a separate method for directory > anything that resembles union-mount we won't have a struct file for all > components. And there are other reasons why the current interface is a mess... > > -->ioctl() on regular files is superceded by the ->unlocked_ioctl() that > -doesn't take the BKL. > - > ->read on directories probably must go away - we should just enforce -EISDIR > in sys_read() and friends. > > diff --git a/Documentation/filesystems/vfs.txt b/Documentation/filesystems/vfs.txt > index b668585..caa7989 100644 > --- a/Documentation/filesystems/vfs.txt > +++ b/Documentation/filesystems/vfs.txt > @@ -722,7 +722,6 @@ struct file_operations { > ssize_t (*aio_write) (struct kiocb *, const struct iovec *, unsigned long, loff_t); > int (*readdir) (struct file *, void *, filldir_t); > unsigned int (*poll) (struct file *, struct poll_table_struct *); > - int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long); > long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long); > long (*compat_ioctl) (struct file *, unsigned int, unsigned long); > int (*mmap) (struct file *, struct vm_area_struct *); > @@ -763,10 +762,7 @@ otherwise noted. > activity on this file and (optionally) go to sleep until there > is activity. Called by the select(2) and poll(2) system calls > > - ioctl: called by the ioctl(2) system call > - > - unlocked_ioctl: called by the ioctl(2) system call. Filesystems that do not > - require the BKL should use this method instead of the ioctl() above. > + unlocked_ioctl: called by the ioctl(2) system call. > > compat_ioctl: called by the ioctl(2) system call when 32 bit system calls > are used on 64 bit kernels. > diff --git a/fs/bad_inode.c b/fs/bad_inode.c > index a05287a..7cb2730 100644 > --- a/fs/bad_inode.c > +++ b/fs/bad_inode.c > @@ -55,12 +55,6 @@ static unsigned int bad_file_poll(struct file *filp, poll_table *wait) > return POLLERR; > } > > -static int bad_file_ioctl (struct inode *inode, struct file *filp, > - unsigned int cmd, unsigned long arg) > -{ > - return -EIO; > -} > - > static long bad_file_unlocked_ioctl(struct file *file, unsigned cmd, > unsigned long arg) > { > @@ -160,7 +154,6 @@ static const struct file_operations bad_file_ops = > .aio_write = bad_file_aio_write, > .readdir = bad_file_readdir, > .poll = bad_file_poll, > - .ioctl = bad_file_ioctl, > .unlocked_ioctl = bad_file_unlocked_ioctl, > .compat_ioctl = bad_file_compat_ioctl, > .mmap = bad_file_mmap, > diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c > index 641640d..edf72e3 100644 > --- a/fs/compat_ioctl.c > +++ b/fs/compat_ioctl.c > @@ -1729,8 +1729,7 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd, > goto out_fput; > } > > - if (!filp->f_op || > - (!filp->f_op->ioctl && !filp->f_op->unlocked_ioctl)) > + if (!filp->f_op || !filp->f_op->unlocked_ioctl) > goto do_ioctl; > break; > } > diff --git a/fs/ioctl.c b/fs/ioctl.c > index 2d140a7..f855ea4 100644 > --- a/fs/ioctl.c > +++ b/fs/ioctl.c > @@ -29,7 +29,6 @@ > * @arg: command-specific argument for ioctl > * > * Invokes filesystem specific ->unlocked_ioctl, if one exists; otherwise > - * invokes filesystem specific ->ioctl method. If neither method exists, > * returns -ENOTTY. > * > * Returns 0 on success, -errno on error. > @@ -39,21 +38,12 @@ static long vfs_ioctl(struct file *filp, unsigned int cmd, > { > int error = -ENOTTY; > > - if (!filp->f_op) > + if (!filp->f_op || !filp->f_op->unlocked_ioctl) > goto out; > > - if (filp->f_op->unlocked_ioctl) { > - error = filp->f_op->unlocked_ioctl(filp, cmd, arg); > - if (error == -ENOIOCTLCMD) > - error = -EINVAL; > - goto out; > - } else if (filp->f_op->ioctl) { > - lock_kernel(); > - error = filp->f_op->ioctl(filp->f_path.dentry->d_inode, > - filp, cmd, arg); > - unlock_kernel(); > - } > - > + error = filp->f_op->unlocked_ioctl(filp, cmd, arg); > + if (error == -ENOIOCTLCMD) > + error = -EINVAL; > out: > return error; > } > diff --git a/fs/proc/inode.c b/fs/proc/inode.c > index aea8502..041517c 100644 > --- a/fs/proc/inode.c > +++ b/fs/proc/inode.c > @@ -214,8 +214,7 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne > { > struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode); > long rv = -ENOTTY; > - long (*unlocked_ioctl)(struct file *, unsigned int, unsigned long); > - int (*ioctl)(struct inode *, struct file *, unsigned int, unsigned long); > + long (*ioctl)(struct file *, unsigned int, unsigned long); > > spin_lock(&pde->pde_unload_lock); > if (!pde->proc_fops) { > @@ -223,19 +222,11 @@ static long proc_reg_unlocked_ioctl(struct file *file, unsigned int cmd, unsigne > return rv; > } > pde->pde_users++; > - unlocked_ioctl = pde->proc_fops->unlocked_ioctl; > - ioctl = pde->proc_fops->ioctl; > + ioctl = pde->proc_fops->unlocked_ioctl; > spin_unlock(&pde->pde_unload_lock); > > - if (unlocked_ioctl) { > - rv = unlocked_ioctl(file, cmd, arg); > - if (rv == -ENOIOCTLCMD) > - rv = -EINVAL; > - } else if (ioctl) { > - WARN_ONCE(1, "Procfs ioctl handlers must use unlocked_ioctl, " > - "%pf will be called without the Bkl held\n", ioctl); > - rv = ioctl(file->f_path.dentry->d_inode, file, cmd, arg); > - } > + if (ioctl) > + rv = ioctl(file, cmd, arg); > > pde_users_dec(pde); > return rv; > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 85e823a..a92f91c 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1478,8 +1478,8 @@ struct block_device_operations; > > /* > * NOTE: > - * read, write, poll, fsync, readv, writev, unlocked_ioctl and compat_ioctl > - * can be called without the big kernel lock held in all filesystems. > + * all file operations except setlease can be called without > + * the big kernel lock held in all filesystems. > */ > struct file_operations { > struct module *owner; > @@ -1490,7 +1490,6 @@ struct file_operations { > ssize_t (*aio_write) (struct kiocb *, const struct iovec *, unsigned long, loff_t); > int (*readdir) (struct file *, void *, filldir_t); > unsigned int (*poll) (struct file *, struct poll_table_struct *); > - int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long); > long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long); > long (*compat_ioctl) (struct file *, unsigned int, unsigned long); > int (*mmap) (struct file *, struct vm_area_struct *);