[patch 00/28] BKL removal queued patches

[patch 00/28] BKL removal queued patches

[Thomas Gleixner]

Hi all,

I'm sending out the pending BKL removal patches which I collected so
far to avoid duplicate work and to solicit review/comments.

If nobody objects I'd like to keep them in one git branch which is
going to be included into linux-next.

Thanks,

	tglx

[patch 01/28] pm_qos: remove BKL

[Thomas Gleixner]

[-- Attachment #1: pm-qos-remove-bkl.patch --]
[-- Type: text/plain, Size: 1440 bytes --]

pm_qos_power_open got its lock_kernel() calls from the open() pushdown.  A
look at the code shows that the only global resources accessed are
pm_qos_array and "name".  pm_qos_array doesn't change (things pointed to
therein do change, but they are atomics and/or are protected by
pm_qos_lock).  Accesses to "name" are totally unprotected with or without
the BKL; that will be fixed shortly.  The BKL is not helpful here; take it
out.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
Cc: Mark Gross <mgross@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index dfdec52..d96b83e 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -29,7 +29,6 @@
 
 #include <linux/pm_qos_params.h>
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 #include <linux/time.h>
@@ -352,20 +351,15 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
 	int ret;
 	long pm_qos_class;
 
-	lock_kernel();
 	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
 	if (pm_qos_class >= 0) {
 		filp->private_data = (void *)pm_qos_class;
 		sprintf(name, "process_%d", current->pid);
 		ret = pm_qos_add_requirement(pm_qos_class, name,
 					PM_QOS_DEFAULT_VALUE);
-		if (ret >= 0) {
-			unlock_kernel();
+		if (ret >= 0)
 			return 0;
-		}
 	}
-	unlock_kernel();
-
 	return -EPERM;
 }
 

Re: [patch 01/28] pm_qos: remove BKL

[Frederic Weisbecker]

On Sat, Oct 10, 2009 at 03:35:24PM -0000, Thomas Gleixner wrote:
> pm_qos_power_open got its lock_kernel() calls from the open() pushdown.  A
> look at the code shows that the only global resources accessed are
> pm_qos_array and "name".  pm_qos_array doesn't change (things pointed to
> therein do change, but they are atomics and/or are protected by
> pm_qos_lock).  Accesses to "name" are totally unprotected with or without
> the BKL; that will be fixed shortly.  The BKL is not helpful here; take it
> out.
> 
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> Cc: Mark Gross <mgross@linux.intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>


Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>


> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index dfdec52..d96b83e 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -29,7 +29,6 @@
>  
>  #include <linux/pm_qos_params.h>
>  #include <linux/sched.h>
> -#include <linux/smp_lock.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
>  #include <linux/time.h>
> @@ -352,20 +351,15 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
>  	int ret;
>  	long pm_qos_class;
>  
> -	lock_kernel();
>  	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
>  	if (pm_qos_class >= 0) {
>  		filp->private_data = (void *)pm_qos_class;
>  		sprintf(name, "process_%d", current->pid);
>  		ret = pm_qos_add_requirement(pm_qos_class, name,
>  					PM_QOS_DEFAULT_VALUE);
> -		if (ret >= 0) {
> -			unlock_kernel();
> +		if (ret >= 0)
>  			return 0;
> -		}
>  	}
> -	unlock_kernel();
> -
>  	return -EPERM;
>  }
>  
> 
> 

Re: [patch 01/28] pm_qos: remove BKL

[mgross]

I thought we made this change a month or 3 ago.

Signed-off-by: Mark Gross <mgross@linux.intel.com>

--mgross


On Sat, Oct 10, 2009 at 03:35:24PM -0000, Thomas Gleixner wrote:
> pm_qos_power_open got its lock_kernel() calls from the open() pushdown.  A
> look at the code shows that the only global resources accessed are
> pm_qos_array and "name".  pm_qos_array doesn't change (things pointed to
> therein do change, but they are atomics and/or are protected by
> pm_qos_lock).  Accesses to "name" are totally unprotected with or without
> the BKL; that will be fixed shortly.  The BKL is not helpful here; take it
> out.
> 
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> Cc: Mark Gross <mgross@linux.intel.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index dfdec52..d96b83e 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -29,7 +29,6 @@
>  
>  #include <linux/pm_qos_params.h>
>  #include <linux/sched.h>
> -#include <linux/smp_lock.h>
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
>  #include <linux/time.h>
> @@ -352,20 +351,15 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
>  	int ret;
>  	long pm_qos_class;
>  
> -	lock_kernel();
>  	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
>  	if (pm_qos_class >= 0) {
>  		filp->private_data = (void *)pm_qos_class;
>  		sprintf(name, "process_%d", current->pid);
>  		ret = pm_qos_add_requirement(pm_qos_class, name,
>  					PM_QOS_DEFAULT_VALUE);
> -		if (ret >= 0) {
> -			unlock_kernel();
> +		if (ret >= 0)
>  			return 0;
> -		}
>  	}
> -	unlock_kernel();
> -
>  	return -EPERM;
>  }
>  
> 

Re: [patch 01/28] pm_qos: remove BKL

[Jonathan Corbet]

On Tue, 13 Oct 2009 12:12:18 -0700
mgross <mgross@linux.intel.com> wrote:

> I thought we made this change a month or 3 ago.
> 
> Signed-off-by: Mark Gross <mgross@linux.intel.com>

Change went out but it was caught snoozing during the 2.6.32 merge
window.

I think you really want "Acked-by", no?

Thanks,

jon

Re: [patch 01/28] pm_qos: remove BKL

[mgross]

On Tue, Oct 13, 2009 at 01:21:08PM -0600, Jonathan Corbet wrote:
> On Tue, 13 Oct 2009 12:12:18 -0700
> mgross <mgross@linux.intel.com> wrote:
> 
> > I thought we made this change a month or 3 ago.
> > 
> > Signed-off-by: Mark Gross <mgross@linux.intel.com>
> 
> Change went out but it was caught snoozing during the 2.6.32 merge
> window.
> 
> I think you really want "Acked-by", no?

Yes.  Sorry.

--mgross

> 
> Thanks,
> 
> jon

[tip:bkl/core] pm_qos: remove BKL

[tip-bot for Jonathan Corbet]

Commit-ID:  e6fe07a014c7a3466dcd1a387a9ac04d84c2703c
Gitweb:     http://git.kernel.org/tip/e6fe07a014c7a3466dcd1a387a9ac04d84c2703c
Author:     Jonathan Corbet <corbet@lwn.net>
AuthorDate: Thu, 6 Aug 2009 13:22:40 -0600
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 15:31:10 +0200

pm_qos: remove BKL

pm_qos_power_open got its lock_kernel() calls from the open() pushdown.  A
look at the code shows that the only global resources accessed are
pm_qos_array and "name".  pm_qos_array doesn't change (things pointed to
therein do change, but they are atomics and/or are protected by
pm_qos_lock).  Accesses to "name" are totally unprotected with or without
the BKL; that will be fixed shortly.  The BKL is not helpful here; take it
out.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
LKML-Reference: <20091010153349.071381158@linutronix.de>
Acked-by: Mark Gross <mgross@linux.intel.com>
Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/pm_qos_params.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index dfdec52..d96b83e 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -29,7 +29,6 @@
 
 #include <linux/pm_qos_params.h>
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 #include <linux/time.h>
@@ -352,20 +351,15 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
 	int ret;
 	long pm_qos_class;
 
-	lock_kernel();
 	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
 	if (pm_qos_class >= 0) {
 		filp->private_data = (void *)pm_qos_class;
 		sprintf(name, "process_%d", current->pid);
 		ret = pm_qos_add_requirement(pm_qos_class, name,
 					PM_QOS_DEFAULT_VALUE);
-		if (ret >= 0) {
-			unlock_kernel();
+		if (ret >= 0)
 			return 0;
-		}
 	}
-	unlock_kernel();
-
 	return -EPERM;
 }
 

[patch 02/28] pm_qos: clean up racy global "name" variable

[Thomas Gleixner]

[-- Attachment #1: pm-qos-cleanup-racy-name-variable.patch --]
[-- Type: text/plain, Size: 2273 bytes --]

"name" is a poor name for a file-global variable.  It was used in three
different functions, with no mutual exclusion.  But it's just a tiny,
temporary string; let's just move it onto the stack in the functions that
need it.  Also use snprintf() just in case.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
Cc: Mark Gross <mgross@linux.intel.com>
Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index d96b83e..3db49b9 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -343,18 +343,18 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
 }
 EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
 
