[PATCH 0/6] kill .ioctl file_operation

[PATCH 0/6] kill .ioctl file_operation

[Arnd Bergmann]

This removes the .ioctl file operation from all the
remaining users that are in today's linux-next
tree.

I'd like to have this added to linux-next to make
sure we don't get any new users and we can seamlessly
apply the final patches for this in 2.6.36.

The intention behind removing the .ioctl operation
is to be able to remove the big kernel lock (BKL)
from the ioctl system call.

Ideally, maintainers of the code in question should
just apply the respective patches to their -next
tree so we can simply apply the final patch removing
the definition of .ioctl.

	Arnd

Cc: alsa-devel@alsa-project.org
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: autofs@linux.kernel.org
Cc: Cris <linux-cris-kernel@axis.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Ian Kent <raven@themaw.net>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Jesper Nilsson <jesper.nilsson@axis.com>
Cc: John Kacur <jkacur@redhat.com>
Cc: linux-ia64@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Mikael Starvik <starvik@axis.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>

Arnd Bergmann (5):
  ia64/perfmon: convert to unlocked_ioctl
  sound/oss: convert to unlocked_ioctl
  autofs/autofs4: move compat_ioctl handling into fs
  v4l: convert v4l2-dev to unlocked_ioctl
  bkl: remove locked .ioctl file operation

Frederic Weisbecker (1):
  cris: Pushdown the bkl from ioctl

 Documentation/filesystems/Locking         |    8 +---
 Documentation/filesystems/vfs.txt         |    6 +--
 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 +++++++++----
 arch/ia64/kernel/perfmon.c                |   22 +++++-----
 drivers/media/video/v4l2-dev.c            |   52 ++++++----------------
 fs/autofs/root.c                          |   67 +++++++++++++++++++++++++++-
 fs/autofs4/root.c                         |   49 +++++++++++++++++++++
 fs/bad_inode.c                            |    7 ---
 fs/compat_ioctl.c                         |   39 +----------------
 fs/ioctl.c                                |   18 ++------
 fs/proc/inode.c                           |   17 ++------
 include/linux/auto_fs.h                   |    1 +
 include/linux/fs.h                        |    5 +-
 sound/oss/au1550_ac97.c                   |   54 +++++++++++++++---------
 sound/oss/dmasound/dmasound_core.c        |   35 ++++++++++++---
 sound/oss/msnd_pinnacle.c                 |   15 ++++--
 sound/oss/sh_dac_audio.c                  |   18 ++++++-
 sound/oss/swarm_cs4297a.c                 |   25 +++++++++--
 sound/oss/vwsnd.c                         |   24 ++++++-----
 25 files changed, 410 insertions(+), 249 deletions(-)

[PATCH 1/6] ia64/perfmon: convert to unlocked_ioctl

[Arnd Bergmann]

The ioctl function in this driver does not
do anything that requires the BKL, so make
it use unlocked_ioctl.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: linux-ia64@vger.kernel.org
---
 arch/ia64/kernel/perfmon.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
index ab985f7..7443290 100644
--- a/arch/ia64/kernel/perfmon.c
+++ b/arch/ia64/kernel/perfmon.c
@@ -1696,8 +1696,8 @@ pfm_poll(struct file *filp, poll_table * wait)
 	return mask;
 }
 
-static int
-pfm_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
+static long
+pfm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	DPRINT(("pfm_ioctl called\n"));
 	return -EINVAL;
@@ -2174,15 +2174,15 @@ pfm_no_open(struct inode *irrelevant, struct file *dontcare)
 
 
 static const struct file_operations pfm_file_ops = {
-	.llseek   = no_llseek,
-	.read     = pfm_read,
-	.write    = pfm_write,
-	.poll     = pfm_poll,
-	.ioctl    = pfm_ioctl,
-	.open     = pfm_no_open,	/* special open code to disallow open via /proc */
-	.fasync   = pfm_fasync,
-	.release  = pfm_close,
-	.flush	  = pfm_flush
+	.llseek		= no_llseek,
+	.read		= pfm_read,
+	.write		= pfm_write,
+	.poll		= pfm_poll,
+	.unlocked_ioctl = pfm_ioctl,
+	.open		= pfm_no_open,	/* special open code to disallow open via /proc */
+	.fasync		= pfm_fasync,
+	.release	= pfm_close,
+	.flush		= pfm_flush
 };
 
 static int
-- 
1.7.1

Re: [PATCH 1/6] ia64/perfmon: convert to unlocked_ioctl

[Frederic Weisbecker]

On Sun, Jul 04, 2010 at 12:15:05AM +0200, Arnd Bergmann wrote:
> The ioctl function in this driver does not
> do anything that requires the BKL, so make
> it use unlocked_ioctl.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: linux-ia64@vger.kernel.org
> ---


Applied, thanks!

[PATCH 2/6] sound/oss: convert to unlocked_ioctl

[Arnd Bergmann]

These are the final conversions for the ioctl
file operation so we can remove it in the next
merge window.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: John Kacur <jkacur@redhat.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
---
 sound/oss/au1550_ac97.c            |   54 ++++++++++++++++++++++-------------
 sound/oss/dmasound/dmasound_core.c |   35 ++++++++++++++++++----
 sound/oss/msnd_pinnacle.c          |   15 ++++++---
 sound/oss/sh_dac_audio.c           |   18 ++++++++++--
 sound/oss/swarm_cs4297a.c          |   25 +++++++++++++---
 sound/oss/vwsnd.c                  |   24 ++++++++-------
 6 files changed, 120 insertions(+), 51 deletions(-)

diff --git a/sound/oss/au1550_ac97.c b/sound/oss/au1550_ac97.c
index c1070e3..3547450 100644
--- a/sound/oss/au1550_ac97.c
+++ b/sound/oss/au1550_ac97.c
@@ -824,22 +824,26 @@ mixdev_ioctl(struct ac97_codec *codec, unsigned int cmd,
 	return codec->mixer_ioctl(codec, cmd, arg);
 }
 
-static int
-au1550_ioctl_mixdev(struct inode *inode, struct file *file,
-			       unsigned int cmd, unsigned long arg)
+static long
+au1550_ioctl_mixdev(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct au1550_state *s = (struct au1550_state *)file->private_data;
 	struct ac97_codec *codec = s->codec;
+	int ret;
+
+	lock_kernel();
+	ret = mixdev_ioctl(codec, cmd, arg);
+	unlock_kernel();
 
-	return mixdev_ioctl(codec, cmd, arg);
+	return ret;
 }
 
 static /*const */ struct file_operations au1550_mixer_fops = {
-	owner:THIS_MODULE,
-	llseek:au1550_llseek,
-	ioctl:au1550_ioctl_mixdev,
-	open:au1550_open_mixdev,
-	release:au1550_release_mixdev,
+	.owner		= THIS_MODULE,
+	.llseek		= au1550_llseek,
+	.unlocked_ioctl	= au1550_ioctl_mixdev,
+	.open		= au1550_open_mixdev,
+	.release	= au1550_release_mixdev,
 };
 
 static int
@@ -1343,8 +1347,7 @@ dma_count_done(struct dmabuf *db)
 
 
 static int
-au1550_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
-							unsigned long arg)
+au1550_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct au1550_state *s = (struct au1550_state *)file->private_data;
 	unsigned long   flags;
@@ -1780,6 +1783,17 @@ au1550_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
 	return mixdev_ioctl(s->codec, cmd, arg);
 }
 
+static long
+au1550_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = au1550_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
 
 static int
 au1550_open(struct inode *inode, struct file *file)
@@ -1885,15 +1899,15 @@ au1550_release(struct inode *inode, struct file *file)
 }
 
 static /*const */ struct file_operations au1550_audio_fops = {
-	owner:		THIS_MODULE,
-	llseek:		au1550_llseek,
-	read:		au1550_read,
-	write:		au1550_write,
-	poll:		au1550_poll,
-	ioctl:		au1550_ioctl,
-	mmap:		au1550_mmap,
-	open:		au1550_open,
-	release:	au1550_release,
+	.owner		= THIS_MODULE,
+	.llseek		= au1550_llseek,
+	.read		= au1550_read,
+	.write		= au1550_write,
+	.poll		= au1550_poll,
+	.unlocked_ioctl	= au1550_unlocked_ioctl,
+	.mmap		= au1550_mmap,
+	.open		= au1550_open,
+	.release	= au1550_release,
 };
 
 MODULE_AUTHOR("Advanced Micro Devices (AMD), dan@embeddededge.com");
diff --git a/sound/oss/dmasound/dmasound_core.c b/sound/oss/dmasound/dmasound_core.c
index 3f3c3f7..b5464fb 100644
--- a/sound/oss/dmasound/dmasound_core.c
+++ b/sound/oss/dmasound/dmasound_core.c
@@ -337,8 +337,8 @@ static int mixer_release(struct inode *inode, struct file *file)
 	unlock_kernel();
 	return 0;
 }
-static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
-		       u_long arg)
+
+static long mixer_ioctl(struct file *file, u_int cmd, u_long arg)
 {
 	if (_SIOC_DIR(cmd) & _SIOC_WRITE)
 	    mixer.modify_counter++;
@@ -362,11 +362,22 @@ static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
 	return -EINVAL;
 }
 
