[PATCH, RFC] uio BKL removal

[PATCH, RFC] uio BKL removal

[Jonathan Corbet]

I had a moment to dedicate to the BKL removal cause, so I went for the
UIO driver, which seemed simple.  The main thing I found was that there
was locking around some idr accesses, but not all, so I filled that
in.  With that in place, removing the BKL from uio_open() seems safe,
especially since none of the in-tree UIO drivers have open() or
release() methods.  

(Incidentally, I don't see how uio_pdrv.c could ever work - who initializes
uioinfo?)

If there's no complaints, I'll feed this into linux-next via the
bkl-removal tree.

Thanks,

jon

---
UIO: BKL removal

Remove the BKL from the UIO driver, and add complete locking where needed
to serialize idr accesses.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 3a6934b..4f28f4b 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -47,6 +47,9 @@ static struct uio_class {
 	struct class *class;
 } *uio_class;
 
+/* Protect idr accesses */
+static DEFINE_MUTEX(minor_lock);
+
 /*
  * attributes
  */
@@ -231,7 +234,6 @@ static void uio_dev_del_attributes(struct uio_device *idev)
 
 static int uio_get_minor(struct uio_device *idev)
 {
-	static DEFINE_MUTEX(minor_lock);
 	int retval = -ENOMEM;
 	int id;
 
@@ -253,7 +255,9 @@ exit:
 
 static void uio_free_minor(struct uio_device *idev)
 {
+	mutex_lock(&minor_lock);
 	idr_remove(&uio_idr, idev->minor);
+	mutex_unlock(&minor_lock);
 }
 
 /**
@@ -297,8 +301,9 @@ static int uio_open(struct inode *inode, struct file *filep)
 	struct uio_listener *listener;
 	int ret = 0;
 
-	lock_kernel();
+	mutex_lock(&minor_lock);
 	idev = idr_find(&uio_idr, iminor(inode));
+	mutex_unlock(&minor_lock);
 	if (!idev) {
 		ret = -ENODEV;
 		goto out;
@@ -324,18 +329,15 @@ static int uio_open(struct inode *inode, struct file *filep)
 		if (ret)
 			goto err_infoopen;
 	}
-	unlock_kernel();
 	return 0;
 
 err_infoopen:
-
 	kfree(listener);
-err_alloc_listener:
 
+err_alloc_listener:
 	module_put(idev->owner);
 
 out:
-	unlock_kernel();
 	return ret;
 }
 

Re: [PATCH, RFC] uio BKL removal

[Greg KH]

On Tue, Aug 26, 2008 at 05:15:32PM -0600, Jonathan Corbet wrote:
> I had a moment to dedicate to the BKL removal cause, so I went for the
> UIO driver, which seemed simple.  The main thing I found was that there
> was locking around some idr accesses, but not all, so I filled that
> in.  With that in place, removing the BKL from uio_open() seems safe,
> especially since none of the in-tree UIO drivers have open() or
> release() methods.  
> 
> (Incidentally, I don't see how uio_pdrv.c could ever work - who initializes
> uioinfo?)
> 
> If there's no complaints, I'll feed this into linux-next via the
> bkl-removal tree.

Looks good to me, feel free to add an:
	Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
to it.

greg k-h

Re: [PATCH, RFC] uio BKL removal

[Hans J. Koch]

On Tue, Aug 26, 2008 at 05:15:32PM -0600, Jonathan Corbet wrote:
> I had a moment to dedicate to the BKL removal cause, so I went for the
> UIO driver, which seemed simple.  The main thing I found was that there
> was locking around some idr accesses, but not all, so I filled that
> in.  With that in place, removing the BKL from uio_open() seems safe,
> especially since none of the in-tree UIO drivers have open() or
> release() methods.  
> 
> (Incidentally, I don't see how uio_pdrv.c could ever work - who initializes
> uioinfo?)

That has to be done somewhere in your board specific file, e.g. for ARM
boards in arch/arm/mach-xxx/board-something.c - the same place where you
setup your struct platform_device.

> 
> If there's no complaints, I'll feed this into linux-next via the
> bkl-removal tree.

Seems right.

Acked-by: Hans J. Koch <hjk@linutronix.de>

> 
> Thanks,
> 
> jon
> 
> ---
> UIO: BKL removal
> 
> Remove the BKL from the UIO driver, and add complete locking where needed
> to serialize idr accesses.
> 
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 3a6934b..4f28f4b 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -47,6 +47,9 @@ static struct uio_class {
>  	struct class *class;
>  } *uio_class;
>  
> +/* Protect idr accesses */
> +static DEFINE_MUTEX(minor_lock);
> +
>  /*
>   * attributes
>   */
> @@ -231,7 +234,6 @@ static void uio_dev_del_attributes(struct uio_device *idev)
>  
>  static int uio_get_minor(struct uio_device *idev)
>  {
> -	static DEFINE_MUTEX(minor_lock);
>  	int retval = -ENOMEM;
>  	int id;
>  
> @@ -253,7 +255,9 @@ exit:
>  
>  static void uio_free_minor(struct uio_device *idev)
>  {
> +	mutex_lock(&minor_lock);
>  	idr_remove(&uio_idr, idev->minor);
> +	mutex_unlock(&minor_lock);
>  }
>  
>  /**
> @@ -297,8 +301,9 @@ static int uio_open(struct inode *inode, struct file *filep)
>  	struct uio_listener *listener;
>  	int ret = 0;
>  
> -	lock_kernel();
> +	mutex_lock(&minor_lock);
>  	idev = idr_find(&uio_idr, iminor(inode));
> +	mutex_unlock(&minor_lock);
>  	if (!idev) {
>  		ret = -ENODEV;
>  		goto out;
> @@ -324,18 +329,15 @@ static int uio_open(struct inode *inode, struct file *filep)
>  		if (ret)
>  			goto err_infoopen;
>  	}
> -	unlock_kernel();
>  	return 0;
>  
>  err_infoopen:
> -
>  	kfree(listener);
> -err_alloc_listener:
>  
> +err_alloc_listener:
>  	module_put(idev->owner);
>  
>  out:
> -	unlock_kernel();
>  	return ret;
>  }
>  

Re: [PATCH, RFC] uio BKL removal

[Alexey Dobriyan]

On Tue, Aug 26, 2008 at 05:15:32PM -0600, Jonathan Corbet wrote:
> UIO: BKL removal
> 
> Remove the BKL from the UIO driver, and add complete locking where needed
> to serialize idr accesses.

> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -47,6 +47,9 @@ static struct uio_class {
>  	struct class *class;
>  } *uio_class;
>  
> +/* Protect idr accesses */

You can rename it to uio_idr_mutex (it's mutex, dammit!) and put next to
uio_idr and avoid comment ;-) Or if locking around _pre_get() isn't
necessary you can actually make it spinlock ;-) Anyway, put it close.

> +static DEFINE_MUTEX(minor_lock);

Indeed, BKL protects nothing because of GFP_KERNEL allocation.