[PATCH] mem_class: Drop the bkl from memory_open()

[PATCH] mem_class: Drop the bkl from memory_open()

[Frederic Weisbecker]

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: 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>
---
 drivers/char/mem.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index a074fce..599846a 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -892,29 +892,23 @@ static int memory_open(struct inode *inode, struct file *filp)
 {
 	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 = {
-- 
1.6.2.3

Re: [PATCH] mem_class: Drop the bkl from memory_open()

[Thomas Gleixner]

On Fri, 9 Oct 2009, Frederic Weisbecker wrote:

> 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.

Thanks, queued.

	tglx

> 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>
> ---
>  drivers/char/mem.c |   16 +++++-----------
>  1 files changed, 5 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index a074fce..599846a 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -892,29 +892,23 @@ static int memory_open(struct inode *inode, struct file *filp)
>  {
>  	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 = {
> -- 
> 1.6.2.3
> 

[tip:bkl/drivers] mem_class: Drop the bkl from memory_open()

[tip-bot for Frederic Weisbecker]

Commit-ID:  205153aa40b7fb36dc7fe76c1798584ace55b288
Gitweb:     http://git.kernel.org/tip/205153aa40b7fb36dc7fe76c1798584ace55b288
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Fri, 9 Oct 2009 20:31:02 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 14 Oct 2009 17:36:49 +0200

mem_class: Drop the bkl from memory_open()

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>
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 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index a074fce..ad82ec9 100644
--- a/drivers/char/mem.c
+++ b/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 *inode, struct file *filp)
 {
 	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 = {