+static long mixer_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = mixer_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 static const struct file_operations mixer_fops =
 {
 	.owner		= THIS_MODULE,
 	.llseek		= no_llseek,
-	.ioctl		= mixer_ioctl,
+	.unlocked_ioctl	= mixer_unlocked_ioctl,
 	.open		= mixer_open,
 	.release	= mixer_release,
 };
@@ -955,8 +966,7 @@ printk("dmasound_core: tried to set_queue_frags on a locked queue\n") ;
 	return 0 ;
 }
 
-static int sq_ioctl(struct inode *inode, struct file *file, u_int cmd,
-		    u_long arg)
+static long sq_ioctl(struct file *file, u_int cmd, u_long arg)
 {
 	int val, result;
 	u_long fmt;
@@ -1114,18 +1124,29 @@ static int sq_ioctl(struct inode *inode, struct file *file, u_int cmd,
 		return IOCTL_OUT(arg,val);
 
 	default:
-		return mixer_ioctl(inode, file, cmd, arg);
+		return mixer_ioctl(file, cmd, arg);
 	}
 	return -EINVAL;
 }
 
+static long sq_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = sq_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 static const struct file_operations sq_fops =
 {
 	.owner		= THIS_MODULE,
 	.llseek		= no_llseek,
 	.write		= sq_write,
 	.poll		= sq_poll,
-	.ioctl		= sq_ioctl,
+	.unlocked_ioctl	= sq_unlocked_ioctl,
 	.open		= sq_open,
 	.release	= sq_release,
 };
diff --git a/sound/oss/msnd_pinnacle.c b/sound/oss/msnd_pinnacle.c
index a1e3f96..9759e9c 100644
--- a/sound/oss/msnd_pinnacle.c
+++ b/sound/oss/msnd_pinnacle.c
@@ -639,21 +639,26 @@ static int mixer_ioctl(unsigned int cmd, unsigned long arg)
 	return -EINVAL;
 }
 
-static int dev_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
+static long dev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	int minor = iminor(inode);
+	int ret;
 
 	if (cmd == OSS_GETVERSION) {
 		int sound_version = SOUND_VERSION;
 		return put_user(sound_version, (int __user *)arg);
 	}
 
+	ret = -EINVAL;
+
+	lock_kernel();
 	if (minor == dev.dsp_minor)
-		return dsp_ioctl(file, cmd, arg);
+		ret = dsp_ioctl(file, cmd, arg);
 	else if (minor == dev.mixer_minor)
-		return mixer_ioctl(cmd, arg);
+		ret = mixer_ioctl(cmd, arg);
+	unlock_kernel();
 
-	return -EINVAL;
+	return ret;
 }
 
 static void dsp_write_flush(void)
@@ -1105,7 +1110,7 @@ static const struct file_operations dev_fileops = {
 	.owner		= THIS_MODULE,
 	.read		= dev_read,
 	.write		= dev_write,
-	.ioctl		= dev_ioctl,
+	.unlocked_ioctl	= dev_ioctl,
 	.open		= dev_open,
 	.release	= dev_release,
 };
diff --git a/sound/oss/sh_dac_audio.c b/sound/oss/sh_dac_audio.c
index 4153752..0f9f81c 100644
--- a/sound/oss/sh_dac_audio.c
+++ b/sound/oss/sh_dac_audio.c
@@ -15,6 +15,7 @@
 #include <linux/linkage.h>
 #include <linux/slab.h>
 #include <linux/fs.h>
+#include <linux/smp_lock.h>
 #include <linux/sound.h>
 #include <linux/soundcard.h>
 #include <linux/interrupt.h>
@@ -92,7 +93,7 @@ static void dac_audio_set_rate(void)
 	wakeups_per_second = ktime_set(0, 1000000000 / rate);
 }
 