-#define PID_NAME_LEN sizeof("process_1234567890")
-static char name[PID_NAME_LEN];
+#define PID_NAME_LEN 32
 
 static int pm_qos_power_open(struct inode *inode, struct file *filp)
 {
 	int ret;
 	long pm_qos_class;
+	char name[PID_NAME_LEN];
 
 	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
 	if (pm_qos_class >= 0) {
 		filp->private_data = (void *)pm_qos_class;
-		sprintf(name, "process_%d", current->pid);
+		snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
 		ret = pm_qos_add_requirement(pm_qos_class, name,
 					PM_QOS_DEFAULT_VALUE);
 		if (ret >= 0)
@@ -366,9 +366,10 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
 static int pm_qos_power_release(struct inode *inode, struct file *filp)
 {
 	int pm_qos_class;
+	char name[PID_NAME_LEN];
 
 	pm_qos_class = (long)filp->private_data;
-	sprintf(name, "process_%d", current->pid);
+	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
 	pm_qos_remove_requirement(pm_qos_class, name);
 
 	return 0;
@@ -379,13 +380,14 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 {
 	s32 value;
 	int pm_qos_class;
+	char name[PID_NAME_LEN];
 
 	pm_qos_class = (long)filp->private_data;
 	if (count != sizeof(s32))
 		return -EINVAL;
 	if (copy_from_user(&value, buf, sizeof(s32)))
 		return -EFAULT;
-	sprintf(name, "process_%d", current->pid);
+	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
 	pm_qos_update_requirement(pm_qos_class, name, value);
 
 	return  sizeof(s32);

Re: [patch 02/28] pm_qos: clean up racy global "name" variable

[John Kacur]

On Sat, 10 Oct 2009, Thomas Gleixner wrote:

> "name" is a poor name for a file-global variable.  It was used in three
> different functions, with no mutual exclusion.  But it's just a tiny,
> temporary string; let's just move it onto the stack in the functions that
> need it.  Also use snprintf() just in case.
> 
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> Cc: Mark Gross <mgross@linux.intel.com>
> Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index d96b83e..3db49b9 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -343,18 +343,18 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
>  
> -#define PID_NAME_LEN sizeof("process_1234567890")
> -static char name[PID_NAME_LEN];
> +#define PID_NAME_LEN 32

Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890" 
which is 19, an attempt to show what the maximum string size would be. If 
a system were configured to enlarge the maximum PID from 32767 to 4194303 
that would still only be 7 digits, so "process_1234567" - which is 16 
digits with the newline would enough.

So, I suggest you change that to
#define PID_NAME_LEN sizeof("process_1234567")

Other than that, Reviewed-by: John Kacur <jkacur@redhat.com>

>  
>  static int pm_qos_power_open(struct inode *inode, struct file *filp)
>  {
>  	int ret;
>  	long pm_qos_class;
> +	char name[PID_NAME_LEN];
>  
>  	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
>  	if (pm_qos_class >= 0) {
>  		filp->private_data = (void *)pm_qos_class;
> -		sprintf(name, "process_%d", current->pid);
> +		snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
>  		ret = pm_qos_add_requirement(pm_qos_class, name,
>  					PM_QOS_DEFAULT_VALUE);
>  		if (ret >= 0)
> @@ -366,9 +366,10 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
>  static int pm_qos_power_release(struct inode *inode, struct file *filp)
>  {
>  	int pm_qos_class;
> +	char name[PID_NAME_LEN];
>  
>  	pm_qos_class = (long)filp->private_data;
> -	sprintf(name, "process_%d", current->pid);
> +	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
>  	pm_qos_remove_requirement(pm_qos_class, name);
>  
>  	return 0;
> @@ -379,13 +380,14 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
>  {
>  	s32 value;
>  	int pm_qos_class;
> +	char name[PID_NAME_LEN];
>  
>  	pm_qos_class = (long)filp->private_data;
>  	if (count != sizeof(s32))
>  		return -EINVAL;
>  	if (copy_from_user(&value, buf, sizeof(s32)))
>  		return -EFAULT;
> -	sprintf(name, "process_%d", current->pid);
> +	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
>  	pm_qos_update_requirement(pm_qos_class, name, value);
>  
>  	return  sizeof(s32);
> 
> 
> 

Re: [patch 02/28] pm_qos: clean up racy global "name" variable

[Jonathan Corbet]

On Sat, 10 Oct 2009 21:54:22 +0200 (CEST)
John Kacur <jkacur@redhat.com> wrote:

> Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890" 
> which is 19, an attempt to show what the maximum string size would be. If 
> a system were configured to enlarge the maximum PID from 32767 to 4194303 
> that would still only be 7 digits, so "process_1234567" - which is 16 
> digits with the newline would enough.
> 
> So, I suggest you change that to
> #define PID_NAME_LEN sizeof("process_1234567")

...which works great until somebody enables 64-bit process IDs...:)

We're talking about 20 bytes of stack space in an almost-never-called
function. I honestly don't think it's worth worrying about, but if
somebody wants to tweak it, I'll not complain.

(Thanks for looking at the patch).

jon

Re: [patch 02/28] pm_qos: clean up racy global "name" variable

[Peter Zijlstra]

On Sat, 2009-10-10 at 14:03 -0600, Jonathan Corbet wrote:
> On Sat, 10 Oct 2009 21:54:22 +0200 (CEST)
> John Kacur <jkacur@redhat.com> wrote:
> 
> > Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890" 
> > which is 19, an attempt to show what the maximum string size would be. If 
> > a system were configured to enlarge the maximum PID from 32767 to 4194303 
> > that would still only be 7 digits, so "process_1234567" - which is 16 
> > digits with the newline would enough.
> > 
> > So, I suggest you change that to
> > #define PID_NAME_LEN sizeof("process_1234567")
> 
> ....which works great until somebody enables 64-bit process IDs...:)

PID/TIDs are limited to 2^29, raising it above that will break things
like futexes. Raising it above 2^32 will break heaps of userspace.

That said, 512M tasks still seems like a lot, but if history is
something to go by we'll eventually run out...

Re: [patch 02/28] pm_qos: clean up racy global "name" variable

[John Kacur]

On Sat, 10 Oct 2009, Jonathan Corbet wrote:

> On Sat, 10 Oct 2009 21:54:22 +0200 (CEST)
> John Kacur <jkacur@redhat.com> wrote:
> 
> > Hmnn, why 32? Seems arbitrary. At least you see with "process_1234567890" 
> > which is 19, an attempt to show what the maximum string size would be. If 
> > a system were configured to enlarge the maximum PID from 32767 to 4194303 
> > that would still only be 7 digits, so "process_1234567" - which is 16 
> > digits with the newline would enough.
> > 
> > So, I suggest you change that to
> > #define PID_NAME_LEN sizeof("process_1234567")
> 
> ...which works great until somebody enables 64-bit process IDs...:)
> 
> We're talking about 20 bytes of stack space in an almost-never-called
> function. I honestly don't think it's worth worrying about, but if
> somebody wants to tweak it, I'll not complain.
> 
> (Thanks for looking at the patch).
> 

It was a minor nit at best! My point was less about the stack space than 
the readability - which you could argue is a personal style choice here. 
There is nothing else to criticize in these patches. :)

Thomas, I reviewed all 28 patches and applied them.

Thanks

[tip:bkl/core] pm_qos: clean up racy global "name" variable

[tip-bot for Jonathan Corbet]

Commit-ID:  1a6deaea3584fd7af1cad492b1fe0867060b45db
Gitweb:     http://git.kernel.org/tip/1a6deaea3584fd7af1cad492b1fe0867060b45db
Author:     Jonathan Corbet <corbet@lwn.net>
AuthorDate: Thu, 6 Aug 2009 13:35:44 -0600
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 15:31:10 +0200

pm_qos: clean up racy global "name" variable

"name" is a poor name for a file-global variable.  It was used in three
different functions, with no mutual exclusion.  But it's just a tiny,
temporary string; let's just move it onto the stack in the functions that
need it.  Also use snprintf() just in case.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
LKML-Reference: <20091010153349.113570550@linutronix.de>
Acked-by: Mark Gross <mgross@linux.intel.com>
Reviewed-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 kernel/pm_qos_params.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index d96b83e..3db49b9 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -343,18 +343,18 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
 }
 EXPORT_SYMBOL_GPL(pm_qos_remove_notifier);
 
-#define PID_NAME_LEN sizeof("process_1234567890")
-static char name[PID_NAME_LEN];
+#define PID_NAME_LEN 32
 
 static int pm_qos_power_open(struct inode *inode, struct file *filp)
 {
 	int ret;
 	long pm_qos_class;
+	char name[PID_NAME_LEN];
 
 	pm_qos_class = find_pm_qos_object_by_minor(iminor(inode));
 	if (pm_qos_class >= 0) {
 		filp->private_data = (void *)pm_qos_class;
-		sprintf(name, "process_%d", current->pid);
+		snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
 		ret = pm_qos_add_requirement(pm_qos_class, name,
 					PM_QOS_DEFAULT_VALUE);
 		if (ret >= 0)
@@ -366,9 +366,10 @@ static int pm_qos_power_open(struct inode *inode, struct file *filp)
 static int pm_qos_power_release(struct inode *inode, struct file *filp)
 {
 	int pm_qos_class;
+	char name[PID_NAME_LEN];
 
 	pm_qos_class = (long)filp->private_data;
-	sprintf(name, "process_%d", current->pid);
+	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
 	pm_qos_remove_requirement(pm_qos_class, name);
 
 	return 0;
@@ -379,13 +380,14 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 {
 	s32 value;
 	int pm_qos_class;
+	char name[PID_NAME_LEN];
 
 	pm_qos_class = (long)filp->private_data;
 	if (count != sizeof(s32))
 		return -EINVAL;
 	if (copy_from_user(&value, buf, sizeof(s32)))
 		return -EFAULT;
-	sprintf(name, "process_%d", current->pid);
+	snprintf(name, PID_NAME_LEN, "process_%d", current->pid);
 	pm_qos_update_requirement(pm_qos_class, name, value);
 
 	return  sizeof(s32);

[patch 03/28] net: Remove BKL from tun

[Thomas Gleixner]

[-- Attachment #1: drivers-net-tun-remove-bkl.patch --]
[-- Type: text/plain, Size: 1580 bytes --]

The lock_kernel/unlock_kernel() in cycle_kernel_lock() which is called
in tun_chr_open() is not serializing against anything and safe to
remove.

tun_chr_fasync() is serialized by get/put_tun() and fasync_helper()
has no dependency on BKL. The modification of tun->flags is racy with
and without the BKL so removing it does not make it worse.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>

---
 drivers/net/tun.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Index: linux-2.6-tip/drivers/net/tun.c
===================================================================
--- linux-2.6-tip.orig/drivers/net/tun.c
+++ linux-2.6-tip/drivers/net/tun.c
@@ -44,7 +44,6 @@
 #include <linux/kernel.h>
 #include <linux/major.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 #include <linux/poll.h>
 #include <linux/fcntl.h>
 #include <linux/init.h>
@@ -1285,7 +1284,6 @@ static int tun_chr_fasync(int fd, struct
 
 	DBG(KERN_INFO "%s: tun_chr_fasync %d\n", tun->dev->name, on);
 
-	lock_kernel();
 	if ((ret = fasync_helper(fd, file, on, &tun->fasync)) < 0)
 		goto out;
 
@@ -1298,7 +1296,6 @@ static int tun_chr_fasync(int fd, struct
 		tun->flags &= ~TUN_FASYNC;
 	ret = 0;
 out:
-	unlock_kernel();
 	tun_put(tun);
 	return ret;
 }
@@ -1306,7 +1303,7 @@ out:
 static int tun_chr_open(struct inode *inode, struct file * file)
 {
 	struct tun_file *tfile;
-	cycle_kernel_lock();
+
 	DBG1(KERN_INFO "tunX: tun_chr_open\n");
 
 	tfile = kmalloc(sizeof(*tfile), GFP_KERNEL);

Re: [patch 03/28] net: Remove BKL from tun

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Sat, 10 Oct 2009 15:35:33 -0000

> The lock_kernel/unlock_kernel() in cycle_kernel_lock() which is called
> in tun_chr_open() is not serializing against anything and safe to
> remove.
> 
> tun_chr_fasync() is serialized by get/put_tun() and fasync_helper()
> has no dependency on BKL. The modification of tun->flags is racy with
> and without the BKL so removing it does not make it worse.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Applied to net-next-2.6, thanks!

[patch 04/28] x86: Remove BKL from microcode

[Thomas Gleixner]

[-- Attachment #1: x86-remove-bkl-from-micro-code.patch --]
[-- Type: text/plain, Size: 910 bytes --]

cycle_lock_kernel() in microcode_open() is a worthless exercise as
there is nothing to wait for. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/microcode_core.c |    2 --
 1 file changed, 2 deletions(-)

Index: linux-2.6-tip/arch/x86/kernel/microcode_core.c
===================================================================
--- linux-2.6-tip.orig/arch/x86/kernel/microcode_core.c
+++ linux-2.6-tip/arch/x86/kernel/microcode_core.c
@@ -73,7 +73,6 @@
 #include <linux/platform_device.h>
 #include <linux/miscdevice.h>
 #include <linux/capability.h>
-#include <linux/smp_lock.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -201,7 +200,6 @@ static int do_microcode_update(const voi
 
 static int microcode_open(struct inode *unused1, struct file *unused2)
 {
-	cycle_kernel_lock();
 	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
 }
 

[tip:x86/cleanups] x86: Remove BKL from microcode

[tip-bot for Thomas Gleixner]

Commit-ID:  ac06ea2cd06291e63951b51dd7c9a23e6a1f2683
Gitweb:     http://git.kernel.org/tip/ac06ea2cd06291e63951b51dd7c9a23e6a1f2683
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 09:35:48 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:04:48 +0200

x86: Remove BKL from microcode

cycle_lock_kernel() in microcode_open() is a worthless exercise as
there is nothing to wait for. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153349.196074920@linutronix.de>

---
 arch/x86/kernel/microcode_core.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 378e9a8..2bcad39 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -73,7 +73,6 @@
 #include <linux/platform_device.h>
 #include <linux/miscdevice.h>
 #include <linux/capability.h>
-#include <linux/smp_lock.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -201,7 +200,6 @@ static int do_microcode_update(const void __user *buf, size_t size)
 
 static int microcode_open(struct inode *unused1, struct file *unused2)
 {
-	cycle_kernel_lock();
 	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
 }
 

[patch 05/28] drivers: Remove BKL from drivers/char/misc.c

[Thomas Gleixner]

[-- Attachment #1: driver-char-misc-remove-bkl.patch --]
[-- Type: text/plain, Size: 1080 bytes --]

misc_open() is already serialized with misc_mtx. Remove the BKL
locking which got there via the BKL pushdown.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/char/misc.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Index: linux-2.6-tip/drivers/char/misc.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/misc.c
+++ linux-2.6-tip/drivers/char/misc.c
@@ -49,7 +49,6 @@
 #include <linux/device.h>
 #include <linux/tty.h>
 #include <linux/kmod.h>
-#include <linux/smp_lock.h>
 
 /*
  * Head entry for the doubly linked miscdevice list
@@ -118,8 +117,7 @@ static int misc_open(struct inode * inod
 	struct miscdevice *c;
 	int err = -ENODEV;
 	const struct file_operations *old_fops, *new_fops = NULL;
-	
-	lock_kernel();
+
 	mutex_lock(&misc_mtx);
 	
 	list_for_each_entry(c, &misc_list, list) {
@@ -157,7 +155,6 @@ static int misc_open(struct inode * inod
 	fops_put(old_fops);
 fail:
 	mutex_unlock(&misc_mtx);
-	unlock_kernel();
 	return err;
 }
 

Re: [patch 05/28] drivers: Remove BKL from drivers/char/misc.c

[Arnd Bergmann]

On Saturday 10 October 2009, Thomas Gleixner wrote:
> misc_open() is already serialized with misc_mtx. Remove the BKL
> locking which got there via the BKL pushdown.

This only works as long as all misc drivers also don't need the BKL
in /their/ open methods. I believe that we did a pushdown into them
last year, but new ones may have crept up.

I guess we really should have kill this one off back then, but somehow
it was forgotten about.

	Arnd <><

[tip:bkl/drivers] drivers: Remove BKL from misc_open

[tip-bot for Thomas Gleixner]

Commit-ID:  40b798efe3460797a4ac928ee2e038774e2758eb
Gitweb:     http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:33:32 +0200

drivers: Remove BKL from misc_open

misc_open() is already serialized with misc_mtx. Remove the BKL
locking which got there via the BKL pushdown.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
LKML-Reference: <20091010153349.237173041@linutronix.de>
---
 drivers/char/misc.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/char/misc.c b/drivers/char/misc.c
index 07fa612..96f1cd0 100644
--- a/drivers/char/misc.c
+++ b/drivers/char/misc.c
@@ -49,7 +49,6 @@
 #include <linux/device.h>
 #include <linux/tty.h>
 #include <linux/kmod.h>
-#include <linux/smp_lock.h>
 
 /*
  * Head entry for the doubly linked miscdevice list
@@ -118,8 +117,7 @@ static int misc_open(struct inode * inode, struct file * file)
 	struct miscdevice *c;
 	int err = -ENODEV;
 	const struct file_operations *old_fops, *new_fops = NULL;
-	
-	lock_kernel();
+
 	mutex_lock(&misc_mtx);
 	
 	list_for_each_entry(c, &misc_list, list) {
@@ -157,7 +155,6 @@ static int misc_open(struct inode * inode, struct file * file)
 	fops_put(old_fops);
 fail:
 	mutex_unlock(&misc_mtx);
-	unlock_kernel();
 	return err;
 }
 

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Arnd Bergmann]

On Wednesday 14 October 2009, tip-bot for Thomas Gleixner wrote:
> Commit-ID:  40b798efe3460797a4ac928ee2e038774e2758eb
> Gitweb:     http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> Author:     Thomas Gleixner <tglx@linutronix.de>
> AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> 
> drivers: Remove BKL from misc_open
> 
> misc_open() is already serialized with misc_mtx. Remove the BKL
> locking which got there via the BKL pushdown.

Did you check the misc drivers to make sure this is not silently
removing the BKL from drivers that need it?

	Arnd <><

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Thomas Gleixner]

On Wed, 14 Oct 2009, Arnd Bergmann wrote:
> On Wednesday 14 October 2009, tip-bot for Thomas Gleixner wrote:
> > Commit-ID:  40b798efe3460797a4ac928ee2e038774e2758eb
> > Gitweb:     http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > Author:     Thomas Gleixner <tglx@linutronix.de>
> > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > Committer:  Thomas Gleixner <tglx@linutronix.de>
> > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> > 
> > drivers: Remove BKL from misc_open
> > 
> > misc_open() is already serialized with misc_mtx. Remove the BKL
> > locking which got there via the BKL pushdown.
> 
> Did you check the misc drivers to make sure this is not silently
> removing the BKL from drivers that need it?

Yep, they are plastered with cycle_kernel_lock and lock/unlock_kernel
already. I might have missed some, but ...

Thanks,

	tglx

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Alan Cox]

On Wed, 14 Oct 2009 15:47:39 GMT
tip-bot for Thomas Gleixner <tglx@linutronix.de> wrote:

> Commit-ID:  40b798efe3460797a4ac928ee2e038774e2758eb
> Gitweb:     http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> Author:     Thomas Gleixner <tglx@linutronix.de>
> AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> Committer:  Thomas Gleixner <tglx@linutronix.de>
> CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> 
> drivers: Remove BKL from misc_open
> 
> misc_open() is already serialized with misc_mtx. Remove the BKL
> locking which got there via the BKL pushdown.

NAK.

You can't simply assume the mutex is enough - you have to either push it
down or review *every* possible called point below the lock_kernel take.

In this case the open method of the misc devices below sometimes uses the
BKL.

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Thomas Gleixner]

on Wed, 14 Oct 2009, Alan Cox wrote:
> On Wed, 14 Oct 2009 15:47:39 GMT
> tip-bot for Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Commit-ID:  40b798efe3460797a4ac928ee2e038774e2758eb
> > Gitweb:     http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > Author:     Thomas Gleixner <tglx@linutronix.de>
> > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > Committer:  Thomas Gleixner <tglx@linutronix.de>
> > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> > 
> > drivers: Remove BKL from misc_open
> > 
> > misc_open() is already serialized with misc_mtx. Remove the BKL
> > locking which got there via the BKL pushdown.
> 
> NAK.
> 
> You can't simply assume the mutex is enough - you have to either push it
> down or review *every* possible called point below the lock_kernel take.
> 
> In this case the open method of the misc devices below sometimes uses the
> BKL.

The BKL got pushed down into the open methods of misc dev users and we
do not need to take it twice in a row, right ?

Thanks,

	tglx

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Arnd Bergmann]

On Wednesday 14 October 2009, Thomas Gleixner wrote:
> on Wed, 14 Oct 2009, Alan Cox wrote:
> > You can't simply assume the mutex is enough - you have to either push it
> > down or review *every* possible called point below the lock_kernel take.
> > 
> > In this case the open method of the misc devices below sometimes uses the
> > BKL.
> 
> The BKL got pushed down into the open methods of misc dev users and we
> do not need to take it twice in a row, right ?
> 

I worked on the original pushdown back then and believed it to be
complete. New drivers have come in and some obviously unneeded
instances got cleared of the BKL usage. The ones with an open method
that does not take the BKL are:

arch/mips/basler/excite/excite_iodev.c
arch/x86/kernel/apm_32.c
drivers/buetooth/hci_vhci.c
drivers/char/nvram.c
drivers/char/rtc.c
drivers/gpu/vga/vgaarb.c
drivers/hwmon/fschmd.c
drivers/hwmon/lis3lv02d.c
drivers/infiniband/core/ucma.c
drivers/isdn/mISDN/timerdev.c
drivers/s390/char/vmcp.c
fs/cachefiles/daemon.c
fs/dlm/user.c
fs/fuse/cuse.c
kernel/power/user.c
net/rfkill/core.c

As well as some drivers from staging and watchdog. I believe the
BKL usage in watchdog drivers was removed shortly after the pushdown.

	Arnd <><

drivers/staging/android/binder.c
drivers/staging/dream/qdsp5/audio_aac.c
drivers/staging/dream/qdsp5/audio_evrc.c
drivers/staging/dream/qdsp5/audio_in.c
drivers/staging/dream/qdsp5/audio_out.c
drivers/staging/dream/qdsp5/snd.c
drivers/staging/dream/smd/smd_qmi.c
drivers/staging/panel/panel.c
drivers/watchdog/acquirewdt.c
drivers/watchdog/adx_wdt.c
drivers/watchdog/alim7101_wdt.c
drivers/watchdog/at32ap700x_wdt.c
drivers/watchdog/at91sam9_wdt.c
drivers/watchdog/fin_wdt.c
drivers/watchdog/coh901327_wdt.c
drivers/watchdog/ep93xx_wdt.c
drivers/watchdog/gef_wdt.c
drivers/watchdog/geodewdt.c
drivers/watchdog/i6300esb.c
drivers/watchdog/b700wwdt.c
drivers/watchdog/ibmasr.c
drivers/watchdog/iop_wdt.c
drivers/watchdog/it8_wdt.c
drivers/watchdog/ixp4xx_wdt.c
drivers/watchdog/machzwd.c
drivers/watchdog/mpc5200_wdt.c
drivers/watchdog/mpcore_wdt.c
drivers/watchdog/mv64x60_wdt.c
drivers/watchdog/omap_wdt.c
drivers/watchdog/pc87413_wdt.c
drivers/watchdog/pcwd.c
drivers/watchdog/pcwd_pci.c
drivers/watchdog/pcwd_usb.c
drivers/watchdog/pnx4008_wdt.c
drivers/watchdog/rc32434_wdt.c
drivers/watchdog/s3c2410_wdt.c
drivers/watchdog/sb_wdog.c
drivers/watchdog/sbc7240_wdt.c
drivers/watchdog/sbc_epx_c3.c
drivers/watchdog/c1200wdt.c
drivers/watchdog/sch311x_wdt.c
drivers/watchdog/shwdt.c
drivers/watchdog/smsc37b787_wdt.c
drivers/watchdog/stmp3xxx_wdt.c
drivers/watchdog/txx9wdt.c
drivers/watchdog/w83697hf_wdt.c
drivers/watchdog/w83877f_wdt.c
drivers/watchdog/wafer5823wdt.c
drivers/watchdog/wdrtas.c
drivers/watchdog/wdt.c
drivers/watchdog/wdt977.c
drivers/watchdog/wdt_pci.c
drivers/watchdog/wm8350_wdt.c

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Arnd Bergmann]

On Wednesday 14 October 2009, Arnd Bergmann wrote:

> arch/mips/basler/excite/excite_iodev.c
> drivers/char/rtc.c
> drivers/infiniband/core/ucma.c
> drivers/s390/char/vmcp.c
> fs/dlm/user.c
> kernel/power/user.c
> arch/x86/kernel/apm_32.c
> drivers/char/nvram.c
> drivers/hwmon/fschmd.c

It turns out that these are all ok, the BKL was removed after
the pushdown.

> drivers/gpu/vga/vgaarb.c
> drivers/hwmon/lis3lv02d.c
> fs/cachefiles/daemon.c
> fs/fuse/cuse.c
> net/rfkill/core.c
> drivers/staging/android/binder.c
> drivers/staging/dream/qdsp5/audio_aac.c
> drivers/staging/dream/qdsp5/audio_evrc.c
> drivers/staging/dream/qdsp5/audio_in.c
> drivers/staging/dream/qdsp5/audio_out.c
> drivers/staging/dream/qdsp5/snd.c
> drivers/staging/dream/smd/smd_qmi.c
> drivers/staging/panel/panel.c

These are new drivers that were merged after the pushdown
and should be looked at, though I don't expect that any of
them to need it because none of them use the locked ioctl
method or any other form of BKL.

> drivers/isdn/mISDN/timerdev.c

This one implicitly holds the BKL in both open and ioctl,
it's probably not required but I didn't look closely.

	Arnd <><

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Thomas Gleixner]

On Wed, 14 Oct 2009, Arnd Bergmann wrote:
> > drivers/gpu/vga/vgaarb.c
> > drivers/hwmon/lis3lv02d.c
> > fs/cachefiles/daemon.c
> > fs/fuse/cuse.c
> > net/rfkill/core.c

Sigh, will have a look. 

> > drivers/staging/android/binder.c
> > drivers/staging/dream/qdsp5/audio_aac.c
> > drivers/staging/dream/qdsp5/audio_evrc.c
> > drivers/staging/dream/qdsp5/audio_in.c
> > drivers/staging/dream/qdsp5/audio_out.c
> > drivers/staging/dream/qdsp5/snd.c
> > drivers/staging/dream/smd/smd_qmi.c
> > drivers/staging/panel/panel.c

More sigh.
 
> These are new drivers that were merged after the pushdown
> and should be looked at, though I don't expect that any of
> them to need it because none of them use the locked ioctl
> method or any other form of BKL.

Right I looked at quite a bunch of them.
 
Thanks,

	tglx

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Pavel Machek]

Hi!!

> > > drivers/staging/android/binder.c
> > > drivers/staging/dream/qdsp5/audio_aac.c
> > > drivers/staging/dream/qdsp5/audio_evrc.c
> > > drivers/staging/dream/qdsp5/audio_in.c
> > > drivers/staging/dream/qdsp5/audio_out.c
> > > drivers/staging/dream/qdsp5/snd.c
> > > drivers/staging/dream/smd/smd_qmi.c
> > > drivers/staging/panel/panel.c
> 
> More sigh.

I guess you can just pretend these do not exist.
							Pavel 
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Alan Cox]

On Wed, 14 Oct 2009 18:16:58 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> on Wed, 14 Oct 2009, Alan Cox wrote:
> > On Wed, 14 Oct 2009 15:47:39 GMT
> > tip-bot for Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > Commit-ID:  40b798efe3460797a4ac928ee2e038774e2758eb
> > > Gitweb:     http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > > Author:     Thomas Gleixner <tglx@linutronix.de>
> > > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > > Committer:  Thomas Gleixner <tglx@linutronix.de>
> > > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> > > 
> > > drivers: Remove BKL from misc_open
> > > 
> > > misc_open() is already serialized with misc_mtx. Remove the BKL
> > > locking which got there via the BKL pushdown.
> > 
> > NAK.
> > 
> > You can't simply assume the mutex is enough - you have to either push it
> > down or review *every* possible called point below the lock_kernel take.
> > 
> > In this case the open method of the misc devices below sometimes uses the
> > BKL.
> 
> The BKL got pushed down into the open methods of misc dev users and we
> do not need to take it twice in a row, right ?

Then the comment is misleading and you need to document that you've
already pushed the BKL down into each user and checked them.

Alan

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Thomas Gleixner]

On Wed, 14 Oct 2009, Alan Cox wrote:
> On Wed, 14 Oct 2009 18:16:58 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > on Wed, 14 Oct 2009, Alan Cox wrote:
> > > On Wed, 14 Oct 2009 15:47:39 GMT
> > > tip-bot for Thomas Gleixner <tglx@linutronix.de> wrote:
> > > 
> > > > Commit-ID:  40b798efe3460797a4ac928ee2e038774e2758eb
> > > > Gitweb:     http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > > > Author:     Thomas Gleixner <tglx@linutronix.de>
> > > > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > > > Committer:  Thomas Gleixner <tglx@linutronix.de>
> > > > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> > > > 
> > > > drivers: Remove BKL from misc_open
> > > > 
> > > > misc_open() is already serialized with misc_mtx. Remove the BKL
> > > > locking which got there via the BKL pushdown.
> > > 
> > > NAK.
> > > 
> > > You can't simply assume the mutex is enough - you have to either push it
> > > down or review *every* possible called point below the lock_kernel take.
> > > 
> > > In this case the open method of the misc devices below sometimes uses the
> > > BKL.
> > 
> > The BKL got pushed down into the open methods of misc dev users and we
> > do not need to take it twice in a row, right ?
> 
> Then the comment is misleading and you need to document that you've
> already pushed the BKL down into each user and checked them.

Fair enough. I fix that.

     tglx

Re: [tip:bkl/drivers] drivers: Remove BKL from misc_open

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Wed, 14 Oct 2009 15:47:39 GMT
> tip-bot for Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Commit-ID:  40b798efe3460797a4ac928ee2e038774e2758eb
> > Gitweb:     http://git.kernel.org/tip/40b798efe3460797a4ac928ee2e038774e2758eb
> > Author:     Thomas Gleixner <tglx@linutronix.de>
> > AuthorDate: Sat, 10 Oct 2009 15:35:43 +0000
> > Committer:  Thomas Gleixner <tglx@linutronix.de>
> > CommitDate: Wed, 14 Oct 2009 17:33:32 +0200
> > 
> > drivers: Remove BKL from misc_open
> > 
> > misc_open() is already serialized with misc_mtx. Remove the BKL
> > locking which got there via the BKL pushdown.
> 
> NAK.
> 
> You can't simply assume the mutex is enough - you have to either push 
> it down or review *every* possible called point below the lock_kernel 
> take.
> 
> In this case the open method of the misc devices below sometimes uses 
> the BKL.

We did that and it's safe in nvram.c. If you know an unsafe please let 
us know.

	Ingo

[patch 06/28] drivers: Remove BKL from cs5535_gpio

[Thomas Gleixner]

[-- Attachment #1: drivers-char-cs5535-gpio-remove-BKL.patch --]
[-- Type: text/plain, Size: 858 bytes --]

The big BKL pushdown added cycle_kernel_lock(). There is nothing to
wait for in this driver. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/char/cs5535_gpio.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: linux-2.6-tip/drivers/char/cs5535_gpio.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/cs5535_gpio.c
+++ linux-2.6-tip/drivers/char/cs5535_gpio.c
@@ -17,7 +17,7 @@
 #include <linux/cdev.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
-#include <linux/smp_lock.h>
+
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
@@ -158,7 +158,6 @@ static int cs5535_gpio_open(struct inode
 {
 	u32 m = iminor(inode);
 
-	cycle_kernel_lock();
 	/* the mask says which pins are usable by this driver */
 	if ((mask & (1 << m)) == 0)
 		return -EINVAL;

[tip:bkl/drivers] drivers: Remove BKL from cs5535_gpio

[tip-bot for Thomas Gleixner]

Commit-ID:  a7e63bb5f08378620d913824ab42e49027f22194
Gitweb:     http://git.kernel.org/tip/a7e63bb5f08378620d913824ab42e49027f22194
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 15:35:48 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:48 +0200

drivers: Remove BKL from cs5535_gpio

The big BKL pushdown added cycle_kernel_lock(). There is nothing to
wait for in this driver. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153349.277882707@linutronix.de>
---
 drivers/char/cs5535_gpio.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/char/cs5535_gpio.c b/drivers/char/cs5535_gpio.c
index 04ba906..4d830dc 100644
--- a/drivers/char/cs5535_gpio.c
+++ b/drivers/char/cs5535_gpio.c
@@ -17,7 +17,7 @@
 #include <linux/cdev.h>
 #include <linux/ioport.h>
 #include <linux/pci.h>
-#include <linux/smp_lock.h>
+
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
@@ -158,7 +158,6 @@ static int cs5535_gpio_open(struct inode *inode, struct file *file)
 {
 	u32 m = iminor(inode);
 
-	cycle_kernel_lock();
 	/* the mask says which pins are usable by this driver */
 	if ((mask & (1 << m)) == 0)
 		return -EINVAL;

[patch 07/28] spi: Remove BKL from spidev_open

[Thomas Gleixner]

[-- Attachment #1: spi-remove-bkl.patch --]
[-- Type: text/plain, Size: 1080 bytes --]

The BKL was added there with the big pushdown. Remove it as the code
is serialized already.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>

---
 drivers/spi/spidev.c |    3 ---
 1 file changed, 3 deletions(-)

Index: linux-2.6-tip/drivers/spi/spidev.c
===================================================================
--- linux-2.6-tip.orig/drivers/spi/spidev.c
+++ linux-2.6-tip/drivers/spi/spidev.c
@@ -30,7 +30,6 @@
 #include <linux/errno.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/spidev.h>
@@ -477,7 +476,6 @@ static int spidev_open(struct inode *ino
 	struct spidev_data	*spidev;
 	int			status = -ENXIO;
 
-	lock_kernel();
 	mutex_lock(&device_list_lock);
 
 	list_for_each_entry(spidev, &device_list, device_entry) {
@@ -503,7 +501,6 @@ static int spidev_open(struct inode *ino
 		pr_debug("spidev: nothing for minor %d\n", iminor(inode));
 
 	mutex_unlock(&device_list_lock);
-	unlock_kernel();
 	return status;
 }
 

[tip:bkl/drivers] spi: Remove BKL from spidev_open

[tip-bot for Thomas Gleixner]

Commit-ID:  4c2aedc2543248c3fdc8c06c662b589d36c93bbb
Gitweb:     http://git.kernel.org/tip/4c2aedc2543248c3fdc8c06c662b589d36c93bbb
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 15:35:52 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:48 +0200

spi: Remove BKL from spidev_open

The BKL was added there with the big pushdown. Remove it as the code
is serialized already.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153349.318535932@linutronix.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/spi/spidev.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 5d23983..815a650 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -30,7 +30,6 @@
 #include <linux/errno.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/spidev.h>
@@ -477,7 +476,6 @@ static int spidev_open(struct inode *inode, struct file *filp)
 	struct spidev_data	*spidev;
 	int			status = -ENXIO;
 
-	lock_kernel();
 	mutex_lock(&device_list_lock);
 
 	list_for_each_entry(spidev, &device_list, device_entry) {
@@ -503,7 +501,6 @@ static int spidev_open(struct inode *inode, struct file *filp)
 		pr_debug("spidev: nothing for minor %d\n", iminor(inode));
 
 	mutex_unlock(&device_list_lock);
-	unlock_kernel();
 	return status;
 }
 

[patch 08/28] x86: Remove BKL from apm_32

[Thomas Gleixner]

[-- Attachment #1: x86-remove-bkl-from-apm-32.patch --]
[-- Type: text/plain, Size: 2633 bytes --]

The lock/unlock kernel pair in do_open() got there with the BKL push
down and protects nothing. Remove it.

Replace the lock/unlock kernel in the ioctl code with a mutex to
protect standbys_pending and suspends_pending.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/apm_32.c |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Index: linux-2.6-tip/arch/x86/kernel/apm_32.c
===================================================================
--- linux-2.6-tip.orig/arch/x86/kernel/apm_32.c
+++ linux-2.6-tip/arch/x86/kernel/apm_32.c
@@ -204,7 +204,6 @@
 #include <linux/module.h>
 
 #include <linux/poll.h>
-#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/stddef.h>
 #include <linux/timer.h>
@@ -403,6 +402,7 @@ static DECLARE_WAIT_QUEUE_HEAD(apm_waitq
 static DECLARE_WAIT_QUEUE_HEAD(apm_suspend_waitqueue);
 static struct apm_user *user_list;
 static DEFINE_SPINLOCK(user_list_lock);
+static DEFINE_MUTEX(apm_mutex);
 
 /*
  * Set up a segment that references the real mode segment 0x40
@@ -1531,7 +1531,7 @@ static long do_ioctl(struct file *filp, 
 		return -EPERM;
 	switch (cmd) {
 	case APM_IOC_STANDBY:
-		lock_kernel();
+		mutex_lock(&apm_mutex);
 		if (as->standbys_read > 0) {
 			as->standbys_read--;
 			as->standbys_pending--;
@@ -1540,10 +1540,10 @@ static long do_ioctl(struct file *filp, 
 			queue_event(APM_USER_STANDBY, as);
 		if (standbys_pending <= 0)
 			standby();
-		unlock_kernel();
+		mutex_unlock(&apm_mutex);
 		break;
 	case APM_IOC_SUSPEND:
-		lock_kernel();
+		mutex_lock(&apm_mutex);
 		if (as->suspends_read > 0) {
 			as->suspends_read--;
 			as->suspends_pending--;
@@ -1552,13 +1552,14 @@ static long do_ioctl(struct file *filp, 
 			queue_event(APM_USER_SUSPEND, as);
 		if (suspends_pending <= 0) {
 			ret = suspend(1);
+			mutex_unlock(&apm_mutex);
 		} else {
 			as->suspend_wait = 1;
+			mutex_unlock(&apm_mutex);
 			wait_event_interruptible(apm_suspend_waitqueue,
 					as->suspend_wait == 0);
 			ret = as->suspend_result;
 		}
-		unlock_kernel();
 		return ret;
 	default:
 		return -ENOTTY;
@@ -1608,12 +1609,10 @@ static int do_open(struct inode *inode, 
 {
 	struct apm_user *as;
 
-	lock_kernel();
 	as = kmalloc(sizeof(*as), GFP_KERNEL);
 	if (as == NULL) {
 		printk(KERN_ERR "apm: cannot allocate struct of size %d bytes\n",
 		       sizeof(*as));
-		       unlock_kernel();
 		return -ENOMEM;
 	}
 	as->magic = APM_BIOS_MAGIC;
@@ -1635,7 +1634,6 @@ static int do_open(struct inode *inode, 
 	user_list = as;
 	spin_unlock(&user_list_lock);
 	filp->private_data = as;
-	unlock_kernel();
 	return 0;
 }
 

[tip:x86/cleanups] x86: Remove BKL from apm_32

[tip-bot for Thomas Gleixner]

Commit-ID:  05d86412eab6a18cf57697474cc4f8fbfcd6936f
Gitweb:     http://git.kernel.org/tip/05d86412eab6a18cf57697474cc4f8fbfcd6936f
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Fri, 9 Oct 2009 19:02:20 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:04:48 +0200

x86: Remove BKL from apm_32

The lock/unlock kernel pair in do_open() got there with the BKL push
down and protects nothing. Remove it.

Replace the lock/unlock kernel in the ioctl code with a mutex to
protect standbys_pending and suspends_pending.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153349.365236337@linutronix.de>

---
 arch/x86/kernel/apm_32.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/apm_32.c b/arch/x86/kernel/apm_32.c
index 151ace6..b5b6b23 100644
--- a/arch/x86/kernel/apm_32.c
+++ b/arch/x86/kernel/apm_32.c
@@ -204,7 +204,6 @@
 #include <linux/module.h>
 
 #include <linux/poll.h>
-#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/stddef.h>
 #include <linux/timer.h>
@@ -403,6 +402,7 @@ static DECLARE_WAIT_QUEUE_HEAD(apm_waitqueue);
 static DECLARE_WAIT_QUEUE_HEAD(apm_suspend_waitqueue);
 static struct apm_user *user_list;
 static DEFINE_SPINLOCK(user_list_lock);
+static DEFINE_MUTEX(apm_mutex);
 
 /*
  * Set up a segment that references the real mode segment 0x40
@@ -1531,7 +1531,7 @@ static long do_ioctl(struct file *filp, u_int cmd, u_long arg)
 		return -EPERM;
 	switch (cmd) {
 	case APM_IOC_STANDBY:
-		lock_kernel();
+		mutex_lock(&apm_mutex);
 		if (as->standbys_read > 0) {
 			as->standbys_read--;
 			as->standbys_pending--;
@@ -1540,10 +1540,10 @@ static long do_ioctl(struct file *filp, u_int cmd, u_long arg)
 			queue_event(APM_USER_STANDBY, as);
 		if (standbys_pending <= 0)
 			standby();
-		unlock_kernel();
+		mutex_unlock(&apm_mutex);
 		break;
 	case APM_IOC_SUSPEND:
-		lock_kernel();
+		mutex_lock(&apm_mutex);
 		if (as->suspends_read > 0) {
 			as->suspends_read--;
 			as->suspends_pending--;
@@ -1552,13 +1552,14 @@ static long do_ioctl(struct file *filp, u_int cmd, u_long arg)
 			queue_event(APM_USER_SUSPEND, as);
 		if (suspends_pending <= 0) {
 			ret = suspend(1);
+			mutex_unlock(&apm_mutex);
 		} else {
 			as->suspend_wait = 1;
+			mutex_unlock(&apm_mutex);
 			wait_event_interruptible(apm_suspend_waitqueue,
 					as->suspend_wait == 0);
 			ret = as->suspend_result;
 		}
-		unlock_kernel();
 		return ret;
 	default:
 		return -ENOTTY;
@@ -1608,12 +1609,10 @@ static int do_open(struct inode *inode, struct file *filp)
 {
 	struct apm_user *as;
 
-	lock_kernel();
 	as = kmalloc(sizeof(*as), GFP_KERNEL);
 	if (as == NULL) {
 		printk(KERN_ERR "apm: cannot allocate struct of size %d bytes\n",
 		       sizeof(*as));
-		       unlock_kernel();
 		return -ENOMEM;
 	}
 	as->magic = APM_BIOS_MAGIC;
@@ -1635,7 +1634,6 @@ static int do_open(struct inode *inode, struct file *filp)
 	user_list = as;
 	spin_unlock(&user_list_lock);
 	filp->private_data = as;
-	unlock_kernel();
 	return 0;
 }
 

[patch 09/28] sys: Remove BKL from sys_reboot

[Thomas Gleixner]

[-- Attachment #1: sys-remove-bkl-from-reboot.patch --]
[-- Type: text/plain, Size: 1835 bytes --]

Serialization of sys_reboot can be done local. The BKL is not
protecting anything else.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/sys.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: linux-2.6-tip/kernel/sys.c
===================================================================
--- linux-2.6-tip.orig/kernel/sys.c
+++ linux-2.6-tip/kernel/sys.c
@@ -8,7 +8,6 @@
 #include <linux/mm.h>
 #include <linux/utsname.h>
 #include <linux/mman.h>
-#include <linux/smp_lock.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
 #include <linux/prctl.h>
@@ -349,6 +348,9 @@ void kernel_power_off(void)
 	machine_power_off();
 }
 EXPORT_SYMBOL_GPL(kernel_power_off);
+
+static DEFINE_MUTEX(reboot_mutex);
+
 /*
  * Reboot system call: for obvious reasons only root may call it,
  * and even root needs to set up some magic numbers in the registers
@@ -381,7 +383,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int
 	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
 		cmd = LINUX_REBOOT_CMD_HALT;
 
-	lock_kernel();
+	mutex_lock(&reboot_mutex);
 	switch (cmd) {
 	case LINUX_REBOOT_CMD_RESTART:
 		kernel_restart(NULL);
@@ -397,20 +399,18 @@ SYSCALL_DEFINE4(reboot, int, magic1, int
 
 	case LINUX_REBOOT_CMD_HALT:
 		kernel_halt();
-		unlock_kernel();
 		do_exit(0);
 		panic("cannot halt");
 
 	case LINUX_REBOOT_CMD_POWER_OFF:
 		kernel_power_off();
-		unlock_kernel();
 		do_exit(0);
 		break;
 
 	case LINUX_REBOOT_CMD_RESTART2:
 		if (strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1) < 0) {
-			unlock_kernel();
-			return -EFAULT;
+			ret = -EFAULT;
+			break;
 		}
 		buffer[sizeof(buffer) - 1] = '\0';
 
@@ -433,7 +433,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int
 		ret = -EINVAL;
 		break;
 	}
-	unlock_kernel();
+	mutex_unlock(&reboot_mutex);
 	return ret;
 }
 

[tip:bkl/core] sys: Remove BKL from sys_reboot

[tip-bot for Thomas Gleixner]

Commit-ID:  6f15fa50087c8317e353145319466afbeb27a75d
Gitweb:     http://git.kernel.org/tip/6f15fa50087c8317e353145319466afbeb27a75d
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Fri, 9 Oct 2009 20:31:33 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 15:31:10 +0200

sys: Remove BKL from sys_reboot

Serialization of sys_reboot can be done local. The BKL is not
protecting anything else.

LKML-Reference: <20091010153349.405590702@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/sys.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 255475d..22ea955 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -8,7 +8,6 @@
 #include <linux/mm.h>
 #include <linux/utsname.h>
 #include <linux/mman.h>
-#include <linux/smp_lock.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
 #include <linux/prctl.h>
@@ -349,6 +348,9 @@ void kernel_power_off(void)
 	machine_power_off();
 }
 EXPORT_SYMBOL_GPL(kernel_power_off);
+
+static DEFINE_MUTEX(reboot_mutex);
+
 /*
  * Reboot system call: for obvious reasons only root may call it,
  * and even root needs to set up some magic numbers in the registers
@@ -381,7 +383,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
 	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
 		cmd = LINUX_REBOOT_CMD_HALT;
 
-	lock_kernel();
+	mutex_lock(&reboot_mutex);
 	switch (cmd) {
 	case LINUX_REBOOT_CMD_RESTART:
 		kernel_restart(NULL);
@@ -397,20 +399,18 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
 
 	case LINUX_REBOOT_CMD_HALT:
 		kernel_halt();
-		unlock_kernel();
 		do_exit(0);
 		panic("cannot halt");
 
 	case LINUX_REBOOT_CMD_POWER_OFF:
 		kernel_power_off();
-		unlock_kernel();
 		do_exit(0);
 		break;
 
 	case LINUX_REBOOT_CMD_RESTART2:
 		if (strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1) < 0) {
-			unlock_kernel();
-			return -EFAULT;
+			ret = -EFAULT;
+			break;
 		}
 		buffer[sizeof(buffer) - 1] = '\0';
 
@@ -433,7 +433,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
 		ret = -EINVAL;
 		break;
 	}
-	unlock_kernel();
+	mutex_unlock(&reboot_mutex);
 	return ret;
 }
 

[patch 10/28] mem_class: Drop the bkl from memory_open()

[Thomas Gleixner]

[-- Attachment #1: mem_class-drop-the-bkl-from-memory_open.patch --]
[-- Type: text/plain, Size: 2061 bytes --]

The generic open callback for the mem class devices is "protected" by
the bkl.

Let's look at the datas manipulated inside memory_open:

- inode and file: safe
- the devlist: safe because it is constant
- the memdev classes inside this array are safe too (constant)

After we find out which memdev file operation we need to use, we call
its open callback. Depending on the targeted memdev, we call either
open_port() that doesn't manipulate any racy data (just a capable()
check), or we call nothing.

So it's safe to remove the big kernel lock there.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: John Kacur <jkacur@redhat.com>
Cc: Sven-Thorsten Dietrich <sven@thebigcorporation.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Alessio Igor Bogani <abogani@texware.it>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
LKML-Reference: <1255113062-5835-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/char/mem.c |   17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

Index: linux-2.6-tip/drivers/char/mem.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/mem.c
+++ linux-2.6-tip/drivers/char/mem.c
@@ -26,7 +26,6 @@
 #include <linux/bootmem.h>
 #include <linux/splice.h>
 #include <linux/pfn.h>
-#include <linux/smp_lock.h>
 
 #include <asm/uaccess.h>
 #include <asm/io.h>
@@ -892,29 +891,23 @@ static int memory_open(struct inode *ino
 {
 	int minor;
 	const struct memdev *dev;
-	int ret = -ENXIO;
-
-	lock_kernel();
 
 	minor = iminor(inode);
 	if (minor >= ARRAY_SIZE(devlist))
-		goto out;
+		return -ENXIO;
 
 	dev = &devlist[minor];
 	if (!dev->fops)
-		goto out;
+		return -ENXIO;
 
 	filp->f_op = dev->fops;
 	if (dev->dev_info)
 		filp->f_mapping->backing_dev_info = dev->dev_info;
 
 	if (dev->fops->open)
-		ret = dev->fops->open(inode, filp);
-	else
-		ret = 0;
-out:
-	unlock_kernel();
-	return ret;
+		return dev->fops->open(inode, filp);
+
+	return 0;
 }
 
 static const struct file_operations memory_fops = {

[patch 11/28] nvram: Drop the bkl from nvram_llseek()

[Thomas Gleixner]

[-- Attachment #1: nvram-drop-the-bkl-from-nvram_llseek.patch --]
[-- Type: text/plain, Size: 1607 bytes --]

There is nothing to protect inside nvram_llseek(), the file
offset doesn't need to be protected and nvram_len is only
initialized from an __init path.

It's safe to remove the big kernel lock there.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: John Kacur <jkacur@redhat.com>
Cc: Sven-Thorsten Dietrich <sven@thebigcorporation.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Alessio Igor Bogani <abogani@texware.it>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Greg KH <gregkh@suse.de>
LKML-Reference: <1255116030-6929-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/char/generic_nvram.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Index: linux-2.6-tip/drivers/char/generic_nvram.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/generic_nvram.c
+++ linux-2.6-tip/drivers/char/generic_nvram.c
@@ -19,7 +19,6 @@
 #include <linux/miscdevice.h>
 #include <linux/fcntl.h>
 #include <linux/init.h>
-#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 #include <asm/nvram.h>
 #ifdef CONFIG_PPC_PMAC
@@ -32,7 +31,6 @@ static ssize_t nvram_len;
 
 static loff_t nvram_llseek(struct file *file, loff_t offset, int origin)
 {
-	lock_kernel();
 	switch (origin) {
 	case 1:
 		offset += file->f_pos;
@@ -41,12 +39,11 @@ static loff_t nvram_llseek(struct file *
 		offset += nvram_len;
 		break;
 	}
-	if (offset < 0) {
-		unlock_kernel();
+	if (offset < 0)
 		return -EINVAL;
-	}
+
 	file->f_pos = offset;
-	unlock_kernel();
+
 	return file->f_pos;
 }
 

Re: [patch 11/28] nvram: Drop the bkl from nvram_llseek()

[Arnd Bergmann]

On Saturday 10 October 2009, Thomas Gleixner wrote:
> There is nothing to protect inside nvram_llseek(), the file
> offset doesn't need to be protected and nvram_len is only
> initialized from an __init path.
> 
> It's safe to remove the big kernel lock there.
> 

The generic_nvram driver still uses ->ioctl instead of ->unlocked_ioctl.
I guess it would be helpful to change that in the same series, so we
don't get the BKL back as soon as someone does a pushdown into the
remaining ioctl functions.

	Arnd <><

Re: [patch 11/28] nvram: Drop the bkl from nvram_llseek()

[Frederic Weisbecker]

On Sun, Oct 11, 2009 at 09:31:40PM +0200, Arnd Bergmann wrote:
> On Saturday 10 October 2009, Thomas Gleixner wrote:
> > There is nothing to protect inside nvram_llseek(), the file
> > offset doesn't need to be protected and nvram_len is only
> > initialized from an __init path.
> > 
> > It's safe to remove the big kernel lock there.
> > 
> 
> The generic_nvram driver still uses ->ioctl instead of ->unlocked_ioctl.
> I guess it would be helpful to change that in the same series, so we
> don't get the BKL back as soon as someone does a pushdown into the
> remaining ioctl functions.
> 
> 	Arnd <><


Right!
I'll add that in a second patch.

I've completely forgotten this ioctl/unlocked_ioctl thing.

Re: [patch 11/28] nvram: Drop the bkl from nvram_llseek()

[Frederic Weisbecker]

On Sun, Oct 11, 2009 at 11:08:10PM +0200, Frederic Weisbecker wrote:
> On Sun, Oct 11, 2009 at 09:31:40PM +0200, Arnd Bergmann wrote:
> > On Saturday 10 October 2009, Thomas Gleixner wrote:
> > > There is nothing to protect inside nvram_llseek(), the file
> > > offset doesn't need to be protected and nvram_len is only
> > > initialized from an __init path.
> > > 
> > > It's safe to remove the big kernel lock there.
> > > 
> > 
> > The generic_nvram driver still uses ->ioctl instead of ->unlocked_ioctl.
> > I guess it would be helpful to change that in the same series, so we
> > don't get the BKL back as soon as someone does a pushdown into the
> > remaining ioctl functions.
> > 
> > 	Arnd <><
> 
> 
> Right!
> I'll add that in a second patch.
> 
> I've completely forgotten this ioctl/unlocked_ioctl thing.


BTW, I was focusing on the lock_kernel() callsites in the kernel which
are around 626 (I've excluded reiserfs)

Now I'm adding the ioctl() sites too:

git-grep "\.ioctl *=" | grep -P "^\S+\.c" | wc -l
452

Hehe :)

Re: [patch 11/28] nvram: Drop the bkl from nvram_llseek()

[Arnd Bergmann]

On Sunday 11 October 2009, Frederic Weisbecker wrote:
> Now I'm adding the ioctl() sites too:
> 
> git-grep "\.ioctl *=" | grep -P "^\S+\.c" | wc -l
> 452
> 
> Hehe :)

Not all of them, fortunately.

There are various *_operations structures that have a .ioctl pointer.
While there are a lot of struct file_operations with a locked .ioctl
operation, stuff like block_device_operations does not hold the
BKL in .ioctl but in .locked_ioctl.

	Arnd <><

Re: [patch 11/28] nvram: Drop the bkl from nvram_llseek()

[Frederic Weisbecker]

On Sun, Oct 11, 2009 at 11:50:24PM +0200, Arnd Bergmann wrote:
> On Sunday 11 October 2009, Frederic Weisbecker wrote:
> > Now I'm adding the ioctl() sites too:
> > 
> > git-grep "\.ioctl *=" | grep -P "^\S+\.c" | wc -l
> > 452
> > 
> > Hehe :)
> 
> Not all of them, fortunately.
> 
> There are various *_operations structures that have a .ioctl pointer.
> While there are a lot of struct file_operations with a locked .ioctl
> operation, stuff like block_device_operations does not hold the
> BKL in .ioctl but in .locked_ioctl.
> 
> 	Arnd <><


Oh right. Thanks for the tip.

Re: [patch 11/28] nvram: Drop the bkl from nvram_llseek()

[Arnd Bergmann]

On Monday 12 October 2009, Frederic Weisbecker wrote:
> On Sun, Oct 11, 2009 at 11:50:24PM +0200, Arnd Bergmann wrote:
> >
> > There are various *_operations structures that have a .ioctl pointer.
> > While there are a lot of struct file_operations with a locked .ioctl
> > operation, stuff like block_device_operations does not hold the
> > BKL in .ioctl but in .locked_ioctl.
> 
> Oh right. Thanks for the tip.
> 

FWIW, I've done a grep through the current source tree, this should be
the full list of all .ioctl methods in struct file_operations, a total
of 141 instances in 2.6.32-rc4.

When we do a pushdown of the BKL into these functions, we can kill off
the file operation.

	Arnd <><

arch/blackfin/mach-bf561/coreb.c:	.ioctl   = coreb_ioctl,
arch/cris/arch-v10/drivers/ds1302.c:	.ioctl =	rtc_ioctl,
arch/cris/arch-v10/drivers/gpio.c:	.ioctl       = gpio_ioctl,
arch/cris/arch-v10/drivers/i2c.c:	.ioctl    = i2c_ioctl,
arch/cris/arch-v10/drivers/pcf8563.c:	.ioctl = pcf8563_ioctl,
arch/cris/arch-v10/drivers/sync_serial.c:	.ioctl   = sync_serial_ioctl,
arch/cris/arch-v32/drivers/cryptocop.c:	.ioctl =	cryptocop_ioctl
arch/cris/arch-v32/drivers/i2c.c:	.ioctl =    i2c_ioctl,
arch/cris/arch-v32/drivers/mach-a3/gpio.c:	.ioctl       = gpio_ioctl,
arch/cris/arch-v32/drivers/mach-fs/gpio.c:	.ioctl       = gpio_ioctl,
arch/cris/arch-v32/drivers/pcf8563.c:	.ioctl =	pcf8563_ioctl
arch/cris/arch-v32/drivers/sync_serial.c:	.ioctl   = sync_serial_ioctl,
arch/ia64/kernel/perfmon.c:	.ioctl    = pfm_ioctl,
arch/ia64/sn/kernel/sn2/sn_hwperf.c:	.ioctl = sn_hwperf_ioctl,
arch/m68k/bvme6000/rtc.c:	.ioctl =	rtc_ioctl,
arch/m68k/mvme16x/rtc.c:	.ioctl =	rtc_ioctl,
arch/powerpc/kernel/nvram_64.c:	.ioctl =	dev_nvram_ioctl,
arch/sh/boards/mach-landisk/gio.c:      .ioctl = gio_ioctl,     /* ioctl */
arch/um/drivers/harddog_kern.c:	.ioctl		= harddog_ioctl,
arch/um/drivers/hostaudio_kern.c:	.ioctl          = hostaudio_ioctl,
arch/um/drivers/mmapper_kern.c:	.ioctl		= mmapper_ioctl,
drivers/block/pktcdvd.c:	.ioctl	 = pkt_ctl_ioctl,
drivers/bluetooth/hci_vhci.c:	.ioctl		= vhci_ioctl,
drivers/char/apm-emulation.c:	.ioctl		= apm_ioctl,
drivers/char/applicom.c:	.ioctl = ac_ioctl,
drivers/char/ds1620.c:	.ioctl		= ds1620_ioctl,
drivers/char/dtlk.c:	.ioctl		= dtlk_ioctl,
drivers/char/nvram.c:	.ioctl		= nvram_ioctl,
drivers/char/generic_nvram.c:	.ioctl		= nvram_ioctl,
drivers/char/genrtc.c:	.ioctl		= gen_rtc_ioctl,
drivers/char/hpet.c:	.ioctl = hpet_ioctl,
drivers/char/i8k.c:	.ioctl		= i8k_ioctl,
drivers/char/ipmi/ipmi_devintf.c:	.ioctl		= ipmi_ioctl,
drivers/char/ipmi/ipmi_watchdog.c:	.ioctl   = ipmi_ioctl,
drivers/char/istallion.c:	.ioctl		= stli_memioctl,
drivers/char/lp.c:	.ioctl		= lp_ioctl,
drivers/char/nwflash.c:	.ioctl		= flash_ioctl,
drivers/char/raw.c:	.ioctl	=	raw_ioctl,
drivers/char/sonypi.c:	.ioctl		= sonypi_misc_ioctl,
drivers/char/stallion.c:	.ioctl		= stl_memioctl,
drivers/char/toshiba.c:	.ioctl		= tosh_ioctl,
drivers/gpu/drm/i810/i810_dma.c:	.ioctl = drm_ioctl,
drivers/gpu/drm/i810/i810_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/i830/i830_dma.c:	.ioctl = drm_ioctl,
drivers/gpu/drm/i830/i830_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/i915/i915_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/mga/mga_drv.c:		.ioctl = drm_ioctl,
drivers/gpu/drm/r128/r128_drv.c:		.ioctl = drm_ioctl,
drivers/gpu/drm/radeon/radeon_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/radeon/radeon_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/savage/savage_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/sis/sis_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/tdfx/tdfx_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/via/via_drv.c:		.ioctl = drm_ioctl,
drivers/gpu/drm/i810/i810_dma.c:	.ioctl = drm_ioctl,
drivers/gpu/drm/i810/i810_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/i830/i830_dma.c:	.ioctl = drm_ioctl,
drivers/gpu/drm/i830/i830_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/i915/i915_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/mga/mga_drv.c:		.ioctl = drm_ioctl,
drivers/gpu/drm/r128/r128_drv.c:		.ioctl = drm_ioctl,
drivers/gpu/drm/radeon/radeon_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/radeon/radeon_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/savage/savage_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/sis/sis_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/tdfx/tdfx_drv.c:		 .ioctl = drm_ioctl,
drivers/gpu/drm/via/via_drv.c:		.ioctl = drm_ioctl,
drivers/hwmon/fschmd.c:	.ioctl = watchdog_ioctl,
drivers/ide/ide-tape.c:	.ioctl		= idetape_chrdev_ioctl,
drivers/input/misc/hp_sdc_rtc.c:        .ioctl =	hp_sdc_rtc_ioctl,
drivers/isdn/capi/capi.c:	.ioctl		= capi_ioctl,
drivers/isdn/divert/divert_procfs.c:	.ioctl          = isdn_divert_ioctl,
drivers/isdn/i4l/isdn_common.c:	.ioctl		= isdn_ioctl,
drivers/isdn/mISDN/timerdev.c:	.ioctl		= mISDN_ioctl,
drivers/macintosh/ans-lcd.c:	.ioctl	= anslcd_ioctl,
drivers/macintosh/nvram.c:	.ioctl		= nvram_ioctl,
drivers/macintosh/via-pmu.c:	.ioctl		= pmu_ioctl,
drivers/media/dvb/dvb-core/dmxdev.c:	.ioctl = dvb_demux_ioctl,
drivers/media/dvb/dvb-core/dmxdev.c:	.ioctl = dvb_dvr_ioctl,
drivers/media/dvb/dvb-core/dvb_ca_en50221.c:	.ioctl = dvb_ca_en50221_io_ioctl,
drivers/media/dvb/dvb-core/dvb_net.c:	.ioctl = dvb_net_ioctl,
drivers/media/dvb/dvb-core/dvb_frontend.c:	.ioctl		= dvb_generic_ioctl,
drivers/media/dvb/firewire/firedtv-ci.c:	.ioctl		= dvb_generic_ioctl,
drivers/media/dvb/ttpci/av7110.c:	.ioctl		= dvb_generic_ioctl,
drivers/media/dvb/ttpci/av7110_av.c:	.ioctl		= dvb_generic_ioctl,
drivers/media/dvb/ttpci/av7110_av.c:	.ioctl		= dvb_generic_ioctl,
drivers/media/dvb/ttpci/av7110_ca.c:	.ioctl		= dvb_generic_ioctl,
drivers/message/i2o/i2o_config.c:	.ioctl = i2o_cfg_ioctl,
drivers/mtd/mtdchar.c:	.ioctl		= mtd_ioctl,
drivers/net/wan/cosa.c:	.ioctl		= cosa_chardev_ioctl,
drivers/parisc/eisa_eeprom.c:	.ioctl =	eisa_eeprom_ioctl,
drivers/pcmcia/pcmcia_ioctl.c:	.ioctl		= ds_ioctl,
drivers/rtc/rtc-m41t80.c:	.ioctl	= wdt_ioctl,
drivers/s390/char/tape_char.c:	.ioctl = tapechar_ioctl,
drivers/s390/char/vmwatchdog.c:	.ioctl   = &vmwdt_ioctl,
drivers/sbus/char/openprom.c:	.ioctl =	openprom_ioctl,
drivers/scsi/3w-9xxx.c:	.ioctl		= twa_chrdev_ioctl,
drivers/scsi/3w-xxxx.c:	.ioctl		= tw_chrdev_ioctl,
drivers/scsi/aacraid/linit.c:	.ioctl		= aac_cfg_ioctl,
drivers/scsi/dpt_i2o.c:	.ioctl		= adpt_ioctl,
drivers/scsi/gdth.c:    .ioctl   = gdth_ioctl,
drivers/scsi/megaraid.c:	.ioctl		= megadev_ioctl,
drivers/scsi/megaraid/megaraid_mm.c:	.ioctl	= mraid_mm_ioctl,
drivers/scsi/osst.c:	.ioctl =        osst_ioctl,
drivers/scsi/sg.c:	.ioctl = sg_ioctl,
drivers/staging/comedi/comedi_fops.c:	.ioctl = comedi_ioctl,
drivers/staging/poch/poch.c:	.ioctl = poch_ioctl,
drivers/staging/sep/sep_driver.c:	.ioctl = sep_ioctl,
drivers/staging/vme/devices/vme_user.c:        .ioctl = vme_user_ioctl,
drivers/usb/core/devio.c:	.ioctl =	usbdev_ioctl,
drivers/usb/mon/mon_bin.c:	.ioctl =	mon_bin_ioctl,
fs/autofs/root.c:	.ioctl		= autofs_root_ioctl,
fs/autofs4/root.c:	.ioctl		= autofs4_root_ioctl,
fs/coda/pioctl.c:	.ioctl		= coda_pioctl,
fs/coda/psdev.c:	.ioctl		= coda_psdev_ioctl,
fs/ecryptfs/file.c:	.ioctl = ecryptfs_ioctl,
fs/ecryptfs/file.c:	.ioctl = ecryptfs_ioctl,
fs/fat/dir.c:	.ioctl		= fat_dir_ioctl,
fs/fat/file.c:	.ioctl		= fat_generic_ioctl,
fs/hfsplus/dir.c:	.ioctl          = hfsplus_ioctl,
fs/hfsplus/inode.c:	.ioctl          = hfsplus_ioctl,
fs/ncpfs/dir.c:	.ioctl		= ncp_ioctl,
fs/ncpfs/file.c:	.ioctl		= ncp_ioctl,
fs/reiserfs/dir.c:	.ioctl = reiserfs_ioctl,
fs/reiserfs/file.c:	.ioctl = reiserfs_ioctl,
fs/smbfs/dir.c:	.ioctl		= smb_ioctl,
fs/smbfs/file.c:	.ioctl		= smb_ioctl,
fs/udf/dir.c:	.ioctl			= udf_ioctl,
fs/udf/file.c:	.ioctl			= udf_ioctl,
net/sunrpc/cache.c:     .ioctl          = cache_ioctl_procfs, /* for FIONREAD */
net/sunrpc/cache.c:     .ioctl          = cache_ioctl_pipefs, /* for FIONREAD */
net/sunrpc/rpc_pipe.c:	.ioctl		= rpc_pipe_ioctl,
sound/oss/dmasound/dmasound_core.c:	.ioctl		= mixer_ioctl,
sound/oss/dmasound/dmasound_core.c:	.ioctl		= sq_ioctl,
sound/oss/msnd_pinnacle.c:	.ioctl		= dev_ioctl,
sound/oss/sh_dac_audio.c:      .ioctl =	dac_audio_ioctl,
sound/oss/soundcard.c:	.ioctl		= sound_ioctl,
sound/oss/swarm_cs4297a.c:	.ioctl		= cs4297a_ioctl_mixdev,
sound/oss/swarm_cs4297a.c:	.ioctl		= cs4297a_ioctl,
sound/oss/vwsnd.c:	.ioctl =	vwsnd_audio_ioctl,
sound/oss/vwsnd.c:	.ioctl =	vwsnd_mixer_ioctl,

Re: [patch 11/28] nvram: Drop the bkl from nvram_llseek()

[Thomas Gleixner]

On Tue, 13 Oct 2009, Arnd Bergmann wrote:
> On Monday 12 October 2009, Frederic Weisbecker wrote:
> > On Sun, Oct 11, 2009 at 11:50:24PM +0200, Arnd Bergmann wrote:
> > >
> > > There are various *_operations structures that have a .ioctl pointer.
> > > While there are a lot of struct file_operations with a locked .ioctl
> > > operation, stuff like block_device_operations does not hold the
> > > BKL in .ioctl but in .locked_ioctl.
> > 
> > Oh right. Thanks for the tip.
> > 
> 
> FWIW, I've done a grep through the current source tree, this should be
> the full list of all .ioctl methods in struct file_operations, a total
> of 141 instances in 2.6.32-rc4.
> 
> When we do a pushdown of the BKL into these functions, we can kill off
> the file operation.
> 
> 	Arnd <><
> 
> arch/blackfin/mach-bf561/coreb.c:	.ioctl   = coreb_ioctl,

That one is scary. The BKL is protecting against parallel ioctl
operations, but the operation on the sys control registers is not
protected against other operations on the same registers in
arch/blackfin/mach-bf561/smp.c. IPI code does not take the BKL
afaict. Mike ?

Thanks,

	tglx

[PATCH] generic_nvram: Turn nvram_ioctl into an unlocked ioctl

[Frederic Weisbecker]

nvram_ioctl is a bkl locked ioctl, but it can be an unlocked ioctl.

- part is provided by the user
- offset is provided by pmac_get_partition() which is safe as it only
  touches nvram_partitions, an array inistialized on __init time and
  read-only the rest of the time.
- nvram_sync() only relies on core99_nvram_sync() which checks
  is_core_99, nvram_data, nvram_image. Those are variables initialized
  on __init time only and their direct values are not touched further.
  The rest modifies the nvram image header, protected by nv_lock
  already.

So it's safe to call nvram_ioctl without the big kernel lock held.

Reported-by: Arnd Bergmann <arndbergmann@googlemail.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: John Kacur <jkacur@redhat.com>
Cc: Sven-Thorsten Dietrich <sven@thebigcorporation.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Alessio Igor Bogani <abogani@texware.it>
Cc: Arnd Bergmann <arndbergmann@googlemail.com>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/char/generic_nvram.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
index c49200e..fd448aa 100644
--- a/drivers/char/generic_nvram.c
+++ b/drivers/char/generic_nvram.c
@@ -118,11 +118,11 @@ static int nvram_ioctl(struct inode *inode, struct file *file,
 }
 
 const struct file_operations nvram_fops = {
-	.owner		= THIS_MODULE,
-	.llseek		= nvram_llseek,
-	.read		= read_nvram,
-	.write		= write_nvram,
-	.ioctl		= nvram_ioctl,
+	.owner			= THIS_MODULE,
+	.llseek			= nvram_llseek,
+	.read			= read_nvram,
+	.write			= write_nvram,
+	.unlocked_ioctl		= nvram_ioctl,
 };
 
 static struct miscdevice nvram_dev = {
-- 
1.6.2.3

Re: [PATCH] generic_nvram: Turn nvram_ioctl into an unlocked ioctl

[Arnd Bergmann]

On Monday 12 October 2009, Frederic Weisbecker wrote:
> nvram_ioctl is a bkl locked ioctl, but it can be an unlocked ioctl.
> 
> - part is provided by the user
> - offset is provided by pmac_get_partition() which is safe as it only
>   touches nvram_partitions, an array inistialized on __init time and
>   read-only the rest of the time.
> - nvram_sync() only relies on core99_nvram_sync() which checks
>   is_core_99, nvram_data, nvram_image. Those are variables initialized
>   on __init time only and their direct values are not touched further.
>   The rest modifies the nvram image header, protected by nv_lock
>   already.
> 
> So it's safe to call nvram_ioctl without the big kernel lock held.
> 
> Reported-by: Arnd Bergmann <arndbergmann@googlemail.com>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: John Kacur <jkacur@redhat.com>
> Cc: Sven-Thorsten Dietrich <sven@thebigcorporation.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Alessio Igor Bogani <abogani@texware.it>
> Cc: Arnd Bergmann <arndbergmann@googlemail.com>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>

Hmm, I wish I had not started using the gmail MTA for sending out
mail, not that address is public forever.

>  drivers/char/generic_nvram.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
> index c49200e..fd448aa 100644
> --- a/drivers/char/generic_nvram.c
> +++ b/drivers/char/generic_nvram.c
> @@ -118,11 +118,11 @@ static int nvram_ioctl(struct inode *inode, struct file *file,
>  }
>  
>  const struct file_operations nvram_fops = {
> -	.owner		= THIS_MODULE,
> -	.llseek		= nvram_llseek,
> -	.read		= read_nvram,
> -	.write		= write_nvram,
> -	.ioctl		= nvram_ioctl,
> +	.owner			= THIS_MODULE,
> +	.llseek			= nvram_llseek,
> +	.read			= read_nvram,
> +	.write			= write_nvram,
> +	.unlocked_ioctl		= nvram_ioctl,
>  };
>  
>  static struct miscdevice nvram_dev = {

The ioctl and unlocked_ioctl functions have a different signature, so you
need to adapt that, not just rename it.

Not sure if we should do it in the same patch, but this driver should also 
have a compat_ioctl method pointing to the same function. The ioctl numbers
in this driver are all 32/64 bit clean, but are not listed in fs/compat_ioctl.c,
so adding a .compat_ioctl pointer is the easiest way to fix 32 bit userland
accessing the device.

	Arnd <><

[PATCH v2] generic_nvram: Turn nvram_ioctl into an unlocked ioctl

[Frederic Weisbecker]

nvram_ioctl is a bkl locked ioctl, but it can be an unlocked ioctl.

- part is provided by the user
- offset is provided by pmac_get_partition() which is safe as it only
  touches nvram_partitions, an array inistialized on __init time and
  read-only the rest of the time.
- nvram_sync() only relies on core99_nvram_sync() which checks
  is_core_99, nvram_data, nvram_image. Those are variables initialized
  on __init time only and their direct values are not touched further.
  The rest modifies the nvram image header, protected by nv_lock
  already.

So it's safe to call nvram_ioctl without the big kernel lock held.

v2: Fix nvram_ioctl to fit unlocked_ioctl signature.

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: John Kacur <jkacur@redhat.com>
Cc: Sven-Thorsten Dietrich <sven@thebigcorporation.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Alessio Igor Bogani <abogani@texware.it>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 drivers/char/generic_nvram.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
index c49200e..866d04d 100644
--- a/drivers/char/generic_nvram.c
+++ b/drivers/char/generic_nvram.c
@@ -85,8 +85,7 @@ static ssize_t write_nvram(struct file *file, const char __user *buf,
 	return p - buf;
 }
 
-static int nvram_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg)
+static int nvram_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	switch(cmd) {
 #ifdef CONFIG_PPC_PMAC
@@ -118,11 +117,11 @@ static int nvram_ioctl(struct inode *inode, struct file *file,
 }
 
 const struct file_operations nvram_fops = {
-	.owner		= THIS_MODULE,
-	.llseek		= nvram_llseek,
-	.read		= read_nvram,
-	.write		= write_nvram,
-	.ioctl		= nvram_ioctl,
+	.owner			= THIS_MODULE,
+	.llseek			= nvram_llseek,
+	.read			= read_nvram,
+	.write			= write_nvram,
+	.unlocked_ioctl		= nvram_ioctl,
 };
 
 static struct miscdevice nvram_dev = {
-- 
1.6.2.3

Re: [PATCH] generic_nvram: Turn nvram_ioctl into an unlocked ioctl

[Frederic Weisbecker]

On Mon, Oct 12, 2009 at 12:25:05AM +0200, Arnd Bergmann wrote:
> On Monday 12 October 2009, Frederic Weisbecker wrote:
> > nvram_ioctl is a bkl locked ioctl, but it can be an unlocked ioctl.
> > 
> > - part is provided by the user
> > - offset is provided by pmac_get_partition() which is safe as it only
> >   touches nvram_partitions, an array inistialized on __init time and
> >   read-only the rest of the time.
> > - nvram_sync() only relies on core99_nvram_sync() which checks
> >   is_core_99, nvram_data, nvram_image. Those are variables initialized
> >   on __init time only and their direct values are not touched further.
> >   The rest modifies the nvram image header, protected by nv_lock
> >   already.
> > 
> > So it's safe to call nvram_ioctl without the big kernel lock held.
> > 
> > Reported-by: Arnd Bergmann <arndbergmann@googlemail.com>
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: John Kacur <jkacur@redhat.com>
> > Cc: Sven-Thorsten Dietrich <sven@thebigcorporation.com>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: Alessio Igor Bogani <abogani@texware.it>
> > Cc: Arnd Bergmann <arndbergmann@googlemail.com>
> > Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> 
> Hmm, I wish I had not started using the gmail MTA for sending out
> mail, not that address is public forever.


Sorry, didn't know about that. I'm then taking Arnd Bergmann <arnd@arndb.de>
found in the MAINTAINER file for the v2 patch.


 
> >  drivers/char/generic_nvram.c |   10 +++++-----
> >  1 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
> > index c49200e..fd448aa 100644
> > --- a/drivers/char/generic_nvram.c
> > +++ b/drivers/char/generic_nvram.c
> > @@ -118,11 +118,11 @@ static int nvram_ioctl(struct inode *inode, struct file *file,
> >  }
> >  
> >  const struct file_operations nvram_fops = {
> > -	.owner		= THIS_MODULE,
> > -	.llseek		= nvram_llseek,
> > -	.read		= read_nvram,
> > -	.write		= write_nvram,
> > -	.ioctl		= nvram_ioctl,
> > +	.owner			= THIS_MODULE,
> > +	.llseek			= nvram_llseek,
> > +	.read			= read_nvram,
> > +	.write			= write_nvram,
> > +	.unlocked_ioctl		= nvram_ioctl,
> >  };
> >  
> >  static struct miscdevice nvram_dev = {
> 
> The ioctl and unlocked_ioctl functions have a different signature, so you
> need to adapt that, not just rename it.


Hmm, that's the problem with this file I can't event build test. I really
should grab a cross compiler for powerpc targets.


> 
> Not sure if we should do it in the same patch, but this driver should also 
> have a compat_ioctl method pointing to the same function. The ioctl numbers
> in this driver are all 32/64 bit clean, but are not listed in fs/compat_ioctl.c,
> so adding a .compat_ioctl pointer is the easiest way to fix 32 bit userland
> accessing the device.
> 
> 	Arnd <><


Added in my todolist then, for a saperate patch because it fixes another
issue.

Thanks!

Re: [PATCH] generic_nvram: Turn nvram_ioctl into an unlocked ioctl

[Arnd Bergmann]

On Monday 12 October 2009, Frederic Weisbecker wrote:
> Sorry, didn't know about that. I'm then taking Arnd Bergmann <arnd@arndb.de>
> found in the MAINTAINER file for the v2 patch.

Sure, there's no way you could have known about this. Vger started rejecting
mail from my usual MTA last week, so until I've found a solution for that
I have to use another MTA, but I didn't realize that gmail rewrites the
From: headers...

	Arnd <><

[patch 12/28] nvram: Drop the bkl from non-generic nvram_llseek()

[Thomas Gleixner]

[-- Attachment #1: nvram-drop-the-bkl-from-non-generic-nvram_llseek.patch --]
[-- Type: text/plain, Size: 1299 bytes --]

Drop the bkl from nvram_llseek() as it obviously protects nothing.
The file offset is safe in essence.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: John Kacur <jkacur@redhat.com>
Cc: Sven-Thorsten Dietrich <sven@thebigcorporation.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Alessio Igor Bogani <abogani@texware.it>
Cc: Greg KH <gregkh@suse.de>
LKML-Reference: <1255116426-7270-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/char/nvram.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: linux-2.6-tip/drivers/char/nvram.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/nvram.c
+++ linux-2.6-tip/drivers/char/nvram.c
@@ -38,7 +38,6 @@
 #define NVRAM_VERSION	"1.3"
 
 #include <linux/module.h>
-#include <linux/smp_lock.h>
 #include <linux/nvram.h>
 
 #define PC		1
@@ -214,7 +213,6 @@ void nvram_set_checksum(void)
 
 static loff_t nvram_llseek(struct file *file, loff_t offset, int origin)
 {
-	lock_kernel();
 	switch (origin) {
 	case 0:
 		/* nothing to do */
@@ -226,7 +224,7 @@ static loff_t nvram_llseek(struct file *
 		offset += NVRAM_BYTES;
 		break;
 	}
-	unlock_kernel();
+
 	return (offset >= 0) ? (file->f_pos = offset) : -EINVAL;
 }
 

[patch 13/28] s390: Remove BKL from prng

[Thomas Gleixner]

[-- Attachment #1: s390-remove-bkl-from-prng.patch --]
[-- Type: text/plain, Size: 956 bytes --]

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/crypto/prng.c |    2 --
 1 file changed, 2 deletions(-)

Index: linux-2.6-tip/arch/s390/crypto/prng.c
===================================================================
--- linux-2.6-tip.orig/arch/s390/crypto/prng.c
+++ linux-2.6-tip/arch/s390/crypto/prng.c
@@ -6,7 +6,6 @@
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
-#include <linux/smp_lock.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -49,7 +48,6 @@ static unsigned char parm_block[32] = {
 
 static int prng_open(struct inode *inode, struct file *file)
 {
-	cycle_kernel_lock();
 	return nonseekable_open(inode, file);
 }
 

Re: [patch 13/28] s390: Remove BKL from prng

[Jan Glauber]

On Sat, 2009-10-10 at 15:36 +0000, Thomas Gleixner wrote:
> einfaches Textdokument attachment (s390-remove-bkl-from-prng.patch)
> cycle_kernel_lock() was added during the big BKL pushdown. It should
> ensure the serializiation against driver init code. In this case there
> is nothing to serialize. Remove it.

Looks good.
Acked-by: Jan Glauber <jang.linux.vnet.ibm.com>

-- Jan

> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  arch/s390/crypto/prng.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> Index: linux-2.6-tip/arch/s390/crypto/prng.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/s390/crypto/prng.c
> +++ linux-2.6-tip/arch/s390/crypto/prng.c
> @@ -6,7 +6,6 @@
>  #include <linux/fs.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> -#include <linux/smp_lock.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/moduleparam.h>
> @@ -49,7 +48,6 @@ static unsigned char parm_block[32] = {
>  
>  static int prng_open(struct inode *inode, struct file *file)
>  {
> -	cycle_kernel_lock();
>  	return nonseekable_open(inode, file);
>  }
>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[tip:bkl/arch] s390: Remove BKL from prng

[tip-bot for Thomas Gleixner]

Commit-ID:  ca1b82ba0888e742a7efdb89ed8e2aab453e091f
Gitweb:     http://git.kernel.org/tip/ca1b82ba0888e742a7efdb89ed8e2aab453e091f
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 10:21:03 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 15:35:50 +0200

s390: Remove BKL from prng

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
LKML-Reference: <20091010153349.601625576@linutronix.de>
Acked-by: Jan Glauber <jang@linux.vnet.ibm.com>

---
 arch/s390/crypto/prng.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/arch/s390/crypto/prng.c b/arch/s390/crypto/prng.c
index b49c00c..a320990 100644
--- a/arch/s390/crypto/prng.c
+++ b/arch/s390/crypto/prng.c
@@ -6,7 +6,6 @@
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
-#include <linux/smp_lock.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -49,7 +48,6 @@ static unsigned char parm_block[32] = {
 
 static int prng_open(struct inode *inode, struct file *file)
 {
-	cycle_kernel_lock();
 	return nonseekable_open(inode, file);
 }
 

[patch 14/28] um: Remove BKL from random

[Thomas Gleixner]

[-- Attachment #1: um-remove-BKL-from-random.patch --]
[-- Type: text/plain, Size: 1035 bytes --]

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

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

Index: linux-2.6-tip/arch/um/drivers/random.c
===================================================================
--- linux-2.6-tip.orig/arch/um/drivers/random.c
+++ linux-2.6-tip/arch/um/drivers/random.c
@@ -7,7 +7,6 @@
  * of the GNU General Public License, incorporated herein by reference.
  */
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
@@ -34,8 +33,6 @@ static DECLARE_WAIT_QUEUE_HEAD(host_read
 
 static int rng_dev_open (struct inode *inode, struct file *filp)
 {
-	cycle_kernel_lock();
-
 	/* enforce read-only access to this chrdev */
 	if ((filp->f_mode & FMODE_READ) == 0)
 		return -EINVAL;

[tip:bkl/arch] um: Remove BKL from random

[tip-bot for Thomas Gleixner]

Commit-ID:  df502e389383b219e44819fe757614450d95f297
Gitweb:     http://git.kernel.org/tip/df502e389383b219e44819fe757614450d95f297
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 15:36:30 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 15:35:51 +0200

um: Remove BKL from random

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153349.641118498@linutronix.de>
Cc: Jeff Dike <jdike@addtoit.com>
---
 arch/um/drivers/random.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/arch/um/drivers/random.c b/arch/um/drivers/random.c
index 6eabb70..4949044 100644
--- a/arch/um/drivers/random.c
+++ b/arch/um/drivers/random.c
@@ -7,7 +7,6 @@
  * of the GNU General Public License, incorporated herein by reference.
  */
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 #include <linux/module.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
@@ -34,8 +33,6 @@ static DECLARE_WAIT_QUEUE_HEAD(host_read_wait);
 
 static int rng_dev_open (struct inode *inode, struct file *filp)
 {
-	cycle_kernel_lock();
-
 	/* enforce read-only access to this chrdev */
 	if ((filp->f_mode & FMODE_READ) == 0)
 		return -EINVAL;

[patch 15/28] um: Remove BKL from mmapper

[Thomas Gleixner]

[-- Attachment #1: um-remove-BKL-from-mmapper.patch --]
[-- Type: text/plain, Size: 1529 bytes --]

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code.

mmapper_open() cannot be called before misc_register() succeeded, but
p_buf might be uninitialized.

Move the initialization of p_buf before the misc_register() call and
get rid of cycle_kernel_lock().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Jeff Dike <jdike@addtoit.com>
---
 arch/um/drivers/mmapper_kern.c |   11 ++++-------
 1 file changed, 4 insertions(+), 7 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
@@ -16,7 +16,7 @@
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mm.h>
-#include <linux/smp_lock.h>
+
 #include <asm/uaccess.h>
 #include "mem_user.h"
 
@@ -78,7 +78,6 @@ out:
 
 static int mmapper_open(struct inode *inode, struct file *file)
 {
-	cycle_kernel_lock();
 	return 0;
 }
 
@@ -115,18 +114,16 @@ static int __init mmapper_init(void)
 	v_buf = (char *) find_iomem("mmapper", &mmapper_size);
 	if (mmapper_size == 0) {
 		printk(KERN_ERR "mmapper_init - find_iomem failed\n");
-		goto out;
+		return -ENODEV;
 	}
+	p_buf = __pa(v_buf);
 
 	err = misc_register(&mmapper_dev);
 	if (err) {
 		printk(KERN_ERR "mmapper - misc_register failed, err = %d\n",
 		       err);
-		goto out;
+		return err;;
 	}
-
-	p_buf = __pa(v_buf);
-out:
 	return 0;
 }
 

[tip:bkl/arch] um: Remove BKL from mmapper

[tip-bot for Thomas Gleixner]

Commit-ID:  d63c489b881707adf9c0b89f771b30a1d78f4197
Gitweb:     http://git.kernel.org/tip/d63c489b881707adf9c0b89f771b30a1d78f4197
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 15:36:34 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 15:35:51 +0200

um: Remove BKL from mmapper

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code.

mmapper_open() cannot be called before misc_register() succeeded, but
p_buf might be uninitialized.

Move the initialization of p_buf before the misc_register() call and
get rid of cycle_kernel_lock().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153349.682213670@linutronix.de>
Cc: Jeff Dike <jdike@addtoit.com>
---
 arch/um/drivers/mmapper_kern.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/um/drivers/mmapper_kern.c b/arch/um/drivers/mmapper_kern.c
index eb24032..d22f9e5 100644
--- a/arch/um/drivers/mmapper_kern.c
+++ b/arch/um/drivers/mmapper_kern.c
@@ -16,7 +16,7 @@
 #include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/mm.h>
-#include <linux/smp_lock.h>
+
 #include <asm/uaccess.h>
 #include "mem_user.h"
 
@@ -78,7 +78,6 @@ out:
 
 static int mmapper_open(struct inode *inode, struct file *file)
 {
-	cycle_kernel_lock();
 	return 0;
 }
 
@@ -115,18 +114,16 @@ static int __init mmapper_init(void)
 	v_buf = (char *) find_iomem("mmapper", &mmapper_size);
 	if (mmapper_size == 0) {
 		printk(KERN_ERR "mmapper_init - find_iomem failed\n");
-		goto out;
+		return -ENODEV;
 	}
+	p_buf = __pa(v_buf);
 
 	err = misc_register(&mmapper_dev);
 	if (err) {
 		printk(KERN_ERR "mmapper - misc_register failed, err = %d\n",
 		       err);
-		goto out;
+		return err;;
 	}
-
-	p_buf = __pa(v_buf);
-out:
 	return 0;
 }
 

[patch 16/28] sparc: Remove BKL from apc

[Thomas Gleixner]

[-- Attachment #1: sparc-Remove-BKL-from-apc.patch --]
[-- Type: text/plain, Size: 2800 bytes --]

commit ab772027 (sparc: arch/sparc/kernel/apc.c to unlocked_ioctl)
added lock/unlock_kernel() to the apc ioctl code.

The code needs no serialization at all. Neither put/get_user nor the
read/write access to the sbus devices require it. Remove BKL.

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it as well.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: David S. Miller <davem@davemloft.net>
---
 arch/sparc/kernel/apc.c |   37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

Index: linux-2.6-tip/arch/sparc/kernel/apc.c
===================================================================
--- linux-2.6-tip.orig/arch/sparc/kernel/apc.c
+++ linux-2.6-tip/arch/sparc/kernel/apc.c
@@ -10,7 +10,6 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/miscdevice.h>
-#include <linux/smp_lock.h>
 #include <linux/pm.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -76,7 +75,6 @@ static inline void apc_free(struct of_de
 
 static int apc_open(struct inode *inode, struct file *f)
 {
-	cycle_kernel_lock();
 	return 0;
 }
 
@@ -87,61 +85,46 @@ static int apc_release(struct inode *ino
 
 static long apc_ioctl(struct file *f, unsigned int cmd, unsigned long __arg)
 {
-	__u8 inarg, __user *arg;
-
-	arg = (__u8 __user *) __arg;
-
-	lock_kernel();
+	__u8 inarg, __user *arg = (__u8 __user *) __arg;
 
 	switch (cmd) {
 	case APCIOCGFANCTL:
-		if (put_user(apc_readb(APC_FANCTL_REG) & APC_REGMASK, arg)) {
-			unlock_kernel();
+		if (put_user(apc_readb(APC_FANCTL_REG) & APC_REGMASK, arg))
 			return -EFAULT;
-		}
 		break;
 
 	case APCIOCGCPWR:
-		if (put_user(apc_readb(APC_CPOWER_REG) & APC_REGMASK, arg)) {
-			unlock_kernel();
+		if (put_user(apc_readb(APC_CPOWER_REG) & APC_REGMASK, arg))
 			return -EFAULT;
-		}
 		break;
 
 	case APCIOCGBPORT:
-		if (put_user(apc_readb(APC_BPORT_REG) & APC_BPMASK, arg)) {
-			unlock_kernel();
+		if (put_user(apc_readb(APC_BPORT_REG) & APC_BPMASK, arg))
 			return -EFAULT;
-		}
 		break;
 
 	case APCIOCSFANCTL:
-		if (get_user(inarg, arg)) {
-			unlock_kernel();
+		if (get_user(inarg, arg))
 			return -EFAULT;
-		}
 		apc_writeb(inarg & APC_REGMASK, APC_FANCTL_REG);
 		break;
+
 	case APCIOCSCPWR:
-		if (get_user(inarg, arg)) {
-			unlock_kernel();
+		if (get_user(inarg, arg))
 			return -EFAULT;
-		}
 		apc_writeb(inarg & APC_REGMASK, APC_CPOWER_REG);
 		break;
+
 	case APCIOCSBPORT:
-		if (get_user(inarg, arg)) {
-			unlock_kernel();
+		if (get_user(inarg, arg))
 			return -EFAULT;
-		}
 		apc_writeb(inarg & APC_BPMASK, APC_BPORT_REG);
 		break;
+
 	default:
-		unlock_kernel();
 		return -EINVAL;
 	};
 
-	unlock_kernel();
 	return 0;
 }
 

Re: [patch 16/28] sparc: Remove BKL from apc

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Sat, 10 Oct 2009 15:36:39 -0000

> commit ab772027 (sparc: arch/sparc/kernel/apc.c to unlocked_ioctl)
> added lock/unlock_kernel() to the apc ioctl code.
> 
> The code needs no serialization at all. Neither put/get_user nor the
> read/write access to the sbus devices require it. Remove BKL.
> 
> cycle_kernel_lock() was added during the big BKL pushdown. It should
> ensure the serializiation against driver init code. In this case there
> is nothing to serialize. Remove it as well.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Sorry, forgot to explicitly say I applied this to sparc-next-2.6, thanks.

[patch 17/28] watchdog: Fix probe function of riowd

[Thomas Gleixner]

[-- Attachment #1: drivers-watchdog-rio-fix-probe-function.patch --]
[-- Type: text/plain, Size: 918 bytes --]

After sucessfully registering the misc device the driver iounmaps the
device registers and kfree's the device data structure. Ouch !

This was introduced with commit e42311d75 (riowatchdog: Convert to
pure OF driver) and went unnoticed for more than a year :)

Return success instead of dropping into the error cleanup code path.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: David S. Miller <davem@davemloft.net>
---
 drivers/watchdog/riowd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6-tip/drivers/watchdog/riowd.c
===================================================================
--- linux-2.6-tip.orig/drivers/watchdog/riowd.c
+++ linux-2.6-tip/drivers/watchdog/riowd.c
@@ -206,7 +206,7 @@ static int __devinit riowd_probe(struct 
 
 	dev_set_drvdata(&op->dev, p);
 	riowd_device = p;
-	err = 0;
+	return 0;
 
 out_iounmap:
 	of_iounmap(&op->resource[0], p->regs, 2);

[patch 18/28] watchdog: Remove BKL from rio watchdog driver

[Thomas Gleixner]

[-- Attachment #1: drivers-watchdog-rio-remove-bkl.patch --]
[-- Type: text/plain, Size: 1713 bytes --]

cycle_kernel_lock() was added with the BKL pushdown. The rio driver
indeed needs that because riowd_device is initialized after
misc_register(). So an open(), write/ioctl() which happens to get
between misc_register returning and riowd_device initialization would
dereference a NULL pointer.

Move riowd_device initialization before misc_register() and get rid of
cycle_kernel_lock().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: David S. Miller <davem@davemloft.net>
---
 drivers/watchdog/riowd.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-2.6-tip/drivers/watchdog/riowd.c
===================================================================
--- linux-2.6-tip.orig/drivers/watchdog/riowd.c
+++ linux-2.6-tip/drivers/watchdog/riowd.c
@@ -10,7 +10,6 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/miscdevice.h>
-#include <linux/smp_lock.h>
 #include <linux/watchdog.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -75,7 +74,6 @@ static void riowd_writereg(struct riowd 
 
 static int riowd_open(struct inode *inode, struct file *filp)
 {
-	cycle_kernel_lock();
 	nonseekable_open(inode, filp);
 	return 0;
 }
@@ -194,6 +192,8 @@ static int __devinit riowd_probe(struct 
 		printk(KERN_ERR PFX "Cannot map registers.\n");
 		goto out_free;
 	}
+	/* Make miscdev useable right away */
+	riowd_device = p;
 
 	err = misc_register(&riowd_miscdev);
 	if (err) {
@@ -205,10 +205,10 @@ static int __devinit riowd_probe(struct 
 	       "regs at %p\n", riowd_timeout, p->regs);
 
 	dev_set_drvdata(&op->dev, p);
-	riowd_device = p;
 	return 0;
 
 out_iounmap:
+	riowd_device = NULL;
 	of_iounmap(&op->resource[0], p->regs, 2);
 
 out_free:

Re: [patch 18/28] watchdog: Remove BKL from rio watchdog driver

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Sat, 10 Oct 2009 15:36:48 -0000

> cycle_kernel_lock() was added with the BKL pushdown. The rio driver
> indeed needs that because riowd_device is initialized after
> misc_register(). So an open(), write/ioctl() which happens to get
> between misc_register returning and riowd_device initialization would
> dereference a NULL pointer.
> 
> Move riowd_device initialization before misc_register() and get rid of
> cycle_kernel_lock().
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Also applied to sparc-next-2.6, thanks!

[patch 19/28] hw_random: Remove BKL from core

[Thomas Gleixner]

[-- Attachment #1: driver-hw-random-core-remove-bkl.patch --]
[-- Type: text/plain, Size: 666 bytes --]

hw_random core is completely serialized with rng_mutex. No need for
the cycle_kernel_lock() magic.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 drivers/char/hw_random/core.c |    1 -
 1 file changed, 1 deletion(-)

Index: linux-2.6-tip/drivers/char/hw_random/core.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/hw_random/core.c
+++ linux-2.6-tip/drivers/char/hw_random/core.c
@@ -87,7 +87,6 @@ static int rng_dev_open(struct inode *in
 		return -EINVAL;
 	if (filp->f_mode & FMODE_WRITE)
 		return -EINVAL;
-	cycle_kernel_lock();
 	return 0;
 }
 

[tip:bkl/drivers] hw_random: Remove BKL from core

[tip-bot for Thomas Gleixner]

Commit-ID:  a09ba31a54dbc9a548c4ff90619e4c7128a4282e
Gitweb:     http://git.kernel.org/tip/a09ba31a54dbc9a548c4ff90619e4c7128a4282e
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 12:36:32 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:50 +0200

hw_random: Remove BKL from core

hw_random core is completely serialized with rng_mutex. No need for
the cycle_kernel_lock() magic.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153349.844488872@linutronix.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 drivers/char/hw_random/core.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 1573aeb..75fb859 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -87,7 +87,6 @@ static int rng_dev_open(struct inode *inode, struct file *filp)
 		return -EINVAL;
 	if (filp->f_mode & FMODE_WRITE)
 		return -EINVAL;
-	cycle_kernel_lock();
 	return 0;
 }
 

Re: [patch 19/28] hw_random: Remove BKL from core

[John Kacur]

>From d3da91fe463a0799911ed7c826ec133c38c0a18b Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Wed, 21 Oct 2009 22:46:30 +0200
Subject: [PATCH] RNG: Explicitly set llseek = no_llseek after the BKL is removed.

Now that we've removed the BKL, lets explicitly set llseek to no_llseek
since it is not used here.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/char/hw_random/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 75fb859..9cfd338 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -147,6 +147,7 @@ static const struct file_operations rng_chrdev_ops = {
 	.owner		= THIS_MODULE,
 	.open		= rng_dev_open,
 	.read		= rng_dev_read,
+	.llseek		= no_llseek,
 };
 
 static struct miscdevice rng_miscdev = {
-- 
1.6.0.6

[patch 20/28] input: Remove BKL from hp_sdc_rtc

[Thomas Gleixner]

[-- Attachment #1: hp-sdc-rtc-remove-bkl.patch --]
[-- Type: text/plain, Size: 979 bytes --]

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/input/misc/hp_sdc_rtc.c |    2 --
 1 file changed, 2 deletions(-)

Index: linux-2.6-tip/drivers/input/misc/hp_sdc_rtc.c
===================================================================
--- linux-2.6-tip.orig/drivers/input/misc/hp_sdc_rtc.c
+++ linux-2.6-tip/drivers/input/misc/hp_sdc_rtc.c
@@ -35,7 +35,6 @@
 
 #include <linux/hp_sdc.h>
 #include <linux/errno.h>
-#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -409,7 +408,6 @@ static unsigned int hp_sdc_rtc_poll(stru
 
 static int hp_sdc_rtc_open(struct inode *inode, struct file *file)
 {
-	cycle_kernel_lock();
         return 0;
 }
 

Re: [patch 20/28] input: Remove BKL from hp_sdc_rtc

[Arnd Bergmann]

On Saturday 10 October 2009, Thomas Gleixner wrote:
> cycle_kernel_lock() was added during the big BKL pushdown. It should
> ensure the serializiation against driver init code. In this case there
> is nothing to serialize. Remove it.

The driver still holds the BKL in its ioctl method, which should be removed
as well if the above is true.

	Arnd <><

Re: [patch 20/28] input: Remove BKL from hp_sdc_rtc

[Thomas Gleixner]

On Sun, 11 Oct 2009, Arnd Bergmann wrote:

> On Saturday 10 October 2009, Thomas Gleixner wrote:
> > cycle_kernel_lock() was added during the big BKL pushdown. It should
> > ensure the serializiation against driver init code. In this case there
> > is nothing to serialize. Remove it.
> 
> The driver still holds the BKL in its ioctl method, which should be removed
> as well if the above is true.

On my list already :)

[tip:bkl/drivers] input: Remove BKL from hp_sdc_rtc

[tip-bot for Thomas Gleixner]

Commit-ID:  d2d23559857e5f34762c61487f8ffb2fa4d7442d
Gitweb:     http://git.kernel.org/tip/d2d23559857e5f34762c61487f8ffb2fa4d7442d
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 12:41:43 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:51 +0200

input: Remove BKL from hp_sdc_rtc

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153349.884891604@linutronix.de>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/input/misc/hp_sdc_rtc.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/hp_sdc_rtc.c b/drivers/input/misc/hp_sdc_rtc.c
index 216a559..4d1aa9a 100644
--- a/drivers/input/misc/hp_sdc_rtc.c
+++ b/drivers/input/misc/hp_sdc_rtc.c
@@ -35,7 +35,6 @@
 
 #include <linux/hp_sdc.h>
 #include <linux/errno.h>
-#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -409,7 +408,6 @@ static unsigned int hp_sdc_rtc_poll(struct file *file, poll_table *wait)
 
 static int hp_sdc_rtc_open(struct inode *inode, struct file *file)
 {
-	cycle_kernel_lock();
         return 0;
 }
 

[patch 21/28] bkl: pushdown BKL locking to do_sysctl()

[Thomas Gleixner]

[-- Attachment #1: push-bkl-to-do-sysctl.patch --]
[-- Type: text/plain, Size: 5655 bytes --]

Push lock/unlock_kernel() into do_sysctl() and remove it from all call
sites of do_sysctl().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>

---
 arch/ia64/ia32/sys_ia32.c         |    2 --
 arch/mips/kernel/linux32.c        |    2 --
 arch/parisc/kernel/sys_parisc32.c |    2 --
 arch/powerpc/kernel/sys_ppc32.c   |    2 --
 arch/s390/kernel/compat_linux.c   |    2 --
 arch/sparc/kernel/sys_sparc32.c   |    2 --
 arch/x86/ia32/sys_ia32.c          |    2 --
 kernel/sysctl.c                   |    6 ++++--
 8 files changed, 4 insertions(+), 16 deletions(-)

Index: linux-2.6-tip/arch/ia64/ia32/sys_ia32.c
===================================================================
--- linux-2.6-tip.orig/arch/ia64/ia32/sys_ia32.c
+++ linux-2.6-tip/arch/ia64/ia32/sys_ia32.c
@@ -1670,10 +1670,8 @@ sys32_sysctl (struct sysctl32 __user *ar
 		return -EFAULT;
 
 	set_fs(KERNEL_DS);
-	lock_kernel();
 	ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *) &oldlen,
 			newvalp, (size_t) a32.newlen);
-	unlock_kernel();
 	set_fs(old_fs);
 
 	if (oldvalp && put_user (oldlen, (int __user *) compat_ptr(a32.oldlenp)))
Index: linux-2.6-tip/arch/mips/kernel/linux32.c
===================================================================
--- linux-2.6-tip.orig/arch/mips/kernel/linux32.c
+++ linux-2.6-tip/arch/mips/kernel/linux32.c
@@ -302,10 +302,8 @@ SYSCALL_DEFINE1(32_sysctl, struct sysctl
 		oldlenp = (size_t __user *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl((int __user *)A(tmp.name), tmp.nlen, (void __user *)A(tmp.oldval),
 			  oldlenp, (void __user *)A(tmp.newval), tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t __user *)addr) ||
Index: linux-2.6-tip/arch/parisc/kernel/sys_parisc32.c
===================================================================
--- linux-2.6-tip.orig/arch/parisc/kernel/sys_parisc32.c
+++ linux-2.6-tip/arch/parisc/kernel/sys_parisc32.c
@@ -137,11 +137,9 @@ asmlinkage long sys32_sysctl(struct __sy
 		oldlenp = (size_t *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl((int __user *)(u64)tmp.name, tmp.nlen,
 			  (void __user *)(u64)tmp.oldval, oldlenp,
 			  (void __user *)(u64)tmp.newval, tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t *)addr)) {
Index: linux-2.6-tip/arch/powerpc/kernel/sys_ppc32.c
===================================================================
--- linux-2.6-tip.orig/arch/powerpc/kernel/sys_ppc32.c
+++ linux-2.6-tip/arch/powerpc/kernel/sys_ppc32.c
@@ -555,11 +555,9 @@ asmlinkage long compat_sys_sysctl(struct
 			return -EFAULT;
 	}
 
-	lock_kernel();
 	error = do_sysctl(compat_ptr(tmp.name), tmp.nlen,
 			  compat_ptr(tmp.oldval), oldlenp,
 			  compat_ptr(tmp.newval), tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, oldlenp) ||
Index: linux-2.6-tip/arch/s390/kernel/compat_linux.c
===================================================================
--- linux-2.6-tip.orig/arch/s390/kernel/compat_linux.c
+++ linux-2.6-tip/arch/s390/kernel/compat_linux.c
@@ -562,10 +562,8 @@ asmlinkage long sys32_sysctl(struct __sy
 		oldlenp = (size_t __user *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl(compat_ptr(tmp.name), tmp.nlen, compat_ptr(tmp.oldval),
 			  oldlenp, compat_ptr(tmp.newval), tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t __user *)addr) ||
Index: linux-2.6-tip/arch/sparc/kernel/sys_sparc32.c
===================================================================
--- linux-2.6-tip.orig/arch/sparc/kernel/sys_sparc32.c
+++ linux-2.6-tip/arch/sparc/kernel/sys_sparc32.c
@@ -627,14 +627,12 @@ asmlinkage long sys32_sysctl(struct __sy
 		oldlenp = (size_t __user *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl((int __user *)(unsigned long) tmp.name,
 			  tmp.nlen,
 			  (void __user *)(unsigned long) tmp.oldval,
 			  oldlenp,
 			  (void __user *)(unsigned long) tmp.newval,
 			  tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t __user *)addr) ||
Index: linux-2.6-tip/arch/x86/ia32/sys_ia32.c
===================================================================
--- linux-2.6-tip.orig/arch/x86/ia32/sys_ia32.c
+++ linux-2.6-tip/arch/x86/ia32/sys_ia32.c
@@ -477,10 +477,8 @@ asmlinkage long sys32_sysctl(struct sysc
 		return -EFAULT;
 
 	set_fs(KERNEL_DS);
-	lock_kernel();
 	ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *)&oldlen,
 			newvalp, (size_t) a32.newlen);
-	unlock_kernel();
 	set_fs(old_fs);
 
 	if (oldvalp && put_user(oldlen, (int __user *)compat_ptr(a32.oldlenp)))
Index: linux-2.6-tip/kernel/sysctl.c
===================================================================
--- linux-2.6-tip.orig/kernel/sysctl.c
+++ linux-2.6-tip/kernel/sysctl.c
@@ -1848,6 +1848,8 @@ int do_sysctl(int __user *name, int nlen
 			return -EFAULT;
 	}
 
+	lock_kernel();
+
 	for (head = sysctl_head_next(NULL); head;
 			head = sysctl_head_next(head)) {
 		error = parse_table(name, nlen, oldval, oldlenp, 
@@ -1858,6 +1860,8 @@ int do_sysctl(int __user *name, int nlen
 			break;
 		}
 	}
+
+	unlock_kernel();
 	return error;
 }
 
@@ -1873,10 +1877,8 @@ SYSCALL_DEFINE1(sysctl, struct __sysctl_
 	if (error)
 		goto out;
 
-	lock_kernel();
 	error = do_sysctl(tmp.name, tmp.nlen, tmp.oldval, tmp.oldlenp,
 			  tmp.newval, tmp.newlen);
-	unlock_kernel();
 out:
 	return error;
 }

Re: [patch 21/28] bkl: pushdown BKL locking to do_sysctl()

[Benjamin Herrenschmidt]

On Sat, 2009-10-10 at 15:37 +0000, Thomas Gleixner wrote:
> plain text document attachment (push-bkl-to-do-sysctl.patch)
> Push lock/unlock_kernel() into do_sysctl() and remove it from all call
> sites of do_sysctl().
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Cc: "David S. Miller" <davem@davemloft.net>

For the powerpc parts,

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> ---
>  arch/ia64/ia32/sys_ia32.c         |    2 --
>  arch/mips/kernel/linux32.c        |    2 --
>  arch/parisc/kernel/sys_parisc32.c |    2 --
>  arch/powerpc/kernel/sys_ppc32.c   |    2 --
>  arch/s390/kernel/compat_linux.c   |    2 --
>  arch/sparc/kernel/sys_sparc32.c   |    2 --
>  arch/x86/ia32/sys_ia32.c          |    2 --
>  kernel/sysctl.c                   |    6 ++++--
>  8 files changed, 4 insertions(+), 16 deletions(-)
> 
> Index: linux-2.6-tip/arch/ia64/ia32/sys_ia32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/ia64/ia32/sys_ia32.c
> +++ linux-2.6-tip/arch/ia64/ia32/sys_ia32.c
> @@ -1670,10 +1670,8 @@ sys32_sysctl (struct sysctl32 __user *ar
>  		return -EFAULT;
>  
>  	set_fs(KERNEL_DS);
> -	lock_kernel();
>  	ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *) &oldlen,
>  			newvalp, (size_t) a32.newlen);
> -	unlock_kernel();
>  	set_fs(old_fs);
>  
>  	if (oldvalp && put_user (oldlen, (int __user *) compat_ptr(a32.oldlenp)))
> Index: linux-2.6-tip/arch/mips/kernel/linux32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/mips/kernel/linux32.c
> +++ linux-2.6-tip/arch/mips/kernel/linux32.c
> @@ -302,10 +302,8 @@ SYSCALL_DEFINE1(32_sysctl, struct sysctl
>  		oldlenp = (size_t __user *)addr;
>  	}
>  
> -	lock_kernel();
>  	error = do_sysctl((int __user *)A(tmp.name), tmp.nlen, (void __user *)A(tmp.oldval),
>  			  oldlenp, (void __user *)A(tmp.newval), tmp.newlen);
> -	unlock_kernel();
>  	if (oldlenp) {
>  		if (!error) {
>  			if (get_user(oldlen, (size_t __user *)addr) ||
> Index: linux-2.6-tip/arch/parisc/kernel/sys_parisc32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/parisc/kernel/sys_parisc32.c
> +++ linux-2.6-tip/arch/parisc/kernel/sys_parisc32.c
> @@ -137,11 +137,9 @@ asmlinkage long sys32_sysctl(struct __sy
>  		oldlenp = (size_t *)addr;
>  	}
>  
> -	lock_kernel();
>  	error = do_sysctl((int __user *)(u64)tmp.name, tmp.nlen,
>  			  (void __user *)(u64)tmp.oldval, oldlenp,
>  			  (void __user *)(u64)tmp.newval, tmp.newlen);
> -	unlock_kernel();
>  	if (oldlenp) {
>  		if (!error) {
>  			if (get_user(oldlen, (size_t *)addr)) {
> Index: linux-2.6-tip/arch/powerpc/kernel/sys_ppc32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/powerpc/kernel/sys_ppc32.c
> +++ linux-2.6-tip/arch/powerpc/kernel/sys_ppc32.c
> @@ -555,11 +555,9 @@ asmlinkage long compat_sys_sysctl(struct
>  			return -EFAULT;
>  	}
>  
> -	lock_kernel();
>  	error = do_sysctl(compat_ptr(tmp.name), tmp.nlen,
>  			  compat_ptr(tmp.oldval), oldlenp,
>  			  compat_ptr(tmp.newval), tmp.newlen);
> -	unlock_kernel();
>  	if (oldlenp) {
>  		if (!error) {
>  			if (get_user(oldlen, oldlenp) ||
> Index: linux-2.6-tip/arch/s390/kernel/compat_linux.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/s390/kernel/compat_linux.c
> +++ linux-2.6-tip/arch/s390/kernel/compat_linux.c
> @@ -562,10 +562,8 @@ asmlinkage long sys32_sysctl(struct __sy
>  		oldlenp = (size_t __user *)addr;
>  	}
>  
> -	lock_kernel();
>  	error = do_sysctl(compat_ptr(tmp.name), tmp.nlen, compat_ptr(tmp.oldval),
>  			  oldlenp, compat_ptr(tmp.newval), tmp.newlen);
> -	unlock_kernel();
>  	if (oldlenp) {
>  		if (!error) {
>  			if (get_user(oldlen, (size_t __user *)addr) ||
> Index: linux-2.6-tip/arch/sparc/kernel/sys_sparc32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/sparc/kernel/sys_sparc32.c
> +++ linux-2.6-tip/arch/sparc/kernel/sys_sparc32.c
> @@ -627,14 +627,12 @@ asmlinkage long sys32_sysctl(struct __sy
>  		oldlenp = (size_t __user *)addr;
>  	}
>  
> -	lock_kernel();
>  	error = do_sysctl((int __user *)(unsigned long) tmp.name,
>  			  tmp.nlen,
>  			  (void __user *)(unsigned long) tmp.oldval,
>  			  oldlenp,
>  			  (void __user *)(unsigned long) tmp.newval,
>  			  tmp.newlen);
> -	unlock_kernel();
>  	if (oldlenp) {
>  		if (!error) {
>  			if (get_user(oldlen, (size_t __user *)addr) ||
> Index: linux-2.6-tip/arch/x86/ia32/sys_ia32.c
> ===================================================================
> --- linux-2.6-tip.orig/arch/x86/ia32/sys_ia32.c
> +++ linux-2.6-tip/arch/x86/ia32/sys_ia32.c
> @@ -477,10 +477,8 @@ asmlinkage long sys32_sysctl(struct sysc
>  		return -EFAULT;
>  
>  	set_fs(KERNEL_DS);
> -	lock_kernel();
>  	ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *)&oldlen,
>  			newvalp, (size_t) a32.newlen);
> -	unlock_kernel();
>  	set_fs(old_fs);
>  
>  	if (oldvalp && put_user(oldlen, (int __user *)compat_ptr(a32.oldlenp)))
> Index: linux-2.6-tip/kernel/sysctl.c
> ===================================================================
> --- linux-2.6-tip.orig/kernel/sysctl.c
> +++ linux-2.6-tip/kernel/sysctl.c
> @@ -1848,6 +1848,8 @@ int do_sysctl(int __user *name, int nlen
>  			return -EFAULT;
>  	}
>  
> +	lock_kernel();
> +
>  	for (head = sysctl_head_next(NULL); head;
>  			head = sysctl_head_next(head)) {
>  		error = parse_table(name, nlen, oldval, oldlenp, 
> @@ -1858,6 +1860,8 @@ int do_sysctl(int __user *name, int nlen
>  			break;
>  		}
>  	}
> +
> +	unlock_kernel();
>  	return error;
>  }
>  
> @@ -1873,10 +1877,8 @@ SYSCALL_DEFINE1(sysctl, struct __sysctl_
>  	if (error)
>  		goto out;
>  
> -	lock_kernel();
>  	error = do_sysctl(tmp.name, tmp.nlen, tmp.oldval, tmp.oldlenp,
>  			  tmp.newval, tmp.newlen);
> -	unlock_kernel();
>  out:
>  	return error;
>  }
> 

[tip:bkl/core] bkl: pushdown BKL locking to do_sysctl()

[tip-bot for Thomas Gleixner]

Commit-ID:  e362106d6dcf6607906eff2260837748b1d6c8d5
Gitweb:     http://git.kernel.org/tip/e362106d6dcf6607906eff2260837748b1d6c8d5
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 15:37:06 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 15:31:41 +0200

bkl: pushdown BKL locking to do_sysctl()

Push lock/unlock_kernel() into do_sysctl() and remove it from all call
sites of do_sysctl().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: "David S. Miller" <davem@davemloft.net>
LKML-Reference: <20091010153349.925243928@linutronix.de>
---
 arch/ia64/ia32/sys_ia32.c         |    2 --
 arch/mips/kernel/linux32.c        |    2 --
 arch/parisc/kernel/sys_parisc32.c |    2 --
 arch/powerpc/kernel/sys_ppc32.c   |    2 --
 arch/s390/kernel/compat_linux.c   |    2 --
 arch/sparc/kernel/sys_sparc32.c   |    2 --
 arch/x86/ia32/sys_ia32.c          |    2 --
 kernel/sysctl.c                   |    6 ++++--
 8 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/ia32/sys_ia32.c b/arch/ia64/ia32/sys_ia32.c
index 625ed8f..c0ed49c 100644
--- a/arch/ia64/ia32/sys_ia32.c
+++ b/arch/ia64/ia32/sys_ia32.c
@@ -1670,10 +1670,8 @@ sys32_sysctl (struct sysctl32 __user *args)
 		return -EFAULT;
 
 	set_fs(KERNEL_DS);
-	lock_kernel();
 	ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *) &oldlen,
 			newvalp, (size_t) a32.newlen);
-	unlock_kernel();
 	set_fs(old_fs);
 
 	if (oldvalp && put_user (oldlen, (int __user *) compat_ptr(a32.oldlenp)))
diff --git a/arch/mips/kernel/linux32.c b/arch/mips/kernel/linux32.c
index 6242bc6..3744608 100644
--- a/arch/mips/kernel/linux32.c
+++ b/arch/mips/kernel/linux32.c
@@ -302,10 +302,8 @@ SYSCALL_DEFINE1(32_sysctl, struct sysctl_args32 __user *, args)
 		oldlenp = (size_t __user *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl((int __user *)A(tmp.name), tmp.nlen, (void __user *)A(tmp.oldval),
 			  oldlenp, (void __user *)A(tmp.newval), tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t __user *)addr) ||
diff --git a/arch/parisc/kernel/sys_parisc32.c b/arch/parisc/kernel/sys_parisc32.c
index 561388b..0d3bff2 100644
--- a/arch/parisc/kernel/sys_parisc32.c
+++ b/arch/parisc/kernel/sys_parisc32.c
@@ -137,11 +137,9 @@ asmlinkage long sys32_sysctl(struct __sysctl_args32 __user *args)
 		oldlenp = (size_t *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl((int __user *)(u64)tmp.name, tmp.nlen,
 			  (void __user *)(u64)tmp.oldval, oldlenp,
 			  (void __user *)(u64)tmp.newval, tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t *)addr)) {
diff --git a/arch/powerpc/kernel/sys_ppc32.c b/arch/powerpc/kernel/sys_ppc32.c
index b97c2d6..934c0f9 100644
--- a/arch/powerpc/kernel/sys_ppc32.c
+++ b/arch/powerpc/kernel/sys_ppc32.c
@@ -555,11 +555,9 @@ asmlinkage long compat_sys_sysctl(struct __sysctl_args32 __user *args)
 			return -EFAULT;
 	}
 
-	lock_kernel();
 	error = do_sysctl(compat_ptr(tmp.name), tmp.nlen,
 			  compat_ptr(tmp.oldval), oldlenp,
 			  compat_ptr(tmp.newval), tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, oldlenp) ||
diff --git a/arch/s390/kernel/compat_linux.c b/arch/s390/kernel/compat_linux.c
index 0debcec..ef9c2c1 100644
--- a/arch/s390/kernel/compat_linux.c
+++ b/arch/s390/kernel/compat_linux.c
@@ -562,10 +562,8 @@ asmlinkage long sys32_sysctl(struct __sysctl_args32 __user *args)
 		oldlenp = (size_t __user *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl(compat_ptr(tmp.name), tmp.nlen, compat_ptr(tmp.oldval),
 			  oldlenp, compat_ptr(tmp.newval), tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t __user *)addr) ||
diff --git a/arch/sparc/kernel/sys_sparc32.c b/arch/sparc/kernel/sys_sparc32.c
index 04e28b2..6266ef0 100644
--- a/arch/sparc/kernel/sys_sparc32.c
+++ b/arch/sparc/kernel/sys_sparc32.c
@@ -627,14 +627,12 @@ asmlinkage long sys32_sysctl(struct __sysctl_args32 __user *args)
 		oldlenp = (size_t __user *)addr;
 	}
 
-	lock_kernel();
 	error = do_sysctl((int __user *)(unsigned long) tmp.name,
 			  tmp.nlen,
 			  (void __user *)(unsigned long) tmp.oldval,
 			  oldlenp,
 			  (void __user *)(unsigned long) tmp.newval,
 			  tmp.newlen);
-	unlock_kernel();
 	if (oldlenp) {
 		if (!error) {
 			if (get_user(oldlen, (size_t __user *)addr) ||
diff --git a/arch/x86/ia32/sys_ia32.c b/arch/x86/ia32/sys_ia32.c
index 9f55271..32a88e4 100644
--- a/arch/x86/ia32/sys_ia32.c
+++ b/arch/x86/ia32/sys_ia32.c
@@ -477,10 +477,8 @@ asmlinkage long sys32_sysctl(struct sysctl_ia32 __user *args32)
 		return -EFAULT;
 
 	set_fs(KERNEL_DS);
-	lock_kernel();
 	ret = do_sysctl(namep, a32.nlen, oldvalp, (size_t __user *)&oldlen,
 			newvalp, (size_t) a32.newlen);
-	unlock_kernel();
 	set_fs(old_fs);
 
 	if (oldvalp && put_user(oldlen, (int __user *)compat_ptr(a32.oldlenp)))
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 0d949c5..72040e5 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1848,6 +1848,8 @@ int do_sysctl(int __user *name, int nlen, void __user *oldval, size_t __user *ol
 			return -EFAULT;
 	}
 
+	lock_kernel();
+
 	for (head = sysctl_head_next(NULL); head;
 			head = sysctl_head_next(head)) {
 		error = parse_table(name, nlen, oldval, oldlenp, 
@@ -1858,6 +1860,8 @@ int do_sysctl(int __user *name, int nlen, void __user *oldval, size_t __user *ol
 			break;
 		}
 	}
+
+	unlock_kernel();
 	return error;
 }
 
@@ -1873,10 +1877,8 @@ SYSCALL_DEFINE1(sysctl, struct __sysctl_args __user *, args)
 	if (error)
 		goto out;
 
-	lock_kernel();
 	error = do_sysctl(tmp.name, tmp.nlen, tmp.oldval, tmp.oldlenp,
 			  tmp.newval, tmp.newlen);
-	unlock_kernel();
 out:
 	return error;
 }

[patch 22/28] macintosh: Remove BKL from ans-lcd

[Thomas Gleixner]

[-- Attachment #1: drivers-mac-ans-lcd-remove-bkl.patch --]
[-- Type: text/plain, Size: 3563 bytes --]

The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
the BKL pushdown and it still uses the locked ioctl.

The BKL serialization in this driver is more than obscure and
definitely does not cover all possible corner cases. Protect the
access to the hardware with a local mutex and get rid of BKL and
locked ioctl.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@ozlabs.org
---
 drivers/macintosh/ans-lcd.c |   45 +++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

Index: linux-2.6-tip/drivers/macintosh/ans-lcd.c
===================================================================
--- linux-2.6-tip.orig/drivers/macintosh/ans-lcd.c
+++ linux-2.6-tip/drivers/macintosh/ans-lcd.c
@@ -3,7 +3,6 @@
  */
 
 #include <linux/types.h>
-#include <linux/smp_lock.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
@@ -26,6 +25,7 @@
 static unsigned long anslcd_short_delay = 80;
 static unsigned long anslcd_long_delay = 3280;
 static volatile unsigned char __iomem *anslcd_ptr;
+static DEFINE_MUTEX(anslcd_mutex);
 
 #undef DEBUG
 
@@ -65,26 +65,31 @@ anslcd_write( struct file * file, const 
 
 	if (!access_ok(VERIFY_READ, buf, count))
 		return -EFAULT;
+
+	mutex_lock(&anslcd_mutex);
 	for ( i = *ppos; count > 0; ++i, ++p, --count ) 
 	{
 		char c;
 		__get_user(c, p);
 		anslcd_write_byte_data( c );
 	}
+	mutex_unlock(&anslcd_mutex);
 	*ppos = i;
 	return p - buf;
 }
 
-static int
-anslcd_ioctl( struct inode * inode, struct file * file,
-				unsigned int cmd, unsigned long arg )
+static long
+anslcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	char ch, __user *temp;
+	long ret = 0;
 
 #ifdef DEBUG
 	printk(KERN_DEBUG "LCD: ioctl(%d,%d)\n",cmd,arg);
 #endif
 
+	mutex_lock(&anslcd_mutex);
+
 	switch ( cmd )
 	{
 	case ANSLCD_CLEAR:
@@ -93,7 +98,7 @@ anslcd_ioctl( struct inode * inode, stru
 		anslcd_write_byte_ctrl ( 0x06 );
 		anslcd_write_byte_ctrl ( 0x01 );
 		anslcd_write_byte_ctrl ( 0x02 );
-		return 0;
+		break;
 	case ANSLCD_SENDCTRL:
 		temp = (char __user *) arg;
 		__get_user(ch, temp);
@@ -101,33 +106,37 @@ anslcd_ioctl( struct inode * inode, stru
 			anslcd_write_byte_ctrl ( ch );
 			__get_user(ch, temp);
 		}
-		return 0;
+		break;
 	case ANSLCD_SETSHORTDELAY:
 		if (!capable(CAP_SYS_ADMIN))
-			return -EACCES;
-		anslcd_short_delay=arg;
-		return 0;
+			ret =-EACCES;
+		else
+			anslcd_short_delay=arg;
+		break;
 	case ANSLCD_SETLONGDELAY:
 		if (!capable(CAP_SYS_ADMIN))
-			return -EACCES;
-		anslcd_long_delay=arg;
-		return 0;
+			ret = -EACCES;
+		else
+			anslcd_long_delay=arg;
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+
+	mutex_unlock(&anslcd_mutex);
+	return ret;
 }
 
 static int
 anslcd_open( struct inode * inode, struct file * file )
 {
-	cycle_kernel_lock();
 	return 0;
 }
 
 const struct file_operations anslcd_fops = {
-	.write	= anslcd_write,
-	.ioctl	= anslcd_ioctl,
-	.open	= anslcd_open,
+	.write		= anslcd_write,
+	.unlocked_ioctl	= anslcd_ioctl,
+	.open		= anslcd_open,
 };
 
 static struct miscdevice anslcd_dev = {
@@ -168,6 +177,7 @@ anslcd_init(void)
 	printk(KERN_DEBUG "LCD: init\n");
 #endif
 
+	mutex_lock(&anslcd_mutex);
 	anslcd_write_byte_ctrl ( 0x38 );
 	anslcd_write_byte_ctrl ( 0x0c );
 	anslcd_write_byte_ctrl ( 0x06 );
@@ -176,6 +186,7 @@ anslcd_init(void)
 	for(a=0;a<80;a++) {
 		anslcd_write_byte_data(anslcd_logo[a]);
 	}
+	mutex_unlock(&anslcd_mutex);
 	return 0;
 }
 

[patch 22/28] macintosh: Remove BKL from ans-lcd

[Thomas Gleixner]

The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
the BKL pushdown and it still uses the locked ioctl.

The BKL serialization in this driver is more than obscure and
definitely does not cover all possible corner cases. Protect the
access to the hardware with a local mutex and get rid of BKL and
locked ioctl.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@ozlabs.org
---
 drivers/macintosh/ans-lcd.c |   45 +++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

Index: linux-2.6-tip/drivers/macintosh/ans-lcd.c
===================================================================
--- linux-2.6-tip.orig/drivers/macintosh/ans-lcd.c
+++ linux-2.6-tip/drivers/macintosh/ans-lcd.c
@@ -3,7 +3,6 @@
  */
 
 #include <linux/types.h>
-#include <linux/smp_lock.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
@@ -26,6 +25,7 @@
 static unsigned long anslcd_short_delay = 80;
 static unsigned long anslcd_long_delay = 3280;
 static volatile unsigned char __iomem *anslcd_ptr;
+static DEFINE_MUTEX(anslcd_mutex);
 
 #undef DEBUG
 
@@ -65,26 +65,31 @@ anslcd_write( struct file * file, const 
 
 	if (!access_ok(VERIFY_READ, buf, count))
 		return -EFAULT;
+
+	mutex_lock(&anslcd_mutex);
 	for ( i = *ppos; count > 0; ++i, ++p, --count ) 
 	{
 		char c;
 		__get_user(c, p);
 		anslcd_write_byte_data( c );
 	}
+	mutex_unlock(&anslcd_mutex);
 	*ppos = i;
 	return p - buf;
 }
 
-static int
-anslcd_ioctl( struct inode * inode, struct file * file,
-				unsigned int cmd, unsigned long arg )
+static long
+anslcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	char ch, __user *temp;
+	long ret = 0;
 
 #ifdef DEBUG
 	printk(KERN_DEBUG "LCD: ioctl(%d,%d)\n",cmd,arg);
 #endif
 
+	mutex_lock(&anslcd_mutex);
+
 	switch ( cmd )
 	{
 	case ANSLCD_CLEAR:
@@ -93,7 +98,7 @@ anslcd_ioctl( struct inode * inode, stru
 		anslcd_write_byte_ctrl ( 0x06 );
 		anslcd_write_byte_ctrl ( 0x01 );
 		anslcd_write_byte_ctrl ( 0x02 );
-		return 0;
+		break;
 	case ANSLCD_SENDCTRL:
 		temp = (char __user *) arg;
 		__get_user(ch, temp);
@@ -101,33 +106,37 @@ anslcd_ioctl( struct inode * inode, stru
 			anslcd_write_byte_ctrl ( ch );
 			__get_user(ch, temp);
 		}
-		return 0;
+		break;
 	case ANSLCD_SETSHORTDELAY:
 		if (!capable(CAP_SYS_ADMIN))
-			return -EACCES;
-		anslcd_short_delay=arg;
-		return 0;
+			ret =-EACCES;
+		else
+			anslcd_short_delay=arg;
+		break;
 	case ANSLCD_SETLONGDELAY:
 		if (!capable(CAP_SYS_ADMIN))
-			return -EACCES;
-		anslcd_long_delay=arg;
-		return 0;
+			ret = -EACCES;
+		else
+			anslcd_long_delay=arg;
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+
+	mutex_unlock(&anslcd_mutex);
+	return ret;
 }
 
 static int
 anslcd_open( struct inode * inode, struct file * file )
 {
-	cycle_kernel_lock();
 	return 0;
 }
 
 const struct file_operations anslcd_fops = {
-	.write	= anslcd_write,
-	.ioctl	= anslcd_ioctl,
-	.open	= anslcd_open,
+	.write		= anslcd_write,
+	.unlocked_ioctl	= anslcd_ioctl,
+	.open		= anslcd_open,
 };
 
 static struct miscdevice anslcd_dev = {
@@ -168,6 +177,7 @@ anslcd_init(void)
 	printk(KERN_DEBUG "LCD: init\n");
 #endif
 
+	mutex_lock(&anslcd_mutex);
 	anslcd_write_byte_ctrl ( 0x38 );
 	anslcd_write_byte_ctrl ( 0x0c );
 	anslcd_write_byte_ctrl ( 0x06 );
@@ -176,6 +186,7 @@ anslcd_init(void)
 	for(a=0;a<80;a++) {
 		anslcd_write_byte_data(anslcd_logo[a]);
 	}
+	mutex_unlock(&anslcd_mutex);
 	return 0;
 }
 

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[John Kacur]

On Sat, 10 Oct 2009, Thomas Gleixner wrote:

> The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
> the BKL pushdown and it still uses the locked ioctl.
> 
> The BKL serialization in this driver is more than obscure and
> definitely does not cover all possible corner cases. Protect the
> access to the hardware with a local mutex and get rid of BKL and
> locked ioctl.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@ozlabs.org
> ---
>  drivers/macintosh/ans-lcd.c |   45 +++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> Index: linux-2.6-tip/drivers/macintosh/ans-lcd.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/macintosh/ans-lcd.c
> +++ linux-2.6-tip/drivers/macintosh/ans-lcd.c
> @@ -3,7 +3,6 @@
>   */
>  
>  #include <linux/types.h>
> -#include <linux/smp_lock.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
>  #include <linux/miscdevice.h>
> @@ -26,6 +25,7 @@
>  static unsigned long anslcd_short_delay = 80;
>  static unsigned long anslcd_long_delay = 3280;
>  static volatile unsigned char __iomem *anslcd_ptr;
> +static DEFINE_MUTEX(anslcd_mutex);
>  
>  #undef DEBUG
>  
> @@ -65,26 +65,31 @@ anslcd_write( struct file * file, const 
>  
>  	if (!access_ok(VERIFY_READ, buf, count))
>  		return -EFAULT;
> +
> +	mutex_lock(&anslcd_mutex);
>  	for ( i = *ppos; count > 0; ++i, ++p, --count ) 
>  	{
>  		char c;
>  		__get_user(c, p);
>  		anslcd_write_byte_data( c );
>  	}
> +	mutex_unlock(&anslcd_mutex);
>  	*ppos = i;
>  	return p - buf;
>  }
>  
> -static int
> -anslcd_ioctl( struct inode * inode, struct file * file,
> -				unsigned int cmd, unsigned long arg )
> +static long
> +anslcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	char ch, __user *temp;
> +	long ret = 0;
>  
>  #ifdef DEBUG
>  	printk(KERN_DEBUG "LCD: ioctl(%d,%d)\n",cmd,arg);
>  #endif
>  
> +	mutex_lock(&anslcd_mutex);
> +
>  	switch ( cmd )
>  	{
>  	case ANSLCD_CLEAR:
> @@ -93,7 +98,7 @@ anslcd_ioctl( struct inode * inode, stru
>  		anslcd_write_byte_ctrl ( 0x06 );
>  		anslcd_write_byte_ctrl ( 0x01 );
>  		anslcd_write_byte_ctrl ( 0x02 );
> -		return 0;
> +		break;
>  	case ANSLCD_SENDCTRL:
>  		temp = (char __user *) arg;
>  		__get_user(ch, temp);
> @@ -101,33 +106,37 @@ anslcd_ioctl( struct inode * inode, stru
>  			anslcd_write_byte_ctrl ( ch );
>  			__get_user(ch, temp);
>  		}
> -		return 0;
> +		break;
>  	case ANSLCD_SETSHORTDELAY:
>  		if (!capable(CAP_SYS_ADMIN))
> -			return -EACCES;
> -		anslcd_short_delay=arg;
> -		return 0;
> +			ret =-EACCES;
> +		else
> +			anslcd_short_delay=arg;
> +		break;
>  	case ANSLCD_SETLONGDELAY:
>  		if (!capable(CAP_SYS_ADMIN))
> -			return -EACCES;
> -		anslcd_long_delay=arg;
> -		return 0;
> +			ret = -EACCES;
> +		else
> +			anslcd_long_delay=arg;
> +		break;
>  	default:
> -		return -EINVAL;
> +		ret = -EINVAL;
>  	}
> +
> +	mutex_unlock(&anslcd_mutex);
> +	return ret;
>  }
>  
>  static int
>  anslcd_open( struct inode * inode, struct file * file )
>  {
> -	cycle_kernel_lock();
>  	return 0;
>  }
>  
>  const struct file_operations anslcd_fops = {
> -	.write	= anslcd_write,
> -	.ioctl	= anslcd_ioctl,
> -	.open	= anslcd_open,
> +	.write		= anslcd_write,
> +	.unlocked_ioctl	= anslcd_ioctl,
> +	.open		= anslcd_open,
>  };
>  
>  static struct miscdevice anslcd_dev = {
> @@ -168,6 +177,7 @@ anslcd_init(void)
>  	printk(KERN_DEBUG "LCD: init\n");
>  #endif
>  
> +	mutex_lock(&anslcd_mutex);
>  	anslcd_write_byte_ctrl ( 0x38 );
>  	anslcd_write_byte_ctrl ( 0x0c );
>  	anslcd_write_byte_ctrl ( 0x06 );
> @@ -176,6 +186,7 @@ anslcd_init(void)
>  	for(a=0;a<80;a++) {
>  		anslcd_write_byte_data(anslcd_logo[a]);
>  	}
> +	mutex_unlock(&anslcd_mutex);
>  	return 0;
>  }
>  
> 
> 
> 

There were 4 checkpatch errors on this patch, all of the type
ERROR: spaces required around that '=' (ctx:WxO)
#1466: FILE: drivers/macintosh/ans-lcd.c:112:
+			ret =-EACCES;

Cheers

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[John Kacur]

On Sat, 10 Oct 2009, Thomas Gleixner wrote:

> The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
> the BKL pushdown and it still uses the locked ioctl.
> 
> The BKL serialization in this driver is more than obscure and
> definitely does not cover all possible corner cases. Protect the
> access to the hardware with a local mutex and get rid of BKL and
> locked ioctl.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@ozlabs.org
> ---
>  drivers/macintosh/ans-lcd.c |   45 +++++++++++++++++++++++++++-----------------
>  1 file changed, 28 insertions(+), 17 deletions(-)
> 
> Index: linux-2.6-tip/drivers/macintosh/ans-lcd.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/macintosh/ans-lcd.c
> +++ linux-2.6-tip/drivers/macintosh/ans-lcd.c
> @@ -3,7 +3,6 @@
>   */
>  
>  #include <linux/types.h>
> -#include <linux/smp_lock.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>
>  #include <linux/miscdevice.h>
> @@ -26,6 +25,7 @@
>  static unsigned long anslcd_short_delay = 80;
>  static unsigned long anslcd_long_delay = 3280;
>  static volatile unsigned char __iomem *anslcd_ptr;
> +static DEFINE_MUTEX(anslcd_mutex);
>  
>  #undef DEBUG
>  
> @@ -65,26 +65,31 @@ anslcd_write( struct file * file, const 
>  
>  	if (!access_ok(VERIFY_READ, buf, count))
>  		return -EFAULT;
> +
> +	mutex_lock(&anslcd_mutex);
>  	for ( i = *ppos; count > 0; ++i, ++p, --count ) 
>  	{
>  		char c;
>  		__get_user(c, p);
>  		anslcd_write_byte_data( c );
>  	}
> +	mutex_unlock(&anslcd_mutex);
>  	*ppos = i;
>  	return p - buf;
>  }
>  
> -static int
> -anslcd_ioctl( struct inode * inode, struct file * file,
> -				unsigned int cmd, unsigned long arg )
> +static long
> +anslcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	char ch, __user *temp;
> +	long ret = 0;
>  
>  #ifdef DEBUG
>  	printk(KERN_DEBUG "LCD: ioctl(%d,%d)\n",cmd,arg);
>  #endif
>  
> +	mutex_lock(&anslcd_mutex);
> +
>  	switch ( cmd )
>  	{
>  	case ANSLCD_CLEAR:
> @@ -93,7 +98,7 @@ anslcd_ioctl( struct inode * inode, stru
>  		anslcd_write_byte_ctrl ( 0x06 );
>  		anslcd_write_byte_ctrl ( 0x01 );
>  		anslcd_write_byte_ctrl ( 0x02 );
> -		return 0;
> +		break;
>  	case ANSLCD_SENDCTRL:
>  		temp = (char __user *) arg;
>  		__get_user(ch, temp);
> @@ -101,33 +106,37 @@ anslcd_ioctl( struct inode * inode, stru
>  			anslcd_write_byte_ctrl ( ch );
>  			__get_user(ch, temp);
>  		}
> -		return 0;
> +		break;
>  	case ANSLCD_SETSHORTDELAY:
>  		if (!capable(CAP_SYS_ADMIN))
> -			return -EACCES;
> -		anslcd_short_delay=arg;
> -		return 0;
> +			ret =-EACCES;
> +		else
> +			anslcd_short_delay=arg;
> +		break;
>  	case ANSLCD_SETLONGDELAY:
>  		if (!capable(CAP_SYS_ADMIN))
> -			return -EACCES;
> -		anslcd_long_delay=arg;
> -		return 0;
> +			ret = -EACCES;
> +		else
> +			anslcd_long_delay=arg;
> +		break;
>  	default:
> -		return -EINVAL;
> +		ret = -EINVAL;
>  	}
> +
> +	mutex_unlock(&anslcd_mutex);
> +	return ret;
>  }
>  
>  static int
>  anslcd_open( struct inode * inode, struct file * file )
>  {
> -	cycle_kernel_lock();
>  	return 0;
>  }
>  
>  const struct file_operations anslcd_fops = {
> -	.write	= anslcd_write,
> -	.ioctl	= anslcd_ioctl,
> -	.open	= anslcd_open,
> +	.write		= anslcd_write,
> +	.unlocked_ioctl	= anslcd_ioctl,
> +	.open		= anslcd_open,
>  };
>  
>  static struct miscdevice anslcd_dev = {
> @@ -168,6 +177,7 @@ anslcd_init(void)
>  	printk(KERN_DEBUG "LCD: init\n");
>  #endif
>  
> +	mutex_lock(&anslcd_mutex);
>  	anslcd_write_byte_ctrl ( 0x38 );
>  	anslcd_write_byte_ctrl ( 0x0c );
>  	anslcd_write_byte_ctrl ( 0x06 );
> @@ -176,6 +186,7 @@ anslcd_init(void)
>  	for(a=0;a<80;a++) {
>  		anslcd_write_byte_data(anslcd_logo[a]);
>  	}
> +	mutex_unlock(&anslcd_mutex);
>  	return 0;
>  }
>  
> 
> 
> 

There were 4 checkpatch errors on this patch, all of the type
ERROR: spaces required around that '=' (ctx:WxO)
#1466: FILE: drivers/macintosh/ans-lcd.c:112:
+			ret =-EACCES;

Cheers

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[Alan Cox]

> There were 4 checkpatch errors on this patch, all of the type
> ERROR: spaces required around that '=' (ctx:WxO)
> #1466: FILE: drivers/macintosh/ans-lcd.c:112:
> +			ret =-EACCES;

Here's a suggestion. If a few spaces bug you that much then instead of
complaining about it and posting checkpatch results deal with the file
itself. 

Wait until the patch goes in and send a follow up patch that fixes up the
file to fit codingstyle. There's no point whining about the bits a patch
touches when the file wasn't in that format before, but if you've got
nothing better to do then doing a pass over the whole file *is* useful.

(Plus it gets a patch to your name ;))

Checkpatch whines on files that simple don't follow style are usually
best ignored because they make the file formatting less internally
consistent.

Alan

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[Alan Cox]

> There were 4 checkpatch errors on this patch, all of the type
> ERROR: spaces required around that '=' (ctx:WxO)
> #1466: FILE: drivers/macintosh/ans-lcd.c:112:
> +			ret =-EACCES;

Here's a suggestion. If a few spaces bug you that much then instead of
complaining about it and posting checkpatch results deal with the file
itself. 

Wait until the patch goes in and send a follow up patch that fixes up the
file to fit codingstyle. There's no point whining about the bits a patch
touches when the file wasn't in that format before, but if you've got
nothing better to do then doing a pass over the whole file *is* useful.

(Plus it gets a patch to your name ;))

Checkpatch whines on files that simple don't follow style are usually
best ignored because they make the file formatting less internally
consistent.

Alan

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[John Kacur]

On Sun, 11 Oct 2009, Alan Cox wrote:

> > There were 4 checkpatch errors on this patch, all of the type
> > ERROR: spaces required around that '=' (ctx:WxO)
> > #1466: FILE: drivers/macintosh/ans-lcd.c:112:
> > +			ret =-EACCES;
> 
> Here's a suggestion. If a few spaces bug you that much then instead of
> complaining about it and posting checkpatch results deal with the file
> itself. 
> 
> Wait until the patch goes in and send a follow up patch that fixes up the
> file to fit codingstyle. There's no point whining about the bits a patch
> touches when the file wasn't in that format before, but if you've got
> nothing better to do then doing a pass over the whole file *is* useful.
> 
> (Plus it gets a patch to your name ;))
> 
> Checkpatch whines on files that simple don't follow style are usually
> best ignored because they make the file formatting less internally
> consistent.
> 
Thanks Alan, I was sincerely debatting whether to send this because I know 
that checkpatch can be annoying - but on the other hand, I thought it 
prudent to run it since I was claiming to have reviewed all of those 
patches.

I like your suggestion though - next time, I won't send the mail, since 
since the folks submitting these patches are more than capable of checking 
that kind of thing themselves, and if I feel it's important enough, I'll 
follow up with a trivial style patch.

Cheers!

John

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[John Kacur]

On Sun, 11 Oct 2009, Alan Cox wrote:

> > There were 4 checkpatch errors on this patch, all of the type
> > ERROR: spaces required around that '=' (ctx:WxO)
> > #1466: FILE: drivers/macintosh/ans-lcd.c:112:
> > +			ret =-EACCES;
> 
> Here's a suggestion. If a few spaces bug you that much then instead of
> complaining about it and posting checkpatch results deal with the file
> itself. 
> 
> Wait until the patch goes in and send a follow up patch that fixes up the
> file to fit codingstyle. There's no point whining about the bits a patch
> touches when the file wasn't in that format before, but if you've got
> nothing better to do then doing a pass over the whole file *is* useful.
> 
> (Plus it gets a patch to your name ;))
> 
> Checkpatch whines on files that simple don't follow style are usually
> best ignored because they make the file formatting less internally
> consistent.
> 
Thanks Alan, I was sincerely debatting whether to send this because I know 
that checkpatch can be annoying - but on the other hand, I thought it 
prudent to run it since I was claiming to have reviewed all of those 
patches.

I like your suggestion though - next time, I won't send the mail, since 
since the folks submitting these patches are more than capable of checking 
that kind of thing themselves, and if I feel it's important enough, I'll 
follow up with a trivial style patch.

Cheers!

John

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[Benjamin Herrenschmidt]

On Sat, 2009-10-10 at 15:37 +0000, Thomas Gleixner wrote:
> plain text document attachment (drivers-mac-ans-lcd-remove-bkl.patch)
> The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
> the BKL pushdown and it still uses the locked ioctl.
> 
> The BKL serialization in this driver is more than obscure and
> definitely does not cover all possible corner cases. Protect the
> access to the hardware with a local mutex and get rid of BKL and
> locked ioctl.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@ozlabs.org
> ---

While I -do- have an ANS ... it's rusting in the back of my garage and I
really don't have time nor space to set it up and get it back to booting
shape :-) (It's a pretty huge thing)

Patch looks good, so if it builds...

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Cheers,
Ben.

Re: [patch 22/28] macintosh: Remove BKL from ans-lcd

[Benjamin Herrenschmidt]

On Sat, 2009-10-10 at 15:37 +0000, Thomas Gleixner wrote:
> plain text document attachment (drivers-mac-ans-lcd-remove-bkl.patch)
> The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
> the BKL pushdown and it still uses the locked ioctl.
> 
> The BKL serialization in this driver is more than obscure and
> definitely does not cover all possible corner cases. Protect the
> access to the hardware with a local mutex and get rid of BKL and
> locked ioctl.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@ozlabs.org
> ---

While I -do- have an ANS ... it's rusting in the back of my garage and I
really don't have time nor space to set it up and get it back to booting
shape :-) (It's a pretty huge thing)

Patch looks good, so if it builds...

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Cheers,
Ben.

[tip:bkl/drivers] macintosh: Remove BKL from ans-lcd

[tip-bot for Thomas Gleixner]

Commit-ID:  95fdac73725c15072d068ac7f131958cf5c324e4
Gitweb:     http://git.kernel.org/tip/95fdac73725c15072d068ac7f131958cf5c324e4
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 13:38:57 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:50 +0200

macintosh: Remove BKL from ans-lcd

The ans-lcd driver got the cycle_kernel_lock() in anslcd_open() from
the BKL pushdown and it still uses the locked ioctl.

The BKL serialization in this driver is more than obscure and
definitely does not cover all possible corner cases. Protect the
access to the hardware with a local mutex and get rid of BKL and
locked ioctl.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153349.966159859@linutronix.de>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/macintosh/ans-lcd.c |   45 ++++++++++++++++++++++++++----------------
 1 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
index 6a82218..a3d25da 100644
--- a/drivers/macintosh/ans-lcd.c
+++ b/drivers/macintosh/ans-lcd.c
@@ -3,7 +3,6 @@
  */
 
 #include <linux/types.h>
-#include <linux/smp_lock.h>
 #include <linux/errno.h>
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
@@ -26,6 +25,7 @@
 static unsigned long anslcd_short_delay = 80;
 static unsigned long anslcd_long_delay = 3280;
 static volatile unsigned char __iomem *anslcd_ptr;
+static DEFINE_MUTEX(anslcd_mutex);
 
 #undef DEBUG
 
@@ -65,26 +65,31 @@ anslcd_write( struct file * file, const char __user * buf,
 
 	if (!access_ok(VERIFY_READ, buf, count))
 		return -EFAULT;
+
+	mutex_lock(&anslcd_mutex);
 	for ( i = *ppos; count > 0; ++i, ++p, --count ) 
 	{
 		char c;
 		__get_user(c, p);
 		anslcd_write_byte_data( c );
 	}
+	mutex_unlock(&anslcd_mutex);
 	*ppos = i;
 	return p - buf;
 }
 
-static int
-anslcd_ioctl( struct inode * inode, struct file * file,
-				unsigned int cmd, unsigned long arg )
+static long
+anslcd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	char ch, __user *temp;
+	long ret = 0;
 
 #ifdef DEBUG
 	printk(KERN_DEBUG "LCD: ioctl(%d,%d)\n",cmd,arg);
 #endif
 
+	mutex_lock(&anslcd_mutex);
+
 	switch ( cmd )
 	{
 	case ANSLCD_CLEAR:
@@ -93,7 +98,7 @@ anslcd_ioctl( struct inode * inode, struct file * file,
 		anslcd_write_byte_ctrl ( 0x06 );
 		anslcd_write_byte_ctrl ( 0x01 );
 		anslcd_write_byte_ctrl ( 0x02 );
-		return 0;
+		break;
 	case ANSLCD_SENDCTRL:
 		temp = (char __user *) arg;
 		__get_user(ch, temp);
@@ -101,33 +106,37 @@ anslcd_ioctl( struct inode * inode, struct file * file,
 			anslcd_write_byte_ctrl ( ch );
 			__get_user(ch, temp);
 		}
-		return 0;
+		break;
 	case ANSLCD_SETSHORTDELAY:
 		if (!capable(CAP_SYS_ADMIN))
-			return -EACCES;
-		anslcd_short_delay=arg;
-		return 0;
+			ret =-EACCES;
+		else
+			anslcd_short_delay=arg;
+		break;
 	case ANSLCD_SETLONGDELAY:
 		if (!capable(CAP_SYS_ADMIN))
-			return -EACCES;
-		anslcd_long_delay=arg;
-		return 0;
+			ret = -EACCES;
+		else
+			anslcd_long_delay=arg;
+		break;
 	default:
-		return -EINVAL;
+		ret = -EINVAL;
 	}
+
+	mutex_unlock(&anslcd_mutex);
+	return ret;
 }
 
 static int
 anslcd_open( struct inode * inode, struct file * file )
 {
-	cycle_kernel_lock();
 	return 0;
 }
 
 const struct file_operations anslcd_fops = {
-	.write	= anslcd_write,
-	.ioctl	= anslcd_ioctl,
-	.open	= anslcd_open,
+	.write		= anslcd_write,
+	.unlocked_ioctl	= anslcd_ioctl,
+	.open		= anslcd_open,
 };
 
 static struct miscdevice anslcd_dev = {
@@ -168,6 +177,7 @@ anslcd_init(void)
 	printk(KERN_DEBUG "LCD: init\n");
 #endif
 
+	mutex_lock(&anslcd_mutex);
 	anslcd_write_byte_ctrl ( 0x38 );
 	anslcd_write_byte_ctrl ( 0x0c );
 	anslcd_write_byte_ctrl ( 0x06 );
@@ -176,6 +186,7 @@ anslcd_init(void)
 	for(a=0;a<80;a++) {
 		anslcd_write_byte_data(anslcd_logo[a]);
 	}
+	mutex_unlock(&anslcd_mutex);
 	return 0;
 }
 

[PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[John Kacur]

>From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Wed, 21 Oct 2009 23:01:12 +0200
Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

Now that we've removed the BKL here, let's explicitly set lleek to no_llseek

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/macintosh/ans-lcd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
index 4ae8ec9..a1a1bde 100644
--- a/drivers/macintosh/ans-lcd.c
+++ b/drivers/macintosh/ans-lcd.c
@@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
 	.write		= anslcd_write,
 	.unlocked_ioctl	= anslcd_ioctl,
 	.open		= anslcd_open,
+	.llseedk	= no_llseek,
 };
 
 static struct miscdevice anslcd_dev = {
-- 
1.6.0.6

[PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[John Kacur]

>From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Wed, 21 Oct 2009 23:01:12 +0200
Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

Now that we've removed the BKL here, let's explicitly set lleek to no_llseek

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/macintosh/ans-lcd.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
index 4ae8ec9..a1a1bde 100644
--- a/drivers/macintosh/ans-lcd.c
+++ b/drivers/macintosh/ans-lcd.c
@@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
 	.write		= anslcd_write,
 	.unlocked_ioctl	= anslcd_ioctl,
 	.open		= anslcd_open,
+	.llseedk	= no_llseek,
 };
 
 static struct miscdevice anslcd_dev = {
-- 
1.6.0.6

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:07:18PM +0200, John Kacur wrote:
> From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Wed, 21 Oct 2009 23:01:12 +0200
> Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd
> 
> Now that we've removed the BKL here, let's explicitly set lleek to no_llseek
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  drivers/macintosh/ans-lcd.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
> index 4ae8ec9..a1a1bde 100644
> --- a/drivers/macintosh/ans-lcd.c
> +++ b/drivers/macintosh/ans-lcd.c
> @@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
>  	.write		= anslcd_write,
>  	.unlocked_ioctl	= anslcd_ioctl,
>  	.open		= anslcd_open,
> +	.llseedk	= no_llseek,


llseedk? :)


Should we better pushdown default_llseek to every to every
file operations that don't implement llseek?
I don't know how many of them don't implement llseek() though.

That said we can't continue anymore with this default attribution
of default_llseek() on new fops.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:07:18PM +0200, John Kacur wrote:
> From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Wed, 21 Oct 2009 23:01:12 +0200
> Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd
> 
> Now that we've removed the BKL here, let's explicitly set lleek to no_llseek
> 
> Signed-off-by: John Kacur <jkacur@redhat.com>
> ---
>  drivers/macintosh/ans-lcd.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
> index 4ae8ec9..a1a1bde 100644
> --- a/drivers/macintosh/ans-lcd.c
> +++ b/drivers/macintosh/ans-lcd.c
> @@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
>  	.write		= anslcd_write,
>  	.unlocked_ioctl	= anslcd_ioctl,
>  	.open		= anslcd_open,
> +	.llseedk	= no_llseek,


llseedk? :)


Should we better pushdown default_llseek to every to every
file operations that don't implement llseek?
I don't know how many of them don't implement llseek() though.

That said we can't continue anymore with this default attribution
of default_llseek() on new fops.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[John Kacur]

On Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Wed, Oct 21, 2009 at 11:07:18PM +0200, John Kacur wrote:
> > From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Wed, 21 Oct 2009 23:01:12 +0200
> > Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd
> > 
> > Now that we've removed the BKL here, let's explicitly set lleek to no_llseek
> > 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> > ---
> >  drivers/macintosh/ans-lcd.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
> > index 4ae8ec9..a1a1bde 100644
> > --- a/drivers/macintosh/ans-lcd.c
> > +++ b/drivers/macintosh/ans-lcd.c
> > @@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
> >  	.write		= anslcd_write,
> >  	.unlocked_ioctl	= anslcd_ioctl,
> >  	.open		= anslcd_open,
> > +	.llseedk	= no_llseek,
> 
> 
> llseedk? :)
> 
> 
> Should we better pushdown default_llseek to every to every
> file operations that don't implement llseek?
> I don't know how many of them don't implement llseek() though.
> 
> That said we can't continue anymore with this default attribution
> of default_llseek() on new fops.
> 

If you don't explicitly set it to no_llseek, you automatically get the
default_llseek, which uses the BKL. So if your driver doesn't need it, it 
is best to explicitly set it to no_llseek.

There is also a generic_file_llseek_unlocked, somewhat analogous to the 
unlocked_ioctls that you can use if you don't need to provide a full 
llseek yourself.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[John Kacur]

On Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Wed, Oct 21, 2009 at 11:07:18PM +0200, John Kacur wrote:
> > From 0c2b412cdccf73bdeb19bb866bfe556942eaeca2 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Wed, 21 Oct 2009 23:01:12 +0200
> > Subject: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd
> > 
> > Now that we've removed the BKL here, let's explicitly set lleek to no_llseek
> > 
> > Signed-off-by: John Kacur <jkacur@redhat.com>
> > ---
> >  drivers/macintosh/ans-lcd.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/macintosh/ans-lcd.c b/drivers/macintosh/ans-lcd.c
> > index 4ae8ec9..a1a1bde 100644
> > --- a/drivers/macintosh/ans-lcd.c
> > +++ b/drivers/macintosh/ans-lcd.c
> > @@ -137,6 +137,7 @@ const struct file_operations anslcd_fops = {
> >  	.write		= anslcd_write,
> >  	.unlocked_ioctl	= anslcd_ioctl,
> >  	.open		= anslcd_open,
> > +	.llseedk	= no_llseek,
> 
> 
> llseedk? :)
> 
> 
> Should we better pushdown default_llseek to every to every
> file operations that don't implement llseek?
> I don't know how many of them don't implement llseek() though.
> 
> That said we can't continue anymore with this default attribution
> of default_llseek() on new fops.
> 

If you don't explicitly set it to no_llseek, you automatically get the
default_llseek, which uses the BKL. So if your driver doesn't need it, it 
is best to explicitly set it to no_llseek.

There is also a generic_file_llseek_unlocked, somewhat analogous to the 
unlocked_ioctls that you can use if you don't need to provide a full 
llseek yourself.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:33:17PM +0200, John Kacur wrote:
> > Should we better pushdown default_llseek to every to every
> > file operations that don't implement llseek?
> > I don't know how many of them don't implement llseek() though.
> > 
> > That said we can't continue anymore with this default attribution
> > of default_llseek() on new fops.
> > 
> 
> If you don't explicitly set it to no_llseek, you automatically get the
> default_llseek, which uses the BKL. So if your driver doesn't need it, it 
> is best to explicitly set it to no_llseek.


Sure, that's the right thing to do.

 
> There is also a generic_file_llseek_unlocked, somewhat analogous to the 
> unlocked_ioctls that you can use if you don't need to provide a full 
> llseek yourself.


No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
depending on the context is the right thing to do.

What I'm wondering about concerns the future code that will have
no llsek() implemented in their fops.

We can't continue to use default_llseek() for future code unless we
want to continue these post reviews and fixes forever.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:33:17PM +0200, John Kacur wrote:
> > Should we better pushdown default_llseek to every to every
> > file operations that don't implement llseek?
> > I don't know how many of them don't implement llseek() though.
> > 
> > That said we can't continue anymore with this default attribution
> > of default_llseek() on new fops.
> > 
> 
> If you don't explicitly set it to no_llseek, you automatically get the
> default_llseek, which uses the BKL. So if your driver doesn't need it, it 
> is best to explicitly set it to no_llseek.


Sure, that's the right thing to do.

 
> There is also a generic_file_llseek_unlocked, somewhat analogous to the 
> unlocked_ioctls that you can use if you don't need to provide a full 
> llseek yourself.


No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
depending on the context is the right thing to do.

What I'm wondering about concerns the future code that will have
no llsek() implemented in their fops.

We can't continue to use default_llseek() for future code unless we
want to continue these post reviews and fixes forever.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[John Kacur]

On Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Wed, Oct 21, 2009 at 11:33:17PM +0200, John Kacur wrote:
> > > Should we better pushdown default_llseek to every to every
> > > file operations that don't implement llseek?
> > > I don't know how many of them don't implement llseek() though.
> > > 
> > > That said we can't continue anymore with this default attribution
> > > of default_llseek() on new fops.
> > > 
> > 
> > If you don't explicitly set it to no_llseek, you automatically get the
> > default_llseek, which uses the BKL. So if your driver doesn't need it, it 
> > is best to explicitly set it to no_llseek.
> 
> 
> Sure, that's the right thing to do.
> 
>  
> > There is also a generic_file_llseek_unlocked, somewhat analogous to the 
> > unlocked_ioctls that you can use if you don't need to provide a full 
> > llseek yourself.
> 
> 
> No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
> depending on the context is the right thing to do.
> 
> What I'm wondering about concerns the future code that will have
> no llsek() implemented in their fops.
> 
> We can't continue to use default_llseek() for future code unless we
> want to continue these post reviews and fixes forever.
> 

I'm thinking that the simplier approach, would be to make the 
default_llseek the unlocked one. Then you only have to audit the drivers 
that have the BKL - ie the ones we are auditing anyway, and explicitly set 
them to the bkl locked llseek.

There might be a hidden interaction though between the non-unlocked 
variety of ioctls and default llseek though.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[John Kacur]

On Wed, 21 Oct 2009, Frederic Weisbecker wrote:

> On Wed, Oct 21, 2009 at 11:33:17PM +0200, John Kacur wrote:
> > > Should we better pushdown default_llseek to every to every
> > > file operations that don't implement llseek?
> > > I don't know how many of them don't implement llseek() though.
> > > 
> > > That said we can't continue anymore with this default attribution
> > > of default_llseek() on new fops.
> > > 
> > 
> > If you don't explicitly set it to no_llseek, you automatically get the
> > default_llseek, which uses the BKL. So if your driver doesn't need it, it 
> > is best to explicitly set it to no_llseek.
> 
> 
> Sure, that's the right thing to do.
> 
>  
> > There is also a generic_file_llseek_unlocked, somewhat analogous to the 
> > unlocked_ioctls that you can use if you don't need to provide a full 
> > llseek yourself.
> 
> 
> No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
> depending on the context is the right thing to do.
> 
> What I'm wondering about concerns the future code that will have
> no llsek() implemented in their fops.
> 
> We can't continue to use default_llseek() for future code unless we
> want to continue these post reviews and fixes forever.
> 

I'm thinking that the simplier approach, would be to make the 
default_llseek the unlocked one. Then you only have to audit the drivers 
that have the BKL - ie the ones we are auditing anyway, and explicitly set 
them to the bkl locked llseek.

There might be a hidden interaction though between the non-unlocked 
variety of ioctls and default llseek though.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:53:21PM +0200, John Kacur wrote:
> > No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
> > depending on the context is the right thing to do.
> > 
> > What I'm wondering about concerns the future code that will have
> > no llsek() implemented in their fops.
> > 
> > We can't continue to use default_llseek() for future code unless we
> > want to continue these post reviews and fixes forever.
> > 
> 
> I'm thinking that the simplier approach, would be to make the 
> default_llseek the unlocked one. Then you only have to audit the drivers 
> that have the BKL - ie the ones we are auditing anyway, and explicitly set 
> them to the bkl locked llseek.
> 
> There might be a hidden interaction though between the non-unlocked 
> variety of ioctls and default llseek though.


I fear that won't work because the bkl in default_llseek() does not
only synchronizes with others uses of the bkl in a driver, it also
synchronizes lseek() itself.

As an example offset change is not atomic. This is a long long, so
updating its value is not atomic in 32 bits archs.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Frederic Weisbecker]

On Wed, Oct 21, 2009 at 11:53:21PM +0200, John Kacur wrote:
> > No problem with that. Setting no_llseek or generic_file_llseek_unlocked,
> > depending on the context is the right thing to do.
> > 
> > What I'm wondering about concerns the future code that will have
> > no llsek() implemented in their fops.
> > 
> > We can't continue to use default_llseek() for future code unless we
> > want to continue these post reviews and fixes forever.
> > 
> 
> I'm thinking that the simplier approach, would be to make the 
> default_llseek the unlocked one. Then you only have to audit the drivers 
> that have the BKL - ie the ones we are auditing anyway, and explicitly set 
> them to the bkl locked llseek.
> 
> There might be a hidden interaction though between the non-unlocked 
> variety of ioctls and default llseek though.


I fear that won't work because the bkl in default_llseek() does not
only synchronizes with others uses of the bkl in a driver, it also
synchronizes lseek() itself.

As an example offset change is not atomic. This is a long long, so
updating its value is not atomic in 32 bits archs.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Arnd Bergmann]

On Thursday 22 October 2009, Frederic Weisbecker wrote:
> > I'm thinking that the simplier approach, would be to make the 
> > default_llseek the unlocked one. Then you only have to audit the drivers 
> > that have the BKL - ie the ones we are auditing anyway, and explicitly set 
> > them to the bkl locked llseek.
> > 
> > There might be a hidden interaction though between the non-unlocked 
> > variety of ioctls and default llseek though.
> 
> 
> I fear that won't work because the bkl in default_llseek() does not
> only synchronizes with others uses of the bkl in a driver, it also
> synchronizes lseek() itself.
> 
> As an example offset change is not atomic. This is a long long, so
> updating its value is not atomic in 32 bits archs.

A late follow-up on this one:

I looked at what places in the code manipulate file->f_pos directly
and found that almost all the uses in driver code are broken because
they don't take any locks. Most of them are in driver specific
lseek operations. Others are in read/write methods and are even
more broken because the change gets immediately overwritten by
vfs_read/vfs_write when the driver method returns.

IMHO, we should complete the pushdown into all ioctl methods
that John and Thomas have started working on, fixing the lseek
methods in those files we touch.

When that is done, all interaction between default_llseek and
driver locking has to be with explicit calls to lock_kernel
in those drivers, so we can reasonably well script the search
for drivers needing the BKL in llseek: everyhing that
 a) defines file_operations without an llseek function,
 b) touches f_pos somewhere, and
 c) calls lock_kernel() somewhere
That should only be a small number and when they are fixed,
we can remove default_llseek and instead call generic_file_llseek
for any file operation without a separate llseek method.

	Arnd <><

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Arnd Bergmann]

On Thursday 22 October 2009, Frederic Weisbecker wrote:
> > I'm thinking that the simplier approach, would be to make the 
> > default_llseek the unlocked one. Then you only have to audit the drivers 
> > that have the BKL - ie the ones we are auditing anyway, and explicitly set 
> > them to the bkl locked llseek.
> > 
> > There might be a hidden interaction though between the non-unlocked 
> > variety of ioctls and default llseek though.
> 
> 
> I fear that won't work because the bkl in default_llseek() does not
> only synchronizes with others uses of the bkl in a driver, it also
> synchronizes lseek() itself.
> 
> As an example offset change is not atomic. This is a long long, so
> updating its value is not atomic in 32 bits archs.

A late follow-up on this one:

I looked at what places in the code manipulate file->f_pos directly
and found that almost all the uses in driver code are broken because
they don't take any locks. Most of them are in driver specific
lseek operations. Others are in read/write methods and are even
more broken because the change gets immediately overwritten by
vfs_read/vfs_write when the driver method returns.

IMHO, we should complete the pushdown into all ioctl methods
that John and Thomas have started working on, fixing the lseek
methods in those files we touch.

When that is done, all interaction between default_llseek and
driver locking has to be with explicit calls to lock_kernel
in those drivers, so we can reasonably well script the search
for drivers needing the BKL in llseek: everyhing that
 a) defines file_operations without an llseek function,
 b) touches f_pos somewhere, and
 c) calls lock_kernel() somewhere
That should only be a small number and when they are fixed,
we can remove default_llseek and instead call generic_file_llseek
for any file operation without a separate llseek method.

	Arnd <><

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Christoph Hellwig]

On Mon, Nov 02, 2009 at 04:51:52PM +0100, Arnd Bergmann wrote:
> I looked at what places in the code manipulate file->f_pos directly
> and found that almost all the uses in driver code are broken because
> they don't take any locks. Most of them are in driver specific
> lseek operations. Others are in read/write methods and are even
> more broken because the change gets immediately overwritten by
> vfs_read/vfs_write when the driver method returns.
> 
> IMHO, we should complete the pushdown into all ioctl methods
> that John and Thomas have started working on, fixing the lseek
> methods in those files we touch.
> 
> When that is done, all interaction between default_llseek and
> driver locking has to be with explicit calls to lock_kernel
> in those drivers, so we can reasonably well script the search
> for drivers needing the BKL in llseek: everyhing that
>  a) defines file_operations without an llseek function,
>  b) touches f_pos somewhere, and
>  c) calls lock_kernel() somewhere
> That should only be a small number and when they are fixed,
> we can remove default_llseek and instead call generic_file_llseek
> for any file operation without a separate llseek method.

As mentioned before making generic_file_llseek the new default is
probably a bad idea.  The majority of our file_operations instances
don't actually support seeking, so no_llseek should become the new
default if you spend some effort on converting things.  Anything that
wants to allow seeking will have to set a llseek method.  This also
mirrors what we do for other file operations.  None of the major ones
has a non-trivial default, it's either silently succeeding for a
selected few like open or release or returning an error for operatings
that actually do something like read and write.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Christoph Hellwig]

On Mon, Nov 02, 2009 at 04:51:52PM +0100, Arnd Bergmann wrote:
> I looked at what places in the code manipulate file->f_pos directly
> and found that almost all the uses in driver code are broken because
> they don't take any locks. Most of them are in driver specific
> lseek operations. Others are in read/write methods and are even
> more broken because the change gets immediately overwritten by
> vfs_read/vfs_write when the driver method returns.
> 
> IMHO, we should complete the pushdown into all ioctl methods
> that John and Thomas have started working on, fixing the lseek
> methods in those files we touch.
> 
> When that is done, all interaction between default_llseek and
> driver locking has to be with explicit calls to lock_kernel
> in those drivers, so we can reasonably well script the search
> for drivers needing the BKL in llseek: everyhing that
>  a) defines file_operations without an llseek function,
>  b) touches f_pos somewhere, and
>  c) calls lock_kernel() somewhere
> That should only be a small number and when they are fixed,
> we can remove default_llseek and instead call generic_file_llseek
> for any file operation without a separate llseek method.

As mentioned before making generic_file_llseek the new default is
probably a bad idea.  The majority of our file_operations instances
don't actually support seeking, so no_llseek should become the new
default if you spend some effort on converting things.  Anything that
wants to allow seeking will have to set a llseek method.  This also
mirrors what we do for other file operations.  None of the major ones
has a non-trivial default, it's either silently succeeding for a
selected few like open or release or returning an error for operatings
that actually do something like read and write.

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Arnd Bergmann]

On Monday 16 November 2009, Christoph Hellwig wrote:
> As mentioned before making generic_file_llseek the new default is
> probably a bad idea.  The majority of our file_operations instances
> don't actually support seeking, so no_llseek should become the new
> default if you spend some effort on converting things.  Anything that
> wants to allow seeking will have to set a llseek method.  This also
> mirrors what we do for other file operations.  None of the major ones
> has a non-trivial default, it's either silently succeeding for a
> selected few like open or release or returning an error for operatings
> that actually do something like read and write.

Ok, good point.

Do you think we should also prevent pread/pwrite for devices without
an llseek operation, like nonseekable_open does? I guess that would
be consistent.

Then there is the point that (I forgot who) brought up that changing
code to do no_llseek is actually an ABI change. Even if the file
position is never used anywhere, some random user application might
expect a chardev not to return an error when its llseek method is
called, resulting in regressions.

	Arnd <><

Re: [PATCH] macintosh: Explicitly set llseek to no_llseek in ans-lcd

[Arnd Bergmann]

On Monday 16 November 2009, Christoph Hellwig wrote:
> As mentioned before making generic_file_llseek the new default is
> probably a bad idea.  The majority of our file_operations instances
> don't actually support seeking, so no_llseek should become the new
> default if you spend some effort on converting things.  Anything that
> wants to allow seeking will have to set a llseek method.  This also
> mirrors what we do for other file operations.  None of the major ones
> has a non-trivial default, it's either silently succeeding for a
> selected few like open or release or returning an error for operatings
> that actually do something like read and write.

Ok, good point.

Do you think we should also prevent pread/pwrite for devices without
an llseek operation, like nonseekable_open does? I guess that would
be consistent.

Then there is the point that (I forgot who) brought up that changing
code to do no_llseek is actually an ABI change. Even if the file
position is never used anywhere, some random user application might
expect a chardev not to return an error when its llseek method is
called, resulting in regressions.

	Arnd <><

[patch 23/28] i2c: Remove big kernel lock from i2cdev_open

[Thomas Gleixner]

[-- Attachment #1: remove-big-kernel-lock-use-from-i2cdev_open.patch --]
[-- Type: text/plain, Size: 2019 bytes --]

The BKL is held over a kmalloc so cannot protect anything beyond that.
The two calls before the kmalloc have their own locking.
Improve device open function by removing the now unnecessary ret variable

Signed-off-by: Vincent Sanders <vince@simtec.co.uk>
LKML-Reference: <1255175172-2666-1-git-send-email-vince@simtec.co.uk>
Cc: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/i2c/i2c-dev.c |   22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

Index: linux-2.6-tip/drivers/i2c/i2c-dev.c
===================================================================
--- linux-2.6-tip.orig/drivers/i2c/i2c-dev.c
+++ linux-2.6-tip/drivers/i2c/i2c-dev.c
@@ -34,7 +34,6 @@
 #include <linux/list.h>
 #include <linux/i2c.h>
 #include <linux/i2c-dev.h>
-#include <linux/smp_lock.h>
 #include <linux/jiffies.h>
 #include <asm/uaccess.h>
 
@@ -445,20 +444,14 @@ static int i2cdev_open(struct inode *ino
 	struct i2c_client *client;
 	struct i2c_adapter *adap;
 	struct i2c_dev *i2c_dev;
-	int ret = 0;
 
-	lock_kernel();
 	i2c_dev = i2c_dev_get_by_minor(minor);
-	if (!i2c_dev) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (!i2c_dev)
+		return -ENODEV;
 
 	adap = i2c_get_adapter(i2c_dev->adap->nr);
-	if (!adap) {
-		ret = -ENODEV;
-		goto out;
-	}
+	if (!adap)
+		return -ENODEV;
 
 	/* This creates an anonymous i2c_client, which may later be
 	 * pointed to some address using I2C_SLAVE or I2C_SLAVE_FORCE.
@@ -470,8 +463,7 @@ static int i2cdev_open(struct inode *ino
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client) {
 		i2c_put_adapter(adap);
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 	snprintf(client->name, I2C_NAME_SIZE, "i2c-dev %d", adap->nr);
 	client->driver = &i2cdev_driver;
@@ -479,9 +471,7 @@ static int i2cdev_open(struct inode *ino
 	client->adapter = adap;
 	file->private_data = client;
 
-out:
-	unlock_kernel();
-	return ret;
+	return 0;
 }
 
 static int i2cdev_release(struct inode *inode, struct file *file)

Re: [patch 23/28] i2c: Remove big kernel lock from i2cdev_open

[Jean Delvare]

Hi Thomas,

On Sat, 10 Oct 2009 15:37:17 -0000, Thomas Gleixner wrote:
> The BKL is held over a kmalloc so cannot protect anything beyond that.
> The two calls before the kmalloc have their own locking.
> Improve device open function by removing the now unnecessary ret variable
> 
> Signed-off-by: Vincent Sanders <vince@simtec.co.uk>
> LKML-Reference: <1255175172-2666-1-git-send-email-vince@simtec.co.uk>
> Cc: Jean Delvare <khali@linux-fr.org>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/i2c/i2c-dev.c |   22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> Index: linux-2.6-tip/drivers/i2c/i2c-dev.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/i2c/i2c-dev.c
> +++ linux-2.6-tip/drivers/i2c/i2c-dev.c
> @@ -34,7 +34,6 @@
>  #include <linux/list.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-dev.h>
> -#include <linux/smp_lock.h>
>  #include <linux/jiffies.h>
>  #include <asm/uaccess.h>
>  
> @@ -445,20 +444,14 @@ static int i2cdev_open(struct inode *ino
>  	struct i2c_client *client;
>  	struct i2c_adapter *adap;
>  	struct i2c_dev *i2c_dev;
> -	int ret = 0;
>  
> -	lock_kernel();
>  	i2c_dev = i2c_dev_get_by_minor(minor);
> -	if (!i2c_dev) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> +	if (!i2c_dev)
> +		return -ENODEV;
>  
>  	adap = i2c_get_adapter(i2c_dev->adap->nr);
> -	if (!adap) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> +	if (!adap)
> +		return -ENODEV;
>  
>  	/* This creates an anonymous i2c_client, which may later be
>  	 * pointed to some address using I2C_SLAVE or I2C_SLAVE_FORCE.
> @@ -470,8 +463,7 @@ static int i2cdev_open(struct inode *ino
>  	client = kzalloc(sizeof(*client), GFP_KERNEL);
>  	if (!client) {
>  		i2c_put_adapter(adap);
> -		ret = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
>  	snprintf(client->name, I2C_NAME_SIZE, "i2c-dev %d", adap->nr);
>  	client->driver = &i2cdev_driver;
> @@ -479,9 +471,7 @@ static int i2cdev_open(struct inode *ino
>  	client->adapter = adap;
>  	file->private_data = client;
>  
> -out:
> -	unlock_kernel();
> -	return ret;
> +	return 0;
>  }
>  
>  static int i2cdev_release(struct inode *inode, struct file *file)
> 

Looks good to me:

Acked-by: Jean Delvare <khali@linux-fr.org>

Do you want me to pick this patch in my i2c tree, or will you take care
of pushing it upstream? If you're not going to push it quickly, I'd
prefer the first solution, to avoid conflicts.

(As a side note, i2c-dev would deserve a partial rewrite as it doesn't
integrate into the standard device driver model, and certainly has a
number of race conditions. But this is well beyond the scope of your
patch.)

Thanks,
-- 
Jean Delvare

Re: [patch 23/28] i2c: Remove big kernel lock from i2cdev_open

[Thomas Gleixner]

Jean,

On Sat, 10 Oct 2009, Jean Delvare wrote:
> Looks good to me:
> 
> Acked-by: Jean Delvare <khali@linux-fr.org>
> 
> Do you want me to pick this patch in my i2c tree, or will you take care
> of pushing it upstream? If you're not going to push it quickly, I'd
> prefer the first solution, to avoid conflicts.

I'm targeting for .33, but please pick it up. I drop it from my queue
then.

> (As a side note, i2c-dev would deserve a partial rewrite as it doesn't
> integrate into the standard device driver model, and certainly has a
> number of race conditions. But this is well beyond the scope of your
> patch.)

Yeah, noticed that this code needs some care, but my current dosis of
brain wreckage with the BKL stuff is enough :)

Thanks,

	tglx

Re: [patch 23/28] i2c: Remove big kernel lock from i2cdev_open

[Jean Delvare]

On Sat, 10 Oct 2009 19:09:45 +0200 (CEST), Thomas Gleixner wrote:
> Jean,
> 
> On Sat, 10 Oct 2009, Jean Delvare wrote:
> > Looks good to me:
> > 
> > Acked-by: Jean Delvare <khali@linux-fr.org>
> > 
> > Do you want me to pick this patch in my i2c tree, or will you take care
> > of pushing it upstream? If you're not going to push it quickly, I'd
> > prefer the first solution, to avoid conflicts.
> 
> I'm targeting for .33, but please pick it up. I drop it from my queue
> then.

Will do. I guess I can assume that the patch is from Vincent Sanders,
not you, despite the lack of From: header?

-- 
Jean Delvare

Re: [patch 23/28] i2c: Remove big kernel lock from i2cdev_open

[Thomas Gleixner]

Jean,

On Sat, 10 Oct 2009, Jean Delvare wrote:

> On Sat, 10 Oct 2009 19:09:45 +0200 (CEST), Thomas Gleixner wrote:
> > Jean,
> > 
> > On Sat, 10 Oct 2009, Jean Delvare wrote:
> > > Looks good to me:
> > > 
> > > Acked-by: Jean Delvare <khali@linux-fr.org>
> > > 
> > > Do you want me to pick this patch in my i2c tree, or will you take care
> > > of pushing it upstream? If you're not going to push it quickly, I'd
> > > prefer the first solution, to avoid conflicts.
> > 
> > I'm targeting for .33, but please pick it up. I drop it from my queue
> > then.
> 
> Will do. I guess I can assume that the patch is from Vincent Sanders,
> not you, despite the lack of From: header?

Duh, yes. Missed that.

Thanks,

	tglx

Re: [patch 23/28] i2c: Remove big kernel lock from i2cdev_open

[Peter Zijlstra]

On Sat, 2009-10-10 at 20:10 +0200, Thomas Gleixner wrote:
> 
> > Will do. I guess I can assume that the patch is from Vincent Sanders,
> > not you, despite the lack of From: header?
> 
> Duh, yes. Missed that.

I'm still wanting to fix quilt mail to not mess that up... maybe I
should sit down and try and grok that stuff again.

Re: [patch 23/28] i2c: Remove big kernel lock from i2cdev_open

[Thomas Gleixner]

On Sat, 10 Oct 2009, Peter Zijlstra wrote:

> On Sat, 2009-10-10 at 20:10 +0200, Thomas Gleixner wrote:
> > 
> > > Will do. I guess I can assume that the patch is from Vincent Sanders,
> > > not you, despite the lack of From: header?
> > 
> > Duh, yes. Missed that.
> 
> I'm still wanting to fix quilt mail to not mess that up... maybe I
> should sit down and try and grok that stuff again.

IIRC it worked some time ago. That's why I did not notice.

Thanks,

	tglx
 

[patch 24/28] rtc: Remove BKL from efirtc

[Thomas Gleixner]

[-- Attachment #1: efi-rtc-remove-bkl.patch --]
[-- Type: text/plain, Size: 2430 bytes --]

BKL locking came to efirtc via the big BKL push down, but the access
to the efi functions is protected by efi_rtc_lock already.

Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
drivers/char/efirtc.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-)

---
 drivers/char/efirtc.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

Index: linux-2.6-tip/drivers/char/efirtc.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/efirtc.c
+++ linux-2.6-tip/drivers/char/efirtc.c
@@ -27,8 +27,6 @@
  * 	- Add module support
  */
 
-
-#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/miscdevice.h>
@@ -174,13 +172,12 @@ static long efi_rtc_ioctl(struct file *f
 			return -EINVAL;
 
 		case RTC_RD_TIME:
-			lock_kernel();
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 
 			status = efi.get_time(&eft, &cap);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
-			unlock_kernel();
+
 			if (status != EFI_SUCCESS) {
 				/* should never happen */
 				printk(KERN_ERR "efitime: can't read time\n");
@@ -202,13 +199,11 @@ static long efi_rtc_ioctl(struct file *f
 
 			convert_to_efi_time(&wtime, &eft);
 
-			lock_kernel();
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 
 			status = efi.set_time(&eft);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
-			unlock_kernel();
 
 			return status == EFI_SUCCESS ? 0 : -EINVAL;
 
@@ -224,7 +219,6 @@ static long efi_rtc_ioctl(struct file *f
 
 			convert_to_efi_time(&wtime, &eft);
 
-			lock_kernel();
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 			/*
 			 * XXX Fixme:
@@ -235,19 +229,16 @@ static long efi_rtc_ioctl(struct file *f
 			status = efi.set_wakeup_time((efi_bool_t)enabled, &eft);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
-			unlock_kernel();
 
 			return status == EFI_SUCCESS ? 0 : -EINVAL;
 
 		case RTC_WKALM_RD:
 
-			lock_kernel();
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 
 			status = efi.get_wakeup_time((efi_bool_t *)&enabled, (efi_bool_t *)&pending, &eft);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
-			unlock_kernel();
 
 			if (status != EFI_SUCCESS) return -EINVAL;
 
@@ -277,7 +268,6 @@ static int efi_rtc_open(struct inode *in
 	 * We do accept multiple open files at the same time as we
 	 * synchronize on the per call operation.
 	 */
-	cycle_kernel_lock();
 	return 0;
 }
 

[tip:bkl/drivers] rtc: Remove BKL from efirtc

[tip-bot for Thomas Gleixner]

Commit-ID:  a5ee6dc9ebe8fc2640ee3fbf2c340bd853e2fd36
Gitweb:     http://git.kernel.org/tip/a5ee6dc9ebe8fc2640ee3fbf2c340bd853e2fd36
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 15:14:03 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:51 +0200

rtc: Remove BKL from efirtc

BKL locking came to efirtc via the big BKL push down, but the access
to the efi functions is protected by efi_rtc_lock already.

Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153350.046644063@linutronix.de>
---
 drivers/char/efirtc.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/drivers/char/efirtc.c b/drivers/char/efirtc.c
index 34d15d5..26a47dc 100644
--- a/drivers/char/efirtc.c
+++ b/drivers/char/efirtc.c
@@ -27,8 +27,6 @@
  * 	- Add module support
  */
 
-
-#include <linux/smp_lock.h>
 #include <linux/types.h>
 #include <linux/errno.h>
 #include <linux/miscdevice.h>
@@ -174,13 +172,12 @@ static long efi_rtc_ioctl(struct file *file, unsigned int cmd,
 			return -EINVAL;
 
 		case RTC_RD_TIME:
-			lock_kernel();
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 
 			status = efi.get_time(&eft, &cap);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
-			unlock_kernel();
+
 			if (status != EFI_SUCCESS) {
 				/* should never happen */
 				printk(KERN_ERR "efitime: can't read time\n");
@@ -202,13 +199,11 @@ static long efi_rtc_ioctl(struct file *file, unsigned int cmd,
 
 			convert_to_efi_time(&wtime, &eft);
 
-			lock_kernel();
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 
 			status = efi.set_time(&eft);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
-			unlock_kernel();
 
 			return status == EFI_SUCCESS ? 0 : -EINVAL;
 
@@ -224,7 +219,6 @@ static long efi_rtc_ioctl(struct file *file, unsigned int cmd,
 
 			convert_to_efi_time(&wtime, &eft);
 
-			lock_kernel();
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 			/*
 			 * XXX Fixme:
@@ -235,19 +229,16 @@ static long efi_rtc_ioctl(struct file *file, unsigned int cmd,
 			status = efi.set_wakeup_time((efi_bool_t)enabled, &eft);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
-			unlock_kernel();
 
 			return status == EFI_SUCCESS ? 0 : -EINVAL;
 
 		case RTC_WKALM_RD:
 
-			lock_kernel();
 			spin_lock_irqsave(&efi_rtc_lock, flags);
 
 			status = efi.get_wakeup_time((efi_bool_t *)&enabled, (efi_bool_t *)&pending, &eft);
 
 			spin_unlock_irqrestore(&efi_rtc_lock,flags);
-			unlock_kernel();
 
 			if (status != EFI_SUCCESS) return -EINVAL;
 
@@ -277,7 +268,6 @@ static int efi_rtc_open(struct inode *inode, struct file *file)
 	 * We do accept multiple open files at the same time as we
 	 * synchronize on the per call operation.
 	 */
-	cycle_kernel_lock();
 	return 0;
 }
 

Subject: [PATCH] rtc: Explicitly set llseek to no_llseek

[John Kacur]

>From e1b7175258b33da3b0564ef04a0b1956f04f0cc7 Mon Sep 17 00:00:00 2001
From: John Kacur <jkacur@redhat.com>
Date: Wed, 21 Oct 2009 23:10:30 +0200
Subject: [PATCH] rtc: Explicitly set llseek to no_llseek

Now that we've removed the BKL here, lets explicitly set llseek to no_llseek
since the default llseek is not used here.

Signed-off-by: John Kacur <jkacur@redhat.com>
---
 drivers/char/efirtc.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/char/efirtc.c b/drivers/char/efirtc.c
index 26a47dc..53c524e 100644
--- a/drivers/char/efirtc.c
+++ b/drivers/char/efirtc.c
@@ -285,6 +285,7 @@ static const struct file_operations efi_rtc_fops = {
 	.unlocked_ioctl	= efi_rtc_ioctl,
 	.open		= efi_rtc_open,
 	.release	= efi_rtc_close,
+	.llseek		= no_llseek,
 };
 
 static struct miscdevice efi_rtc_dev= {
-- 
1.6.0.6

Re: Subject: [PATCH] rtc: Explicitly set llseek to no_llseek

[Andrew Morton]

On Wed, 21 Oct 2009 23:13:26 +0200 (CEST)
John Kacur <jkacur@redhat.com> wrote:

> >From e1b7175258b33da3b0564ef04a0b1956f04f0cc7 Mon Sep 17 00:00:00 2001
> From: John Kacur <jkacur@redhat.com>
> Date: Wed, 21 Oct 2009 23:10:30 +0200
> Subject: [PATCH] rtc: Explicitly set llseek to no_llseek
> 
> Now that we've removed the BKL here, lets explicitly set llseek to no_llseek
> since the default llseek is not used here.
> 

I don't understand.

> 
> diff --git a/drivers/char/efirtc.c b/drivers/char/efirtc.c
> index 26a47dc..53c524e 100644
> --- a/drivers/char/efirtc.c
> +++ b/drivers/char/efirtc.c
> @@ -285,6 +285,7 @@ static const struct file_operations efi_rtc_fops = {
>  	.unlocked_ioctl	= efi_rtc_ioctl,
>  	.open		= efi_rtc_open,
>  	.release	= efi_rtc_close,
> +	.llseek		= no_llseek,
>  };
>  
>  static struct miscdevice efi_rtc_dev= {

What has this change to do with the BKL?

Re: Subject: [PATCH] rtc: Explicitly set llseek to no_llseek

[John Kacur]

On Tue, 3 Nov 2009, Andrew Morton wrote:

> On Wed, 21 Oct 2009 23:13:26 +0200 (CEST)
> John Kacur <jkacur@redhat.com> wrote:
> 
> > >From e1b7175258b33da3b0564ef04a0b1956f04f0cc7 Mon Sep 17 00:00:00 2001
> > From: John Kacur <jkacur@redhat.com>
> > Date: Wed, 21 Oct 2009 23:10:30 +0200
> > Subject: [PATCH] rtc: Explicitly set llseek to no_llseek
> > 
> > Now that we've removed the BKL here, lets explicitly set llseek to no_llseek
> > since the default llseek is not used here.
> > 
> 
> I don't understand.
> 
> > 
> > diff --git a/drivers/char/efirtc.c b/drivers/char/efirtc.c
> > index 26a47dc..53c524e 100644
> > --- a/drivers/char/efirtc.c
> > +++ b/drivers/char/efirtc.c
> > @@ -285,6 +285,7 @@ static const struct file_operations efi_rtc_fops = {
> >  	.unlocked_ioctl	= efi_rtc_ioctl,
> >  	.open		= efi_rtc_open,
> >  	.release	= efi_rtc_close,
> > +	.llseek		= no_llseek,
> >  };
> >  
> >  static struct miscdevice efi_rtc_dev= {
> 
> What has this change to do with the BKL?

The default_llseek function still contains the BKL.
When we are auditing code to see if we can remove the BKL, this is one of 
the hidden considerations we need to take into account.
i.e., is there syncronization between code that has the BKL and llseek.

At the same time we remove the BKL it would be a good idea to do indicate 
when no llseek function is required, so we don't have to revisit this code 
again, when we are trying to determine if we can remove the BKL from
the default_llseek.

[patch 25/28] parisc: Remove BKL from eisa_eeprom

[Thomas Gleixner]

[-- Attachment #1: parisc-eisa-eeprom-remove-bkl.patch --]
[-- Type: text/plain, Size: 1233 bytes --]

Remove the empty ioctl and the cycle_kernel_lock() in
eisa_eeprom_open() which got there with the big BKL push down. There
is nothing to wait for and sychronize with after the misc device has
been registered.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Kyle McMartin <kyle@parisc-linux.org>
---
 drivers/parisc/eisa_eeprom.c |   10 ----------
 1 file changed, 10 deletions(-)

Index: linux-2.6-tip/drivers/parisc/eisa_eeprom.c
===================================================================
--- linux-2.6-tip.orig/drivers/parisc/eisa_eeprom.c
+++ linux-2.6-tip/drivers/parisc/eisa_eeprom.c
@@ -75,17 +75,8 @@ static ssize_t eisa_eeprom_read(struct f
 	return ret;
 }
 
-static int eisa_eeprom_ioctl(struct inode *inode, struct file *file, 
-			   unsigned int cmd,
-			   unsigned long arg)
-{
-	return -ENOTTY;
-}
-
 static int eisa_eeprom_open(struct inode *inode, struct file *file)
 {
-	cycle_kernel_lock();
-
 	if (file->f_mode & FMODE_WRITE)
 		return -EINVAL;
    
@@ -104,7 +95,6 @@ static const struct file_operations eisa
 	.owner =	THIS_MODULE,
 	.llseek =	eisa_eeprom_llseek,
 	.read =		eisa_eeprom_read,
-	.ioctl =	eisa_eeprom_ioctl,
 	.open =		eisa_eeprom_open,
 	.release =	eisa_eeprom_release,
 };

[tip:bkl/drivers] parisc: Remove BKL from eisa_eeprom

[tip-bot for Thomas Gleixner]

Commit-ID:  eb29b758a8b0b2dbffd8dc898490237d3ee783e4
Gitweb:     http://git.kernel.org/tip/eb29b758a8b0b2dbffd8dc898490237d3ee783e4
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 15:33:17 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:52 +0200

parisc: Remove BKL from eisa_eeprom

Remove the empty ioctl and the cycle_kernel_lock() in
eisa_eeprom_open() which got there with the big BKL push down. There
is nothing to wait for and sychronize with after the misc device has
been registered.

Remove the empty ioctl as well. The generic code handles the -ENOTTY
if no ioctl function is provided.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153350.086917493@linutronix.de>
Cc: Kyle McMartin <kyle@parisc-linux.org>
---
 drivers/parisc/eisa_eeprom.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/parisc/eisa_eeprom.c b/drivers/parisc/eisa_eeprom.c
index 8c0b26e..cce00ed 100644
--- a/drivers/parisc/eisa_eeprom.c
+++ b/drivers/parisc/eisa_eeprom.c
@@ -75,17 +75,8 @@ static ssize_t eisa_eeprom_read(struct file * file,
 	return ret;
 }
 
-static int eisa_eeprom_ioctl(struct inode *inode, struct file *file, 
-			   unsigned int cmd,
-			   unsigned long arg)
-{
-	return -ENOTTY;
-}
-
 static int eisa_eeprom_open(struct inode *inode, struct file *file)
 {
-	cycle_kernel_lock();
-
 	if (file->f_mode & FMODE_WRITE)
 		return -EINVAL;
    
@@ -104,7 +95,6 @@ static const struct file_operations eisa_eeprom_fops = {
 	.owner =	THIS_MODULE,
 	.llseek =	eisa_eeprom_llseek,
 	.read =		eisa_eeprom_read,
-	.ioctl =	eisa_eeprom_ioctl,
 	.open =		eisa_eeprom_open,
 	.release =	eisa_eeprom_release,
 };

Re: [patch 25/28] parisc: Remove BKL from eisa_eeprom

[Kyle McMartin]

On Sat, Oct 10, 2009 at 03:37:26PM -0000, Thomas Gleixner wrote:
> Remove the empty ioctl and the cycle_kernel_lock() in
> eisa_eeprom_open() which got there with the big BKL push down. There
> is nothing to wait for and sychronize with after the misc device has
> been registered.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Kyle McMartin <kyle@parisc-linux.org>

Looks good to me.

Acked-by: Kyle McMartin <kyle@mcmartin.ca>

[patch 26/28] drivers: Remove BKL from pc8736x_gpio

[Thomas Gleixner]

[-- Attachment #1: pc8736x_gpio-remove-bkl.patch --]
[-- Type: text/plain, Size: 984 bytes --]

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/char/pc8736x_gpio.c |    2 --
 1 file changed, 2 deletions(-)

Index: linux-2.6-tip/drivers/char/pc8736x_gpio.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/pc8736x_gpio.c
+++ linux-2.6-tip/drivers/char/pc8736x_gpio.c
@@ -20,7 +20,6 @@
 #include <linux/mutex.h>
 #include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
-#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 
 #define DEVNAME "pc8736x_gpio"
@@ -223,7 +222,6 @@ static int pc8736x_gpio_open(struct inod
 	unsigned m = iminor(inode);
 	file->private_data = &pc8736x_gpio_ops;
 
-	cycle_kernel_lock();
 	dev_dbg(&pdev->dev, "open %d\n", m);
 
 	if (m >= PC8736X_GPIO_CT)

[tip:bkl/drivers] drivers: Remove BKL from pc8736x_gpio

[tip-bot for Thomas Gleixner]

Commit-ID:  71d69bc2c0202f438669073d849999d2f6b6ca31
Gitweb:     http://git.kernel.org/tip/71d69bc2c0202f438669073d849999d2f6b6ca31
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 15:56:00 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:52 +0200

drivers: Remove BKL from pc8736x_gpio

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153350.127093710@linutronix.de>
Acked-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/char/pc8736x_gpio.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/char/pc8736x_gpio.c b/drivers/char/pc8736x_gpio.c
index 3f7da8c..8ecbcc1 100644
--- a/drivers/char/pc8736x_gpio.c
+++ b/drivers/char/pc8736x_gpio.c
@@ -20,7 +20,6 @@
 #include <linux/mutex.h>
 #include <linux/nsc_gpio.h>
 #include <linux/platform_device.h>
-#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 
 #define DEVNAME "pc8736x_gpio"
@@ -223,7 +222,6 @@ static int pc8736x_gpio_open(struct inode *inode, struct file *file)
 	unsigned m = iminor(inode);
 	file->private_data = &pc8736x_gpio_ops;
 
-	cycle_kernel_lock();
 	dev_dbg(&pdev->dev, "open %d\n", m);
 
 	if (m >= PC8736X_GPIO_CT)

[patch 27/28] drivers: Remove BKL from scx200_gpio

[Thomas Gleixner]

[-- Attachment #1: scx200_gpio-remove-bkl.patch --]
[-- Type: text/plain, Size: 974 bytes --]

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/char/scx200_gpio.c |    2 --
 1 file changed, 2 deletions(-)

Index: linux-2.6-tip/drivers/char/scx200_gpio.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/scx200_gpio.c
+++ linux-2.6-tip/drivers/char/scx200_gpio.c
@@ -12,7 +12,6 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
-#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
@@ -52,7 +51,6 @@ static int scx200_gpio_open(struct inode
 	unsigned m = iminor(inode);
 	file->private_data = &scx200_gpio_ops;
 
-	cycle_kernel_lock();
 	if (m >= MAX_PINS)
 		return -EINVAL;
 	return nonseekable_open(inode, file);

[tip:bkl/drivers] drivers: Remove BKL from scx200_gpio

[tip-bot for Thomas Gleixner]

Commit-ID:  3a8183a2061ba54c4c2b3cd31c3add6fd717e853
Gitweb:     http://git.kernel.org/tip/3a8183a2061ba54c4c2b3cd31c3add6fd717e853
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 16:02:53 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:53 +0200

drivers: Remove BKL from scx200_gpio

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code. In this case there
is nothing to serialize. Remove it.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153350.167321547@linutronix.de>
Acked-by: Jim Cromie <jim.cromie@gmail.com>
---
 drivers/char/scx200_gpio.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/char/scx200_gpio.c b/drivers/char/scx200_gpio.c
index 1d91005..99e5272 100644
--- a/drivers/char/scx200_gpio.c
+++ b/drivers/char/scx200_gpio.c
@@ -12,7 +12,6 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
-#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 #include <asm/io.h>
 
@@ -52,7 +51,6 @@ static int scx200_gpio_open(struct inode *inode, struct file *file)
 	unsigned m = iminor(inode);
 	file->private_data = &scx200_gpio_ops;
 
-	cycle_kernel_lock();
 	if (m >= MAX_PINS)
 		return -EINVAL;
 	return nonseekable_open(inode, file);

[patch 28/28] mips: Remove BKL from tb0219

[Thomas Gleixner]

[-- Attachment #1: tb0219-remove-bkl.patch --]
[-- Type: text/plain, Size: 1609 bytes --]

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code.

tb0219_base is initialized before the character device is
registered, but the spinlock is not initialized.

Initialize the spinlock statically and remove cycle_kernel_lock().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Yoichi Yuasa <yuasa@linux-mips.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/char/tb0219.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Index: linux-2.6-tip/drivers/char/tb0219.c
===================================================================
--- linux-2.6-tip.orig/drivers/char/tb0219.c
+++ linux-2.6-tip/drivers/char/tb0219.c
@@ -21,7 +21,6 @@
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/smp_lock.h>
 
 #include <asm/io.h>
 #include <asm/reboot.h>
@@ -38,7 +37,7 @@ MODULE_PARM_DESC(major, "Major device nu
 
 static void (*old_machine_restart)(char *command);
 static void __iomem *tb0219_base;
-static spinlock_t tb0219_lock;
+static DEFINE_SPINLOCK(tb0219_lock);
 
 #define tb0219_read(offset)		readw(tb0219_base + (offset))
 #define tb0219_write(offset, value)	writew((value), tb0219_base + (offset))
@@ -237,7 +236,6 @@ static int tanbac_tb0219_open(struct ino
 {
 	unsigned int minor;
 
-	cycle_kernel_lock();
 	minor = iminor(inode);
 	switch (minor) {
 	case 0:
@@ -306,8 +304,6 @@ static int __devinit tb0219_probe(struct
 		return retval;
 	}
 
-	spin_lock_init(&tb0219_lock);
-
 	old_machine_restart = _machine_restart;
 	_machine_restart = tb0219_restart;
 

[tip:bkl/drivers] mips: Remove BKL from tb0219

[tip-bot for Thomas Gleixner]

Commit-ID:  d2a7be0be1099c2554f4705d2c1c5081f8f96efc
Gitweb:     http://git.kernel.org/tip/d2a7be0be1099c2554f4705d2c1c5081f8f96efc
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Sat, 10 Oct 2009 16:07:03 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:53 +0200

mips: Remove BKL from tb0219

cycle_kernel_lock() was added during the big BKL pushdown. It should
ensure the serializiation against driver init code.

tb0219_base is initialized before the character device is
registered, but the spinlock is not initialized.

Initialize the spinlock statically and remove cycle_kernel_lock().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
LKML-Reference: <20091010153350.222654356@linutronix.de>
Cc: Yoichi Yuasa <yuasa@linux-mips.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/char/tb0219.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tb0219.c b/drivers/char/tb0219.c
index b3ec9b1..cad4eb6 100644
--- a/drivers/char/tb0219.c
+++ b/drivers/char/tb0219.c
@@ -21,7 +21,6 @@
 #include <linux/fs.h>
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/smp_lock.h>
 
 #include <asm/io.h>
 #include <asm/reboot.h>
@@ -38,7 +37,7 @@ MODULE_PARM_DESC(major, "Major device number");
 
 static void (*old_machine_restart)(char *command);
 static void __iomem *tb0219_base;
-static spinlock_t tb0219_lock;
+static DEFINE_SPINLOCK(tb0219_lock);
 
 #define tb0219_read(offset)		readw(tb0219_base + (offset))
 #define tb0219_write(offset, value)	writew((value), tb0219_base + (offset))
@@ -237,7 +236,6 @@ static int tanbac_tb0219_open(struct inode *inode, struct file *file)
 {
 	unsigned int minor;
 
-	cycle_kernel_lock();
 	minor = iminor(inode);
 	switch (minor) {
 	case 0:
@@ -306,8 +304,6 @@ static int __devinit tb0219_probe(struct platform_device *dev)
 		return retval;
 	}
 
-	spin_lock_init(&tb0219_lock);
-
 	old_machine_restart = _machine_restart;
 	_machine_restart = tb0219_restart;
 

Re: [patch 00/28] BKL removal queued patches

[John Kacur]

On Sat, 10 Oct 2009, Thomas Gleixner wrote:

> Hi all,
> 
> I'm sending out the pending BKL removal patches which I collected so
> far to avoid duplicate work and to solicit review/comments.
> 
> If nobody objects I'd like to keep them in one git branch which is
> going to be included into linux-next.
> 
> Thanks,
> 
> 	tglx

Seems like a great idea to me. I'm applying them all to a private 
git-branch against the latest from Linus to avoid duplicate work.

Please let us know when there is a tip branch we can work with.

John

[PATCH 1/2] compat_ioctl: remove VT specific ioctl handlers

[Arnd Bergmann]

The VT driver now handles all of these ioctls directly, so
we can remove the handlers from common code.

These are the only handlers that require the BKL because they
directly perform the ioctl action rather than just converting
the data structures. Once they are gone, we can remove the
BKL from the remaining ioctl conversion handlers.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/compat_ioctl.c |  188 +----------------------------------------------------
 1 files changed, 1 insertions(+), 187 deletions(-)

I guess it makes sense to merge these two through the BKL removal
queue.

	Arnd <><

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index f91fd51..40904c6 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -1039,161 +1039,6 @@ static int mt_ioctl_trans(unsigned int fd, unsigned int cmd, unsigned long arg)
 
 #endif /* CONFIG_BLOCK */
 
-#ifdef CONFIG_VT
-
-static int vt_check(struct file *file)
-{
-	struct tty_struct *tty;
-	struct inode *inode = file->f_path.dentry->d_inode;
-	struct vc_data *vc;
-	
-	if (file->f_op->unlocked_ioctl != tty_ioctl)
-		return -EINVAL;
-	                
-	tty = (struct tty_struct *)file->private_data;
-	if (tty_paranoia_check(tty, inode, "tty_ioctl"))
-		return -EINVAL;
-	                                                
-	if (tty->ops->ioctl != vt_ioctl)
-		return -EINVAL;
-
-	vc = (struct vc_data *)tty->driver_data;
-	if (!vc_cons_allocated(vc->vc_num)) 	/* impossible? */
-		return -ENOIOCTLCMD;
-
-	/*
-	 * To have permissions to do most of the vt ioctls, we either have
-	 * to be the owner of the tty, or have CAP_SYS_TTY_CONFIG.
-	 */
-	if (current->signal->tty == tty || capable(CAP_SYS_TTY_CONFIG))
-		return 1;
-	return 0;                                                    
-}
-
-struct consolefontdesc32 {
-	unsigned short charcount;       /* characters in font (256 or 512) */
-	unsigned short charheight;      /* scan lines per character (1-32) */
-	compat_caddr_t chardata;	/* font data in expanded form */
-};
-
-static int do_fontx_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg, struct file *file)
-{
-	struct consolefontdesc32 __user *user_cfd = compat_ptr(arg);
-	struct console_font_op op;
-	compat_caddr_t data;
-	int i, perm;
-
-	perm = vt_check(file);
-	if (perm < 0) return perm;
-	
-	switch (cmd) {
-	case PIO_FONTX:
-		if (!perm)
-			return -EPERM;
-		op.op = KD_FONT_OP_SET;
-		op.flags = 0;
-		op.width = 8;
-		if (get_user(op.height, &user_cfd->charheight) ||
-		    get_user(op.charcount, &user_cfd->charcount) ||
-		    get_user(data, &user_cfd->chardata))
-			return -EFAULT;
-		op.data = compat_ptr(data);
-		return con_font_op(vc_cons[fg_console].d, &op);
-	case GIO_FONTX:
-		op.op = KD_FONT_OP_GET;
-		op.flags = 0;
-		op.width = 8;
-		if (get_user(op.height, &user_cfd->charheight) ||
-		    get_user(op.charcount, &user_cfd->charcount) ||
-		    get_user(data, &user_cfd->chardata))
-			return -EFAULT;
-		if (!data)
-			return 0;
-		op.data = compat_ptr(data);
-		i = con_font_op(vc_cons[fg_console].d, &op);
-		if (i)
-			return i;
-		if (put_user(op.height, &user_cfd->charheight) ||
-		    put_user(op.charcount, &user_cfd->charcount) ||
-		    put_user((compat_caddr_t)(unsigned long)op.data,
-				&user_cfd->chardata))
-			return -EFAULT;
-		return 0;
-	}
-	return -EINVAL;
-}
-
-struct console_font_op32 {
-	compat_uint_t op;        /* operation code KD_FONT_OP_* */
-	compat_uint_t flags;     /* KD_FONT_FLAG_* */
-	compat_uint_t width, height;     /* font size */
-	compat_uint_t charcount;
-	compat_caddr_t data;    /* font data with height fixed to 32 */
-};
-                                        
-static int do_kdfontop_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg, struct file *file)
-{
-	struct console_font_op op;
-	struct console_font_op32 __user *fontop = compat_ptr(arg);
-	int perm = vt_check(file), i;
-	struct vc_data *vc;
-	
-	if (perm < 0) return perm;
-	
-	if (copy_from_user(&op, fontop, sizeof(struct console_font_op32)))
-		return -EFAULT;
-	if (!perm && op.op != KD_FONT_OP_GET)
-		return -EPERM;
-	op.data = compat_ptr(((struct console_font_op32 *)&op)->data);
-	op.flags |= KD_FONT_FLAG_OLD;
-	vc = ((struct tty_struct *)file->private_data)->driver_data;
-	i = con_font_op(vc, &op);
-	if (i)
-		return i;
-	((struct console_font_op32 *)&op)->data = (unsigned long)op.data;
-	if (copy_to_user(fontop, &op, sizeof(struct console_font_op32)))
-		return -EFAULT;
-	return 0;
-}
-
-struct unimapdesc32 {
-	unsigned short entry_ct;
-	compat_caddr_t entries;
-};
-
-static int do_unimap_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg, struct file *file)
-{
-	struct unimapdesc32 tmp;
-	struct unimapdesc32 __user *user_ud = compat_ptr(arg);
-	int perm = vt_check(file);
-	struct vc_data *vc;
-
-	if (perm < 0)
-		return perm;
-	if (copy_from_user(&tmp, user_ud, sizeof tmp))
-		return -EFAULT;
-	if (tmp.entries)
-		if (!access_ok(VERIFY_WRITE, compat_ptr(tmp.entries),
-				tmp.entry_ct*sizeof(struct unipair)))
-			return -EFAULT;
-	vc = ((struct tty_struct *)file->private_data)->driver_data;
-	switch (cmd) {
-	case PIO_UNIMAP:
-		if (!perm)
-			return -EPERM;
-		return con_set_unimap(vc, tmp.entry_ct,
-						compat_ptr(tmp.entries));
-	case GIO_UNIMAP:
-		if (!perm && fg_console != vc->vc_num)
-			return -EPERM;
-		return con_get_unimap(vc, tmp.entry_ct, &(user_ud->entry_ct),
-						compat_ptr(tmp.entries));
-	}
-	return 0;
-}
-
-#endif /* CONFIG_VT */
-
 static int do_smb_getmountuid(unsigned int fd, unsigned int cmd, unsigned long arg)
 {
 	mm_segment_t old_fs = get_fs();
@@ -1934,11 +1779,7 @@ COMPATIBLE_IOCTL(STOP_ARRAY_RO)
 COMPATIBLE_IOCTL(RESTART_ARRAY_RW)
 COMPATIBLE_IOCTL(GET_BITMAP_FILE)
 ULONG_IOCTL(SET_BITMAP_FILE)
-/* Big K */
-COMPATIBLE_IOCTL(PIO_FONT)
-COMPATIBLE_IOCTL(GIO_FONT)
-COMPATIBLE_IOCTL(PIO_CMAP)
-COMPATIBLE_IOCTL(GIO_CMAP)
+/* Keyboard -- can be removed once tty3270 uses ops->compat_ioctl */
 ULONG_IOCTL(KDSIGACCEPT)
 COMPATIBLE_IOCTL(KDGETKEYCODE)
 COMPATIBLE_IOCTL(KDSETKEYCODE)
@@ -1962,12 +1803,6 @@ COMPATIBLE_IOCTL(KDGKBLED)
 ULONG_IOCTL(KDSKBLED)
 COMPATIBLE_IOCTL(KDGETLED)
 ULONG_IOCTL(KDSETLED)
-COMPATIBLE_IOCTL(GIO_SCRNMAP)
-COMPATIBLE_IOCTL(PIO_SCRNMAP)
-COMPATIBLE_IOCTL(GIO_UNISCRNMAP)
-COMPATIBLE_IOCTL(PIO_UNISCRNMAP)
-COMPATIBLE_IOCTL(PIO_FONTRESET)
-COMPATIBLE_IOCTL(PIO_UNIMAPCLR)
 #ifdef CONFIG_BLOCK
 /* Big S */
 COMPATIBLE_IOCTL(SCSI_IOCTL_GET_IDLUN)
@@ -1991,20 +1826,6 @@ COMPATIBLE_IOCTL(TUNSETOFFLOAD)
 COMPATIBLE_IOCTL(TUNSETTXFILTER)
 COMPATIBLE_IOCTL(TUNGETSNDBUF)
 COMPATIBLE_IOCTL(TUNSETSNDBUF)
-/* Big V */
-COMPATIBLE_IOCTL(VT_SETMODE)
-COMPATIBLE_IOCTL(VT_GETMODE)
-COMPATIBLE_IOCTL(VT_GETSTATE)
-COMPATIBLE_IOCTL(VT_OPENQRY)
-ULONG_IOCTL(VT_ACTIVATE)
-ULONG_IOCTL(VT_WAITACTIVE)
-ULONG_IOCTL(VT_RELDISP)
-ULONG_IOCTL(VT_DISALLOCATE)
-COMPATIBLE_IOCTL(VT_RESIZE)
-COMPATIBLE_IOCTL(VT_RESIZEX)
-COMPATIBLE_IOCTL(VT_LOCKSWITCH)
-COMPATIBLE_IOCTL(VT_UNLOCKSWITCH)
-COMPATIBLE_IOCTL(VT_GETHIFONTMASK)
 /* Little p (/dev/rtc, /dev/envctrl, etc.) */
 COMPATIBLE_IOCTL(RTC_AIE_ON)
 COMPATIBLE_IOCTL(RTC_AIE_OFF)
@@ -2603,13 +2424,6 @@ HANDLE_IOCTL(MTIOCPOS32, mt_ioctl_trans)
 #endif
 #define AUTOFS_IOC_SETTIMEOUT32 _IOWR(0x93,0x64,unsigned int)
 HANDLE_IOCTL(AUTOFS_IOC_SETTIMEOUT32, ioc_settimeout)
-#ifdef CONFIG_VT
-HANDLE_IOCTL(PIO_FONTX, do_fontx_ioctl)
-HANDLE_IOCTL(GIO_FONTX, do_fontx_ioctl)
-HANDLE_IOCTL(PIO_UNIMAP, do_unimap_ioctl)
-HANDLE_IOCTL(GIO_UNIMAP, do_unimap_ioctl)
-HANDLE_IOCTL(KDFONTOP, do_kdfontop_ioctl)
-#endif
 /* One SMB ioctl needs translations. */
 #define SMB_IOC_GETMOUNTUID_32 _IOR('u', 1, compat_uid_t)
 HANDLE_IOCTL(SMB_IOC_GETMOUNTUID_32, do_smb_getmountuid)
-- 
1.6.3.3

[PATCH 2/2] compat_ioctl: do not hold BKL in handlers

[Arnd Bergmann]

We have always called ioctl conversion handlers under the big
kernel lock, although that is generally not necessary. In particular
it is not needed for conversion of data structures and for calling
sys_ioctl or do_vfs_ioctl, which will get the BKL again if needed.

Handlers doing more than those two have been moved out, so we
can kill off the BKL from compat_sys_ioctl. This may significantly
improve latencies with 32 bit applications, and it avoids a common
scenario where a thread acquires the BKL twice.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/compat_ioctl.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index 40904c6..320786e 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -2663,9 +2663,7 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
 
  found_handler:
 	if (t->handler) {
-		lock_kernel();
 		error = t->handler(fd, cmd, arg, filp);
-		unlock_kernel();
 		goto out_fput;
 	}
 
-- 
1.6.3.3

Re: [PATCH 1/2] compat_ioctl: remove VT specific ioctl handlers

[Greg KH]

On Wed, Oct 14, 2009 at 05:59:34PM +0200, Arnd Bergmann wrote:
> The VT driver now handles all of these ioctls directly, so
> we can remove the handlers from common code.
> 
> These are the only handlers that require the BKL because they
> directly perform the ioctl action rather than just converting
> the data structures. Once they are gone, we can remove the
> BKL from the remaining ioctl conversion handlers.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  fs/compat_ioctl.c |  188 +----------------------------------------------------
>  1 files changed, 1 insertions(+), 187 deletions(-)
> 
> I guess it makes sense to merge these two through the BKL removal
> queue.

That's fine with me:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

thanks,

greg k-h