[PATCH 1/3] input: remove BKL from mousedev

[PATCH 1/3] input: remove BKL from mousedev

[Thadeu Lima de Souza Cascardo]

There's no need for BKL in mousedev. It uses lots of locking already
that should handle the open case.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 drivers/input/mousedev.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c
index a13d80f..f34b22b 100644
--- a/drivers/input/mousedev.c
+++ b/drivers/input/mousedev.c
@@ -15,7 +15,6 @@
 
 #include <linux/sched.h>
 #include <linux/slab.h>
-#include <linux/smp_lock.h>
 #include <linux/poll.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -542,10 +541,8 @@ static int mousedev_open(struct inode *inode, struct file *file)
 	if (i >= MOUSEDEV_MINORS)
 		return -ENODEV;
 
-	lock_kernel();
 	error = mutex_lock_interruptible(&mousedev_table_mutex);
 	if (error) {
-		unlock_kernel();
 		return error;
 	}
 	mousedev = mousedev_table[i];
@@ -554,7 +551,6 @@ static int mousedev_open(struct inode *inode, struct file *file)
 	mutex_unlock(&mousedev_table_mutex);
 
 	if (!mousedev) {
-		unlock_kernel();
 		return -ENODEV;
 	}
 
@@ -575,7 +571,6 @@ static int mousedev_open(struct inode *inode, struct file *file)
 		goto err_free_client;
 
 	file->private_data = client;
-	unlock_kernel();
 	return 0;
 
  err_free_client:
@@ -583,7 +578,6 @@ static int mousedev_open(struct inode *inode, struct file *file)
 	kfree(client);
  err_put_mousedev:
 	put_device(&mousedev->dev);
-	unlock_kernel();
 	return error;
 }
 
-- 
1.6.6.1

[PATCH 2/3] input: use input_mutex instead of BKL when opening input device

[Thadeu Lima de Souza Cascardo]

There are three types of input character devices: mousedev, joydev and
evdev. They all use the same major device and, then, their opening is
multiplexed in input_open_file.

After the BKL pushdown, this multiplexing is protected by the BKL. But
there's already a mutex used for adding and removing to the table
indexed by input_open_file.

So, we use this mutex instead of the BKL. Since it will call the
handlers' open function under the mutex, I've checked that the current
functions do not call input_{un,}register_{handler,device}, which are
the only other functions to take the mutex.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 drivers/input/input.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 41168d5..af9c246 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -23,7 +23,6 @@
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/rcupdate.h>
-#include <linux/smp_lock.h>
 #include "input-compat.h"
 
 MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
@@ -1881,7 +1880,7 @@ static int input_open_file(struct inode *inode, struct file *file)
 	const struct file_operations *old_fops, *new_fops = NULL;
 	int err;
 
-	lock_kernel();
+	mutex_lock(&input_mutex);
 	/* No load-on-demand here? */
 	handler = input_table[iminor(inode) >> 5];
 	if (!handler || !(new_fops = fops_get(handler->fops))) {
@@ -1909,7 +1908,7 @@ static int input_open_file(struct inode *inode, struct file *file)
 	}
 	fops_put(old_fops);
 out:
-	unlock_kernel();
+	mutex_unlock(&input_mutex);
 	return err;
 }
 
-- 
1.6.6.1

[PATCH 3/3] input: remove BKL from serio_raw

[Thadeu Lima de Souza Cascardo]

serio_raw open function already uses a mutex.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
---
 drivers/input/serio/serio_raw.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/drivers/input/serio/serio_raw.c b/drivers/input/serio/serio_raw.c
index 27fdaaf..debe4a1 100644
--- a/drivers/input/serio/serio_raw.c
+++ b/drivers/input/serio/serio_raw.c
@@ -81,10 +81,9 @@ static int serio_raw_open(struct inode *inode, struct file *file)
 	struct serio_raw_list *list;
 	int retval = 0;
 
-	lock_kernel();
 	retval = mutex_lock_interruptible(&serio_raw_mutex);
 	if (retval)
-		goto out_bkl;
+		return retval;
 
 	if (!(serio_raw = serio_raw_locate(iminor(inode)))) {
 		retval = -ENODEV;
@@ -109,8 +108,6 @@ static int serio_raw_open(struct inode *inode, struct file *file)
 
 out:
 	mutex_unlock(&serio_raw_mutex);
-out_bkl:
-	unlock_kernel();
 	return retval;
 }
 
-- 
1.6.6.1

Re: [PATCH 3/3] input: remove BKL from serio_raw

[Arnd Bergmann]

On Tuesday 09 March 2010, Thadeu Lima de Souza Cascardo wrote:
> serio_raw open function already uses a mutex.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 2/3] input: use input_mutex instead of BKL when opening input device

[Arnd Bergmann]

On Tuesday 09 March 2010, Thadeu Lima de Souza Cascardo wrote:
> There are three types of input character devices: mousedev, joydev and
> evdev. They all use the same major device and, then, their opening is
> multiplexed in input_open_file.
> 
> After the BKL pushdown, this multiplexing is protected by the BKL. But
> there's already a mutex used for adding and removing to the table
> indexed by input_open_file.
> 
> So, we use this mutex instead of the BKL. Since it will call the
> handlers' open function under the mutex, I've checked that the current
> functions do not call input_{un,}register_{handler,device}, which are
> the only other functions to take the mutex.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH 2/3] input: use input_mutex instead of BKL when opening input device

[Arnd Bergmann]

On Tuesday 09 March 2010, Thadeu Lima de Souza Cascardo wrote:
>  MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
> @@ -1881,7 +1880,7 @@ static int input_open_file(struct inode *inode, struct file *file)
>         const struct file_operations *old_fops, *new_fops = NULL;
>         int err;
>  
> -       lock_kernel();
> +       mutex_lock(&input_mutex);
>         /* No load-on-demand here? */
>         handler = input_table[iminor(inode) >> 5];
>         if (!handler || !(new_fops = fops_get(handler->fops))) {
> @@ -1909,7 +1908,7 @@ static int input_open_file(struct inode *inode, struct file *file)
>         }
>         fops_put(old_fops);
>  out:
> -       unlock_kernel();
> +       mutex_unlock(&input_mutex);
>         return err;
>  }
>  
> -- 

Well, actually please have a look at
http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=commitdiff;h=c06fd0234357618a5741ce958d58901ae4cb7ac1

* use mutex_lock_interruptible() where possible
* you probably don't want to hold input_mutex when calling into the lower
  device's open function

	Arnd

Re: [PATCH 2/3] input: use input_mutex instead of BKL when opening input device

[Thadeu Lima de Souza Cascardo]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On Tue, Mar 09, 2010 at 09:12:13PM +0100, Arnd Bergmann wrote:
> On Tuesday 09 March 2010, Thadeu Lima de Souza Cascardo wrote:
> >  MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
> > @@ -1881,7 +1880,7 @@ static int input_open_file(struct inode *inode, struct file *file)
> >         const struct file_operations *old_fops, *new_fops = NULL;
> >         int err;
> >  
> > -       lock_kernel();
> > +       mutex_lock(&input_mutex);
> >         /* No load-on-demand here? */
> >         handler = input_table[iminor(inode) >> 5];
> >         if (!handler || !(new_fops = fops_get(handler->fops))) {
> > @@ -1909,7 +1908,7 @@ static int input_open_file(struct inode *inode, struct file *file)
> >         }
> >         fops_put(old_fops);
> >  out:
> > -       unlock_kernel();
> > +       mutex_unlock(&input_mutex);
> >         return err;
> >  }
> >  
> > -- 
> 
> Well, actually please have a look at
> http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=commitdiff;h=c06fd0234357618a5741ce958d58901ae4cb7ac1
> 
> * use mutex_lock_interruptible() where possible
> * you probably don't want to hold input_mutex when calling into the lower
>   device's open function
> 
> 	Arnd

Yeah. I was just looking at your appointed branch. And I've noticed your
change was better.

I was willing to let the open call go unprotected. But ended up checking
that the three callees were fine (they do not call any of the other
functions that take the mutex).

Since the fops_put/fops_get do protect that section from the handler
removal and I can't think of any other race right now, I think your
version is really better.

Acked-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>

Regards,
Cascardo.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/3] input: use input_mutex instead of BKL when opening input device

[Dmitry Torokhov]

On Tue, Mar 09, 2010 at 05:59:48PM -0300, Thadeu Lima de Souza Cascardo wrote:
> On Tue, Mar 09, 2010 at 09:12:13PM +0100, Arnd Bergmann wrote:
> > On Tuesday 09 March 2010, Thadeu Lima de Souza Cascardo wrote:
> > >  MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
> > > @@ -1881,7 +1880,7 @@ static int input_open_file(struct inode *inode, struct file *file)
> > >         const struct file_operations *old_fops, *new_fops = NULL;
> > >         int err;
> > >  
> > > -       lock_kernel();
> > > +       mutex_lock(&input_mutex);
> > >         /* No load-on-demand here? */
> > >         handler = input_table[iminor(inode) >> 5];
> > >         if (!handler || !(new_fops = fops_get(handler->fops))) {
> > > @@ -1909,7 +1908,7 @@ static int input_open_file(struct inode *inode, struct file *file)
> > >         }
> > >         fops_put(old_fops);
> > >  out:
> > > -       unlock_kernel();
> > > +       mutex_unlock(&input_mutex);
> > >         return err;
> > >  }
> > >  
> > > -- 
> > 
> > Well, actually please have a look at
> > http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=commitdiff;h=c06fd0234357618a5741ce958d58901ae4cb7ac1
> > 
> > * use mutex_lock_interruptible() where possible
> > * you probably don't want to hold input_mutex when calling into the lower
> >   device's open function
> > 
> > 	Arnd
> 
> Yeah. I was just looking at your appointed branch. And I've noticed your
> change was better.
> 
> I was willing to let the open call go unprotected. But ended up checking
> that the three callees were fine (they do not call any of the other
> functions that take the mutex).
> 
> Since the fops_put/fops_get do protect that section from the handler
> removal and I can't think of any other race right now, I think your
> version is really better.
> 
> Acked-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> 

OK, applied 2 Thadeu's patches and one Arnd's.

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] input: use input_mutex instead of BKL when opening input device

[Oliver Neukum]

Am Mittwoch, 10. März 2010 07:20:15 schrieb Dmitry Torokhov:
> > I was willing to let the open call go unprotected. But ended up checking
> > that the three callees were fine (they do not call any of the other
> > functions that take the mutex).
> > 
> > Since the fops_put/fops_get do protect that section from the handler
> > removal and I can't think of any other race right now, I think your
> > version is really better.
> > 
> > Acked-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> > 
> 
> OK, applied 2 Thadeu's patches and one Arnd's.

Jiri,

it is possible that this requires a change in usbhid as it uses BKL.
Do you remember?

	Regards
		Oliver

Re: [PATCH 2/3] input: use input_mutex instead of BKL when opening input device

[Dmitry Torokhov]

On Tuesday 09 March 2010 10:49:20 pm Oliver Neukum wrote:
> Am Mittwoch, 10. März 2010 07:20:15 schrieb Dmitry Torokhov:
> > > I was willing to let the open call go unprotected. But ended up
> > > checking that the three callees were fine (they do not call any of the
> > > other functions that take the mutex).
> > > 
> > > Since the fops_put/fops_get do protect that section from the handler
> > > removal and I can't think of any other race right now, I think your
> > > version is really better.
> > > 
> > > Acked-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> > 
> > OK, applied 2 Thadeu's patches and one Arnd's.
> 
> Jiri,
> 
> it is possible that this requires a change in usbhid as it uses BKL.
> Do you remember?
>

No, different layer altogether.

-- 
Dmitry

Re: [PATCH 1/3] input: remove BKL from mousedev

[Arnd Bergmann]

On Tuesday 09 March 2010, Thadeu Lima de Souza Cascardo wrote:
> There's no need for BKL in mousedev. It uses lots of locking already
> that should handle the open case.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

I actually have the same ones in
http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=shortlog;h=refs/heads/bkl-removal
(yes, that's broken), so I'd really like to see them go in.

	Arnd