-static int dac_audio_ioctl(struct inode *inode, struct file *file,
+static int dac_audio_ioctl(struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
 	int val;
@@ -158,6 +159,17 @@ static int dac_audio_ioctl(struct inode *inode, struct file *file,
 	return -EINVAL;
 }
 
+static long dac_audio_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = dac_audio_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 static ssize_t dac_audio_write(struct file *file, const char *buf, size_t count,
 			       loff_t * ppos)
 {
@@ -237,8 +249,8 @@ static int dac_audio_release(struct inode *inode, struct file *file)
 
 const struct file_operations dac_audio_fops = {
       .read =		dac_audio_read,
-      .write =	dac_audio_write,
-      .ioctl =	dac_audio_ioctl,
+      .write =		dac_audio_write,
+      .unlocked_ioctl =	dac_audio_unlocked_ioctl,
       .open =		dac_audio_open,
       .release =	dac_audio_release,
 };
diff --git a/sound/oss/swarm_cs4297a.c b/sound/oss/swarm_cs4297a.c
index 3136c88..d03f15a 100644
--- a/sound/oss/swarm_cs4297a.c
+++ b/sound/oss/swarm_cs4297a.c
@@ -68,6 +68,7 @@
 #include <linux/delay.h>
 #include <linux/sound.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <linux/soundcard.h>
 #include <linux/ac97_codec.h>
 #include <linux/pci.h>
@@ -1566,11 +1567,15 @@ static int cs4297a_release_mixdev(struct inode *inode, struct file *file)
 }
 
 
-static int cs4297a_ioctl_mixdev(struct inode *inode, struct file *file,
+static int cs4297a_ioctl_mixdev(struct file *file,
 			       unsigned int cmd, unsigned long arg)
 {
-	return mixer_ioctl((struct cs4297a_state *) file->private_data, cmd,
+	int ret;
+	lock_kernel();
+	ret = mixer_ioctl((struct cs4297a_state *) file->private_data, cmd,
 			   arg);
+	unlock_kernel();
+	return ret;
 }
 
 
@@ -1580,7 +1585,7 @@ static int cs4297a_ioctl_mixdev(struct inode *inode, struct file *file,
 static const struct file_operations cs4297a_mixer_fops = {
 	.owner		= THIS_MODULE,
 	.llseek		= no_llseek,
-	.ioctl		= cs4297a_ioctl_mixdev,
+	.unlocked_ioctl	= cs4297a_ioctl_mixdev,
 	.open		= cs4297a_open_mixdev,
 	.release	= cs4297a_release_mixdev,
 };
@@ -1944,7 +1949,7 @@ static int cs4297a_mmap(struct file *file, struct vm_area_struct *vma)
 }
 
 
-static int cs4297a_ioctl(struct inode *inode, struct file *file,
+static int cs4297a_ioctl(struct file *file,
 			unsigned int cmd, unsigned long arg)
 {
 	struct cs4297a_state *s =
@@ -2337,6 +2342,16 @@ static int cs4297a_ioctl(struct inode *inode, struct file *file,
 	return mixer_ioctl(s, cmd, arg);
 }
 
+static long cs4297a_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = cs4297a_ioctl(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
 
 static int cs4297a_release(struct inode *inode, struct file *file)
 {
@@ -2496,7 +2511,7 @@ static const struct file_operations cs4297a_audio_fops = {
 	.read		= cs4297a_read,
 	.write		= cs4297a_write,
 	.poll		= cs4297a_poll,
-	.ioctl		= cs4297a_ioctl,
+	.unlocked_ioctl	= cs4297a_unlocked_ioctl,
 	.mmap		= cs4297a_mmap,
 	.open		= cs4297a_open,
 	.release	= cs4297a_release,
diff --git a/sound/oss/vwsnd.c b/sound/oss/vwsnd.c
index 20b3b32..23b69d6 100644
--- a/sound/oss/vwsnd.c
+++ b/sound/oss/vwsnd.c
@@ -2429,8 +2429,7 @@ static unsigned int vwsnd_audio_poll(struct file *file,
 	return mask;
 }
 
-static int vwsnd_audio_do_ioctl(struct inode *inode,
-				struct file *file,
+static int vwsnd_audio_do_ioctl(struct file *file,
 				unsigned int cmd,
 				unsigned long arg)
 {
@@ -2446,8 +2445,8 @@ static int vwsnd_audio_do_ioctl(struct inode *inode,
 	int ival;
 
 	
-	DBGEV("(inode=0x%p, file=0x%p, cmd=0x%x, arg=0x%lx)\n",
-	      inode, file, cmd, arg);
+	DBGEV("(file=0x%p, cmd=0x%x, arg=0x%lx)\n",
+	      file, cmd, arg);
 	switch (cmd) {
 	case OSS_GETVERSION:		/* _SIOR ('M', 118, int) */
 		DBGX("OSS_GETVERSION\n");
@@ -2885,17 +2884,19 @@ static int vwsnd_audio_do_ioctl(struct inode *inode,
 	return -EINVAL;
 }
 
-static int vwsnd_audio_ioctl(struct inode *inode,
-				struct file *file,
+static long vwsnd_audio_ioctl(struct file *file,
 				unsigned int cmd,
 				unsigned long arg)
 {
 	vwsnd_dev_t *devc = (vwsnd_dev_t *) file->private_data;
 	int ret;
 
+	lock_kernel();
 	mutex_lock(&devc->io_mutex);
-	ret = vwsnd_audio_do_ioctl(inode, file, cmd, arg);
+	ret = vwsnd_audio_do_ioctl(file, cmd, arg);
 	mutex_unlock(&devc->io_mutex);
+	unlock_kernel();
+
 	return ret;
 }
 
@@ -3044,7 +3045,7 @@ static const struct file_operations vwsnd_audio_fops = {
 	.read =		vwsnd_audio_read,
 	.write =	vwsnd_audio_write,
 	.poll =		vwsnd_audio_poll,
-	.ioctl =	vwsnd_audio_ioctl,
+	.unlocked_ioctl = vwsnd_audio_ioctl,
 	.mmap =		vwsnd_audio_mmap,
 	.open =		vwsnd_audio_open,
 	.release =	vwsnd_audio_release,
@@ -3203,8 +3204,7 @@ static int mixer_write_ioctl(vwsnd_dev_t *devc, unsigned int nr, void __user *ar
 
 /* This is the ioctl entry to the mixer driver. */
 
-static int vwsnd_mixer_ioctl(struct inode *ioctl,
-			      struct file *file,
+static long vwsnd_mixer_ioctl(struct file *file,
 			      unsigned int cmd,
 			      unsigned long arg)
 {
@@ -3215,6 +3215,7 @@ static int vwsnd_mixer_ioctl(struct inode *ioctl,
 
 	DBGEV("(devc=0x%p, cmd=0x%x, arg=0x%lx)\n", devc, cmd, arg);
 
+	lock_kernel();
 	mutex_lock(&devc->mix_mutex);
 	{
 		if ((cmd & ~nrmask) == MIXER_READ(0))
@@ -3225,13 +3226,14 @@ static int vwsnd_mixer_ioctl(struct inode *ioctl,
 			retval = -EINVAL;
 	}
 	mutex_unlock(&devc->mix_mutex);
+	unlock_kernel();
 	return retval;
 }
 
 static const struct file_operations vwsnd_mixer_fops = {
 	.owner =	THIS_MODULE,
 	.llseek =	no_llseek,
-	.ioctl =	vwsnd_mixer_ioctl,
+	.unlocked_ioctl = vwsnd_mixer_ioctl,
 	.open =		vwsnd_mixer_open,
 	.release =	vwsnd_mixer_release,
 };
-- 
1.7.1

Re: [PATCH 2/6] sound/oss: convert to unlocked_ioctl

[Sam Ravnborg]

> 
> diff --git a/sound/oss/au1550_ac97.c b/sound/oss/au1550_ac97.c
> index c1070e3..3547450 100644
> --- a/sound/oss/au1550_ac97.c
> +++ b/sound/oss/au1550_ac97.c
> @@ -824,22 +824,26 @@ mixdev_ioctl(struct ac97_codec *codec, unsigned int cmd,
>  	return codec->mixer_ioctl(codec, cmd, arg);
>  }
>  
> -static int
> -au1550_ioctl_mixdev(struct inode *inode, struct file *file,
> -			       unsigned int cmd, unsigned long arg)
> +static long
> +au1550_ioctl_mixdev(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct au1550_state *s = (struct au1550_state *)file->private_data;
>  	struct ac97_codec *codec = s->codec;
> +	int ret;
> +
> +	lock_kernel();
> +	ret = mixdev_ioctl(codec, cmd, arg);
> +	unlock_kernel();
>  
> -	return mixdev_ioctl(codec, cmd, arg);
> +	return ret;
>  }

General comment...
In several places you declare a local variable of type "int",
but the function return a long.

I know that mixdev_ioctl() return an int so nothing is lost but
it just looks wrong.

> diff --git a/sound/oss/dmasound/dmasound_core.c b/sound/oss/dmasound/dmasound_core.c
> index 3f3c3f7..b5464fb 100644
> --- a/sound/oss/dmasound/dmasound_core.c
> +++ b/sound/oss/dmasound/dmasound_core.c
> @@ -337,8 +337,8 @@ static int mixer_release(struct inode *inode, struct file *file)
>  	unlock_kernel();
>  	return 0;
>  }
> -static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
> -		       u_long arg)
> +
> +static long mixer_ioctl(struct file *file, u_int cmd, u_long arg)
>  {
>  	if (_SIOC_DIR(cmd) & _SIOC_WRITE)
>  	    mixer.modify_counter++;
> @@ -362,11 +362,22 @@ static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
>  	return -EINVAL;
>  }
>  
> +static long mixer_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
> +{
> +	int ret;
> +
> +	lock_kernel();
> +	ret = mixer_ioctl(file, cmd, arg);
> +	unlock_kernel();
> +
> +	return ret;
> +}

Here it looks like a potential but.
mixer_ioctl() return a long which is stored in an int and then we return a long.


> -static int sq_ioctl(struct inode *inode, struct file *file, u_int cmd,
> -		    u_long arg)
> +static long sq_ioctl(struct file *file, u_int cmd, u_long arg)
>  {
>  	int val, result;
>  	u_long fmt;
> @@ -1114,18 +1124,29 @@ static int sq_ioctl(struct inode *inode, struct file *file, u_int cmd,
>  		return IOCTL_OUT(arg,val);
>  
>  	default:
> -		return mixer_ioctl(inode, file, cmd, arg);
> +		return mixer_ioctl(file, cmd, arg);
>  	}
>  	return -EINVAL;
>  }
>  
> +static long sq_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
> +{
> +	int ret;
> +
> +	lock_kernel();
> +	ret = sq_ioctl(file, cmd, arg);
> +	unlock_kernel();
> +
> +	return ret;
> +}
ditto: long -> int -> long


	Sam

Re: [PATCH 2/6] sound/oss: convert to unlocked_ioctl

[Arnd Bergmann]

On Sunday 04 July 2010 13:08:35 Sam Ravnborg wrote:
>
> General comment...
> In several places you declare a local variable of type "int",
> but the function return a long.
> 
> I know that mixdev_ioctl() return an int so nothing is lost but
> it just looks wrong.

This is completely intentional and follows what we have done in
lots of other drivers that are already converted the same way.

The idea is that it was a bad choice to make the 'unlocked_ioctl'
operation return 'long' in the first place. I remember Al
ranting about this a few years ago. The ioctl syscall always
returns an 'int' to user space, the only reason we have a
'long' return code in the definition of sys_ioctl and most
other syscalls is to avoid problems for 32 bit emulation
on some architectures.
 
Maybe one day someone will rename all unlocked_ioctl calls
back to ioctl and change the return type back to int.

> > diff --git a/sound/oss/dmasound/dmasound_core.c b/sound/oss/dmasound/dmasound_core.c
> > index 3f3c3f7..b5464fb 100644
> > --- a/sound/oss/dmasound/dmasound_core.c
> > +++ b/sound/oss/dmasound/dmasound_core.c
> > @@ -337,8 +337,8 @@ static int mixer_release(struct inode *inode, struct file *file)
> >  	unlock_kernel();
> >  	return 0;
> >  }
> > -static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
> > -		       u_long arg)
> > +
> > +static long mixer_ioctl(struct file *file, u_int cmd, u_long arg)
> >  {
> >  	if (_SIOC_DIR(cmd) & _SIOC_WRITE)
> >  	    mixer.modify_counter++;
> > @@ -362,11 +362,22 @@ static int mixer_ioctl(struct inode *inode, struct file *file, u_int cmd,
> >  	return -EINVAL;
> >  }
> >  
> > +static long mixer_unlocked_ioctl(struct file *file, u_int cmd, u_long arg)
> > +{
> > +	int ret;
> > +
> > +	lock_kernel();
> > +	ret = mixer_ioctl(file, cmd, arg);
> > +	unlock_kernel();
> > +
> > +	return ret;
> > +}
> 
> Here it looks like a potential but.
> mixer_ioctl() return a long which is stored in an int and then we return a long.

Right. I should probably have left the return value of mixer_ioctl as an int
instead, to follow the scheme used in other drivers.

	Arnd

[PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Arnd Bergmann]

Handling of autofs ioctl numbers does not need to be generic
and can easily be done directly in autofs itself.

This also pushes the BKL into autofs and autofs4 ioctl
methods.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Ian Kent <raven@themaw.net>
Cc: autofs@linux.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 fs/autofs/root.c        |   67 ++++++++++++++++++++++++++++++++++++++++++++--
 fs/autofs4/root.c       |   49 ++++++++++++++++++++++++++++++++++
 fs/compat_ioctl.c       |   36 -------------------------
 include/linux/auto_fs.h |    1 +
 4 files changed, 114 insertions(+), 39 deletions(-)

diff --git a/fs/autofs/root.c b/fs/autofs/root.c
index 9a0520b..11b1ea7 100644
--- a/fs/autofs/root.c
+++ b/fs/autofs/root.c
@@ -16,6 +16,7 @@
 #include <linux/slab.h>
 #include <linux/param.h>
 #include <linux/time.h>
+#include <linux/compat.h>
 #include <linux/smp_lock.h>
 #include "autofs_i.h"
 
@@ -25,13 +26,17 @@ static int autofs_root_symlink(struct inode *,struct dentry *,const char *);
 static int autofs_root_unlink(struct inode *,struct dentry *);
 static int autofs_root_rmdir(struct inode *,struct dentry *);
 static int autofs_root_mkdir(struct inode *,struct dentry *,int);
-static int autofs_root_ioctl(struct inode *, struct file *,unsigned int,unsigned long);
+static long autofs_root_ioctl(struct file *,unsigned int,unsigned long);
+static long autofs_root_compat_ioctl(struct file *,unsigned int,unsigned long);
 
 const struct file_operations autofs_root_operations = {
 	.llseek		= generic_file_llseek,
 	.read		= generic_read_dir,
 	.readdir	= autofs_root_readdir,
-	.ioctl		= autofs_root_ioctl,
+	.unlocked_ioctl	= autofs_root_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= autofs_root_compat_ioctl,
+#endif
 };
 
 const struct inode_operations autofs_root_inode_operations = {
@@ -492,6 +497,25 @@ static int autofs_root_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 }
 
 /* Get/set timeout ioctl() operation */
+#ifdef CONFIG_COMPAT
+static inline int autofs_compat_get_set_timeout(struct autofs_sb_info *sbi,
+					 unsigned int __user *p)
+{
+	unsigned long ntimeout;
+
+	if (get_user(ntimeout, p) ||
+	    put_user(sbi->exp_timeout / HZ, p))
+		return -EFAULT;
+
+	if (ntimeout > UINT_MAX/HZ)
+		sbi->exp_timeout = 0;
+	else
+		sbi->exp_timeout = ntimeout * HZ;
+
+	return 0;
+}
+#endif
+
 static inline int autofs_get_set_timeout(struct autofs_sb_info *sbi,
 					 unsigned long __user *p)
 {
@@ -546,7 +570,7 @@ static inline int autofs_expire_run(struct super_block *sb,
  * ioctl()'s on the root directory is the chief method for the daemon to
  * generate kernel reactions
  */
-static int autofs_root_ioctl(struct inode *inode, struct file *filp,
+static int autofs_do_root_ioctl(struct inode *inode, struct file *filp,
 			     unsigned int cmd, unsigned long arg)
 {
 	struct autofs_sb_info *sbi = autofs_sbi(inode->i_sb);
@@ -571,6 +595,10 @@ static int autofs_root_ioctl(struct inode *inode, struct file *filp,
 		return 0;
 	case AUTOFS_IOC_PROTOVER: /* Get protocol version */
 		return autofs_get_protover(argp);
+#ifdef CONFIG_COMPAT
+	case AUTOFS_IOC_SETTIMEOUT32:
+		return autofs_compat_get_set_timeout(sbi, argp);
+#endif
 	case AUTOFS_IOC_SETTIMEOUT:
 		return autofs_get_set_timeout(sbi, argp);
 	case AUTOFS_IOC_EXPIRE:
@@ -579,4 +607,37 @@ static int autofs_root_ioctl(struct inode *inode, struct file *filp,
 	default:
 		return -ENOSYS;
 	}
+
+}
+
+static long autofs_root_ioctl(struct file *filp,
+			     unsigned int cmd, unsigned long arg)
+{
+	int ret;
+
+	lock_kernel();
+	ret = autofs_do_root_ioctl(filp->f_path.dentry->d_inode,
+				   filp, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
+#ifdef CONFIG_COMPAT
+static long autofs_root_compat_ioctl(struct file *filp,
+			     unsigned int cmd, unsigned long arg)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	int ret;
+
+	lock_kernel();
+	if (cmd == AUTOFS_IOC_READY || cmd == AUTOFS_IOC_FAIL)
+		ret = autofs_do_root_ioctl(inode, filp, cmd, arg);
+	else
+		ret = autofs_do_root_ioctl(inode, filp, cmd,
+			(unsigned long)compat_ptr(arg));
+	unlock_kernel();
+
+	return ret;
 }
+#endif
diff --git a/fs/autofs4/root.c b/fs/autofs4/root.c
index db4117e..48e056e 100644
--- a/fs/autofs4/root.c
+++ b/fs/autofs4/root.c
@@ -18,7 +18,9 @@
 #include <linux/slab.h>
 #include <linux/param.h>
 #include <linux/time.h>
+#include <linux/compat.h>
 #include <linux/smp_lock.h>
+
 #include "autofs_i.h"
 
 static int autofs4_dir_symlink(struct inode *,struct dentry *,const char *);
@@ -26,6 +28,7 @@ static int autofs4_dir_unlink(struct inode *,struct dentry *);
 static int autofs4_dir_rmdir(struct inode *,struct dentry *);
 static int autofs4_dir_mkdir(struct inode *,struct dentry *,int);
 static long autofs4_root_ioctl(struct file *,unsigned int,unsigned long);
+static long autofs4_root_compat_ioctl(struct file *,unsigned int,unsigned long);
 static int autofs4_dir_open(struct inode *inode, struct file *file);
 static struct dentry *autofs4_lookup(struct inode *,struct dentry *, struct nameidata *);
 static void *autofs4_follow_link(struct dentry *, struct nameidata *);
@@ -40,6 +43,9 @@ const struct file_operations autofs4_root_operations = {
 	.readdir	= dcache_readdir,
 	.llseek		= dcache_dir_lseek,
 	.unlocked_ioctl	= autofs4_root_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= autofs4_root_compat_ioctl,
+#endif
 };
 
 const struct file_operations autofs4_dir_operations = {
@@ -840,6 +846,26 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 }
 
 /* Get/set timeout ioctl() operation */
+#ifdef CONFIG_COMPAT
+static inline int autofs4_compat_get_set_timeout(struct autofs_sb_info *sbi,
+					 compat_ulong_t __user *p)
+{
+	int rv;
+	unsigned long ntimeout;
+
+	if ((rv = get_user(ntimeout, p)) ||
+	     (rv = put_user(sbi->exp_timeout/HZ, p)))
+		return rv;
+
+	if (ntimeout > UINT_MAX/HZ)
+		sbi->exp_timeout = 0;
+	else
+		sbi->exp_timeout = ntimeout * HZ;
+
+	return 0;
+}
+#endif
+
 static inline int autofs4_get_set_timeout(struct autofs_sb_info *sbi,
 					 unsigned long __user *p)
 {
@@ -933,6 +959,10 @@ static int autofs4_root_ioctl_unlocked(struct inode *inode, struct file *filp,
 		return autofs4_get_protosubver(sbi, p);
 	case AUTOFS_IOC_SETTIMEOUT:
 		return autofs4_get_set_timeout(sbi, p);
+#ifdef CONFIG_COMPAT
+	case AUTOFS_IOC_SETTIMEOUT32:
+		return autofs4_compat_get_set_timeout(sbi, p);
+#endif
 
 	case AUTOFS_IOC_ASKUMOUNT:
 		return autofs4_ask_umount(filp->f_path.mnt, p);
@@ -961,3 +991,22 @@ static long autofs4_root_ioctl(struct file *filp,
 
 	return ret;
 }
+
+#ifdef CONFIG_COMPAT
+static long autofs4_root_compat_ioctl(struct file *filp,
+			     unsigned int cmd, unsigned long arg)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	int ret;
+
+	lock_kernel();
+	if (cmd == AUTOFS_IOC_READY || cmd == AUTOFS_IOC_FAIL)
+		ret = autofs4_root_ioctl_unlocked(inode, filp, cmd, arg);
+	else
+		ret = autofs4_root_ioctl_unlocked(inode, filp, cmd,
+			(unsigned long)compat_ptr(arg));
+	unlock_kernel();
+
+	return ret;
+}
+#endif
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 641640d..618f381 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -131,23 +131,6 @@ static int w_long(unsigned int fd, unsigned int cmd,
 	return err;
 }
 
-static int rw_long(unsigned int fd, unsigned int cmd,
-		compat_ulong_t __user *argp)
-{
-	mm_segment_t old_fs = get_fs();
-	int err;
-	unsigned long val;
-
-	if(get_user(val, argp))
-		return -EFAULT;
-	set_fs (KERNEL_DS);
-	err = sys_ioctl(fd, cmd, (unsigned long)&val);
-	set_fs (old_fs);
-	if (!err && put_user(val, argp))
-		return -EFAULT;
-	return err;
-}
-
 struct compat_video_event {
 	int32_t		type;
 	compat_time_t	timestamp;
@@ -594,12 +577,6 @@ static int do_smb_getmountuid(unsigned int fd, unsigned int cmd,
 	return err;
 }
 
-static int ioc_settimeout(unsigned int fd, unsigned int cmd,
-		compat_ulong_t __user *argp)
-{
-	return rw_long(fd, AUTOFS_IOC_SETTIMEOUT, argp);
-}
-
 /* Bluetooth ioctls */
 #define HCIUARTSETPROTO	_IOW('U', 200, int)
 #define HCIUARTGETPROTO	_IOR('U', 201, int)
@@ -1281,13 +1258,6 @@ COMPATIBLE_IOCTL(SOUND_MIXER_PRIVATE5)
 COMPATIBLE_IOCTL(SOUND_MIXER_GETLEVELS)
 COMPATIBLE_IOCTL(SOUND_MIXER_SETLEVELS)
 COMPATIBLE_IOCTL(OSS_GETVERSION)
-/* AUTOFS */
-COMPATIBLE_IOCTL(AUTOFS_IOC_CATATONIC)
-COMPATIBLE_IOCTL(AUTOFS_IOC_PROTOVER)
-COMPATIBLE_IOCTL(AUTOFS_IOC_EXPIRE)
-COMPATIBLE_IOCTL(AUTOFS_IOC_EXPIRE_MULTI)
-COMPATIBLE_IOCTL(AUTOFS_IOC_PROTOSUBVER)
-COMPATIBLE_IOCTL(AUTOFS_IOC_ASKUMOUNT)
 /* Raw devices */
 COMPATIBLE_IOCTL(RAW_SETBIND)
 COMPATIBLE_IOCTL(RAW_GETBIND)
@@ -1552,9 +1522,6 @@ static long do_ioctl_trans(int fd, unsigned int cmd,
 	case RAW_GETBIND:
 		return raw_ioctl(fd, cmd, argp);
 #endif
-#define AUTOFS_IOC_SETTIMEOUT32 _IOWR(0x93,0x64,unsigned int)
-	case AUTOFS_IOC_SETTIMEOUT32:
-		return ioc_settimeout(fd, cmd, argp);
 	/* One SMB ioctl needs translations. */
 #define SMB_IOC_GETMOUNTUID_32 _IOR('u', 1, compat_uid_t)
 	case SMB_IOC_GETMOUNTUID_32:
@@ -1609,9 +1576,6 @@ static long do_ioctl_trans(int fd, unsigned int cmd,
 	case KDSKBMETA:
 	case KDSKBLED:
 	case KDSETLED:
-	/* AUTOFS */
-	case AUTOFS_IOC_READY:
-	case AUTOFS_IOC_FAIL:
 	/* NBD */
 	case NBD_SET_SOCK:
 	case NBD_SET_BLKSIZE:
diff --git a/include/linux/auto_fs.h b/include/linux/auto_fs.h
index 7b09c83..da64e15 100644
--- a/include/linux/auto_fs.h
+++ b/include/linux/auto_fs.h
@@ -79,6 +79,7 @@ struct autofs_packet_expire {
 #define AUTOFS_IOC_FAIL       _IO(0x93,0x61)
 #define AUTOFS_IOC_CATATONIC  _IO(0x93,0x62)
 #define AUTOFS_IOC_PROTOVER   _IOR(0x93,0x63,int)
+#define AUTOFS_IOC_SETTIMEOUT32 _IOWR(0x93,0x64,compat_ulong_t)
 #define AUTOFS_IOC_SETTIMEOUT _IOWR(0x93,0x64,unsigned long)
 #define AUTOFS_IOC_EXPIRE     _IOR(0x93,0x65,struct autofs_packet_expire)
 
-- 
1.7.1

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Frederic Weisbecker]

On Sun, Jul 04, 2010 at 12:15:07AM +0200, Arnd Bergmann wrote:
> Handling of autofs ioctl numbers does not need to be generic
> and can easily be done directly in autofs itself.
> 
> This also pushes the BKL into autofs and autofs4 ioctl
> methods.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: H. Peter Anvin <hpa@zytor.com>



Peter, if you're fine with this version. May I apply it?
Unless you have a tree for autofs.

Thanks.

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[H. Peter Anvin]

On 07/05/2010 12:24 PM, Frederic Weisbecker wrote:
> On Sun, Jul 04, 2010 at 12:15:07AM +0200, Arnd Bergmann wrote:
>> Handling of autofs ioctl numbers does not need to be generic
>> and can easily be done directly in autofs itself.
>>
>> This also pushes the BKL into autofs and autofs4 ioctl
>> methods.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Cc: H. Peter Anvin <hpa@zytor.com>
> 
> Peter, if you're fine with this version. May I apply it?
> Unless you have a tree for autofs.
> 
> Thanks.

Ian Kent is the maintainer of autofs4 and patches for autofs4 should go
through him (or acked by him.)

autofs 3 is officially unmaintained; I'm more than happy to have you
push the autofs 3 bits of this patch.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Frederic Weisbecker]

On Mon, Jul 05, 2010 at 12:42:59PM -0700, H. Peter Anvin wrote:
> On 07/05/2010 12:24 PM, Frederic Weisbecker wrote:
> > On Sun, Jul 04, 2010 at 12:15:07AM +0200, Arnd Bergmann wrote:
> >> Handling of autofs ioctl numbers does not need to be generic
> >> and can easily be done directly in autofs itself.
> >>
> >> This also pushes the BKL into autofs and autofs4 ioctl
> >> methods.
> >>
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> Cc: H. Peter Anvin <hpa@zytor.com>
> > 
> > Peter, if you're fine with this version. May I apply it?
> > Unless you have a tree for autofs.
> > 
> > Thanks.
> 
> Ian Kent is the maintainer of autofs4 and patches for autofs4 should go
> through him (or acked by him.)
> 
> autofs 3 is officially unmaintained; I'm more than happy to have you
> push the autofs 3 bits of this patch.


Sure, I can split up the patch and integrate the autofs 3 part, I'll send
the standalone autofs4 version to Ian.

Thanks.

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Arnd Bergmann]

On Monday 05 July 2010 21:48:01 Frederic Weisbecker wrote:
> On Mon, Jul 05, 2010 at 12:42:59PM -0700, H. Peter Anvin wrote:
> > On 07/05/2010 12:24 PM, Frederic Weisbecker wrote:
> > > 
> > > Peter, if you're fine with this version. May I apply it?
> > > Unless you have a tree for autofs.
> > > 
> > Ian Kent is the maintainer of autofs4 and patches for autofs4 should go
> > through him (or acked by him.)
> > 
> > autofs 3 is officially unmaintained; I'm more than happy to have you
> > push the autofs 3 bits of this patch.
> 
> Sure, I can split up the patch and integrate the autofs 3 part, I'll send
> the standalone autofs4 version to Ian.

I think in this case it's really more appropriate to change both autofs3
and autofs4 together, to avoid interdependencies. Whichever way Ian
prefers (ack the patch or take it) would work though.

	Arnd

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Frederic Weisbecker]

On Mon, Jul 05, 2010 at 09:52:55PM +0200, Arnd Bergmann wrote:
> On Monday 05 July 2010 21:48:01 Frederic Weisbecker wrote:
> > On Mon, Jul 05, 2010 at 12:42:59PM -0700, H. Peter Anvin wrote:
> > > On 07/05/2010 12:24 PM, Frederic Weisbecker wrote:
> > > > 
> > > > Peter, if you're fine with this version. May I apply it?
> > > > Unless you have a tree for autofs.
> > > > 
> > > Ian Kent is the maintainer of autofs4 and patches for autofs4 should go
> > > through him (or acked by him.)
> > > 
> > > autofs 3 is officially unmaintained; I'm more than happy to have you
> > > push the autofs 3 bits of this patch.
> > 
> > Sure, I can split up the patch and integrate the autofs 3 part, I'll send
> > the standalone autofs4 version to Ian.
> 
> I think in this case it's really more appropriate to change both autofs3
> and autofs4 together, to avoid interdependencies. Whichever way Ian
> prefers (ack the patch or take it) would work though.
> 
> 	Arnd


Yeah indeed. Ian?

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[H. Peter Anvin]

On 07/05/2010 12:58 PM, Frederic Weisbecker wrote:
> On Mon, Jul 05, 2010 at 09:52:55PM +0200, Arnd Bergmann wrote:
>> On Monday 05 July 2010 21:48:01 Frederic Weisbecker wrote:
>>> On Mon, Jul 05, 2010 at 12:42:59PM -0700, H. Peter Anvin wrote:
>>>> On 07/05/2010 12:24 PM, Frederic Weisbecker wrote:
>>>>>
>>>>> Peter, if you're fine with this version. May I apply it?
>>>>> Unless you have a tree for autofs.
>>>>>
>>>> Ian Kent is the maintainer of autofs4 and patches for autofs4 should go
>>>> through him (or acked by him.)
>>>>
>>>> autofs 3 is officially unmaintained; I'm more than happy to have you
>>>> push the autofs 3 bits of this patch.
>>>
>>> Sure, I can split up the patch and integrate the autofs 3 part, I'll send
>>> the standalone autofs4 version to Ian.
>>
>> I think in this case it's really more appropriate to change both autofs3
>> and autofs4 together, to avoid interdependencies. Whichever way Ian
>> prefers (ack the patch or take it) would work though.
>>
>> 	Arnd
> 
> 
> Yeah indeed. Ian?
> 

For what it's worth, feel free to add my:

Acked-by: H. Peter Anvin <hpa@zytor.com>

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Ian Kent]

On Mon, 2010-07-05 at 21:58 +0200, Frederic Weisbecker wrote:
> On Mon, Jul 05, 2010 at 09:52:55PM +0200, Arnd Bergmann wrote:
> > On Monday 05 July 2010 21:48:01 Frederic Weisbecker wrote:
> > > On Mon, Jul 05, 2010 at 12:42:59PM -0700, H. Peter Anvin wrote:
> > > > On 07/05/2010 12:24 PM, Frederic Weisbecker wrote:
> > > > > 
> > > > > Peter, if you're fine with this version. May I apply it?
> > > > > Unless you have a tree for autofs.
> > > > > 
> > > > Ian Kent is the maintainer of autofs4 and patches for autofs4 should go
> > > > through him (or acked by him.)
> > > > 
> > > > autofs 3 is officially unmaintained; I'm more than happy to have you
> > > > push the autofs 3 bits of this patch.
> > > 
> > > Sure, I can split up the patch and integrate the autofs 3 part, I'll send
> > > the standalone autofs4 version to Ian.
> > 
> > I think in this case it's really more appropriate to change both autofs3
> > and autofs4 together, to avoid interdependencies. Whichever way Ian
> > prefers (ack the patch or take it) would work though.
> > 
> > 	Arnd
> 
> 
> Yeah indeed. Ian?

As Peter says the autofs module isn't supported anymore and autofs4 will
replace autofs when I eventually get around to re-doing and re-testing
my patch for that.

So it makes no difference whether the patches are combined, if it breaks
autofs then it probably won't be fixed but it may cause the replacement
to happen sooner.

As far as the patch goes that should be fine and we should be able to
remove the BKL from autofs4 soon after but I'm not brave enough to try
just yet.

Ian

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Arnd Bergmann]

On Tuesday 06 July 2010, Ian Kent wrote:
> So it makes no difference whether the patches are combined, if it breaks
> autofs then it probably won't be fixed but it may cause the replacement
> to happen sooner.

Well, the main point of applying the patch now is to avoid breaking the
autofs module when we remove the .ioctl operation.

> As far as the patch goes that should be fine and we should be able to
> remove the BKL from autofs4 soon after but I'm not brave enough to try
> just yet.

Well, the only use of the BKL in autofs4 is in the ioctl function. You
can probably replace that trivially with a global mutex, but from a quick
inspection, even that should not be needed: The only ioctl command in
autofs4 that does not already seem to have adequate locking is
autofs4_get_set_timeout, which is even easier to change and still harmless
if you don't do it at all.

	Arnd

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Ian Kent]

On 06/07/2010, at 7:35 PM, Arnd Bergmann <arnd@arndb.de> wrote:

> 
> Well, the only use of the BKL in autofs4 is in the ioctl function. You
> can probably replace that trivially with a global mutex, but from a quick
> inspection, even that should not be needed: The only ioctl command in
> autofs4 that does not already seem to have adequate locking is
> autofs4_get_set_timeout, which is even easier to change and still harmless
> if you don't do it at all.

That's right of course.

Even this shouldn't be a problem as it is called by a single instance, per autofs mount, of the daemon only. A fair amount of effort has gone into trying to make the autofs4 module independent of the BKL over time. However I would still rather not do the change concurrently with the ioctl changes.

Ian
> 

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Frederic Weisbecker]

2010/7/6 Ian Kent <raven@themaw.net>:
>
>
> On 06/07/2010, at 7:35 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
>>
>> Well, the only use of the BKL in autofs4 is in the ioctl function. You
>> can probably replace that trivially with a global mutex, but from a quick
>> inspection, even that should not be needed: The only ioctl command in
>> autofs4 that does not already seem to have adequate locking is
>> autofs4_get_set_timeout, which is even easier to change and still harmless
>> if you don't do it at all.
>
> That's right of course.
>
> Even this shouldn't be a problem as it is called by a single instance, per autofs mount, of the daemon only. A fair amount of effort has gone into trying to make the autofs4 module independent of the BKL over time. However I would still rather not do the change concurrently with the ioctl changes.
>
> Ian


In any case, can we let you handle this patch for 2.6.36 inclusion?

Thanks.

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Ian Kent]

On Fri, 2010-07-16 at 02:14 +0200, Frederic Weisbecker wrote:
> 2010/7/6 Ian Kent <raven@themaw.net>:
> >
> >
> > On 06/07/2010, at 7:35 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> >>
> >> Well, the only use of the BKL in autofs4 is in the ioctl function. You
> >> can probably replace that trivially with a global mutex, but from a quick
> >> inspection, even that should not be needed: The only ioctl command in
> >> autofs4 that does not already seem to have adequate locking is
> >> autofs4_get_set_timeout, which is even easier to change and still harmless
> >> if you don't do it at all.
> >
> > That's right of course.
> >
> > Even this shouldn't be a problem as it is called by a single instance, per autofs mount, of the daemon only. A fair amount of effort has gone into trying to make the autofs4 module independent of the BKL over time. However I would still rather not do the change concurrently with the ioctl changes.
> >
> > Ian
> 
> 
> In any case, can we let you handle this patch for 2.6.36 inclusion?

I think it makes more sense to keep these common patches together so
Arnd probably should keep it with his series.

Ian

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Arnd Bergmann]

On Friday 16 July 2010, Ian Kent wrote:
> On Fri, 2010-07-16 at 02:14 +0200, Frederic Weisbecker wrote:
> > 2010/7/6 Ian Kent <raven@themaw.net>:
> > In any case, can we let you handle this patch for 2.6.36 inclusion?
> 
> I think it makes more sense to keep these common patches together so
> Arnd probably should keep it with his series.

I think that's fine. Takashi Iwai took the alsa patch, but the
rest can probably be merged together.

Frederic, would you like to add the remaining ioctl removal series
into your git tree and have Stephen pull that into linux-next?
I believe he's still pulling your tree anyway, although it doesn't
have much content at the moment.

	Arnd

Re: [PATCH 3/6] autofs/autofs4: move compat_ioctl handling into fs

[Frederic Weisbecker]

On Fri, Jul 16, 2010 at 01:05:31PM +0200, Arnd Bergmann wrote:
> On Friday 16 July 2010, Ian Kent wrote:
> > On Fri, 2010-07-16 at 02:14 +0200, Frederic Weisbecker wrote:
> > > 2010/7/6 Ian Kent <raven@themaw.net>:
> > > In any case, can we let you handle this patch for 2.6.36 inclusion?
> > 
> > I think it makes more sense to keep these common patches together so
> > Arnd probably should keep it with his series.
> 
> I think that's fine. Takashi Iwai took the alsa patch, but the
> rest can probably be merged together.
> 
> Frederic, would you like to add the remaining ioctl removal series
> into your git tree and have Stephen pull that into linux-next?
> I believe he's still pulling your tree anyway, although it doesn't
> have much content at the moment.
> 
> 	Arnd


Sure, I'll do that next week.

Thanks.

[PATCH 4/6] cris: Pushdown the bkl from ioctl

[Arnd Bergmann]

From: Frederic Weisbecker <fweisbec@gmail.com>

Pushdown the bkl to the remaining drivers using the
deprecated .ioctl.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Mikael Starvik <starvik@axis.com>
Cc: Jesper Nilsson <jesper.nilsson@axis.com>
Cc: Cris <linux-cris-kernel@axis.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: John Kacur <jkacur@redhat.com>
---
 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 +++++++++++++++++++--------
 7 files changed, 137 insertions(+), 60 deletions(-)

diff --git a/arch/cris/arch-v10/drivers/gpio.c b/arch/cris/arch-v10/drivers/gpio.c
index 4b0f65f..300bd37 100644
--- a/arch/cris/arch-v10/drivers/gpio.c
+++ b/arch/cris/arch-v10/drivers/gpio.c
@@ -46,8 +46,7 @@ static char gpio_name[] = "etrax gpio";
 static wait_queue_head_t *gpio_wq;
 #endif
 
-static int gpio_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg);
+static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 static ssize_t gpio_write(struct file *file, const char __user *buf,
 	size_t count, loff_t *off);
 static int gpio_open(struct inode *inode, struct file *filp);
@@ -505,8 +504,7 @@ static int
 gpio_leds_ioctl(unsigned int cmd, unsigned long arg);
 
 static int
-gpio_ioctl(struct inode *inode, struct file *file,
-	   unsigned int cmd, unsigned long arg)
+gpio_ioctl_unlocked(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	unsigned long flags;
 	unsigned long val;
@@ -684,6 +682,18 @@ gpio_ioctl(struct inode *inode, struct file *file,
 }
 
 static int
+gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	lock_kernel();
+	ret = gpio_ioctl_unlocked(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
+static int
 gpio_leds_ioctl(unsigned int cmd, unsigned long arg)
 {
 	unsigned char green;
@@ -713,12 +723,12 @@ gpio_leds_ioctl(unsigned int cmd, unsigned long arg)
 }
 
 static const struct file_operations gpio_fops = {
-	.owner       = THIS_MODULE,
-	.poll        = gpio_poll,
-	.ioctl       = gpio_ioctl,
-	.write       = gpio_write,
-	.open        = gpio_open,
-	.release     = gpio_release,
+	.owner		= THIS_MODULE,
+	.poll		= gpio_poll,
+	.unlocked_ioctl	= gpio_ioctl,
+	.write		= gpio_write,
+	.open		= gpio_open,
+	.release	= gpio_release,
 };
 
 static void ioif_watcher(const unsigned int gpio_in_available,
diff --git a/arch/cris/arch-v10/drivers/i2c.c b/arch/cris/arch-v10/drivers/i2c.c
index a8737a8..6da8580 100644
--- a/arch/cris/arch-v10/drivers/i2c.c
+++ b/arch/cris/arch-v10/drivers/i2c.c
@@ -580,8 +580,7 @@ i2c_release(struct inode *inode, struct file *filp)
  */
 
 static int
-i2c_ioctl(struct inode *inode, struct file *file,
-	  unsigned int cmd, unsigned long arg)
+i2c_ioctl_unlocked(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	if(_IOC_TYPE(cmd) != ETRAXI2C_IOCTYPE) {
 		return -EINVAL;
@@ -617,11 +616,24 @@ i2c_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
+static long
+i2c_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	lock_kernel();
+	ret = i2c_ioctl_unlocked(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
+
 static const struct file_operations i2c_fops = {
-	.owner    = THIS_MODULE,
-	.ioctl    = i2c_ioctl,
-	.open     = i2c_open,
-	.release  = i2c_release,
+	.owner    	= THIS_MODULE,
+	.unlocked_ioctl = i2c_ioctl,
+	.open		= i2c_open,
+	.release	= i2c_release,
 };
 
 int __init
diff --git a/arch/cris/arch-v10/drivers/sync_serial.c b/arch/cris/arch-v10/drivers/sync_serial.c
index 109dcd8..5295485 100644
--- a/arch/cris/arch-v10/drivers/sync_serial.c
+++ b/arch/cris/arch-v10/drivers/sync_serial.c
@@ -157,7 +157,7 @@ static int sync_serial_open(struct inode *inode, struct file *file);
 static int sync_serial_release(struct inode *inode, struct file *file);
 static unsigned int sync_serial_poll(struct file *filp, poll_table *wait);
 
-static int sync_serial_ioctl(struct inode *inode, struct file *file,
+static long sync_serial_ioctl(struct file *file,
 	unsigned int cmd, unsigned long arg);
 static ssize_t sync_serial_write(struct file *file, const char *buf,
 	size_t count, loff_t *ppos);
@@ -244,13 +244,13 @@ static unsigned sync_serial_prescale_shadow;
 #define NUMBER_OF_PORTS 2
 
 static const struct file_operations sync_serial_fops = {
-	.owner   = THIS_MODULE,
-	.write   = sync_serial_write,
-	.read    = sync_serial_read,
-	.poll    = sync_serial_poll,
-	.ioctl   = sync_serial_ioctl,
-	.open    = sync_serial_open,
-	.release = sync_serial_release
+	.owner		= THIS_MODULE,
+	.write		= sync_serial_write,
+	.read		= sync_serial_read,
+	.poll		= sync_serial_poll,
+	.unlocked_ioctl = sync_serial_ioctl,
+	.open		= sync_serial_open,
+	.release	= sync_serial_release
 };
 
 static int __init etrax_sync_serial_init(void)
@@ -678,8 +678,8 @@ static unsigned int sync_serial_poll(struct file *file, poll_table *wait)
 	return mask;
 }
 
-static int sync_serial_ioctl(struct inode *inode, struct file *file,
-		  unsigned int cmd, unsigned long arg)
+static int sync_serial_ioctl_unlocked(struct file *file,
+				      unsigned int cmd, unsigned long arg)
 {
 	int return_val = 0;
 	unsigned long flags;
@@ -956,6 +956,18 @@ static int sync_serial_ioctl(struct inode *inode, struct file *file,
 	return return_val;
 }
 
+static long sync_serial_ioctl(struct file *file,
+			      unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	lock_kernel();
+	ret = sync_serial_ioctl_unlocked(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 
 static ssize_t sync_serial_write(struct file *file, const char *buf,
 	size_t count, loff_t *ppos)
diff --git a/arch/cris/arch-v32/drivers/cryptocop.c b/arch/cris/arch-v32/drivers/cryptocop.c
index b70fb34..1070bf2 100644
--- a/arch/cris/arch-v32/drivers/cryptocop.c
+++ b/arch/cris/arch-v32/drivers/cryptocop.c
@@ -217,7 +217,7 @@ static int cryptocop_open(struct inode *, struct file *);
 
 static int cryptocop_release(struct inode *, struct file *);
 
-static int cryptocop_ioctl(struct inode *inode, struct file *file,
+static long cryptocop_ioctl(struct file *file,
 			   unsigned int cmd, unsigned long arg);
 
 static void cryptocop_start_job(void);
@@ -279,10 +279,10 @@ static void print_user_dma_lists(struct cryptocop_dma_list_operation *dma_op);
 
 
 const struct file_operations cryptocop_fops = {
-	.owner =	THIS_MODULE,
-	.open =		cryptocop_open,
-	.release =	cryptocop_release,
-	.ioctl =	cryptocop_ioctl
+	.owner		= THIS_MODULE,
+	.open		= cryptocop_open,
+	.release	= cryptocop_release,
+	.unlocked_ioctl = cryptocop_ioctl,
 };
 
 
@@ -3102,7 +3102,7 @@ static int cryptocop_ioctl_create_session(struct inode *inode, struct file *filp
 	return 0;
 }
 
-static int cryptocop_ioctl(struct inode *inode, struct file *filp, unsigned int cmd, unsigned long arg)
+static int cryptocop_ioctl_unlocked(struct inode *inode, struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	int err = 0;
 	if (_IOC_TYPE(cmd) != ETRAXCRYPTOCOP_IOCTYPE) {
@@ -3134,6 +3134,18 @@ static int cryptocop_ioctl(struct inode *inode, struct file *filp, unsigned int
 	return 0;
 }
 
+static long cryptocop_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
+
+	lock_kernel();
+	ret = cryptocop_ioctl_unlocked(inode, filp, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 
 #ifdef LDEBUG
 static void print_dma_descriptors(struct cryptocop_int_operation *iop)
diff --git a/arch/cris/arch-v32/drivers/mach-a3/gpio.c b/arch/cris/arch-v32/drivers/mach-a3/gpio.c
index 97357cf..4dcfae3 100644
--- a/arch/cris/arch-v32/drivers/mach-a3/gpio.c
+++ b/arch/cris/arch-v32/drivers/mach-a3/gpio.c
@@ -72,8 +72,7 @@ static char gpio_name[] = "etrax gpio";
 static int virtual_gpio_ioctl(struct file *file, unsigned int cmd,
 			      unsigned long arg);
 #endif
-static int gpio_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg);
+static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 static ssize_t gpio_write(struct file *file, const char __user *buf,
 	size_t count, loff_t *off);
 static int gpio_open(struct inode *inode, struct file *filp);
@@ -521,7 +520,7 @@ static inline unsigned long setget_output(struct gpio_private *priv,
 	return dir_shadow;
 } /* setget_output */
 
-static int gpio_ioctl(struct inode *inode, struct file *file,
+static int gpio_ioctl_unlocked(struct file *file,
 	unsigned int cmd, unsigned long arg)
 {
 	unsigned long flags;
@@ -664,6 +663,17 @@ static int gpio_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
+static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	lock_kernel();
+	ret = gpio_ioctl_unlocked(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 #ifdef CONFIG_ETRAX_VIRTUAL_GPIO
 static int virtual_gpio_ioctl(struct file *file, unsigned int cmd,
 	unsigned long arg)
@@ -877,12 +887,12 @@ static int gpio_pwm_ioctl(struct gpio_private *priv, unsigned int cmd,
 }
 
 static const struct file_operations gpio_fops = {
-	.owner       = THIS_MODULE,
-	.poll        = gpio_poll,
-	.ioctl       = gpio_ioctl,
-	.write       = gpio_write,
-	.open        = gpio_open,
-	.release     = gpio_release,
+	.owner		= THIS_MODULE,
+	.poll		= gpio_poll,
+	.unlocked_ioctl = gpio_ioctl,
+	.write		= gpio_write,
+	.open		= gpio_open,
+	.release	= gpio_release,
 };
 
 #ifdef CONFIG_ETRAX_VIRTUAL_GPIO
diff --git a/arch/cris/arch-v32/drivers/mach-fs/gpio.c b/arch/cris/arch-v32/drivers/mach-fs/gpio.c
index d89ab80..d2e184c 100644
--- a/arch/cris/arch-v32/drivers/mach-fs/gpio.c
+++ b/arch/cris/arch-v32/drivers/mach-fs/gpio.c
@@ -74,8 +74,7 @@ static wait_queue_head_t *gpio_wq;
 static int virtual_gpio_ioctl(struct file *file, unsigned int cmd,
 	unsigned long arg);
 #endif
-static int gpio_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg);
+static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 static ssize_t gpio_write(struct file *file, const char *buf, size_t count,
 	loff_t *off);
 static int gpio_open(struct inode *inode, struct file *filp);
@@ -561,8 +560,7 @@ static int
 gpio_leds_ioctl(unsigned int cmd, unsigned long arg);
 
 static int
-gpio_ioctl(struct inode *inode, struct file *file,
-	   unsigned int cmd, unsigned long arg)
+gpio_ioctl_unlocked(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	unsigned long flags;
 	unsigned long val;
@@ -707,6 +705,17 @@ gpio_ioctl(struct inode *inode, struct file *file,
 	return 0;
 }
 
+static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	lock_kernel();
+	ret = gpio_ioctl_unlocked(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 #ifdef CONFIG_ETRAX_VIRTUAL_GPIO
 static int
 virtual_gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
@@ -856,12 +865,12 @@ gpio_leds_ioctl(unsigned int cmd, unsigned long arg)
 }
 
 static const struct file_operations gpio_fops = {
-	.owner       = THIS_MODULE,
-	.poll        = gpio_poll,
-	.ioctl       = gpio_ioctl,
-	.write       = gpio_write,
-	.open        = gpio_open,
-	.release     = gpio_release,
+	.owner		= THIS_MODULE,
+	.poll		= gpio_poll,
+	.unlocked_ioctl	= gpio_ioctl,
+	.write		= gpio_write,
+	.open		= gpio_open,
+	.release	= gpio_release,
 };
 
 #ifdef CONFIG_ETRAX_VIRTUAL_GPIO
diff --git a/arch/cris/arch-v32/drivers/sync_serial.c b/arch/cris/arch-v32/drivers/sync_serial.c
index 4889f19..bf6d2f9 100644
--- a/arch/cris/arch-v32/drivers/sync_serial.c
+++ b/arch/cris/arch-v32/drivers/sync_serial.c
@@ -153,7 +153,7 @@ static int sync_serial_open(struct inode *, struct file*);
 static int sync_serial_release(struct inode*, struct file*);
 static unsigned int sync_serial_poll(struct file *filp, poll_table *wait);
 
-static int sync_serial_ioctl(struct inode*, struct file*,
+static long sync_serial_ioctl(struct file *,
 			     unsigned int cmd, unsigned long arg);
 static ssize_t sync_serial_write(struct file * file, const char * buf,
 				 size_t count, loff_t *ppos);
@@ -241,13 +241,13 @@ static struct sync_port ports[]=
 #define NBR_PORTS ARRAY_SIZE(ports)
 
 static const struct file_operations sync_serial_fops = {
-	.owner   = THIS_MODULE,
-	.write   = sync_serial_write,
-	.read    = sync_serial_read,
-	.poll    = sync_serial_poll,
-	.ioctl   = sync_serial_ioctl,
-	.open    = sync_serial_open,
-	.release = sync_serial_release
+	.owner		= THIS_MODULE,
+	.write		= sync_serial_write,
+	.read		= sync_serial_read,
+	.poll		= sync_serial_poll,
+	.unlocked_ioctl = sync_serial_ioctl,
+	.open		= sync_serial_open,
+	.release	= sync_serial_release
 };
 
 static int __init etrax_sync_serial_init(void)
@@ -650,7 +650,7 @@ static unsigned int sync_serial_poll(struct file *file, poll_table *wait)
 	return mask;
 }
 
-static int sync_serial_ioctl(struct inode *inode, struct file *file,
+static int sync_serial_ioctl_unlocked(struct file *file,
 		  unsigned int cmd, unsigned long arg)
 {
 	int return_val = 0;
@@ -961,6 +961,18 @@ static int sync_serial_ioctl(struct inode *inode, struct file *file,
 	return return_val;
 }
 
+static long sync_serial_ioctl(struct file *file,
+			      unsigned int cmd, unsigned long arg)
+{
+	long ret;
+
+	lock_kernel();
+	ret = sync_serial_ioctl_unlocked(file, cmd, arg);
+	unlock_kernel();
+
+	return ret;
+}
+
 /* NOTE: sync_serial_write does not support concurrency */
 static ssize_t sync_serial_write(struct file *file, const char *buf,
 				 size_t count, loff_t *ppos)
-- 
1.7.1

Re: [PATCH 4/6] cris: Pushdown the bkl from ioctl

[Jesper Nilsson]

On Sun, Jul 04, 2010 at 12:15:08AM +0200, Arnd Bergmann wrote:
> From: Frederic Weisbecker <fweisbec@gmail.com>
> 
> Pushdown the bkl to the remaining drivers using the
> deprecated .ioctl.

Thanks, I've pushed this change to the cris tree.

/^JN - Jesper Nilsson
-- 
               Jesper Nilsson -- jesper.nilsson@axis.com

[PATCH 5/6] v4l: convert v4l2-dev to unlocked_ioctl

[Arnd Bergmann]

v4l2 implements two separate file operations for drivers that use locked
and unlocked ioctl callbacks. Since we want to remove the ioctl file operation
in favor of the unlocked variant, this separation no longer seems helpful.

There is another series from Frederic Weisbecker that takes care
of converting all v4l drivers to use the unlocked_ioctl variant, which
brings the BKL pushdown one level further.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Mauro Carvalho Chehab <mchehab@redhat.com>
Acked-by: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
---
 drivers/media/video/v4l2-dev.c |   52 +++++++++++----------------------------
 1 files changed, 15 insertions(+), 37 deletions(-)

diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 0ca7ec9..a869418 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -25,6 +25,7 @@
 #include <linux/init.h>
 #include <linux/kmod.h>
 #include <linux/slab.h>
+#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 #include <asm/system.h>
 
@@ -215,28 +216,24 @@ static unsigned int v4l2_poll(struct file *filp, struct poll_table_struct *poll)
 	return vdev->fops->poll(filp, poll);
 }
 
-static int v4l2_ioctl(struct inode *inode, struct file *filp,
-		unsigned int cmd, unsigned long arg)
+static long v4l2_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct video_device *vdev = video_devdata(filp);
+	int ret;
 
-	if (!vdev->fops->ioctl)
-		return -ENOTTY;
 	/* Allow ioctl to continue even if the device was unregistered.
 	   Things like dequeueing buffers might still be useful. */
-	return vdev->fops->ioctl(filp, cmd, arg);
-}
-
-static long v4l2_unlocked_ioctl(struct file *filp,
-		unsigned int cmd, unsigned long arg)
-{
-	struct video_device *vdev = video_devdata(filp);
+	if (vdev->fops->unlocked_ioctl) {
+		ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
+	} else if (vdev->fops->ioctl) {
+		/* TODO: convert all drivers to unlocked_ioctl */
+		lock_kernel();
+		ret = vdev->fops->ioctl(filp, cmd, arg);
+		unlock_kernel();
+	} else
+		ret = -ENOTTY;
 
-	if (!vdev->fops->unlocked_ioctl)
-		return -ENOTTY;
-	/* Allow ioctl to continue even if the device was unregistered.
-	   Things like dequeueing buffers might still be useful. */
-	return vdev->fops->unlocked_ioctl(filp, cmd, arg);
+	return ret;
 }
 
 #ifdef CONFIG_MMU
@@ -307,22 +304,6 @@ static int v4l2_release(struct inode *inode, struct file *filp)
 	return ret;
 }
 
-static const struct file_operations v4l2_unlocked_fops = {
-	.owner = THIS_MODULE,
-	.read = v4l2_read,
-	.write = v4l2_write,
-	.open = v4l2_open,
-	.get_unmapped_area = v4l2_get_unmapped_area,
-	.mmap = v4l2_mmap,
-	.unlocked_ioctl = v4l2_unlocked_ioctl,
-#ifdef CONFIG_COMPAT
-	.compat_ioctl = v4l2_compat_ioctl32,
-#endif
-	.release = v4l2_release,
-	.poll = v4l2_poll,
-	.llseek = no_llseek,
-};
-
 static const struct file_operations v4l2_fops = {
 	.owner = THIS_MODULE,
 	.read = v4l2_read,
@@ -330,7 +311,7 @@ static const struct file_operations v4l2_fops = {
 	.open = v4l2_open,
 	.get_unmapped_area = v4l2_get_unmapped_area,
 	.mmap = v4l2_mmap,
-	.ioctl = v4l2_ioctl,
+	.unlocked_ioctl = v4l2_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl = v4l2_compat_ioctl32,
 #endif
@@ -521,10 +502,7 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
 		ret = -ENOMEM;
 		goto cleanup;
 	}
-	if (vdev->fops->unlocked_ioctl)
-		vdev->cdev->ops = &v4l2_unlocked_fops;
-	else
-		vdev->cdev->ops = &v4l2_fops;
+	vdev->cdev->ops = &v4l2_fops;
 	vdev->cdev->owner = vdev->fops->owner;
 	ret = cdev_add(vdev->cdev, MKDEV(VIDEO_MAJOR, vdev->minor), 1);
 	if (ret < 0) {
-- 
1.7.1

Re: [PATCH 5/6] v4l: convert v4l2-dev to unlocked_ioctl

[Frederic Weisbecker]

On Sun, Jul 04, 2010 at 12:15:09AM +0200, Arnd Bergmann wrote:
> v4l2 implements two separate file operations for drivers that use locked
> and unlocked ioctl callbacks. Since we want to remove the ioctl file operation
> in favor of the unlocked variant, this separation no longer seems helpful.
> 
> There is another series from Frederic Weisbecker that takes care
> of converting all v4l drivers to use the unlocked_ioctl variant, which
> brings the BKL pushdown one level further.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Mauro Carvalho Chehab <mchehab@redhat.com>



Mauro, would you prefer to take this version or the other that does the complete
v4l pushdown?

I can rebase the other series to the latest v4l branch if needed.


Thanks.


> Acked-by: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>

[PATCH 6/6] bkl: remove locked .ioctl file operation

[Arnd Bergmann]

The last user is gone, so we can safely remove this

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 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 96d4293..34657e0 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 94677e7..ed7e5ef 100644
--- a/Documentation/filesystems/vfs.txt
+++ b/Documentation/filesystems/vfs.txt
@@ -727,7 +727,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 *);
@@ -768,10 +767,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 52e59bf..f024d8a 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)
 {
@@ -159,7 +153,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 618f381..0fffcdc 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1693,8 +1693,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 471e1ff..019a1b8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1479,8 +1479,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;
@@ -1491,7 +1491,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 *);
-- 
1.7.1