[patch 0/7] BKL the next lot

[patch 0/7] BKL the next lot

[Thomas Gleixner]

I started to go through Arnds list of ioctls.

Please review / comment.

Thanks,

	tglx

[patch 1/7] ia64: Remove the BKL from perfmon

[Thomas Gleixner]

[-- Attachment #1: ia64-remove-bkl-from-perfmon.patch --]
[-- Type: text/plain, Size: 2773 bytes --]

pfm_ioctl() is empty and can be safely converted to unlocked_ioctl. We
could also remove it, but that would change the return value.

sn_hwperf_ioctl() drops the BKL on entry and reacquires is on
exit. Convert to unlocked_ioctl.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
---
 arch/ia64/kernel/perfmon.c          |   21 ++++++++++-----------
 arch/ia64/sn/kernel/sn2/sn_hwperf.c |    8 ++------
 2 files changed, 12 insertions(+), 17 deletions(-)

Index: linux-2.6-tip/arch/ia64/kernel/perfmon.c
===================================================================
--- linux-2.6-tip.orig/arch/ia64/kernel/perfmon.c
+++ linux-2.6-tip/arch/ia64/kernel/perfmon.c
@@ -1701,8 +1701,7 @@ pfm_poll(struct file *filp, poll_table *
 	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;
@@ -2179,15 +2178,15 @@ pfm_no_open(struct inode *irrelevant, st
 
 
 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
Index: linux-2.6-tip/arch/ia64/sn/kernel/sn2/sn_hwperf.c
===================================================================
--- linux-2.6-tip.orig/arch/ia64/sn/kernel/sn2/sn_hwperf.c
+++ linux-2.6-tip/arch/ia64/sn/kernel/sn2/sn_hwperf.c
@@ -682,8 +682,7 @@ static int sn_hwperf_map_err(int hwperf_
 /*
  * ioctl for "sn_hwperf" misc device
  */
-static int
-sn_hwperf_ioctl(struct inode *in, struct file *fp, u32 op, unsigned long arg)
+static long sn_hwperf_ioctl(struct file *fp, u32 op, unsigned long arg)
 {
 	struct sn_hwperf_ioctl_args a;
 	struct cpuinfo_ia64 *cdata;
@@ -699,8 +698,6 @@ sn_hwperf_ioctl(struct inode *in, struct
 	int i;
 	int j;
 
-	unlock_kernel();
-
 	/* only user requests are allowed here */
 	if ((op & SN_HWPERF_OP_MASK) < 10) {
 		r = -EINVAL;
@@ -858,12 +855,11 @@ sn_hwperf_ioctl(struct inode *in, struct
 error:
 	vfree(p);
 
-	lock_kernel();
 	return r;
 }
 
 static const struct file_operations sn_hwperf_fops = {
-	.ioctl = sn_hwperf_ioctl,
+	.unlocked_ioctl = sn_hwperf_ioctl,
 };
 
 static struct miscdevice sn_hwperf_dev = {

[patch 2/7] m68k: Remove BKL from rtc implementations

[Thomas Gleixner]

[-- Attachment #1: m68k-remove-bkl-from-rtc.patch --]
[-- Type: text/plain, Size: 4146 bytes --]

m68k does not support SMP. The access to the rtc is already serialized
with local_irq_save/restore which is sufficient on UP.

The open() protection in arch/m68k/mvme16x/rtc.c is not pretty but
sufficient on UP and safe w/o the BKL.

open() in arch/m68k/bvme6000/rtc.c can do with the same atomic logic
as arch/m68k/mvme16x/rtc.c

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
---
 arch/m68k/bvme6000/rtc.c |   29 +++++++++--------------------
 arch/m68k/mvme16x/rtc.c  |   19 +++++--------------
 2 files changed, 14 insertions(+), 34 deletions(-)

Index: linux-2.6-tip/arch/m68k/bvme6000/rtc.c
===================================================================
--- linux-2.6-tip.orig/arch/m68k/bvme6000/rtc.c
+++ linux-2.6-tip/arch/m68k/bvme6000/rtc.c
@@ -10,7 +10,6 @@
 #include <linux/errno.h>
 #include <linux/miscdevice.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 #include <linux/ioport.h>
 #include <linux/capability.h>
 #include <linux/fcntl.h>
@@ -36,10 +35,9 @@
 static unsigned char days_in_mo[] =
 {0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
 
-static char rtc_status;
+static atomic_t rtc_status = ATOMIC_INIT(1);
 
-static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
-		     unsigned long arg)
+static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	volatile RtcPtr_t rtc = (RtcPtr_t)BVME_RTC_BASE;
 	unsigned char msr;
@@ -133,29 +131,20 @@ static int rtc_ioctl(struct inode *inode
 }
 
 /*
- *	We enforce only one user at a time here with the open/close.
- *	Also clear the previous interrupt data on an open, and clean
- *	up things on a close.
+ * We enforce only one user at a time here with the open/close.
  */
-
 static int rtc_open(struct inode *inode, struct file *file)
 {
-	lock_kernel();
-	if(rtc_status) {
-		unlock_kernel();
+	if (!atomic_dec_and_test(&rtc_status)) {
+		atomic_inc(&rtc_status);
 		return -EBUSY;
 	}
-
-	rtc_status = 1;
-	unlock_kernel();
 	return 0;
 }
 
 static int rtc_release(struct inode *inode, struct file *file)
 {
-	lock_kernel();
-	rtc_status = 0;
-	unlock_kernel();
+	atomic_inc(&rtc_status);
 	return 0;
 }
 
@@ -164,9 +153,9 @@ static int rtc_release(struct inode *ino
  */
 
 static const struct file_operations rtc_fops = {
-	.ioctl =	rtc_ioctl,
-	.open =		rtc_open,
-	.release =	rtc_release,
+	.unlocked_ioctl	= rtc_ioctl,
+	.open		= rtc_open,
+	.release	= rtc_release,
 };
 
 static struct miscdevice rtc_dev = {
Index: linux-2.6-tip/arch/m68k/mvme16x/rtc.c
===================================================================
--- linux-2.6-tip.orig/arch/m68k/mvme16x/rtc.c
+++ linux-2.6-tip/arch/m68k/mvme16x/rtc.c
@@ -10,7 +10,6 @@
 #include <linux/errno.h>
 #include <linux/miscdevice.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 #include <linux/ioport.h>
 #include <linux/capability.h>
 #include <linux/fcntl.h>
@@ -37,8 +36,7 @@ static const unsigned char days_in_mo[] 
 
 static atomic_t rtc_ready = ATOMIC_INIT(1);
 
-static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
-		     unsigned long arg)
+static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	volatile MK48T08ptr_t rtc = (MK48T08ptr_t)MVME_RTC_BASE;
 	unsigned long flags;
@@ -121,22 +119,15 @@ static int rtc_ioctl(struct inode *inode
 }
 
 /*
- *	We enforce only one user at a time here with the open/close.
- *	Also clear the previous interrupt data on an open, and clean
- *	up things on a close.
+ * We enforce only one user at a time here with the open/close.
  */
-
 static int rtc_open(struct inode *inode, struct file *file)
 {
-	lock_kernel();
 	if( !atomic_dec_and_test(&rtc_ready) )
 	{
 		atomic_inc( &rtc_ready );
-		unlock_kernel();
 		return -EBUSY;
 	}
-	unlock_kernel();
-
 	return 0;
 }
 
@@ -151,9 +142,9 @@ static int rtc_release(struct inode *ino
  */
 
 static const struct file_operations rtc_fops = {
-	.ioctl =	rtc_ioctl,
-	.open =		rtc_open,
-	.release =	rtc_release,
+	.unlocked_ioctl	= rtc_ioctl,
+	.open		= rtc_open,
+	.release	= rtc_release,
 };
 
 static struct miscdevice rtc_dev=

[patch 3/7] powerpc: Use unlocked ioctl in nvram_64

[Thomas Gleixner]

[-- Attachment #1: powerpc-use-unlocked-ioctl-in-nvram-64.patch --]
[-- Type: text/plain, Size: 1393 bytes --]

The ioctl is only used for powermac systems and reads a partition
number from an array which is initialized at boot time way before the
nvram code is initialized. So it's safe to switch to unlocked_ioctl.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@ozlabs.org
---
 arch/powerpc/kernel/nvram_64.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: linux-2.6-tip/arch/powerpc/kernel/nvram_64.c
===================================================================
--- linux-2.6-tip.orig/arch/powerpc/kernel/nvram_64.c
+++ linux-2.6-tip/arch/powerpc/kernel/nvram_64.c
@@ -139,8 +139,8 @@ out:
 
 }
 
-static int dev_nvram_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg)
+static long dev_nvram_ioctl(struct file *file, unsigned int cmd,
+			    unsigned long arg)
 {
 	switch(cmd) {
 #ifdef CONFIG_PPC_PMAC
@@ -169,11 +169,11 @@ static int dev_nvram_ioctl(struct inode 
 }
 
 const struct file_operations nvram_fops = {
-	.owner =	THIS_MODULE,
-	.llseek =	dev_nvram_llseek,
-	.read =		dev_nvram_read,
-	.write =	dev_nvram_write,
-	.ioctl =	dev_nvram_ioctl,
+	.owner		= THIS_MODULE,
+	.llseek		= dev_nvram_llseek,
+	.read		= dev_nvram_read,
+	.write		= dev_nvram_write,
+	.unlocked_ioctl	= dev_nvram_ioctl,
 };
 
 static struct miscdevice nvram_dev = {

[patch 4/7] sh: Remove BKL from landisk gio

[Thomas Gleixner]

[-- Attachment #1: sh-remove-bkl-from-landisk-gio.patch --]
[-- Type: text/plain, Size: 1866 bytes --]

The open function got the BKL via the big push down. Replace it by
preempt_enable/disable as this is sufficient for an UP machine.

The ioctl can be unlocked because there is no functionality which
requires serialization. The usage by multiple callers is broken with
and without the BKL due to the local static variable addr.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Paul Mundt <lethal@linux-sh.org>
---
 arch/sh/boards/mach-landisk/gio.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Index: linux-2.6-tip/arch/sh/boards/mach-landisk/gio.c
===================================================================
--- linux-2.6-tip.orig/arch/sh/boards/mach-landisk/gio.c
+++ linux-2.6-tip/arch/sh/boards/mach-landisk/gio.c
@@ -14,7 +14,6 @@
  */
 #include <linux/module.h>
 #include <linux/init.h>
-#include <linux/smp_lock.h>
 #include <linux/kdev_t.h>
 #include <linux/cdev.h>
 #include <linux/fs.h>
@@ -35,7 +34,7 @@ static int gio_open(struct inode *inode,
 	int minor;
 	int ret = -ENOENT;
 
-	lock_kernel();
+	preempt_disable();
 	minor = MINOR(inode->i_rdev);
 	if (minor < DEVCOUNT) {
 		if (openCnt > 0) {
@@ -45,7 +44,7 @@ static int gio_open(struct inode *inode,
 			ret = 0;
 		}
 	}
-	unlock_kernel();
+	preempt_enable();
 	return ret;
 }
 
@@ -60,8 +59,7 @@ static int gio_close(struct inode *inode
 	return 0;
 }
 
-static int gio_ioctl(struct inode *inode, struct file *filp,
-			     unsigned int cmd, unsigned long arg)
+static long gio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	unsigned int data;
 	static unsigned int addr = 0;
@@ -129,7 +127,7 @@ static const struct file_operations gio_
 	.owner = THIS_MODULE,
 	.open = gio_open,	/* open */
 	.release = gio_close,	/* release */
-	.ioctl = gio_ioctl,	/* ioctl */
+	.unlocked_ioctl = gio_ioctl,
 };
 
 static int __init gio_init(void)

[patch 5/7] um: Convert hostaudio to unlocked ioctl

[Thomas Gleixner]

[-- Attachment #1: um-convert-hostaudio-to-unlocked-ioctl.patch --]
[-- Type: text/plain, Size: 2086 bytes --]

The hostaudio ioctls can be converted to unlocked ioctls for the
following reasons:

  - get/put_user do not require the BKL
  - os_ioctl_generic() passes the ioctl to the host and does not
    need serialization.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Jeff Dike <jdike@addtoit.com>
---
 arch/um/drivers/hostaudio_kern.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux-2.6-tip/arch/um/drivers/hostaudio_kern.c
===================================================================
--- linux-2.6-tip.orig/arch/um/drivers/hostaudio_kern.c
+++ linux-2.6-tip/arch/um/drivers/hostaudio_kern.c
@@ -136,8 +136,8 @@ static unsigned int hostaudio_poll(struc
 	return mask;
 }
 
-static int hostaudio_ioctl(struct inode *inode, struct file *file,
-			   unsigned int cmd, unsigned long arg)
+static long hostaudio_ioctl(struct file *file, unsigned int cmd,
+			    unsigned long arg)
 {
 	struct hostaudio_state *state = file->private_data;
 	unsigned long data = 0;
@@ -223,8 +223,8 @@ static int hostaudio_release(struct inod
 
 /* /dev/mixer file operations */
 
-static int hostmixer_ioctl_mixdev(struct inode *inode, struct file *file,
-				  unsigned int cmd, unsigned long arg)
+static long hostmixer_ioctl_mixdev(struct file *file, unsigned int cmd,
+				   unsigned long arg)
 {
 	struct hostmixer_state *state = file->private_data;
 
@@ -289,7 +289,7 @@ static const struct file_operations host
 	.read           = hostaudio_read,
 	.write          = hostaudio_write,
 	.poll           = hostaudio_poll,
-	.ioctl          = hostaudio_ioctl,
+	.unlocked_ioctl = hostaudio_ioctl,
 	.mmap           = NULL,
 	.open           = hostaudio_open,
 	.release        = hostaudio_release,
@@ -298,7 +298,7 @@ static const struct file_operations host
 static const struct file_operations hostmixer_fops = {
 	.owner          = THIS_MODULE,
 	.llseek         = no_llseek,
-	.ioctl          = hostmixer_ioctl_mixdev,
+	.unlocked_ioctl = hostmixer_ioctl_mixdev,
 	.open           = hostmixer_open_mixdev,
 	.release        = hostmixer_release,
 };

[patch 6/7] um: Convert mmapper to unlocked_ioctl

[Thomas Gleixner]

[-- Attachment #1: um-convert-mmapper-to-unlocked-ioctl.patch --]
[-- Type: text/plain, Size: 1142 bytes --]

The ioctl is empty and needs no serialization. We might remove it
completely but that would change the return value from -ENOIOCTLCMD to
-ENOTTY.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Jeff Dike <jdike@addtoit.com>
---
 arch/um/drivers/mmapper_kern.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6-tip/arch/um/drivers/mmapper_kern.c
===================================================================
--- linux-2.6-tip.orig/arch/um/drivers/mmapper_kern.c
+++ linux-2.6-tip/arch/um/drivers/mmapper_kern.c
@@ -46,8 +46,8 @@ static ssize_t mmapper_write(struct file
 	return count;
 }
 
-static int mmapper_ioctl(struct inode *inode, struct file *file,
-			 unsigned int cmd, unsigned long arg)
+static long mmapper_ioctl(struct file *file, unsigned int cmd,
+			  unsigned long arg)
 {
 	return -ENOIOCTLCMD;
 }
@@ -91,7 +91,7 @@ static const struct file_operations mmap
 	.owner		= THIS_MODULE,
 	.read		= mmapper_read,
 	.write		= mmapper_write,
-	.ioctl		= mmapper_ioctl,
+	.unlocked_ioctl	= mmapper_ioctl,
 	.mmap		= mmapper_mmap,
 	.open		= mmapper_open,
 	.release	= mmapper_release,

[patch 7/7] um: Remove BKL from harddog

[Thomas Gleixner]

[-- Attachment #1: um-remove-bkl-from-harddog.patch --]
[-- Type: text/plain, Size: 1996 bytes --]

It's safe to remove the BKL from harddog because:

     - open() needs no serialization versus init
     - open() is already serialized with a local lock

     - ioctl() does not need BKL for put/copy_user
     - ioctl() does not need BKL for pin_watchdog as it
       simply writes to the fd which was created in open()

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Jeff Dike <jdike@addtoit.com>
---
 arch/um/drivers/harddog_kern.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Index: linux-2.6-tip/arch/um/drivers/harddog_kern.c
===================================================================
--- linux-2.6-tip.orig/arch/um/drivers/harddog_kern.c
+++ linux-2.6-tip/arch/um/drivers/harddog_kern.c
@@ -42,7 +42,6 @@
 #include <linux/miscdevice.h>
 #include <linux/watchdog.h>
 #include <linux/reboot.h>
-#include <linux/smp_lock.h>
 #include <linux/init.h>
 #include <linux/spinlock.h>
 #include <asm/uaccess.h>
@@ -66,7 +65,6 @@ static int harddog_open(struct inode *in
 	int err = -EBUSY;
 	char *sock = NULL;
 
-	lock_kernel();
 	spin_lock(&lock);
 	if(timer_alive)
 		goto err;
@@ -83,11 +81,9 @@ static int harddog_open(struct inode *in
 
 	timer_alive = 1;
 	spin_unlock(&lock);
-	unlock_kernel();
 	return nonseekable_open(inode, file);
 err:
 	spin_unlock(&lock);
-	unlock_kernel();
 	return err;
 }
 
@@ -124,8 +120,8 @@ static ssize_t harddog_write(struct file
 	return 0;
 }
 
-static int harddog_ioctl(struct inode *inode, struct file *file,
-			 unsigned int cmd, unsigned long arg)
+static long harddog_ioctl(struct file *file, unsigned int cmd,
+			  unsigned long arg)
 {
 	void __user *argp= (void __user *)arg;
 	static struct watchdog_info ident = {
@@ -151,7 +147,7 @@ static int harddog_ioctl(struct inode *i
 static const struct file_operations harddog_fops = {
 	.owner		= THIS_MODULE,
 	.write		= harddog_write,
-	.ioctl		= harddog_ioctl,
+	.unlocked_ioctl	= harddog_ioctl,
 	.open		= harddog_open,
 	.release	= harddog_release,
 };

Re: [patch 6/7] um: Convert mmapper to unlocked_ioctl

[Arnd Bergmann]

On Thursday 15 October 2009, Thomas Gleixner wrote:
> The ioctl is empty and needs no serialization. We might remove it
> completely but that would change the return value from -ENOIOCTLCMD to
> -ENOTTY.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jeff Dike <jdike@addtoit.com>

This one is tricky if you want to get it right according to the
book. ENOIOCTLCMD is never a valid return code for user space,
but sys_ioctl passes it down anyway.

However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
automatically gets turned into -EINVAL. It does this to allow
the same functions to be used for unlocked_ioctl and compat_ioctl.
In effect, this patch is functionally identical to removing the
ioctl function, which I think is what should be done here.

	Arnd <><

Re: [patch 6/7] um: Convert mmapper to unlocked_ioctl

[Thomas Gleixner]

On Thu, 15 Oct 2009, Arnd Bergmann wrote:

> On Thursday 15 October 2009, Thomas Gleixner wrote:
> > The ioctl is empty and needs no serialization. We might remove it
> > completely but that would change the return value from -ENOIOCTLCMD to
> > -ENOTTY.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Jeff Dike <jdike@addtoit.com>
> 
> This one is tricky if you want to get it right according to the
> book. ENOIOCTLCMD is never a valid return code for user space,
> but sys_ioctl passes it down anyway.
> 
> However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
> automatically gets turned into -EINVAL. It does this to allow
> the same functions to be used for unlocked_ioctl and compat_ioctl.
> In effect, this patch is functionally identical to removing the
> ioctl function, which I think is what should be done here.

Yeah, we get either -ENOTTY or -EINVAL which always changes the user
space return value. So yes, removing it at least saves some line of
code :)

Thanks,

	tglx

Re: [patch 6/7] um: Convert mmapper to unlocked_ioctl

[Alan Cox]

On Thu, 15 Oct 2009 15:00:34 +0200
Arnd Bergmann <arndbergmann@googlemail.com> wrote:

> On Thursday 15 October 2009, Thomas Gleixner wrote:
> > The ioctl is empty and needs no serialization. We might remove it
> > completely but that would change the return value from -ENOIOCTLCMD to
> > -ENOTTY.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Jeff Dike <jdike@addtoit.com>
> 
> This one is tricky if you want to get it right according to the
> book. ENOIOCTLCMD is never a valid return code for user space,
> but sys_ioctl passes it down anyway.
> 
> However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
> automatically gets turned into -EINVAL. It does this to allow
> the same functions to be used for unlocked_ioctl and compat_ioctl.
> In effect, this patch is functionally identical to removing the
> ioctl function, which I think is what should be done here.

That is wrong.

SuS requires an unknown ioctl code returns -ENOTTY. If the code is
currently remapping it to EINVAL then it wants fixing.

Re: [patch 4/7] sh: Remove BKL from landisk gio

[Paul Mundt]

On Thu, Oct 15, 2009 at 08:42:33AM -0000, Thomas Gleixner wrote:
> The open function got the BKL via the big push down. Replace it by
> preempt_enable/disable as this is sufficient for an UP machine.
> 
> The ioctl can be unlocked because there is no functionality which
> requires serialization. The usage by multiple callers is broken with
> and without the BKL due to the local static variable addr.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Paul Mundt <lethal@linux-sh.org>

Applied to my 2.6.32 queue, thanks Thomas.

Re: [patch 6/7] um: Convert mmapper to unlocked_ioctl

[Arnd Bergmann]

On Thursday 15 October 2009, Alan Cox wrote:
> On Thu, 15 Oct 2009 15:00:34 +0200
> Arnd Bergmann <arndbergmann@googlemail.com> wrote:
>
> > However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
> > automatically gets turned into -EINVAL. It does this to allow
> > the same functions to be used for unlocked_ioctl and compat_ioctl.
> > In effect, this patch is functionally identical to removing the
> > ioctl function, which I think is what should be done here.
> 
> That is wrong.
> 
> SuS requires an unknown ioctl code returns -ENOTTY. If the code is
> currently remapping it to EINVAL then it wants fixing.

Right, I forgot about the EINVAL/ENOTTY difference. The code currently
returns -ENOIOCTLCMD, which is worse. Thomas' patch makes it return
-EINVAL, which as you said is still wrong. Removing the ioctl function
will do the right thing and return -ENOTTY, so that should be done
here in um/mmapper, with an appropriate changelog.

For the common code in fs/ioctl.c, I think the current behaviour is
correct. It returns -EINVAL if the driver returns -ENOIOCTLCMD, iow
"the request [...] argument is not valid for this device", as specified
by http://www.opengroup.org/onlinepubs/009695399/functions/ioctl.html.

Drivers returning ENOIOCTLCMD for every request are broken and should
be changed to have no ioctl function.
	
	Arnd <><

Re: [patch 6/7] um: Convert mmapper to unlocked_ioctl

[Thomas Gleixner]

On Fri, 16 Oct 2009, Arnd Bergmann wrote:

> On Thursday 15 October 2009, Alan Cox wrote:
> > On Thu, 15 Oct 2009 15:00:34 +0200
> > Arnd Bergmann <arndbergmann@googlemail.com> wrote:
> >
> > > However, returning -ENOIOCTLCMD from an *unlocked_ioctl* function
> > > automatically gets turned into -EINVAL. It does this to allow
> > > the same functions to be used for unlocked_ioctl and compat_ioctl.
> > > In effect, this patch is functionally identical to removing the
> > > ioctl function, which I think is what should be done here.
> > 
> > That is wrong.
> > 
> > SuS requires an unknown ioctl code returns -ENOTTY. If the code is
> > currently remapping it to EINVAL then it wants fixing.
> 
> Right, I forgot about the EINVAL/ENOTTY difference. The code currently
> returns -ENOIOCTLCMD, which is worse. Thomas' patch makes it return
> -EINVAL, which as you said is still wrong. Removing the ioctl function
> will do the right thing and return -ENOTTY, so that should be done
> here in um/mmapper, with an appropriate changelog.
> 
> For the common code in fs/ioctl.c, I think the current behaviour is
> correct. It returns -EINVAL if the driver returns -ENOIOCTLCMD, iow

Only the unlocked_ioctl code path, the locked one returns whatever
crap comes from the ioctl implementation.

> "the request [...] argument is not valid for this device", as specified
> by http://www.opengroup.org/onlinepubs/009695399/functions/ioctl.html.
 
> Drivers returning ENOIOCTLCMD for every request are broken and should
> be changed to have no ioctl function.

Ack.

	tglx

Re: [patch 2/7] m68k: Remove BKL from rtc implementations

[Geert Uytterhoeven]

On Thu, Oct 15, 2009 at 09:42, Thomas Gleixner <tglx@linutronix.de> wrote:
> m68k does not support SMP. The access to the rtc is already serialized
> with local_irq_save/restore which is sufficient on UP.
>
> The open() protection in arch/m68k/mvme16x/rtc.c is not pretty but
> sufficient on UP and safe w/o the BKL.
>
> open() in arch/m68k/bvme6000/rtc.c can do with the same atomic logic
> as arch/m68k/mvme16x/rtc.c

Thx, applied.

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>  arch/m68k/bvme6000/rtc.c |   29 +++++++++--------------------
>  arch/m68k/mvme16x/rtc.c  |   19 +++++--------------
>  2 files changed, 14 insertions(+), 34 deletions(-)
>
> Index: linux-2.6-tip/arch/m68k/bvme6000/rtc.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/m68k/bvme6000/rtc.c
> +++ linux-2.6-tip/arch/m68k/bvme6000/rtc.c
> @@ -10,7 +10,6 @@
>  #include <linux/errno.h>
>  #include <linux/miscdevice.h>
>  #include <linux/slab.h>
> -#include <linux/smp_lock.h>
>  #include <linux/ioport.h>
>  #include <linux/capability.h>
>  #include <linux/fcntl.h>
> @@ -36,10 +35,9 @@
>  static unsigned char days_in_mo[] =
>  {0, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
>
> -static char rtc_status;
> +static atomic_t rtc_status = ATOMIC_INIT(1);
>
> -static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> -                    unsigned long arg)
> +static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>        volatile RtcPtr_t rtc = (RtcPtr_t)BVME_RTC_BASE;
>        unsigned char msr;
> @@ -133,29 +131,20 @@ static int rtc_ioctl(struct inode *inode
>  }
>
>  /*
> - *     We enforce only one user at a time here with the open/close.
> - *     Also clear the previous interrupt data on an open, and clean
> - *     up things on a close.
> + * We enforce only one user at a time here with the open/close.
>  */
> -
>  static int rtc_open(struct inode *inode, struct file *file)
>  {
> -       lock_kernel();
> -       if(rtc_status) {
> -               unlock_kernel();
> +       if (!atomic_dec_and_test(&rtc_status)) {
> +               atomic_inc(&rtc_status);
>                return -EBUSY;
>        }
> -
> -       rtc_status = 1;
> -       unlock_kernel();
>        return 0;
>  }
>
>  static int rtc_release(struct inode *inode, struct file *file)
>  {
> -       lock_kernel();
> -       rtc_status = 0;
> -       unlock_kernel();
> +       atomic_inc(&rtc_status);
>        return 0;
>  }
>
> @@ -164,9 +153,9 @@ static int rtc_release(struct inode *ino
>  */
>
>  static const struct file_operations rtc_fops = {
> -       .ioctl =        rtc_ioctl,
> -       .open =         rtc_open,
> -       .release =      rtc_release,
> +       .unlocked_ioctl = rtc_ioctl,
> +       .open           = rtc_open,
> +       .release        = rtc_release,
>  };
>
>  static struct miscdevice rtc_dev = {
> Index: linux-2.6-tip/arch/m68k/mvme16x/rtc.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/m68k/mvme16x/rtc.c
> +++ linux-2.6-tip/arch/m68k/mvme16x/rtc.c
> @@ -10,7 +10,6 @@
>  #include <linux/errno.h>
>  #include <linux/miscdevice.h>
>  #include <linux/slab.h>
> -#include <linux/smp_lock.h>
>  #include <linux/ioport.h>
>  #include <linux/capability.h>
>  #include <linux/fcntl.h>
> @@ -37,8 +36,7 @@ static const unsigned char days_in_mo[]
>
>  static atomic_t rtc_ready = ATOMIC_INIT(1);
>
> -static int rtc_ioctl(struct inode *inode, struct file *file, unsigned int cmd,
> -                    unsigned long arg)
> +static long rtc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>        volatile MK48T08ptr_t rtc = (MK48T08ptr_t)MVME_RTC_BASE;
>        unsigned long flags;
> @@ -121,22 +119,15 @@ static int rtc_ioctl(struct inode *inode
>  }
>
>  /*
> - *     We enforce only one user at a time here with the open/close.
> - *     Also clear the previous interrupt data on an open, and clean
> - *     up things on a close.
> + * We enforce only one user at a time here with the open/close.
>  */
> -
>  static int rtc_open(struct inode *inode, struct file *file)
>  {
> -       lock_kernel();
>        if( !atomic_dec_and_test(&rtc_ready) )
>        {
>                atomic_inc( &rtc_ready );
> -               unlock_kernel();
>                return -EBUSY;
>        }
> -       unlock_kernel();
> -
>        return 0;
>  }
>
> @@ -151,9 +142,9 @@ static int rtc_release(struct inode *ino
>  */
>
>  static const struct file_operations rtc_fops = {
> -       .ioctl =        rtc_ioctl,
> -       .open =         rtc_open,
> -       .release =      rtc_release,
> +       .unlocked_ioctl = rtc_ioctl,
> +       .open           = rtc_open,
> +       .release        = rtc_release,
>  };
>
>  static struct miscdevice rtc_dev=

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds