[PATCH 0/5] Uio enhancements

[PATCH 0/5] Uio enhancements

[Eric W. Biederman]

Looking through my patch queue I stumbled upon some uio changes
that I have been using for a while and I forgot to push upstream.

Ultimately I am working on making uio devices hoptlugable, but I figure
I should get these enhancements in first.

Eric W. Biederman (5):
      uio: Fix lack of locking in init_uio_class
      uio: Don't clear driver data
      uio: Cleanup irq handling.
      uio: Support 2^MINOR_BITS minors
      uio: Statically allocate uio_class and use class .dev_attrs.
---
 drivers/uio/uio.c          |  165 +++++++++++++++++++-------------------------
 include/linux/uio_driver.h |    2 +-
 2 files changed, 71 insertions(+), 96 deletions(-)

[PATCH 1/5] uio: Fix lack of locking in init_uio_class

[Eric W. Biederman]

There is no locking in init_uio_class so multiple
drivers can race and create multiple uio classes.

Fix this by simplifying the code.   In particular always
register the uio class during module_init and make things
simpler.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |   58 +++++++++++++---------------------------------------
 1 files changed, 15 insertions(+), 43 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index bff1afb..bc774cc 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -45,10 +45,7 @@ static DEFINE_IDR(uio_idr);
 static const struct file_operations uio_fops;
 
 /* UIO class infrastructure */
-static struct uio_class {
-	struct kref kref;
-	struct class *class;
-} *uio_class;
+static struct class *uio_class;
 
 /* Protect idr accesses */
 static DEFINE_MUTEX(minor_lock);
@@ -757,55 +754,35 @@ static void uio_major_cleanup(void)
 
 static int init_uio_class(void)
 {
-	int ret = 0;
-
-	if (uio_class != NULL) {
-		kref_get(&uio_class->kref);
-		goto exit;
-	}
+	struct class *class;
+	int ret;
 
 	/* This is the first time in here, set everything up properly */
 	ret = uio_major_init();
 	if (ret)
 		goto exit;
 
-	uio_class = kzalloc(sizeof(*uio_class), GFP_KERNEL);
-	if (!uio_class) {
-		ret = -ENOMEM;
-		goto err_kzalloc;
-	}
-
-	kref_init(&uio_class->kref);
-	uio_class->class = class_create(THIS_MODULE, "uio");
-	if (IS_ERR(uio_class->class)) {
-		ret = IS_ERR(uio_class->class);
+	class = class_create(THIS_MODULE, "uio");
+	if (IS_ERR(class)) {
+		ret = IS_ERR(class);
 		printk(KERN_ERR "class_create failed for uio\n");
 		goto err_class_create;
 	}
+	uio_class = class;
 	return 0;
 
 err_class_create:
-	kfree(uio_class);
-	uio_class = NULL;
-err_kzalloc:
 	uio_major_cleanup();
 exit:
 	return ret;
 }
 
-static void release_uio_class(struct kref *kref)
+static void release_uio_class(void)
 {
 	/* Ok, we cheat as we know we only have one uio_class */
-	class_destroy(uio_class->class);
-	kfree(uio_class);
-	uio_major_cleanup();
+	class_destroy(uio_class);
 	uio_class = NULL;
-}
-
-static void uio_class_destroy(void)
-{
-	if (uio_class)
-		kref_put(&uio_class->kref, release_uio_class);
+	uio_major_cleanup();
 }
 
 /**
@@ -828,10 +805,6 @@ int __uio_register_device(struct module *owner,
 
 	info->uio_dev = NULL;
 
-	ret = init_uio_class();
-	if (ret)
-		return ret;
-
 	idev = kzalloc(sizeof(*idev), GFP_KERNEL);
 	if (!idev) {
 		ret = -ENOMEM;
@@ -847,7 +820,7 @@ int __uio_register_device(struct module *owner,
 	if (ret)
 		goto err_get_minor;
 
-	idev->dev = device_create(uio_class->class, parent,
+	idev->dev = device_create(uio_class, parent,
 				  MKDEV(uio_major, idev->minor), idev,
 				  "uio%d", idev->minor);
 	if (IS_ERR(idev->dev)) {
@@ -874,13 +847,12 @@ int __uio_register_device(struct module *owner,
 err_request_irq:
 	uio_dev_del_attributes(idev);
 err_uio_dev_add_attributes:
-	device_destroy(uio_class->class, MKDEV(uio_major, idev->minor));
+	device_destroy(uio_class, MKDEV(uio_major, idev->minor));
 err_device_create:
 	uio_free_minor(idev);
 err_get_minor:
 	kfree(idev);
 err_kzalloc:
-	uio_class_destroy();
 	return ret;
 }
 EXPORT_SYMBOL_GPL(__uio_register_device);
@@ -907,9 +879,8 @@ void uio_unregister_device(struct uio_info *info)
 	uio_dev_del_attributes(idev);
 
 	dev_set_drvdata(idev->dev, NULL);
-	device_destroy(uio_class->class, MKDEV(uio_major, idev->minor));
+	device_destroy(uio_class, MKDEV(uio_major, idev->minor));
 	kfree(idev);
-	uio_class_destroy();
 
 	return;
 }
@@ -917,11 +888,12 @@ EXPORT_SYMBOL_GPL(uio_unregister_device);
 
 static int __init uio_init(void)
 {
-	return 0;
+	return init_uio_class();
 }
 
 static void __exit uio_exit(void)
 {
+	release_uio_class();
 }
 
 module_init(uio_init)
-- 
1.7.2.2

[PATCH 2/5] uio: Don't clear driver data

[Eric W. Biederman]

Currently uio sets it's driver data to NULL just as it is unregistering
attributes.  sysfs maks the guaranatee that it will not call attributes
after device_destroy is called so this is unncessary and leads to lots
of unnecessary code in uio.c

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |   17 +++--------------
 1 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index bc774cc..8132288 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -229,10 +229,7 @@ static ssize_t show_name(struct device *dev,
 			 struct device_attribute *attr, char *buf)
 {
 	struct uio_device *idev = dev_get_drvdata(dev);
-	if (idev)
-		return sprintf(buf, "%s\n", idev->info->name);
-	else
-		return -ENODEV;
+	return sprintf(buf, "%s\n", idev->info->name);
 }
 static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
 
@@ -240,10 +237,7 @@ static ssize_t show_version(struct device *dev,
 			    struct device_attribute *attr, char *buf)
 {
 	struct uio_device *idev = dev_get_drvdata(dev);
-	if (idev)
-		return sprintf(buf, "%s\n", idev->info->version);
-	else
-		return -ENODEV;
+	return sprintf(buf, "%s\n", idev->info->version);
 }
 static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
@@ -251,11 +245,7 @@ static ssize_t show_event(struct device *dev,
 			  struct device_attribute *attr, char *buf)
 {
 	struct uio_device *idev = dev_get_drvdata(dev);
-	if (idev)
-		return sprintf(buf, "%u\n",
-				(unsigned int)atomic_read(&idev->event));
-	else
-		return -ENODEV;
+	return sprintf(buf, "%u\n", (unsigned int)atomic_read(&idev->event));
 }
 static DEVICE_ATTR(event, S_IRUGO, show_event, NULL);
 
@@ -878,7 +868,6 @@ void uio_unregister_device(struct uio_info *info)
 
 	uio_dev_del_attributes(idev);
 
-	dev_set_drvdata(idev->dev, NULL);
 	device_destroy(uio_class, MKDEV(uio_major, idev->minor));
 	kfree(idev);
 
-- 
1.7.2.2

[PATCH 3/5] uio: Cleanup irq handling.

[Eric W. Biederman]

Change the value of UIO_IRQ_NONE -2 to 0.  0 is well defined in the rest
of the kernel as the value to indicate an irq has not been assigned.

Update the calls to request_irq and free_irq to only ignore UIO_IRQ_NONE
and UIO_IRQ_CUSTOM allowing the rest of the kernel's possible irq
numbers to be used.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c          |   14 +++++++-------
 include/linux/uio_driver.h |    2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 8132288..0fd2cda 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -512,7 +512,7 @@ static unsigned int uio_poll(struct file *filep, poll_table *wait)
 	struct uio_listener *listener = filep->private_data;
 	struct uio_device *idev = listener->dev;
 
-	if (idev->info->irq == UIO_IRQ_NONE)
+	if (!idev->info->irq)
 		return -EIO;
 
 	poll_wait(filep, &idev->wait, wait);
@@ -530,7 +530,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 	ssize_t retval;
 	s32 event_count;
 
-	if (idev->info->irq == UIO_IRQ_NONE)
+	if (!idev->info->irq)
 		return -EIO;
 
 	if (count != sizeof(s32))
@@ -578,7 +578,7 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
 	ssize_t retval;
 	s32 irq_on;
 
-	if (idev->info->irq == UIO_IRQ_NONE)
+	if (!idev->info->irq)
 		return -EIO;
 
 	if (count != sizeof(s32))
@@ -825,9 +825,9 @@ int __uio_register_device(struct module *owner,
 
 	info->uio_dev = idev;
 
-	if (idev->info->irq >= 0) {
-		ret = request_irq(idev->info->irq, uio_interrupt,
-				  idev->info->irq_flags, idev->info->name, idev);
+	if (info->irq && (info->irq != UIO_IRQ_CUSTOM)) {
+		ret = request_irq(info->irq, uio_interrupt,
+				  info->irq_flags, info->name, idev);
 		if (ret)
 			goto err_request_irq;
 	}
@@ -863,7 +863,7 @@ void uio_unregister_device(struct uio_info *info)
 
 	uio_free_minor(idev);
 
-	if (info->irq >= 0)
+	if (info->irq && (info->irq != UIO_IRQ_CUSTOM))
 		free_irq(info->irq, idev);
 
 	uio_dev_del_attributes(idev);
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 5dcc9ff..d6188e5 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -108,7 +108,7 @@ extern void uio_event_notify(struct uio_info *info);
 
 /* defines for uio_info->irq */
 #define UIO_IRQ_CUSTOM	-1
-#define UIO_IRQ_NONE	-2
+#define UIO_IRQ_NONE	0
 
 /* defines for uio_mem->memtype */
 #define UIO_MEM_NONE	0
-- 
1.7.2.2

[PATCH 4/5] uio: Support 2^MINOR_BITS minors

[Eric W. Biederman]

register_chrdev limits uio devices to 256 minor numbers which causes
problems on one system I have with 384+ uio devices.  So instead set
UIO_MAX_DEVICES to the maximum number of minors and use
alloc_chrdev_region to reserve the uio minors.

The final result is that the code works the same but the uio driver now
supports any minor the idr allocator comes up with.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |   43 +++++++++++++++++++++++++++++++++++++------
 1 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 0fd2cda..3d4d65b 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -23,9 +23,10 @@
 #include <linux/sched.h>
 #include <linux/string.h>
 #include <linux/kobject.h>
+#include <linux/cdev.h>
 #include <linux/uio_driver.h>
 
-#define UIO_MAX_DEVICES 255
+#define UIO_MAX_DEVICES		(1U << MINORBITS)
 
 struct uio_device {
 	struct module		*owner;
@@ -41,6 +42,7 @@ struct uio_device {
 };
 
 static int uio_major;
+static struct cdev *uio_cdev;
 static DEFINE_IDR(uio_idr);
 static const struct file_operations uio_fops;
 
@@ -731,15 +733,44 @@ static const struct file_operations uio_fops = {
 
 static int uio_major_init(void)
 {
-	uio_major = register_chrdev(0, "uio", &uio_fops);
-	if (uio_major < 0)
-		return uio_major;
-	return 0;
+	static const char name[] = "uio";
+	struct cdev *cdev = NULL;
+	dev_t uio_dev = 0;
+	int result;
+
+	result = alloc_chrdev_region(&uio_dev, 0, UIO_MAX_DEVICES, name);
+	if (result)
+		goto out;
+
+	result = -ENOMEM;
+	cdev = cdev_alloc();
+	if (!cdev)
+		goto out_unregister;
+
+	cdev->owner = THIS_MODULE;
+	cdev->ops = &uio_fops;
+	kobject_set_name(&cdev->kobj, "%s", name);
+
+	result = cdev_add(cdev, uio_dev, UIO_MAX_DEVICES);
+	if (result)
+		goto out_put;
+
+	uio_major = MAJOR(uio_dev);
+	uio_cdev = cdev;
+	result = 0;
+out:
+	return result;
+out_put:
+	kobject_put(&cdev->kobj);
+out_unregister:
+	unregister_chrdev_region(uio_dev, UIO_MAX_DEVICES);
+	goto out;
 }
 
 static void uio_major_cleanup(void)
 {
-	unregister_chrdev(uio_major, "uio");
+	unregister_chrdev_region(MKDEV(uio_major, 0), UIO_MAX_DEVICES);
+	cdev_del(uio_cdev);
 }
 
 static int init_uio_class(void)
-- 
1.7.2.2

[PATCH 5/5] uio: Statically allocate uio_class and use class .dev_attrs.

[Eric W. Biederman]

Instead of adding uio class attributes manually after the uio device has
been created and we have sent a uevent to userspace, use the class
attribute mechanism.  This removes races and makes the code simpler.

At the same time don't bother to dynamically allocate a struct class for
uio, just declare one statically.  Less code is needed and it is easier
to set the class parameters.tune the class

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |   55 ++++++++++++++++++----------------------------------
 1 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 3d4d65b..6285afb 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -46,9 +46,6 @@ static struct cdev *uio_cdev;
 static DEFINE_IDR(uio_idr);
 static const struct file_operations uio_fops;
 
-/* UIO class infrastructure */
-static struct class *uio_class;
-
 /* Protect idr accesses */
 static DEFINE_MUTEX(minor_lock);
 
@@ -233,7 +230,6 @@ static ssize_t show_name(struct device *dev,
 	struct uio_device *idev = dev_get_drvdata(dev);
 	return sprintf(buf, "%s\n", idev->info->name);
 }
-static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
 
 static ssize_t show_version(struct device *dev,
 			    struct device_attribute *attr, char *buf)
@@ -241,7 +237,6 @@ static ssize_t show_version(struct device *dev,
 	struct uio_device *idev = dev_get_drvdata(dev);
 	return sprintf(buf, "%s\n", idev->info->version);
 }
-static DEVICE_ATTR(version, S_IRUGO, show_version, NULL);
 
 static ssize_t show_event(struct device *dev,
 			  struct device_attribute *attr, char *buf)
@@ -249,17 +244,18 @@ static ssize_t show_event(struct device *dev,
 	struct uio_device *idev = dev_get_drvdata(dev);
 	return sprintf(buf, "%u\n", (unsigned int)atomic_read(&idev->event));
 }
-static DEVICE_ATTR(event, S_IRUGO, show_event, NULL);
 
-static struct attribute *uio_attrs[] = {
-	&dev_attr_name.attr,
-	&dev_attr_version.attr,
-	&dev_attr_event.attr,
-	NULL,
+static struct device_attribute uio_class_attributes[] = {
+	__ATTR(name, S_IRUGO, show_name, NULL),
+	__ATTR(version, S_IRUGO, show_version, NULL),
+	__ATTR(event, S_IRUGO, show_event, NULL),
+	{}
 };
 
-static struct attribute_group uio_attr_grp = {
-	.attrs = uio_attrs,
+/* UIO class infrastructure */
+static struct class uio_class = {
+	.name = "uio",
+	.dev_attrs = uio_class_attributes,
 };
 
 /*
@@ -275,11 +271,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 	struct uio_map *map;
 	struct uio_port *port;
 	struct uio_portio *portio;
-
-	ret = sysfs_create_group(&idev->dev->kobj, &uio_attr_grp);
-	if (ret)
-		goto err_group;
-
+	
 	for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
 		mem = &idev->info->mem[mi];
 		if (mem->size == 0)
@@ -347,8 +339,6 @@ err_map:
 		kobject_put(&map->kobj);
 	}
 	kobject_put(idev->map_dir);
-	sysfs_remove_group(&idev->dev->kobj, &uio_attr_grp);
-err_group:
 	dev_err(idev->dev, "error creating sysfs files (%d)\n", ret);
 	return ret;
 }
@@ -374,8 +364,6 @@ static void uio_dev_del_attributes(struct uio_device *idev)
 		kobject_put(&port->portio->kobj);
 	}
 	kobject_put(idev->portio_dir);
-
-	sysfs_remove_group(&idev->dev->kobj, &uio_attr_grp);
 }
 
 static int uio_get_minor(struct uio_device *idev)
@@ -775,7 +763,6 @@ static void uio_major_cleanup(void)
 
 static int init_uio_class(void)
 {
-	struct class *class;
 	int ret;
 
 	/* This is the first time in here, set everything up properly */
@@ -783,16 +770,14 @@ static int init_uio_class(void)
 	if (ret)
 		goto exit;
 
-	class = class_create(THIS_MODULE, "uio");
-	if (IS_ERR(class)) {
-		ret = IS_ERR(class);
-		printk(KERN_ERR "class_create failed for uio\n");
-		goto err_class_create;
+	ret = class_register(&uio_class);
+	if (ret) {
+		printk(KERN_ERR "class_register failed for uio\n");
+		goto err_class_register;
 	}
-	uio_class = class;
 	return 0;
 
-err_class_create:
+err_class_register:
 	uio_major_cleanup();
 exit:
 	return ret;
@@ -800,9 +785,7 @@ exit:
 
 static void release_uio_class(void)
 {
-	/* Ok, we cheat as we know we only have one uio_class */
-	class_destroy(uio_class);
-	uio_class = NULL;
+	class_unregister(&uio_class);
 	uio_major_cleanup();
 }
 
@@ -841,7 +824,7 @@ int __uio_register_device(struct module *owner,
 	if (ret)
 		goto err_get_minor;
 
-	idev->dev = device_create(uio_class, parent,
+	idev->dev = device_create(&uio_class, parent,
 				  MKDEV(uio_major, idev->minor), idev,
 				  "uio%d", idev->minor);
 	if (IS_ERR(idev->dev)) {
@@ -868,7 +851,7 @@ int __uio_register_device(struct module *owner,
 err_request_irq:
 	uio_dev_del_attributes(idev);
 err_uio_dev_add_attributes:
-	device_destroy(uio_class, MKDEV(uio_major, idev->minor));
+	device_destroy(&uio_class, MKDEV(uio_major, idev->minor));
 err_device_create:
 	uio_free_minor(idev);
 err_get_minor:
@@ -899,7 +882,7 @@ void uio_unregister_device(struct uio_info *info)
 
 	uio_dev_del_attributes(idev);
 
-	device_destroy(uio_class, MKDEV(uio_major, idev->minor));
+	device_destroy(&uio_class, MKDEV(uio_major, idev->minor));
 	kfree(idev);
 
 	return;
-- 
1.7.2.2

Re: [PATCH 1/5] uio: Fix lack of locking in init_uio_class

[Thomas Gleixner]

On Tue, 14 Sep 2010, Eric W. Biederman wrote:

> 
> There is no locking in init_uio_class so multiple
> drivers can race and create multiple uio classes.
> 
> Fix this by simplifying the code.   In particular always
> register the uio class during module_init and make things
> simpler.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH 2/5] uio: Don't clear driver data

[Thomas Gleixner]

On Tue, 14 Sep 2010, Eric W. Biederman wrote:

> 
> Currently uio sets it's driver data to NULL just as it is unregistering
> attributes.  sysfs maks the guaranatee that it will not call attributes
  
  s/maks/makes/

> after device_destroy is called so this is unncessary and leads to lots
> of unnecessary code in uio.c
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH 3/5] uio: Cleanup irq handling.

[Thomas Gleixner]

On Tue, 14 Sep 2010, Eric W. Biederman wrote:

> 
> Change the value of UIO_IRQ_NONE -2 to 0.  0 is well defined in the rest
> of the kernel as the value to indicate an irq has not been assigned.
> 
> Update the calls to request_irq and free_irq to only ignore UIO_IRQ_NONE
> and UIO_IRQ_CUSTOM allowing the rest of the kernel's possible irq
> numbers to be used.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
 
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH 4/5] uio: Support 2^MINOR_BITS minors

[Thomas Gleixner]

On Tue, 14 Sep 2010, Eric W. Biederman wrote:

> 
> register_chrdev limits uio devices to 256 minor numbers which causes
> problems on one system I have with 384+ uio devices.  So instead set
> UIO_MAX_DEVICES to the maximum number of minors and use
> alloc_chrdev_region to reserve the uio minors.
> 
> The final result is that the code works the same but the uio driver now
> supports any minor the idr allocator comes up with.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>

One minor nit:

> +	result = alloc_chrdev_region(&uio_dev, 0, UIO_MAX_DEVICES, name);
> +	if (result)
> +		goto out;

  		return result;

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

> +
> +	result = -ENOMEM;
> +	cdev = cdev_alloc();
> +	if (!cdev)
> +		goto out_unregister;
> +
> +	cdev->owner = THIS_MODULE;
> +	cdev->ops = &uio_fops;
> +	kobject_set_name(&cdev->kobj, "%s", name);
> +
> +	result = cdev_add(cdev, uio_dev, UIO_MAX_DEVICES);
> +	if (result)
> +		goto out_put;
> +
> +	uio_major = MAJOR(uio_dev);
> +	uio_cdev = cdev;
> +	result = 0;
> +out:
> +	return result;
> +out_put:
> +	kobject_put(&cdev->kobj);
> +out_unregister:
> +	unregister_chrdev_region(uio_dev, UIO_MAX_DEVICES);
> +	goto out;
>  }
>  
>  static void uio_major_cleanup(void)
>  {
> -	unregister_chrdev(uio_major, "uio");
> +	unregister_chrdev_region(MKDEV(uio_major, 0), UIO_MAX_DEVICES);
> +	cdev_del(uio_cdev);
>  }
>  
>  static int init_uio_class(void)
> -- 
> 1.7.2.2
> 
> --
> 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/
> 

Re: [PATCH 5/5] uio: Statically allocate uio_class and use class .dev_attrs.

[Thomas Gleixner]

On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> 
> Instead of adding uio class attributes manually after the uio device has
> been created and we have sent a uevent to userspace, use the class
> attribute mechanism.  This removes races and makes the code simpler.
> 
> At the same time don't bother to dynamically allocate a struct class for
> uio, just declare one statically.  Less code is needed and it is easier
> to set the class parameters.tune the class
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH 1/5] uio: Fix lack of locking in init_uio_class

[Hans J. Koch]

On Fri, Sep 17, 2010 at 10:32:35PM +0200, Thomas Gleixner wrote:
> 
> 
> On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> 
> > 
> > There is no locking in init_uio_class so multiple
> > drivers can race and create multiple uio classes.
> > 
> > Fix this by simplifying the code.   In particular always
> > register the uio class during module_init and make things
> > simpler.
> > 
> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

Re: [PATCH 2/5] uio: Don't clear driver data

[Hans J. Koch]

On Fri, Sep 17, 2010 at 10:33:57PM +0200, Thomas Gleixner wrote:
> 
> 
> On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> 
> > 
> > Currently uio sets it's driver data to NULL just as it is unregistering
> > attributes.  sysfs maks the guaranatee that it will not call attributes
>   
>   s/maks/makes/
> 
> > after device_destroy is called so this is unncessary and leads to lots
> > of unnecessary code in uio.c
> > 
> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

Re: [PATCH 3/5] uio: Cleanup irq handling.

[Hans J. Koch]

On Fri, Sep 17, 2010 at 10:34:49PM +0200, Thomas Gleixner wrote:
> On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> 
> > 
> > Change the value of UIO_IRQ_NONE -2 to 0.  0 is well defined in the rest
> > of the kernel as the value to indicate an irq has not been assigned.
> > 
> > Update the calls to request_irq and free_irq to only ignore UIO_IRQ_NONE
> > and UIO_IRQ_CUSTOM allowing the rest of the kernel's possible irq
> > numbers to be used.
> > 
> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
>  
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

Re: [PATCH 4/5] uio: Support 2^MINOR_BITS minors

[Hans J. Koch]

On Fri, Sep 17, 2010 at 10:36:50PM +0200, Thomas Gleixner wrote:
> On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> 
> > 
> > register_chrdev limits uio devices to 256 minor numbers which causes
> > problems on one system I have with 384+ uio devices.  So instead set
> > UIO_MAX_DEVICES to the maximum number of minors and use
> > alloc_chrdev_region to reserve the uio minors.
> > 
> > The final result is that the code works the same but the uio driver now
> > supports any minor the idr allocator comes up with.
> > 
> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> 
> One minor nit:
> 
> > +	result = alloc_chrdev_region(&uio_dev, 0, UIO_MAX_DEVICES, name);
> > +	if (result)
> > +		goto out;
> 
>   		return result;

Well, ok... Greg, can you fix this when you take the patch into your tree?

> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

> 
> > +
> > +	result = -ENOMEM;
> > +	cdev = cdev_alloc();
> > +	if (!cdev)
> > +		goto out_unregister;
> > +
> > +	cdev->owner = THIS_MODULE;
> > +	cdev->ops = &uio_fops;
> > +	kobject_set_name(&cdev->kobj, "%s", name);
> > +
> > +	result = cdev_add(cdev, uio_dev, UIO_MAX_DEVICES);
> > +	if (result)
> > +		goto out_put;
> > +
> > +	uio_major = MAJOR(uio_dev);
> > +	uio_cdev = cdev;
> > +	result = 0;
> > +out:
> > +	return result;
> > +out_put:
> > +	kobject_put(&cdev->kobj);
> > +out_unregister:
> > +	unregister_chrdev_region(uio_dev, UIO_MAX_DEVICES);
> > +	goto out;
> >  }
> >  
> >  static void uio_major_cleanup(void)
> >  {
> > -	unregister_chrdev(uio_major, "uio");
> > +	unregister_chrdev_region(MKDEV(uio_major, 0), UIO_MAX_DEVICES);
> > +	cdev_del(uio_cdev);
> >  }
> >  
> >  static int init_uio_class(void)
> > -- 
> > 1.7.2.2
> > 
> > --
> > 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/
> > 

Re: [PATCH 5/5] uio: Statically allocate uio_class and use class .dev_attrs.

[Hans J. Koch]

On Fri, Sep 17, 2010 at 10:37:26PM +0200, Thomas Gleixner wrote:
> On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> > 
> > Instead of adding uio class attributes manually after the uio device has
> > been created and we have sent a uevent to userspace, use the class
> > attribute mechanism.  This removes races and makes the code simpler.
> > 
> > At the same time don't bother to dynamically allocate a struct class for
> > uio, just declare one statically.  Less code is needed and it is easier
> > to set the class parameters.tune the class
> > 
> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

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

Re: [PATCH 0/5] Uio enhancements

[Hans J. Koch]

On Tue, Sep 14, 2010 at 11:35:28AM -0700, Eric W. Biederman wrote:
> 
> Looking through my patch queue I stumbled upon some uio changes
> that I have been using for a while and I forgot to push upstream.
> 
> Ultimately I am working on making uio devices hoptlugable, but I figure
> I should get these enhancements in first.
> 
> Eric W. Biederman (5):
>       uio: Fix lack of locking in init_uio_class
>       uio: Don't clear driver data
>       uio: Cleanup irq handling.
>       uio: Support 2^MINOR_BITS minors
>       uio: Statically allocate uio_class and use class .dev_attrs.

Looks good to me, thanks a lot for your contribution!

Thanks,
Hans

> ---
>  drivers/uio/uio.c          |  165 +++++++++++++++++++-------------------------
>  include/linux/uio_driver.h |    2 +-
>  2 files changed, 71 insertions(+), 96 deletions(-)

Re: [PATCH 4/5] uio: Support 2^MINOR_BITS minors

[Greg KH]

On Fri, Sep 17, 2010 at 10:57:05PM +0200, Hans J. Koch wrote:
> On Fri, Sep 17, 2010 at 10:36:50PM +0200, Thomas Gleixner wrote:
> > On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> > 
> > > 
> > > register_chrdev limits uio devices to 256 minor numbers which causes
> > > problems on one system I have with 384+ uio devices.  So instead set
> > > UIO_MAX_DEVICES to the maximum number of minors and use
> > > alloc_chrdev_region to reserve the uio minors.
> > > 
> > > The final result is that the code works the same but the uio driver now
> > > supports any minor the idr allocator comes up with.
> > > 
> > > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> > 
> > One minor nit:
> > 
> > > +	result = alloc_chrdev_region(&uio_dev, 0, UIO_MAX_DEVICES, name);
> > > +	if (result)
> > > +		goto out;
> > 
> >   		return result;
> 
> Well, ok... Greg, can you fix this when you take the patch into your tree?

Yes I can, and will.

thanks,

greg k-h

[PATCH 0/5] uio hotplug support

[Eric W. Biederman]

Implement the ability to hotunplug a uio device while file handles
are still open without crashing.

I have written the code very carefully, and this survives my
basic testing.  So at the very least this is better than
what is present in uio today.

I have implemented the ``locking'' for hotunplug support in
a generic library, that should be reusable to make this
kind of support easier to add in other pieces of the kernel.

Eric W. Biederman (5):
      uio: Simplify the lifetime logic of struct uio_device.
      uio: Kill unused vma_count.
      uio: Remove unused uio_info mmap method.
      libunload: A library to help remove open files
      uio: Implement hotunplug support, using libunload

---
 drivers/uio/uio.c          |  326 ++++++++++++++++++++++++++++++--------------
 fs/Makefile                |    2 +-
 fs/libunload.c             |  166 ++++++++++++++++++++++
 include/linux/uio_driver.h |   11 +--
 include/linux/unload.h     |   33 +++++
 5 files changed, 426 insertions(+), 112 deletions(-)

[PATCH 1/5] uio: Simplify the lifetime logic of struct uio_device.

[Eric W. Biederman]

Embed struct device into struct uio_device, and use
the refcounting and the release method of struct device
to control struct uio_device.

This allows device_create and device_destroy to be replaced
with the more standard device_register and device_unregister,
and allows the struct device reference count to act as a
reference count on struct idev as well.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |   42 ++++++++++++++++++++++++++++--------------
 1 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 6285afb..565559c 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -30,7 +30,7 @@
 
 struct uio_device {
 	struct module		*owner;
-	struct device		*dev;
+	struct device		device;
 	int			minor;
 	atomic_t		event;
 	struct fasync_struct	*async_queue;
@@ -279,7 +279,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		if (!map_found) {
 			map_found = 1;
 			idev->map_dir = kobject_create_and_add("maps",
-							&idev->dev->kobj);
+							&idev->device.kobj);
 			if (!idev->map_dir)
 				goto err_map;
 		}
@@ -304,7 +304,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		if (!portio_found) {
 			portio_found = 1;
 			idev->portio_dir = kobject_create_and_add("portio",
-							&idev->dev->kobj);
+							&idev->device.kobj);
 			if (!idev->portio_dir)
 				goto err_portio;
 		}
@@ -339,7 +339,7 @@ err_map:
 		kobject_put(&map->kobj);
 	}
 	kobject_put(idev->map_dir);
-	dev_err(idev->dev, "error creating sysfs files (%d)\n", ret);
+	dev_err(&idev->device, "error creating sysfs files (%d)\n", ret);
 	return ret;
 }
 
@@ -789,6 +789,13 @@ static void release_uio_class(void)
 	uio_major_cleanup();
 }
 
+static void uio_device_release(struct device *dev)
+{
+	struct uio_device *idev = dev_get_drvdata(dev);
+
+	kfree(idev);
+}
+
 /**
  * uio_register_device - register a new userspace IO device
  * @owner:	module that creates the new device
@@ -824,14 +831,19 @@ int __uio_register_device(struct module *owner,
 	if (ret)
 		goto err_get_minor;
 
-	idev->dev = device_create(&uio_class, parent,
-				  MKDEV(uio_major, idev->minor), idev,
-				  "uio%d", idev->minor);
-	if (IS_ERR(idev->dev)) {
-		printk(KERN_ERR "UIO: device register failed\n");
-		ret = PTR_ERR(idev->dev);
+	idev->device.devt = MKDEV(uio_major, idev->minor);
+	idev->device.class = &uio_class;
+	idev->device.parent = parent;
+	idev->device.release = uio_device_release;
+	dev_set_drvdata(&idev->device, idev);
+
+	ret = kobject_set_name(&idev->device.kobj, "uio%d", idev->minor);
+	if (ret)
+		goto err_device_create;
+
+	ret = device_register(&idev->device);
+	if (ret)
 		goto err_device_create;
-	}
 
 	ret = uio_dev_add_attributes(idev);
 	if (ret)
@@ -851,7 +863,10 @@ int __uio_register_device(struct module *owner,
 err_request_irq:
 	uio_dev_del_attributes(idev);
 err_uio_dev_add_attributes:
-	device_destroy(&uio_class, MKDEV(uio_major, idev->minor));
+	uio_free_minor(idev);
+	device_unregister(&idev->device);
+	return ret;
+
 err_device_create:
 	uio_free_minor(idev);
 err_get_minor:
@@ -882,8 +897,7 @@ void uio_unregister_device(struct uio_info *info)
 
 	uio_dev_del_attributes(idev);
 
-	device_destroy(&uio_class, MKDEV(uio_major, idev->minor));
-	kfree(idev);
+	device_unregister(&idev->device);
 
 	return;
 }
-- 
1.7.2.2

[PATCH 2/5] uio: Kill unused vma_count.

[Eric W. Biederman]

Remove the tracking of how many vmas are using in the uio memory
mapping mode.  The uio code doesn't use that information anywhere.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |   16 ----------------
 1 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 565559c..95f25ae 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -35,7 +35,6 @@ struct uio_device {
 	atomic_t		event;
 	struct fasync_struct	*async_queue;
 	wait_queue_head_t	wait;
-	int			vma_count;
 	struct uio_info		*info;
 	struct kobject		*map_dir;
 	struct kobject		*portio_dir;
@@ -599,18 +598,6 @@ static int uio_find_mem_index(struct vm_area_struct *vma)
 	return -1;
 }
 
-static void uio_vma_open(struct vm_area_struct *vma)
-{
-	struct uio_device *idev = vma->vm_private_data;
-	idev->vma_count++;
-}
-
-static void uio_vma_close(struct vm_area_struct *vma)
-{
-	struct uio_device *idev = vma->vm_private_data;
-	idev->vma_count--;
-}
-
 static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct uio_device *idev = vma->vm_private_data;
@@ -638,8 +625,6 @@ static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 }
 
 static const struct vm_operations_struct uio_vm_ops = {
-	.open = uio_vma_open,
-	.close = uio_vma_close,
 	.fault = uio_vma_fault,
 };
 
@@ -665,7 +650,6 @@ static int uio_mmap_logical(struct vm_area_struct *vma)
 {
 	vma->vm_flags |= VM_RESERVED;
 	vma->vm_ops = &uio_vm_ops;
-	uio_vma_open(vma);
 	return 0;
 }
 
-- 
1.7.2.2

[PATCH 3/5] uio: Remove unused uio_info mmap method.

[Eric W. Biederman]

There are no drivers in the kernel that implement the uio_info mmap
method so there is no point in keeping it.

Further keeping the mmap method would necessitate wrapping all of the
methods in vm_operations_struct to successfully implement support for
hotunplugable hardware, and it I have yet to find a correct way to wrap
the the vm_operations_struct close method.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c          |    6 ------
 include/linux/uio_driver.h |    1 -
 2 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 95f25ae..fc52fbc 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -659,7 +659,6 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	struct uio_device *idev = listener->dev;
 	int mi;
 	unsigned long requested_pages, actual_pages;
-	int ret = 0;
 
 	if (vma->vm_end < vma->vm_start)
 		return -EINVAL;
@@ -676,11 +675,6 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	if (requested_pages > actual_pages)
 		return -EINVAL;
 
-	if (idev->info->mmap) {
-		ret = idev->info->mmap(idev->info, vma);
-		return ret;
-	}
-
 	switch (idev->info->mem[mi].memtype) {
 		case UIO_MEM_PHYS:
 			return uio_mmap_physical(vma);
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index d6188e5..33789d4 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -88,7 +88,6 @@ struct uio_info {
 	unsigned long		irq_flags;
 	void			*priv;
 	irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
-	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
 	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
-- 
1.7.2.2

[PATCH 4/5] libunload: A library to help remove open files

[Eric W. Biederman]

The problem of how to remove open files due to module unloading or device
hotunplugging keeps coming up.  We have multiple implementations of roughly
the same logic in proc, sysctl, sysfs, tun and now I am working on yet
another one for uio.  It is time to start working on a generic implementation.

This library does not aim to allow wrapping any arbitray set of file operations
and making it safe to unload any module.  This library aims to work in
conjuction with the code implementiong an object to make it safe to remove
safely remove the object while file handles to it are still open.  libunload
implements the necessary locking and logic to make it striaght forward to
implement file_operations for objects that are removed at runtime.

It is hard to arrange for the ->close method of vm_operations_struct to be
called when an object is being removed, and this code doesn't even attempt
to help with that.  Instead it is assumed that calling ->close is not needed.
Without close support mmap at hotunplug time is simply a matter of calling
umap_mapping_range() to invaildate the mappings, and to arrange for vm_fault
to return VM_FAULT_SIGBUS when the unload_trylock fails.

Wait queues and fasync queues can safely be woken up after unload_barrier
making the semantics clean.   The fasync entries can be freed as a list of
all of the file descriptors is kept.  poll entries can not be freed so the
poll wait queue heads must be kept around.   If someone else's poll method is
being wrapped the wrapped poll wait queue head could be freed, but it requires
that there is a wrapping wait queue head that is kept around.  If there is no
other way wrapping a poll wait queue head seems practical but in general it
isn't a particularly useful.

libunload is best understood from the perspective of code that calls
unload_barrier().  Past the unload barrier it is guaranteed that there
is no code in the critical sections protectecd by the unload lock, and the
unload release lock.  Past the unload barrier it is safe to call the release
methods for remaining file descriptors, to ensure some logical state does
not persist.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 fs/Makefile            |    2 +-
 fs/libunload.c         |  166 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/unload.h |   33 ++++++++++
 3 files changed, 200 insertions(+), 1 deletions(-)
 create mode 100644 fs/libunload.c
 create mode 100644 include/linux/unload.h

diff --git a/fs/Makefile b/fs/Makefile
index e6ec1d3..fa6bd11 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -11,7 +11,7 @@ obj-y :=	open.o read_write.o file_table.o super.o \
 		attr.o bad_inode.o file.o filesystems.o namespace.o \
 		seq_file.o xattr.o libfs.o fs-writeback.o \
 		pnode.o drop_caches.o splice.o sync.o utimes.o \
-		stack.o fs_struct.o statfs.o
+		stack.o fs_struct.o statfs.o libunload.o
 
 ifeq ($(CONFIG_BLOCK),y)
 obj-y +=	buffer.o bio.o block_dev.o direct-io.o mpage.o ioprio.o
diff --git a/fs/libunload.c b/fs/libunload.c
new file mode 100644
index 0000000..2470bf2
--- /dev/null
+++ b/fs/libunload.c
@@ -0,0 +1,166 @@
+#include <linux/fs.h>
+#include <linux/mm_types.h>
+#include <linux/mm.h>
+#include <linux/spinlock.h>
+#include <linux/module.h>
+#include <linux/unload.h>
+
+struct unload_barrier {
+	struct completion	completion;
+	int			releasers;
+};
+
+void unload_init(struct unload *unload)
+{
+	INIT_HLIST_HEAD(&unload->ufiles);
+	spin_lock_init(&unload->lock);
+	unload->active = 1;
+	unload->barrier = NULL;
+}
+EXPORT_SYMBOL_GPL(unload_init);
+
+void unload_file_init(struct unload_file *ufile, struct file *file, struct unload *unload)
+{
+	ufile->file = file;
+	ufile->unload = unload;
+	INIT_HLIST_NODE(&ufile->list);
+}
+EXPORT_SYMBOL_GPL(unload_file_init);
+
+bool unload_trylock(struct unload *unload)
+{
+	bool locked = false;
+	spin_lock(&unload->lock);
+	if (likely(!unload->barrier)) {
+		unload->active++;
+		locked = true;
+	}
+	spin_unlock(&unload->lock);
+	return locked;
+}
+EXPORT_SYMBOL_GPL(unload_trylock);
+
+static void __unload_unlock(struct unload *unload)
+{
+	unload->active--;
+	if ((unload->active == 0) && (unload->barrier->releasers == 0))
+		complete(&unload->barrier->completion);
+}
+
+void unload_unlock(struct unload *unload)
+{
+	spin_lock(&unload->lock);
+	__unload_unlock(unload);
+	spin_unlock(&unload->lock);
+}
+EXPORT_SYMBOL_GPL(unload_unlock);
+
+static void __unload_file_attach(struct unload_file *ufile, struct unload *unload)
+{
+	ufile->unload = unload;
+	hlist_add_head(&ufile->list, &unload->ufiles);
+}
+
+void unload_file_attach(struct unload_file *ufile, struct unload *unload)
+{
+	spin_lock(&unload->lock);
+	__unload_file_attach(ufile, unload);
+	spin_unlock(&unload->lock);
+}
+EXPORT_SYMBOL_GPL(unload_file_attach);
+
+static void __unload_file_detach(struct unload_file *ufile)
+{
+	hlist_del_init(&ufile->list);
+}
+
+void unload_file_detach(struct unload_file *ufile)
+{
+	struct unload *unload = ufile->unload;
+
+	spin_lock(&unload->lock);
+	__unload_file_detach(ufile);
+	spin_unlock(&unload->lock);
+}
+EXPORT_SYMBOL_GPL(unload_file_detach);
+
+struct unload_file *find_unload_file(struct unload *unload, struct file *file)
+{
+	struct unload_file *ufile;
+	struct hlist_node *pos;
+
+	spin_lock(&unload->lock);
+	hlist_for_each_entry(ufile, pos, &unload->ufiles, list) {
+		if (ufile->file == file)
+			goto done;
+	}
+	ufile = NULL;
+done:
+	spin_unlock(&unload->lock);
+	return ufile;
+}
+EXPORT_SYMBOL_GPL(find_unload_file);
+
+bool unload_release_trylock(struct unload_file *ufile)
+{
+	struct unload *unload = ufile->unload;
+	bool locked = false;
+
+	spin_lock(&unload->lock);
+	if (!hlist_unhashed(&ufile->list))
+		locked = true;
+	spin_unlock(&unload->lock);
+	return locked;
+}
+EXPORT_SYMBOL_GPL(unload_release_trylock);
+
+void unload_release_unlock(struct unload_file *ufile)
+{
+	struct unload *unload = ufile->unload;
+	struct unload_barrier *barrier;
+
+	spin_lock(&unload->lock);
+	__unload_file_detach(ufile);
+	barrier = unload->barrier;
+	if (barrier) {
+		barrier->releasers -= 1;
+		if ((barrier->releasers == 0) && (unload->active == 0))
+			complete(&barrier->completion);
+	}
+	spin_unlock(&unload->lock);
+}
+EXPORT_SYMBOL_GPL(unload_release_unlock);
+
+
+void unload_barrier(struct unload *unload)
+{
+	struct unload_barrier barrier;
+	struct unload_file *ufile;
+	struct hlist_node *pos;
+
+	/* Guarantee that when this function returns I am not
+	 * executing any code protected by the unload_lock or
+	 * unload_releas_lock, and that I will never again execute
+	 * code protected by those locks.
+	 *
+	 * Also guarantee the file count for every file remaining on
+	 * the unload ufiles list has been incremented.  The increment
+	 * of the file count guarantees __fput will not be called.
+	 */
+	init_completion(&barrier.completion);
+	barrier.releasers = 0;
+
+	spin_lock(&unload->lock);
+	unload->barrier = &barrier;
+
+	hlist_for_each_entry(ufile, pos, &unload->ufiles, list)
+		if (!atomic_long_inc_not_zero(&ufile->file->f_count))
+			barrier.releasers++;
+	unload->active--;
+	if (unload->active || barrier.releasers) {
+		spin_unlock(&unload->lock);
+		wait_for_completion(&barrier.completion);
+		spin_lock(&unload->lock);
+	}
+	spin_unlock(&unload->lock);
+}
diff --git a/include/linux/unload.h b/include/linux/unload.h
new file mode 100644
index 0000000..fc1b4f6
--- /dev/null
+++ b/include/linux/unload.h
@@ -0,0 +1,33 @@
+#ifndef _LINUX_UNLOAD_H
+#define _LINUX_UNLOAD_H
+
+#include <linux/list.h>
+
+struct file;
+struct vm_operations_struct;
+struct unload_barrier;
+
+struct unload {
+	struct hlist_head	ufiles;
+	struct unload_barrier	*barrier;
+	spinlock_t		lock;
+	int			active;
+};
+
+struct unload_file {
+	struct unload		*unload;
+	struct hlist_node	list;
+	struct file 		*file;
+};
+
+void unload_init(struct unload *unload);
+void unload_file_init(struct unload_file *ufile, struct file *file, struct unload *unload);
+bool unload_trylock(struct unload *unload);
+void unload_unlock(struct unload *unload);
+bool unload_release_trylock(struct unload_file *ufile);
+void unload_release_unlock(struct unload_file *ufile);
+void unload_file_attach(struct unload_file *ufile, struct unload *unload);
+void unload_file_detach(struct unload_file *ufile);
+struct unload_file *find_unload_file(struct unload *unload, struct file *file);
+void unload_barrier(struct unload *unload);
+#endif /* _LINUX_UNLOAD_H */
-- 
1.7.2.2

[PATCH 5/5] uio: Implement hotunplug support, using libunload

[Eric W. Biederman]

With this change it is possible to remove a module that implements
a uio device, or to remove the underlying hardware device of a uio
device withot crashing the kernel, or causing user space more problems
than just an I/O error.

The implicit module parameter that was pased  by uio_register_device
to __uio_register_device has been removed as it is now safe to call
uio_unregister_device while uio file handles are open.

In uio all file methods (except release) now must perform their work
inside of the unload_lock.  This guarantees that uio_unregister_device
won't return until all currently running uio methods complete, and
it allows uio to return -EIO after the underlying device has been
unregistered.

The fops->release method must perform the bulk of it's work under the
unload_release_lock.  With release more needs to be done than to
more than wait for the method to finish executing, the release also
needs to handle the info->releae being called early on files that
are not closed when unregister is running.  Taking the unload_release
lock fails if and only if the info->release method has already been
called at the time fops->release runs.

Instead of holding a module reference the uio files have been modified
to instead hold an extra reference to struct uio_device, ensuring
that the uio file_operations functions will not access freed memory
even after the uio device has been unregistered.

The bulk of the changes are simply modifying the code flow of the
uio functions to run under the appropriate unload lock.

uio_open is modestly special in that it needs to run under the
uio_unload lock (to keep the uio device from unloading), but not have
it's file structure on the uio unload list until it is certain
that the open will succeed (ensuring that release will not be called
on a file that we failed to open).  uio_open also careflly grabs
the reference to the uio_device under the idr_mutex to guarnatee
the the uio_device reference held by the idr data structure
is valid while we are incrementing the uio_device reference count.

uio_device_unregister does a fair bit more than what it used to.  All
of the extra work is essentially handling the early shutdown of file
handles when a device is unregistered while files are still open.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c          |  262 +++++++++++++++++++++++++++++++++-----------
 include/linux/uio_driver.h |   10 +--
 2 files changed, 198 insertions(+), 74 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index fc52fbc..b0032e3 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -24,12 +24,13 @@
 #include <linux/string.h>
 #include <linux/kobject.h>
 #include <linux/cdev.h>
+#include <linux/file.h>
 #include <linux/uio_driver.h>
+#include <linux/unload.h>
 
 #define UIO_MAX_DEVICES		(1U << MINORBITS)
 
 struct uio_device {
-	struct module		*owner;
 	struct device		device;
 	int			minor;
 	atomic_t		event;
@@ -38,6 +39,7 @@ struct uio_device {
 	struct uio_info		*info;
 	struct kobject		*map_dir;
 	struct kobject		*portio_dir;
+	struct unload		unload;
 };
 
 static int uio_major;
@@ -424,106 +426,140 @@ static irqreturn_t uio_interrupt(int irq, void *dev_id)
 }
 
 struct uio_listener {
-	struct uio_device *dev;
+	struct unload_file ufile;
 	s32 event_count;
 };
 
+#define listener_idev(LISTENER) \
+	container_of((LISTENER)->ufile.unload, struct uio_device, unload)
+
 static int uio_open(struct inode *inode, struct file *filep)
 {
 	struct uio_device *idev;
-	struct uio_listener *listener;
+	struct uio_listener *listener = NULL;
 	int ret = 0;
 
 	mutex_lock(&minor_lock);
 	idev = idr_find(&uio_idr, iminor(inode));
+	if (idev)
+		get_device(&idev->device);
 	mutex_unlock(&minor_lock);
-	if (!idev) {
-		ret = -ENODEV;
-		goto out;
-	}
-
-	if (!try_module_get(idev->owner)) {
-		ret = -ENODEV;
-		goto out;
-	}
+	ret = -ENODEV;
+	if (!idev)
+		goto err_out;
 
 	listener = kmalloc(sizeof(*listener), GFP_KERNEL);
-	if (!listener) {
-		ret = -ENOMEM;
-		goto err_alloc_listener;
-	}
+	ret = -ENOMEM;
+	if (!listener)
+		goto err_out;
 
-	listener->dev = idev;
+	unload_file_init(&listener->ufile, filep, &idev->unload);
 	listener->event_count = atomic_read(&idev->event);
 	filep->private_data = listener;
 
-	if (idev->info->open) {
+	/*
+	 * Take the users lock before opening the file to ensure the
+	 * file is not unregistered while it is being opened.
+	 */
+	ret = -EIO;
+	if (!unload_trylock(&idev->unload))
+		goto err_out;
+
+	ret = 0;
+	if (idev->info->open)
 		ret = idev->info->open(idev->info, inode);
-		if (ret)
-			goto err_infoopen;
+	
+	/*
+	 * Add to the listener list only if the open succeeds.
+	 * This ensures that uio_unregister_device won't call
+	 * release unless open has succeeded.
+	 */
+	if (ret == 0)
+		unload_file_attach(&listener->ufile, &idev->unload);
+	unload_unlock(&idev->unload);
+err_out:
+	if (ret) {
+		if (idev)
+			put_device(&idev->device);
+		kfree(listener);
 	}
-	return 0;
-
-err_infoopen:
-	kfree(listener);
-
-err_alloc_listener:
-	module_put(idev->owner);
-
-out:
 	return ret;
 }
 
 static int uio_fasync(int fd, struct file *filep, int on)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
+	int ret = -EIO;
 
-	return fasync_helper(fd, filep, on, &idev->async_queue);
+	if (unload_trylock(&idev->unload)) {
+		ret = fasync_helper(fd, filep, on, &idev->async_queue);
+		unload_unlock(&idev->unload);
+	}
+
+	return ret;
 }
 
 static int uio_release(struct inode *inode, struct file *filep)
 {
 	int ret = 0;
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
+
+	if (unload_release_trylock(&listener->ufile)) {
 
-	if (idev->info->release)
-		ret = idev->info->release(idev->info, inode);
+		if (idev->info->release)
+			ret = idev->info->release(idev->info, inode);
 
-	module_put(idev->owner);
+		unload_release_unlock(&listener->ufile);
+	}
 	kfree(listener);
+	put_device(&idev->device);
 	return ret;
 }
 
 static unsigned int uio_poll(struct file *filep, poll_table *wait)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
+	unsigned int ret;
+
+	if (!unload_trylock(&idev->unload))
+		return POLLERR;
 
+	ret = POLLERR;
 	if (!idev->info->irq)
-		return -EIO;
+		goto out;
 
 	poll_wait(filep, &idev->wait, wait);
+	ret = 0;
 	if (listener->event_count != atomic_read(&idev->event))
-		return POLLIN | POLLRDNORM;
-	return 0;
+		ret = POLLIN | POLLRDNORM;
+
+out:
+	unload_unlock(&idev->unload);
+	return ret;
 }
 
 static ssize_t uio_read(struct file *filep, char __user *buf,
 			size_t count, loff_t *ppos)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
 	DECLARE_WAITQUEUE(wait, current);
 	ssize_t retval;
 	s32 event_count;
 
-	if (!idev->info->irq)
+	if (!unload_trylock(&idev->unload))
 		return -EIO;
 
+	retval = -EIO;
+	if (!idev->info->irq)
+		goto out;
+
+	retval = -EIO;
 	if (count != sizeof(s32))
-		return -EINVAL;
+		goto out;
 
 	add_wait_queue(&idev->wait, &wait);
 
@@ -550,12 +586,21 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 			retval = -ERESTARTSYS;
 			break;
 		}
+		unload_unlock(&idev->unload);
+
 		schedule();
+
+		if (!unload_trylock(&idev->unload)) {
+			retval = -EIO;
+			break;
+		}
 	} while (1);
 
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&idev->wait, &wait);
 
+out:
+	unload_unlock(&idev->unload);
 	return retval;
 }
 
@@ -563,24 +608,33 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
 			size_t count, loff_t *ppos)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
 	ssize_t retval;
 	s32 irq_on;
 
-	if (!idev->info->irq)
+	if (!unload_trylock(&idev->unload))
 		return -EIO;
 
+	retval = -EIO;
+	if (!idev->info->irq)
+		goto out;
+
+	retval = -EINVAL;
 	if (count != sizeof(s32))
-		return -EINVAL;
+		goto out;
 
+	retval = -ENOSYS;
 	if (!idev->info->irqcontrol)
-		return -ENOSYS;
+		goto out;
 
+	retval = -EFAULT;
 	if (copy_from_user(&irq_on, buf, count))
-		return -EFAULT;
+		goto out;
 
 	retval = idev->info->irqcontrol(idev->info, irq_on);
 
+out:
+	unload_unlock(&idev->unload);
 	return retval ? retval : sizeof(s32);
 }
 
@@ -598,16 +652,22 @@ static int uio_find_mem_index(struct vm_area_struct *vma)
 	return -1;
 }
 
-static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+static int uio_logical_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct uio_device *idev = vma->vm_private_data;
 	struct page *page;
 	unsigned long offset;
+	int ret;
+	int mi;
 
-	int mi = uio_find_mem_index(vma);
-	if (mi < 0)
+	if (!unload_trylock(&idev->unload))
 		return VM_FAULT_SIGBUS;
 
+	ret = VM_FAULT_SIGBUS;
+	mi = uio_find_mem_index(vma);
+	if (mi < 0)
+		goto out;
+
 	/*
 	 * We need to subtract mi because userspace uses offset = N*PAGE_SIZE
 	 * to use mem[N].
@@ -621,11 +681,26 @@ static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 							+ offset);
 	get_page(page);
 	vmf->page = page;
-	return 0;
+	ret = 0;
+out:
+	unload_unlock(&idev->unload);
+	return ret;
 }
 
-static const struct vm_operations_struct uio_vm_ops = {
-	.fault = uio_vma_fault,
+static const struct vm_operations_struct uio_logical_vm_ops = {
+	.fault = uio_logical_fault,
+};
+
+static int uio_physical_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	/* The pages should always be mapped, so we should never get
+	 * here unless the device has been hotunplugged
+	 */
+	return VM_FAULT_SIGBUS;
+}
+
+static const struct vm_operations_struct uio_physical_vm_ops = {
+	.fault = uio_physical_fault,
 };
 
 static int uio_mmap_physical(struct vm_area_struct *vma)
@@ -638,6 +713,7 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
 	vma->vm_flags |= VM_IO | VM_RESERVED;
 
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	vma->vm_ops = &uio_physical_vm_ops;
 
 	return remap_pfn_range(vma,
 			       vma->vm_start,
@@ -649,41 +725,54 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
 static int uio_mmap_logical(struct vm_area_struct *vma)
 {
 	vma->vm_flags |= VM_RESERVED;
-	vma->vm_ops = &uio_vm_ops;
+	vma->vm_ops = &uio_logical_vm_ops;
 	return 0;
 }
 
 static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
 	int mi;
 	unsigned long requested_pages, actual_pages;
+	int ret;
+
+	if (!unload_trylock(&idev->unload))
+		return -EIO;
 
+	ret = -EINVAL;
 	if (vma->vm_end < vma->vm_start)
-		return -EINVAL;
+		goto out;
 
 	vma->vm_private_data = idev;
 
+	ret = -EINVAL;
 	mi = uio_find_mem_index(vma);
 	if (mi < 0)
-		return -EINVAL;
+		goto out;
 
 	requested_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
 	actual_pages = ((idev->info->mem[mi].addr & ~PAGE_MASK)
 			+ idev->info->mem[mi].size + PAGE_SIZE -1) >> PAGE_SHIFT;
+	ret = -EINVAL;
 	if (requested_pages > actual_pages)
-		return -EINVAL;
+		goto out;
 
 	switch (idev->info->mem[mi].memtype) {
 		case UIO_MEM_PHYS:
-			return uio_mmap_physical(vma);
+			ret = uio_mmap_physical(vma);
+			break;
 		case UIO_MEM_LOGICAL:
 		case UIO_MEM_VIRTUAL:
-			return uio_mmap_logical(vma);
+			ret = uio_mmap_logical(vma);
+			break;
 		default:
-			return -EINVAL;
+			ret = -EINVAL;
+			break;
 	}
+out:
+	unload_unlock(&idev->unload);
+	return ret;
 }
 
 static const struct file_operations uio_fops = {
@@ -782,9 +871,7 @@ static void uio_device_release(struct device *dev)
  *
  * returns zero on success or a negative error code.
  */
-int __uio_register_device(struct module *owner,
-			  struct device *parent,
-			  struct uio_info *info)
+int uio_register_device(struct device *parent, struct uio_info *info)
 {
 	struct uio_device *idev;
 	int ret = 0;
@@ -800,10 +887,10 @@ int __uio_register_device(struct module *owner,
 		goto err_kzalloc;
 	}
 
-	idev->owner = owner;
 	idev->info = info;
 	init_waitqueue_head(&idev->wait);
 	atomic_set(&idev->event, 0);
+	unload_init(&idev->unload);
 
 	ret = uio_get_minor(idev);
 	if (ret)
@@ -852,7 +939,7 @@ err_get_minor:
 err_kzalloc:
 	return ret;
 }
-EXPORT_SYMBOL_GPL(__uio_register_device);
+EXPORT_SYMBOL_GPL(uio_register_device);
 
 /**
  * uio_unregister_device - unregister a industrial IO device
@@ -862,12 +949,54 @@ EXPORT_SYMBOL_GPL(__uio_register_device);
 void uio_unregister_device(struct uio_info *info)
 {
 	struct uio_device *idev;
+	struct unload_file *ufile;
+	struct hlist_node *n1, *n2;
 
 	if (!info || !info->uio_dev)
 		return;
 
 	idev = info->uio_dev;
 
+	/*
+	 * Stop accepting new callers into the uio functions, and wait
+	 * until all existing callers into the uio functions are done.
+	 */
+	unload_barrier(&idev->unload);
+
+	/* Notify listeners that the uio devices has gone. */
+	wake_up_interruptible_all(&idev->wait);
+	kill_fasync(&idev->async_queue, SIGIO, POLL_ERR);
+
+	/*
+	 * unload_barrier froze the idev->unload.ufiles list and grabbed
+	 * a reference to every file on that list.  Now cleanup those files
+	 * and drop the extra reference.
+	 */
+	hlist_for_each_entry_safe(ufile, n1, n2, &idev->unload.ufiles, list) {
+		struct file *file = ufile->file;
+		struct inode *inode = file->f_dentry->d_inode;
+
+		/* Cause all mappings to trigger a SIGBUS */
+		unmap_mapping_range(inode->i_mapping, 0, 0, 1);
+
+		/* Remove the file from the fasync list because any
+		 * future attempts to add/remove this file from this
+		 * list will return -EIO.
+		 */
+		fasync_helper(-1, file, 0, &idev->async_queue);
+
+		/* Now that userspace is totally blocked off run the
+		 * release code to clean up, the uio devices data
+		 * structures.
+		 */
+		if (info->release)
+			info->release(info, inode);
+
+		/* Forget about this file */
+		unload_file_detach(ufile);
+		fput(file);
+	}
+
 	uio_free_minor(idev);
 
 	if (info->irq && (info->irq != UIO_IRQ_CUSTOM))
@@ -875,6 +1004,7 @@ void uio_unregister_device(struct uio_info *info)
 
 	uio_dev_del_attributes(idev);
 
+	idev->info = NULL;
 	device_unregister(&idev->device);
 
 	return;
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 33789d4..2551737 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -94,14 +94,8 @@ struct uio_info {
 };
 
 extern int __must_check
-	__uio_register_device(struct module *owner,
-			      struct device *parent,
-			      struct uio_info *info);
-static inline int __must_check
-	uio_register_device(struct device *parent, struct uio_info *info)
-{
-	return __uio_register_device(THIS_MODULE, parent, info);
-}
+	uio_register_device(struct device *parent, struct uio_info *info);
+
 extern void uio_unregister_device(struct uio_info *info);
 extern void uio_event_notify(struct uio_info *info);
 
-- 
1.7.2.2

Re: [PATCH 4/5] uio: Support 2^MINOR_BITS minors

[Greg KH]

On Fri, Sep 17, 2010 at 10:36:50PM +0200, Thomas Gleixner wrote:
> On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> 
> > 
> > register_chrdev limits uio devices to 256 minor numbers which causes
> > problems on one system I have with 384+ uio devices.  So instead set
> > UIO_MAX_DEVICES to the maximum number of minors and use
> > alloc_chrdev_region to reserve the uio minors.
> > 
> > The final result is that the code works the same but the uio driver now
> > supports any minor the idr allocator comes up with.
> > 
> > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> 
> One minor nit:
> 
> > +	result = alloc_chrdev_region(&uio_dev, 0, UIO_MAX_DEVICES, name);
> > +	if (result)
> > +		goto out;
> 
>   		return result;

Wait, why?  It's the exact came code, as out does:

out:
	return result;

and you need that line due to the code above it.  So I say leave it.

thanks,

greg k-h

Re: [PATCH 4/5] uio: Support 2^MINOR_BITS minors

[Thomas Gleixner]

On Tue, 21 Sep 2010, Greg KH wrote:

> On Fri, Sep 17, 2010 at 10:36:50PM +0200, Thomas Gleixner wrote:
> > On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> > 
> > > 
> > > register_chrdev limits uio devices to 256 minor numbers which causes
> > > problems on one system I have with 384+ uio devices.  So instead set
> > > UIO_MAX_DEVICES to the maximum number of minors and use
> > > alloc_chrdev_region to reserve the uio minors.
> > > 
> > > The final result is that the code works the same but the uio driver now
> > > supports any minor the idr allocator comes up with.
> > > 
> > > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> > 
> > One minor nit:
> > 
> > > +	result = alloc_chrdev_region(&uio_dev, 0, UIO_MAX_DEVICES, name);
> > > +	if (result)
> > > +		goto out;
> > 
> >   		return result;
> 
> Wait, why?  It's the exact came code, as out does:
> 
> out:
> 	return result;
> 
> and you need that line due to the code above it.  So I say leave it.

s/goto out/return result/g

Gotos which end up in a single line "return foo;" are pretty pointless.

Thanks,

	tglx

Re: [PATCH 4/5] uio: Support 2^MINOR_BITS minors

[Greg KH]

On Tue, Sep 21, 2010 at 11:38:26PM +0200, Thomas Gleixner wrote:
> On Tue, 21 Sep 2010, Greg KH wrote:
> 
> > On Fri, Sep 17, 2010 at 10:36:50PM +0200, Thomas Gleixner wrote:
> > > On Tue, 14 Sep 2010, Eric W. Biederman wrote:
> > > 
> > > > 
> > > > register_chrdev limits uio devices to 256 minor numbers which causes
> > > > problems on one system I have with 384+ uio devices.  So instead set
> > > > UIO_MAX_DEVICES to the maximum number of minors and use
> > > > alloc_chrdev_region to reserve the uio minors.
> > > > 
> > > > The final result is that the code works the same but the uio driver now
> > > > supports any minor the idr allocator comes up with.
> > > > 
> > > > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
> > > 
> > > One minor nit:
> > > 
> > > > +	result = alloc_chrdev_region(&uio_dev, 0, UIO_MAX_DEVICES, name);
> > > > +	if (result)
> > > > +		goto out;
> > > 
> > >   		return result;
> > 
> > Wait, why?  It's the exact came code, as out does:
> > 
> > out:
> > 	return result;
> > 
> > and you need that line due to the code above it.  So I say leave it.
> 
> s/goto out/return result/g
> 
> Gotos which end up in a single line "return foo;" are pretty pointless.

Ok, fair enough, but it's not a big deal, right?

thanks,

greg k-h

Re: [PATCH 4/5] uio: Support 2^MINOR_BITS minors

[Eric W. Biederman]

Thomas Gleixner <tglx@linutronix.de> writes:

> On Tue, 21 Sep 2010, Greg KH wrote:
>
>> On Fri, Sep 17, 2010 at 10:36:50PM +0200, Thomas Gleixner wrote:
>> > On Tue, 14 Sep 2010, Eric W. Biederman wrote:
>> > 
>> > > 
>> > > register_chrdev limits uio devices to 256 minor numbers which causes
>> > > problems on one system I have with 384+ uio devices.  So instead set
>> > > UIO_MAX_DEVICES to the maximum number of minors and use
>> > > alloc_chrdev_region to reserve the uio minors.
>> > > 
>> > > The final result is that the code works the same but the uio driver now
>> > > supports any minor the idr allocator comes up with.
>> > > 
>> > > Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
>> > 
>> > One minor nit:
>> > 
>> > > +	result = alloc_chrdev_region(&uio_dev, 0, UIO_MAX_DEVICES, name);
>> > > +	if (result)
>> > > +		goto out;
>> > 
>> >   		return result;
>> 
>> Wait, why?  It's the exact came code, as out does:
>> 
>> out:
>> 	return result;
>> 
>> and you need that line due to the code above it.  So I say leave it.
>
> s/goto out/return result/g
>
> Gotos which end up in a single line "return foo;" are pretty
> pointless.

But they do result in a single exit, which if you insert debugging or
want to understand the code flow can sometimes be beneficial.

The compiler will make this transformation on it's own when it compiles
the code, because it does result in slightly better code.

*shrug*  In the grand scheme of things it isn't a big deal.

I'm much more interested in getting the uio driver hotplug safe so when
I physically remove a device I am using I don't get a kernel crash.

Eric

Re: [PATCH 4/5] uio: Support 2^MINOR_BITS minors

[Thomas Gleixner]

On Tue, 21 Sep 2010, Eric W. Biederman wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
> The compiler will make this transformation on it's own when it compiles
> the code, because it does result in slightly better code.

I doubt the compiler part, but I'm not religious about that. It's a
question of personal preference.
 
> *shrug*  In the grand scheme of things it isn't a big deal.
> 
> I'm much more interested in getting the uio driver hotplug safe so when
> I physically remove a device I am using I don't get a kernel crash.

No objections.

   tglx

Re: [PATCH 0/5] uio hotplug support

[Hans J. Koch]

On Mon, Sep 20, 2010 at 12:19:59AM -0700, Eric W. Biederman wrote:
> 
> Implement the ability to hotunplug a uio device while file handles
> are still open without crashing.

Could you please create patches that apply on top of your already
accepted UIO changes?

Thanks,
Hans

Re: [PATCH 5/5] uio: Implement hotunplug support, using libunload

[Hans J. Koch]

On Mon, Sep 20, 2010 at 12:24:19AM -0700, Eric W. Biederman wrote:
> 
>  }
>  
>  static ssize_t uio_read(struct file *filep, char __user *buf,
>  			size_t count, loff_t *ppos)
>  {
>  	struct uio_listener *listener = filep->private_data;
> -	struct uio_device *idev = listener->dev;
> +	struct uio_device *idev = listener_idev(listener);
>  	DECLARE_WAITQUEUE(wait, current);
>  	ssize_t retval;
>  	s32 event_count;
>  
> -	if (!idev->info->irq)
> +	if (!unload_trylock(&idev->unload))
>  		return -EIO;
>  
> +	retval = -EIO;
> +	if (!idev->info->irq)
> +		goto out;
> +
> +	retval = -EIO;
>  	if (count != sizeof(s32))
> -		return -EINVAL;
> +		goto out;

No. uio_read() should return -EINVAL if count != sizeof(s32). This is
simply wrong userspace code that passes in an illegal value, so it's
not an IO error but an invalid parameter.
BTW, you use -EINVAL in the same situation in uio_write()...

>  
>  	add_wait_queue(&idev->wait, &wait);
>  
> @@ -550,12 +586,21 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
>  			retval = -ERESTARTSYS;
>  			break;
>  		}
> +		unload_unlock(&idev->unload);
> +
>  		schedule();
> +
> +		if (!unload_trylock(&idev->unload)) {
> +			retval = -EIO;
> +			break;
> +		}
>  	} while (1);
>  
>  	__set_current_state(TASK_RUNNING);
>  	remove_wait_queue(&idev->wait, &wait);
>  
> +out:
> +	unload_unlock(&idev->unload);
>  	return retval;
>  }
>  
> @@ -563,24 +608,33 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
>  			size_t count, loff_t *ppos)
>  {
>  	struct uio_listener *listener = filep->private_data;
> -	struct uio_device *idev = listener->dev;
> +	struct uio_device *idev = listener_idev(listener);
>  	ssize_t retval;
>  	s32 irq_on;
>  
> -	if (!idev->info->irq)
> +	if (!unload_trylock(&idev->unload))
>  		return -EIO;
>  
> +	retval = -EIO;
> +	if (!idev->info->irq)
> +		goto out;
> +
> +	retval = -EINVAL;
>  	if (count != sizeof(s32))
> -		return -EINVAL;
> +		goto out;
>  
> +	retval = -ENOSYS;
>  	if (!idev->info->irqcontrol)
> -		return -ENOSYS;
> +		goto out;
>  
> +	retval = -EFAULT;
>  	if (copy_from_user(&irq_on, buf, count))
> -		return -EFAULT;
> +		goto out;
>  
>  	retval = idev->info->irqcontrol(idev->info, irq_on);
>  
> +out:
> +	unload_unlock(&idev->unload);
>  	return retval ? retval : sizeof(s32);
>  }

Re: [PATCH 5/5] uio: Implement hotunplug support, using libunload

[Eric W. Biederman]

"Hans J. Koch" <hjk@linutronix.de> writes:

> On Mon, Sep 20, 2010 at 12:24:19AM -0700, Eric W. Biederman wrote:
>> 
>>  }
>>  
>>  static ssize_t uio_read(struct file *filep, char __user *buf,
>>  			size_t count, loff_t *ppos)
>>  {
>>  	struct uio_listener *listener = filep->private_data;
>> -	struct uio_device *idev = listener->dev;
>> +	struct uio_device *idev = listener_idev(listener);
>>  	DECLARE_WAITQUEUE(wait, current);
>>  	ssize_t retval;
>>  	s32 event_count;
>>  
>> -	if (!idev->info->irq)
>> +	if (!unload_trylock(&idev->unload))
>>  		return -EIO;
>>  
>> +	retval = -EIO;
>> +	if (!idev->info->irq)
>> +		goto out;
>> +
>> +	retval = -EIO;
>>  	if (count != sizeof(s32))
>> -		return -EINVAL;
>> +		goto out;
>
> No. uio_read() should return -EINVAL if count != sizeof(s32). This is
> simply wrong userspace code that passes in an illegal value, so it's
> not an IO error but an invalid parameter.
> BTW, you use -EINVAL in the same situation in uio_write()...

Apologies.  That was a typo.  It was my intention to preserve the
existing return codes.

Eric

Re: [PATCH 0/5] uio hotplug support

[Eric W. Biederman]

"Hans J. Koch" <hjk@linutronix.de> writes:

> On Mon, Sep 20, 2010 at 12:19:59AM -0700, Eric W. Biederman wrote:
>> 
>> Implement the ability to hotunplug a uio device while file handles
>> are still open without crashing.
>
> Could you please create patches that apply on top of your already
> accepted UIO changes?

That is what the patches I sent should be.  My apologies if I had not
made that clear.  Certainly these patches were built against my previous
changes, on top of 2.6.36-rc4.  Are you having problems?

If necessary I can grab Greg's tree can rebase my patches onto it.

Eric

Re: [PATCH 0/5] uio hotplug support

[Hans J. Koch]

On Fri, Sep 24, 2010 at 10:14:11AM -0700, Eric W. Biederman wrote:
> "Hans J. Koch" <hjk@linutronix.de> writes:
> 
> > On Mon, Sep 20, 2010 at 12:19:59AM -0700, Eric W. Biederman wrote:
> >> 
> >> Implement the ability to hotunplug a uio device while file handles
> >> are still open without crashing.
> >
> > Could you please create patches that apply on top of your already
> > accepted UIO changes?
> 
> That is what the patches I sent should be.  My apologies if I had not
> made that clear.  Certainly these patches were built against my previous
> changes, on top of 2.6.36-rc4.  Are you having problems?

Hmm, I applied your first series of 5 patches (which I signed-off). When
trying to apply your second series, I get this from quilt:

Applying patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch
patching file drivers/uio/uio.c
Hunk #2 succeeded at 287 (offset 8 lines).
Hunk #3 succeeded at 312 (offset 8 lines).
Hunk #4 FAILED at 339.
Hunk #5 succeeded at 806 (offset 17 lines).
Hunk #6 FAILED at 831.
Hunk #7 FAILED at 858.
Hunk #8 FAILED at 889.
4 out of 8 hunks FAILED -- rejects in file drivers/uio/uio.c
Patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch does not apply (enforce with -f)

The first series applied without any fuzz against 2.6.36-rc5.

Thanks,
Hans

> 
> If necessary I can grab Greg's tree can rebase my patches onto it.
> 
> Eric

Re: [PATCH 0/5] uio hotplug support

[Eric W. Biederman]

"Hans J. Koch" <hjk@linutronix.de> writes:

> On Fri, Sep 24, 2010 at 10:14:11AM -0700, Eric W. Biederman wrote:
>> "Hans J. Koch" <hjk@linutronix.de> writes:
>> 
>> > On Mon, Sep 20, 2010 at 12:19:59AM -0700, Eric W. Biederman wrote:
>> >> 
>> >> Implement the ability to hotunplug a uio device while file handles
>> >> are still open without crashing.
>> >
>> > Could you please create patches that apply on top of your already
>> > accepted UIO changes?
>> 
>> That is what the patches I sent should be.  My apologies if I had not
>> made that clear.  Certainly these patches were built against my previous
>> changes, on top of 2.6.36-rc4.  Are you having problems?
>
> Hmm, I applied your first series of 5 patches (which I signed-off). When
> trying to apply your second series, I get this from quilt:
>
> Applying patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch
> patching file drivers/uio/uio.c
> Hunk #2 succeeded at 287 (offset 8 lines).
> Hunk #3 succeeded at 312 (offset 8 lines).
> Hunk #4 FAILED at 339.
> Hunk #5 succeeded at 806 (offset 17 lines).
> Hunk #6 FAILED at 831.
> Hunk #7 FAILED at 858.
> Hunk #8 FAILED at 889.
> 4 out of 8 hunks FAILED -- rejects in file drivers/uio/uio.c
> Patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch does not apply (enforce with -f)
>
> The first series applied without any fuzz against 2.6.36-rc5.

Bizarre.  I will take a look.

Eric

Re: [PATCH 0/5] uio hotplug support

[Eric W. Biederman]

"Hans J. Koch" <hjk@linutronix.de> writes:

> On Fri, Sep 24, 2010 at 10:14:11AM -0700, Eric W. Biederman wrote:
>> "Hans J. Koch" <hjk@linutronix.de> writes:
>> 
>> > On Mon, Sep 20, 2010 at 12:19:59AM -0700, Eric W. Biederman wrote:
>> >> 
>> >> Implement the ability to hotunplug a uio device while file handles
>> >> are still open without crashing.
>> >
>> > Could you please create patches that apply on top of your already
>> > accepted UIO changes?
>> 
>> That is what the patches I sent should be.  My apologies if I had not
>> made that clear.  Certainly these patches were built against my previous
>> changes, on top of 2.6.36-rc4.  Are you having problems?
>
> Hmm, I applied your first series of 5 patches (which I signed-off). When
> trying to apply your second series, I get this from quilt:
>
> Applying patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch
> patching file drivers/uio/uio.c
> Hunk #2 succeeded at 287 (offset 8 lines).
> Hunk #3 succeeded at 312 (offset 8 lines).
> Hunk #4 FAILED at 339.
> Hunk #5 succeeded at 806 (offset 17 lines).
> Hunk #6 FAILED at 831.
> Hunk #7 FAILED at 858.
> Hunk #8 FAILED at 889.
> 4 out of 8 hunks FAILED -- rejects in file drivers/uio/uio.c
> Patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch does not apply (enforce with -f)
>
> The first series applied without any fuzz against 2.6.36-rc5.

I am stumped.  Do you perhaps have some local uio changes?

Using 2.6.36-rc5 as a base I tried it twice.  Once by taking my patches
that I sent, and again by saving my email and then applying the changes.

Both times the changes applied cleanly.  No offsets, and no fuzz.  At
least that is what git-am said.

Should I perhaps make a git branch you could pull?

Eric

Re: [PATCH 0/5] uio hotplug support

[Greg KH]

On Fri, Sep 24, 2010 at 05:05:17PM -0700, Eric W. Biederman wrote:
> "Hans J. Koch" <hjk@linutronix.de> writes:
> 
> > On Fri, Sep 24, 2010 at 10:14:11AM -0700, Eric W. Biederman wrote:
> >> "Hans J. Koch" <hjk@linutronix.de> writes:
> >> 
> >> > On Mon, Sep 20, 2010 at 12:19:59AM -0700, Eric W. Biederman wrote:
> >> >> 
> >> >> Implement the ability to hotunplug a uio device while file handles
> >> >> are still open without crashing.
> >> >
> >> > Could you please create patches that apply on top of your already
> >> > accepted UIO changes?
> >> 
> >> That is what the patches I sent should be.  My apologies if I had not
> >> made that clear.  Certainly these patches were built against my previous
> >> changes, on top of 2.6.36-rc4.  Are you having problems?
> >
> > Hmm, I applied your first series of 5 patches (which I signed-off). When
> > trying to apply your second series, I get this from quilt:
> >
> > Applying patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch
> > patching file drivers/uio/uio.c
> > Hunk #2 succeeded at 287 (offset 8 lines).
> > Hunk #3 succeeded at 312 (offset 8 lines).
> > Hunk #4 FAILED at 339.
> > Hunk #5 succeeded at 806 (offset 17 lines).
> > Hunk #6 FAILED at 831.
> > Hunk #7 FAILED at 858.
> > Hunk #8 FAILED at 889.
> > 4 out of 8 hunks FAILED -- rejects in file drivers/uio/uio.c
> > Patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch does not apply (enforce with -f)
> >
> > The first series applied without any fuzz against 2.6.36-rc5.
> 
> I am stumped.  Do you perhaps have some local uio changes?
> 
> Using 2.6.36-rc5 as a base I tried it twice.  Once by taking my patches
> that I sent, and again by saving my email and then applying the changes.
> 
> Both times the changes applied cleanly.  No offsets, and no fuzz.  At
> least that is what git-am said.
> 
> Should I perhaps make a git branch you could pull?

Try it on linux-next, as I've applied your previous patches there
already.

thanks,

greg k-h

Re: [PATCH 0/5] uio hotplug support

[Eric W. Biederman]

Greg KH <gregkh@suse.de> writes:

> On Fri, Sep 24, 2010 at 05:05:17PM -0700, Eric W. Biederman wrote:
>> "Hans J. Koch" <hjk@linutronix.de> writes:
>> 
>> >
>> > Hmm, I applied your first series of 5 patches (which I signed-off). When
>> > trying to apply your second series, I get this from quilt:
>> >
>> > Applying patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch
>> > patching file drivers/uio/uio.c
>> > Hunk #2 succeeded at 287 (offset 8 lines).
>> > Hunk #3 succeeded at 312 (offset 8 lines).
>> > Hunk #4 FAILED at 339.
>> > Hunk #5 succeeded at 806 (offset 17 lines).
>> > Hunk #6 FAILED at 831.
>> > Hunk #7 FAILED at 858.
>> > Hunk #8 FAILED at 889.
>> > 4 out of 8 hunks FAILED -- rejects in file drivers/uio/uio.c
>> > Patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch does not apply (enforce with -f)
>> >
>> > The first series applied without any fuzz against 2.6.36-rc5.
>> 
>> I am stumped.  Do you perhaps have some local uio changes?
>> 
>> Using 2.6.36-rc5 as a base I tried it twice.  Once by taking my patches
>> that I sent, and again by saving my email and then applying the changes.
>> 
>> Both times the changes applied cleanly.  No offsets, and no fuzz.  At
>> least that is what git-am said.
>> 
>> Should I perhaps make a git branch you could pull?
>
> Try it on linux-next, as I've applied your previous patches there
> already.

Done.  git-am on my saved mbox works without problems.  The worst
I get is a warning about trailing whitespace.   Nothing that
looks like the above.

Eric

[PATCH] uio: Fix accidentally changed return value in uio_read

[Eric W. Biederman]

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index b0032e3..20743aa 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -557,7 +557,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 	if (!idev->info->irq)
 		goto out;
 
-	retval = -EIO;
+	retval = -EINVAL;
 	if (count != sizeof(s32))
 		goto out;
 
-- 
1.7.2.3

Re: [PATCH 0/5] uio hotplug support

[Greg KH]

On Fri, Sep 24, 2010 at 06:54:34PM -0700, Eric W. Biederman wrote:
> Greg KH <gregkh@suse.de> writes:
> 
> > On Fri, Sep 24, 2010 at 05:05:17PM -0700, Eric W. Biederman wrote:
> >> "Hans J. Koch" <hjk@linutronix.de> writes:
> >> 
> >> >
> >> > Hmm, I applied your first series of 5 patches (which I signed-off). When
> >> > trying to apply your second series, I get this from quilt:
> >> >
> >> > Applying patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch
> >> > patching file drivers/uio/uio.c
> >> > Hunk #2 succeeded at 287 (offset 8 lines).
> >> > Hunk #3 succeeded at 312 (offset 8 lines).
> >> > Hunk #4 FAILED at 339.
> >> > Hunk #5 succeeded at 806 (offset 17 lines).
> >> > Hunk #6 FAILED at 831.
> >> > Hunk #7 FAILED at 858.
> >> > Hunk #8 FAILED at 889.
> >> > 4 out of 8 hunks FAILED -- rejects in file drivers/uio/uio.c
> >> > Patch 2_1-Simplify-the-lifetime-logic-of-struct-uio_device.patch does not apply (enforce with -f)
> >> >
> >> > The first series applied without any fuzz against 2.6.36-rc5.
> >> 
> >> I am stumped.  Do you perhaps have some local uio changes?
> >> 
> >> Using 2.6.36-rc5 as a base I tried it twice.  Once by taking my patches
> >> that I sent, and again by saving my email and then applying the changes.
> >> 
> >> Both times the changes applied cleanly.  No offsets, and no fuzz.  At
> >> least that is what git-am said.
> >> 
> >> Should I perhaps make a git branch you could pull?
> >
> > Try it on linux-next, as I've applied your previous patches there
> > already.
> 
> Done.  git-am on my saved mbox works without problems.  The worst
> I get is a warning about trailing whitespace.   Nothing that
> looks like the above.

Can you resend your patches?  I no longer have them in my queue as I was
waiting for Hans's ack and then he said they didn't apply so I purged
them.

thanks,

greg k-h

[PATCH 1/5] uio: Simplify the lifetime logic of struct uio_device.

[Eric W. Biederman]

Embed struct device into struct uio_device, and use
the refcounting and the release method of struct device
to control struct uio_device.

This allows device_create and device_destroy to be replaced
with the more standard device_register and device_unregister,
and allows the struct device reference count to act as a
reference count on struct idev as well.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |   42 ++++++++++++++++++++++++++++--------------
 1 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 6285afb..565559c 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -30,7 +30,7 @@
 
 struct uio_device {
 	struct module		*owner;
-	struct device		*dev;
+	struct device		device;
 	int			minor;
 	atomic_t		event;
 	struct fasync_struct	*async_queue;
@@ -279,7 +279,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		if (!map_found) {
 			map_found = 1;
 			idev->map_dir = kobject_create_and_add("maps",
-							&idev->dev->kobj);
+							&idev->device.kobj);
 			if (!idev->map_dir)
 				goto err_map;
 		}
@@ -304,7 +304,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 		if (!portio_found) {
 			portio_found = 1;
 			idev->portio_dir = kobject_create_and_add("portio",
-							&idev->dev->kobj);
+							&idev->device.kobj);
 			if (!idev->portio_dir)
 				goto err_portio;
 		}
@@ -339,7 +339,7 @@ err_map:
 		kobject_put(&map->kobj);
 	}
 	kobject_put(idev->map_dir);
-	dev_err(idev->dev, "error creating sysfs files (%d)\n", ret);
+	dev_err(&idev->device, "error creating sysfs files (%d)\n", ret);
 	return ret;
 }
 
@@ -789,6 +789,13 @@ static void release_uio_class(void)
 	uio_major_cleanup();
 }
 
+static void uio_device_release(struct device *dev)
+{
+	struct uio_device *idev = dev_get_drvdata(dev);
+
+	kfree(idev);
+}
+
 /**
  * uio_register_device - register a new userspace IO device
  * @owner:	module that creates the new device
@@ -824,14 +831,19 @@ int __uio_register_device(struct module *owner,
 	if (ret)
 		goto err_get_minor;
 
-	idev->dev = device_create(&uio_class, parent,
-				  MKDEV(uio_major, idev->minor), idev,
-				  "uio%d", idev->minor);
-	if (IS_ERR(idev->dev)) {
-		printk(KERN_ERR "UIO: device register failed\n");
-		ret = PTR_ERR(idev->dev);
+	idev->device.devt = MKDEV(uio_major, idev->minor);
+	idev->device.class = &uio_class;
+	idev->device.parent = parent;
+	idev->device.release = uio_device_release;
+	dev_set_drvdata(&idev->device, idev);
+
+	ret = kobject_set_name(&idev->device.kobj, "uio%d", idev->minor);
+	if (ret)
+		goto err_device_create;
+
+	ret = device_register(&idev->device);
+	if (ret)
 		goto err_device_create;
-	}
 
 	ret = uio_dev_add_attributes(idev);
 	if (ret)
@@ -851,7 +863,10 @@ int __uio_register_device(struct module *owner,
 err_request_irq:
 	uio_dev_del_attributes(idev);
 err_uio_dev_add_attributes:
-	device_destroy(&uio_class, MKDEV(uio_major, idev->minor));
+	uio_free_minor(idev);
+	device_unregister(&idev->device);
+	return ret;
+
 err_device_create:
 	uio_free_minor(idev);
 err_get_minor:
@@ -882,8 +897,7 @@ void uio_unregister_device(struct uio_info *info)
 
 	uio_dev_del_attributes(idev);
 
-	device_destroy(&uio_class, MKDEV(uio_major, idev->minor));
-	kfree(idev);
+	device_unregister(&idev->device);
 
 	return;
 }
-- 
1.7.2.2

[PATCH 2/5] uio: Kill unused vma_count.

[Eric W. Biederman]

Remove the tracking of how many vmas are using in the uio memory
mapping mode.  The uio code doesn't use that information anywhere.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |   16 ----------------
 1 files changed, 0 insertions(+), 16 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 565559c..95f25ae 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -35,7 +35,6 @@ struct uio_device {
 	atomic_t		event;
 	struct fasync_struct	*async_queue;
 	wait_queue_head_t	wait;
-	int			vma_count;
 	struct uio_info		*info;
 	struct kobject		*map_dir;
 	struct kobject		*portio_dir;
@@ -599,18 +598,6 @@ static int uio_find_mem_index(struct vm_area_struct *vma)
 	return -1;
 }
 
-static void uio_vma_open(struct vm_area_struct *vma)
-{
-	struct uio_device *idev = vma->vm_private_data;
-	idev->vma_count++;
-}
-
-static void uio_vma_close(struct vm_area_struct *vma)
-{
-	struct uio_device *idev = vma->vm_private_data;
-	idev->vma_count--;
-}
-
 static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct uio_device *idev = vma->vm_private_data;
@@ -638,8 +625,6 @@ static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 }
 
 static const struct vm_operations_struct uio_vm_ops = {
-	.open = uio_vma_open,
-	.close = uio_vma_close,
 	.fault = uio_vma_fault,
 };
 
@@ -665,7 +650,6 @@ static int uio_mmap_logical(struct vm_area_struct *vma)
 {
 	vma->vm_flags |= VM_RESERVED;
 	vma->vm_ops = &uio_vm_ops;
-	uio_vma_open(vma);
 	return 0;
 }
 
-- 
1.7.2.2

[PATCH 3/5] uio: Remove unused uio_info mmap method.

[Eric W. Biederman]

There are no drivers in the kernel that implement the uio_info mmap
method there is no point in keeping it.

Further keeping the mmap method would necessiate wrapping all of the
methods in vm_operations_struct to successfully implement suppport
for hotunplugable hardware, and it I have yet to find a correct way to
wrap the the vm_operations_struct close method.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c          |    6 ------
 include/linux/uio_driver.h |    1 -
 2 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 95f25ae..fc52fbc 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -659,7 +659,6 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	struct uio_device *idev = listener->dev;
 	int mi;
 	unsigned long requested_pages, actual_pages;
-	int ret = 0;
 
 	if (vma->vm_end < vma->vm_start)
 		return -EINVAL;
@@ -676,11 +675,6 @@ static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 	if (requested_pages > actual_pages)
 		return -EINVAL;
 
-	if (idev->info->mmap) {
-		ret = idev->info->mmap(idev->info, vma);
-		return ret;
-	}
-
 	switch (idev->info->mem[mi].memtype) {
 		case UIO_MEM_PHYS:
 			return uio_mmap_physical(vma);
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index d6188e5..33789d4 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -88,7 +88,6 @@ struct uio_info {
 	unsigned long		irq_flags;
 	void			*priv;
 	irqreturn_t (*handler)(int irq, struct uio_info *dev_info);
-	int (*mmap)(struct uio_info *info, struct vm_area_struct *vma);
 	int (*open)(struct uio_info *info, struct inode *inode);
 	int (*release)(struct uio_info *info, struct inode *inode);
 	int (*irqcontrol)(struct uio_info *info, s32 irq_on);
-- 
1.7.2.2

[PATCH 4/5] libunload: A library to help remove open files

[Eric W. Biederman]

The problem of how to remove open files due to module unloading or device
hotunplugging keeps coming up.  We have multiple implementations of roughly
the same logic in proc, sysctl, sysfs, tun and now I am working on yet
another one for uio.  It is time to start working on a generic implementation.

This library does not aim to allow wrapping any arbitray set of file operations
and making it safe to unload any module.  This library aims to work in
conjuction with the code implementiong an object to make it safe to remove
safely remove the object while file handles to it are still open.  libunload
implements the necessary locking and logic to make it striaght forward to
implement file_operations for objects that are removed at runtime.

It is hard to arrange for the ->close method of vm_operations_struct to be
called when an object is being removed, and this code doesn't even attempt
to help with that.  Instead it is assumed that calling ->close is not needed.
Without close support mmap at hotunplug time is simply a matter of calling
umap_mapping_range() to invaildate the mappings, and to arrange for vm_fault
to return VM_FAULT_SIGBUS when the unload_trylock fails.

Wait queues and fasync queues can safely be woken up after unload_barrier
making the semantics clean.   The fasync entries can be freed as a list of
all of the file descriptors is kept.  poll entries can not be freed so the
poll wait queue heads must be kept around.   If someone else's poll method is
being wrapped the wrapped poll wait queue head could be freed, but it requires
that there is a wrapping wait queue head that is kept around.  If there is no
other way wrapping a poll wait queue head seems practical but in general it
isn't a particularly useful.

libunload is best understood from the perspective of code that calls
unload_barrier().  Past the unload barrier it is guaranteed that there
is no code in the critical sections protectecd by the unload lock, and the
unload release lock.  Past the unload barrier it is safe to call the release
methods for remaining file descriptors, to ensure some logical state does
not persist.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 fs/Makefile            |    2 +-
 fs/libunload.c         |  166 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/unload.h |   33 ++++++++++
 3 files changed, 200 insertions(+), 1 deletions(-)
 create mode 100644 fs/libunload.c
 create mode 100644 include/linux/unload.h

diff --git a/fs/Makefile b/fs/Makefile
index e6ec1d3..fa6bd11 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -11,7 +11,7 @@ obj-y :=	open.o read_write.o file_table.o super.o \
 		attr.o bad_inode.o file.o filesystems.o namespace.o \
 		seq_file.o xattr.o libfs.o fs-writeback.o \
 		pnode.o drop_caches.o splice.o sync.o utimes.o \
-		stack.o fs_struct.o statfs.o
+		stack.o fs_struct.o statfs.o libunload.o
 
 ifeq ($(CONFIG_BLOCK),y)
 obj-y +=	buffer.o bio.o block_dev.o direct-io.o mpage.o ioprio.o
diff --git a/fs/libunload.c b/fs/libunload.c
new file mode 100644
index 0000000..2470bf2
--- /dev/null
+++ b/fs/libunload.c
@@ -0,0 +1,166 @@
+#include <linux/fs.h>
+#include <linux/mm_types.h>
+#include <linux/mm.h>
+#include <linux/spinlock.h>
+#include <linux/module.h>
+#include <linux/unload.h>
+
+struct unload_barrier {
+	struct completion	completion;
+	int			releasers;
+};
+
+void unload_init(struct unload *unload)
+{
+	INIT_HLIST_HEAD(&unload->ufiles);
+	spin_lock_init(&unload->lock);
+	unload->active = 1;
+	unload->barrier = NULL;
+}
+EXPORT_SYMBOL_GPL(unload_init);
+
+void unload_file_init(struct unload_file *ufile, struct file *file, struct unload *unload)
+{
+	ufile->file = file;
+	ufile->unload = unload;
+	INIT_HLIST_NODE(&ufile->list);
+}
+EXPORT_SYMBOL_GPL(unload_file_init);
+
+bool unload_trylock(struct unload *unload)
+{
+	bool locked = false;
+	spin_lock(&unload->lock);
+	if (likely(!unload->barrier)) {
+		unload->active++;
+		locked = true;
+	}
+	spin_unlock(&unload->lock);
+	return locked;
+}
+EXPORT_SYMBOL_GPL(unload_trylock);
+
+static void __unload_unlock(struct unload *unload)
+{
+	unload->active--;
+	if ((unload->active == 0) && (unload->barrier->releasers == 0))
+		complete(&unload->barrier->completion);
+}
+
+void unload_unlock(struct unload *unload)
+{
+	spin_lock(&unload->lock);
+	__unload_unlock(unload);
+	spin_unlock(&unload->lock);
+}
+EXPORT_SYMBOL_GPL(unload_unlock);
+
+static void __unload_file_attach(struct unload_file *ufile, struct unload *unload)
+{
+	ufile->unload = unload;
+	hlist_add_head(&ufile->list, &unload->ufiles);
+}
+
+void unload_file_attach(struct unload_file *ufile, struct unload *unload)
+{
+	spin_lock(&unload->lock);
+	__unload_file_attach(ufile, unload);
+	spin_unlock(&unload->lock);
+}
+EXPORT_SYMBOL_GPL(unload_file_attach);
+
+static void __unload_file_detach(struct unload_file *ufile)
+{
+	hlist_del_init(&ufile->list);
+}
+
+void unload_file_detach(struct unload_file *ufile)
+{
+	struct unload *unload = ufile->unload;
+
+	spin_lock(&unload->lock);
+	__unload_file_detach(ufile);
+	spin_unlock(&unload->lock);
+}
+EXPORT_SYMBOL_GPL(unload_file_detach);
+
+struct unload_file *find_unload_file(struct unload *unload, struct file *file)
+{
+	struct unload_file *ufile;
+	struct hlist_node *pos;
+
+	spin_lock(&unload->lock);
+	hlist_for_each_entry(ufile, pos, &unload->ufiles, list) {
+		if (ufile->file == file)
+			goto done;
+	}
+	ufile = NULL;
+done:
+	spin_unlock(&unload->lock);
+	return ufile;
+}
+EXPORT_SYMBOL_GPL(find_unload_file);
+
+bool unload_release_trylock(struct unload_file *ufile)
+{
+	struct unload *unload = ufile->unload;
+	bool locked = false;
+
+	spin_lock(&unload->lock);
+	if (!hlist_unhashed(&ufile->list))
+		locked = true;
+	spin_unlock(&unload->lock);
+	return locked;
+}
+EXPORT_SYMBOL_GPL(unload_release_trylock);
+
+void unload_release_unlock(struct unload_file *ufile)
+{
+	struct unload *unload = ufile->unload;
+	struct unload_barrier *barrier;
+
+	spin_lock(&unload->lock);
+	__unload_file_detach(ufile);
+	barrier = unload->barrier;
+	if (barrier) {
+		barrier->releasers -= 1;
+		if ((barrier->releasers == 0) && (unload->active == 0))
+			complete(&barrier->completion);
+	}
+	spin_unlock(&unload->lock);
+}
+EXPORT_SYMBOL_GPL(unload_release_unlock);
+
+
+void unload_barrier(struct unload *unload)
+{
+	struct unload_barrier barrier;
+	struct unload_file *ufile;
+	struct hlist_node *pos;
+
+	/* Guarantee that when this function returns I am not
+	 * executing any code protected by the unload_lock or
+	 * unload_releas_lock, and that I will never again execute
+	 * code protected by those locks.
+	 *
+	 * Also guarantee the file count for every file remaining on
+	 * the unload ufiles list has been incremented.  The increment
+	 * of the file count guarantees __fput will not be called.
+	 */
+	init_completion(&barrier.completion);
+	barrier.releasers = 0;
+
+	spin_lock(&unload->lock);
+	unload->barrier = &barrier;
+
+	hlist_for_each_entry(ufile, pos, &unload->ufiles, list)
+		if (!atomic_long_inc_not_zero(&ufile->file->f_count))
+			barrier.releasers++;
+	unload->active--;
+	if (unload->active || barrier.releasers) {
+		spin_unlock(&unload->lock);
+		wait_for_completion(&barrier.completion);
+		spin_lock(&unload->lock);
+	}
+	spin_unlock(&unload->lock);
+}
diff --git a/include/linux/unload.h b/include/linux/unload.h
new file mode 100644
index 0000000..fc1b4f6
--- /dev/null
+++ b/include/linux/unload.h
@@ -0,0 +1,33 @@
+#ifndef _LINUX_UNLOAD_H
+#define _LINUX_UNLOAD_H
+
+#include <linux/list.h>
+
+struct file;
+struct vm_operations_struct;
+struct unload_barrier;
+
+struct unload {
+	struct hlist_head	ufiles;
+	struct unload_barrier	*barrier;
+	spinlock_t		lock;
+	int			active;
+};
+
+struct unload_file {
+	struct unload		*unload;
+	struct hlist_node	list;
+	struct file 		*file;
+};
+
+void unload_init(struct unload *unload);
+void unload_file_init(struct unload_file *ufile, struct file *file, struct unload *unload);
+bool unload_trylock(struct unload *unload);
+void unload_unlock(struct unload *unload);
+bool unload_release_trylock(struct unload_file *ufile);
+void unload_release_unlock(struct unload_file *ufile);
+void unload_file_attach(struct unload_file *ufile, struct unload *unload);
+void unload_file_detach(struct unload_file *ufile);
+struct unload_file *find_unload_file(struct unload *unload, struct file *file);
+void unload_barrier(struct unload *unload);
+#endif /* _LINUX_UNLOAD_H */
-- 
1.7.2.2

[PATCH 5/5] uio: Implement hotunplug support, using libunload

[Eric W. Biederman]

With this change it is possible to remove a module that implements
a uio device, or to remove the underlying hardware device of a uio
device withot crashing the kernel, or causing user space more problems
than just an I/O error.

The implicit module parameter that was pased  by uio_register_device
to __uio_register_device has been removed as it is now safe to call
uio_unregister_device while uio file handles are open.

In uio all file methods (except release) now must perform their work
inside of the unload_lock.  This guarantees that uio_unregister_device
won't return until all currently running uio methods complete, and
it allows uio to return -EIO after the underlying device has been
unregistered.

The fops->release method must perform the bulk of it's work under the
unload_release_lock.  With release more needs to be done than to
more than wait for the method to finish executing, the release also
needs to handle the info->releae being called early on files that
are not closed when unregister is running.  Taking the unload_release
lock fails if and only if the info->release method has already been
called at the time fops->release runs.

Instead of holding a module reference the uio files have been modified
to instead hold an extra reference to struct uio_device, ensuring
that the uio file_operations functions will not access freed memory
even after the uio device has been unregistered.

The bulk of the changes are simply modifying the code flow of the
uio functions to run under the appropriate unload lock.

uio_open is modestly special in that it needs to run under the
uio_unload lock (to keep the uio device from unloading), but not have
it's file structure on the uio unload list until it is certain
that the open will succeed (ensuring that release will not be called
on a file that we failed to open).  uio_open also careflly grabs
the reference to the uio_device under the idr_mutex to guarnatee
the the uio_device reference held by the idr data structure
is valid while we are incrementing the uio_device reference count.

uio_device_unregister does a fair bit more than what it used to.  All
of the extra work is essentially handling the early shutdown of file
handles when a device is unregistered while files are still open.

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c          |  262 +++++++++++++++++++++++++++++++++-----------
 include/linux/uio_driver.h |   10 +--
 2 files changed, 198 insertions(+), 74 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index fc52fbc..b0032e3 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -24,12 +24,13 @@
 #include <linux/string.h>
 #include <linux/kobject.h>
 #include <linux/cdev.h>
+#include <linux/file.h>
 #include <linux/uio_driver.h>
+#include <linux/unload.h>
 
 #define UIO_MAX_DEVICES		(1U << MINORBITS)
 
 struct uio_device {
-	struct module		*owner;
 	struct device		device;
 	int			minor;
 	atomic_t		event;
@@ -38,6 +39,7 @@ struct uio_device {
 	struct uio_info		*info;
 	struct kobject		*map_dir;
 	struct kobject		*portio_dir;
+	struct unload		unload;
 };
 
 static int uio_major;
@@ -424,106 +426,140 @@ static irqreturn_t uio_interrupt(int irq, void *dev_id)
 }
 
 struct uio_listener {
-	struct uio_device *dev;
+	struct unload_file ufile;
 	s32 event_count;
 };
 
+#define listener_idev(LISTENER) \
+	container_of((LISTENER)->ufile.unload, struct uio_device, unload)
+
 static int uio_open(struct inode *inode, struct file *filep)
 {
 	struct uio_device *idev;
-	struct uio_listener *listener;
+	struct uio_listener *listener = NULL;
 	int ret = 0;
 
 	mutex_lock(&minor_lock);
 	idev = idr_find(&uio_idr, iminor(inode));
+	if (idev)
+		get_device(&idev->device);
 	mutex_unlock(&minor_lock);
-	if (!idev) {
-		ret = -ENODEV;
-		goto out;
-	}
-
-	if (!try_module_get(idev->owner)) {
-		ret = -ENODEV;
-		goto out;
-	}
+	ret = -ENODEV;
+	if (!idev)
+		goto err_out;
 
 	listener = kmalloc(sizeof(*listener), GFP_KERNEL);
-	if (!listener) {
-		ret = -ENOMEM;
-		goto err_alloc_listener;
-	}
+	ret = -ENOMEM;
+	if (!listener)
+		goto err_out;
 
-	listener->dev = idev;
+	unload_file_init(&listener->ufile, filep, &idev->unload);
 	listener->event_count = atomic_read(&idev->event);
 	filep->private_data = listener;
 
-	if (idev->info->open) {
+	/*
+	 * Take the users lock before opening the file to ensure the
+	 * file is not unregistered while it is being opened.
+	 */
+	ret = -EIO;
+	if (!unload_trylock(&idev->unload))
+		goto err_out;
+
+	ret = 0;
+	if (idev->info->open)
 		ret = idev->info->open(idev->info, inode);
-		if (ret)
-			goto err_infoopen;
+	
+	/*
+	 * Add to the listener list only if the open succeeds.
+	 * This ensures that uio_unregister_device won't call
+	 * release unless open has succeeded.
+	 */
+	if (ret == 0)
+		unload_file_attach(&listener->ufile, &idev->unload);
+	unload_unlock(&idev->unload);
+err_out:
+	if (ret) {
+		if (idev)
+			put_device(&idev->device);
+		kfree(listener);
 	}
-	return 0;
-
-err_infoopen:
-	kfree(listener);
-
-err_alloc_listener:
-	module_put(idev->owner);
-
-out:
 	return ret;
 }
 
 static int uio_fasync(int fd, struct file *filep, int on)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
+	int ret = -EIO;
 
-	return fasync_helper(fd, filep, on, &idev->async_queue);
+	if (unload_trylock(&idev->unload)) {
+		ret = fasync_helper(fd, filep, on, &idev->async_queue);
+		unload_unlock(&idev->unload);
+	}
+
+	return ret;
 }
 
 static int uio_release(struct inode *inode, struct file *filep)
 {
 	int ret = 0;
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
+
+	if (unload_release_trylock(&listener->ufile)) {
 
-	if (idev->info->release)
-		ret = idev->info->release(idev->info, inode);
+		if (idev->info->release)
+			ret = idev->info->release(idev->info, inode);
 
-	module_put(idev->owner);
+		unload_release_unlock(&listener->ufile);
+	}
 	kfree(listener);
+	put_device(&idev->device);
 	return ret;
 }
 
 static unsigned int uio_poll(struct file *filep, poll_table *wait)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
+	unsigned int ret;
+
+	if (!unload_trylock(&idev->unload))
+		return POLLERR;
 
+	ret = POLLERR;
 	if (!idev->info->irq)
-		return -EIO;
+		goto out;
 
 	poll_wait(filep, &idev->wait, wait);
+	ret = 0;
 	if (listener->event_count != atomic_read(&idev->event))
-		return POLLIN | POLLRDNORM;
-	return 0;
+		ret = POLLIN | POLLRDNORM;
+
+out:
+	unload_unlock(&idev->unload);
+	return ret;
 }
 
 static ssize_t uio_read(struct file *filep, char __user *buf,
 			size_t count, loff_t *ppos)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
 	DECLARE_WAITQUEUE(wait, current);
 	ssize_t retval;
 	s32 event_count;
 
-	if (!idev->info->irq)
+	if (!unload_trylock(&idev->unload))
 		return -EIO;
 
+	retval = -EIO;
+	if (!idev->info->irq)
+		goto out;
+
+	retval = -EIO;
 	if (count != sizeof(s32))
-		return -EINVAL;
+		goto out;
 
 	add_wait_queue(&idev->wait, &wait);
 
@@ -550,12 +586,21 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 			retval = -ERESTARTSYS;
 			break;
 		}
+		unload_unlock(&idev->unload);
+
 		schedule();
+
+		if (!unload_trylock(&idev->unload)) {
+			retval = -EIO;
+			break;
+		}
 	} while (1);
 
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(&idev->wait, &wait);
 
+out:
+	unload_unlock(&idev->unload);
 	return retval;
 }
 
@@ -563,24 +608,33 @@ static ssize_t uio_write(struct file *filep, const char __user *buf,
 			size_t count, loff_t *ppos)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
 	ssize_t retval;
 	s32 irq_on;
 
-	if (!idev->info->irq)
+	if (!unload_trylock(&idev->unload))
 		return -EIO;
 
+	retval = -EIO;
+	if (!idev->info->irq)
+		goto out;
+
+	retval = -EINVAL;
 	if (count != sizeof(s32))
-		return -EINVAL;
+		goto out;
 
+	retval = -ENOSYS;
 	if (!idev->info->irqcontrol)
-		return -ENOSYS;
+		goto out;
 
+	retval = -EFAULT;
 	if (copy_from_user(&irq_on, buf, count))
-		return -EFAULT;
+		goto out;
 
 	retval = idev->info->irqcontrol(idev->info, irq_on);
 
+out:
+	unload_unlock(&idev->unload);
 	return retval ? retval : sizeof(s32);
 }
 
@@ -598,16 +652,22 @@ static int uio_find_mem_index(struct vm_area_struct *vma)
 	return -1;
 }
 
-static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+static int uio_logical_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct uio_device *idev = vma->vm_private_data;
 	struct page *page;
 	unsigned long offset;
+	int ret;
+	int mi;
 
-	int mi = uio_find_mem_index(vma);
-	if (mi < 0)
+	if (!unload_trylock(&idev->unload))
 		return VM_FAULT_SIGBUS;
 
+	ret = VM_FAULT_SIGBUS;
+	mi = uio_find_mem_index(vma);
+	if (mi < 0)
+		goto out;
+
 	/*
 	 * We need to subtract mi because userspace uses offset = N*PAGE_SIZE
 	 * to use mem[N].
@@ -621,11 +681,26 @@ static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 							+ offset);
 	get_page(page);
 	vmf->page = page;
-	return 0;
+	ret = 0;
+out:
+	unload_unlock(&idev->unload);
+	return ret;
 }
 
-static const struct vm_operations_struct uio_vm_ops = {
-	.fault = uio_vma_fault,
+static const struct vm_operations_struct uio_logical_vm_ops = {
+	.fault = uio_logical_fault,
+};
+
+static int uio_physical_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
+{
+	/* The pages should always be mapped, so we should never get
+	 * here unless the device has been hotunplugged
+	 */
+	return VM_FAULT_SIGBUS;
+}
+
+static const struct vm_operations_struct uio_physical_vm_ops = {
+	.fault = uio_physical_fault,
 };
 
 static int uio_mmap_physical(struct vm_area_struct *vma)
@@ -638,6 +713,7 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
 	vma->vm_flags |= VM_IO | VM_RESERVED;
 
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	vma->vm_ops = &uio_physical_vm_ops;
 
 	return remap_pfn_range(vma,
 			       vma->vm_start,
@@ -649,41 +725,54 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
 static int uio_mmap_logical(struct vm_area_struct *vma)
 {
 	vma->vm_flags |= VM_RESERVED;
-	vma->vm_ops = &uio_vm_ops;
+	vma->vm_ops = &uio_logical_vm_ops;
 	return 0;
 }
 
 static int uio_mmap(struct file *filep, struct vm_area_struct *vma)
 {
 	struct uio_listener *listener = filep->private_data;
-	struct uio_device *idev = listener->dev;
+	struct uio_device *idev = listener_idev(listener);
 	int mi;
 	unsigned long requested_pages, actual_pages;
+	int ret;
+
+	if (!unload_trylock(&idev->unload))
+		return -EIO;
 
+	ret = -EINVAL;
 	if (vma->vm_end < vma->vm_start)
-		return -EINVAL;
+		goto out;
 
 	vma->vm_private_data = idev;
 
+	ret = -EINVAL;
 	mi = uio_find_mem_index(vma);
 	if (mi < 0)
-		return -EINVAL;
+		goto out;
 
 	requested_pages = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
 	actual_pages = ((idev->info->mem[mi].addr & ~PAGE_MASK)
 			+ idev->info->mem[mi].size + PAGE_SIZE -1) >> PAGE_SHIFT;
+	ret = -EINVAL;
 	if (requested_pages > actual_pages)
-		return -EINVAL;
+		goto out;
 
 	switch (idev->info->mem[mi].memtype) {
 		case UIO_MEM_PHYS:
-			return uio_mmap_physical(vma);
+			ret = uio_mmap_physical(vma);
+			break;
 		case UIO_MEM_LOGICAL:
 		case UIO_MEM_VIRTUAL:
-			return uio_mmap_logical(vma);
+			ret = uio_mmap_logical(vma);
+			break;
 		default:
-			return -EINVAL;
+			ret = -EINVAL;
+			break;
 	}
+out:
+	unload_unlock(&idev->unload);
+	return ret;
 }
 
 static const struct file_operations uio_fops = {
@@ -782,9 +871,7 @@ static void uio_device_release(struct device *dev)
  *
  * returns zero on success or a negative error code.
  */
-int __uio_register_device(struct module *owner,
-			  struct device *parent,
-			  struct uio_info *info)
+int uio_register_device(struct device *parent, struct uio_info *info)
 {
 	struct uio_device *idev;
 	int ret = 0;
@@ -800,10 +887,10 @@ int __uio_register_device(struct module *owner,
 		goto err_kzalloc;
 	}
 
-	idev->owner = owner;
 	idev->info = info;
 	init_waitqueue_head(&idev->wait);
 	atomic_set(&idev->event, 0);
+	unload_init(&idev->unload);
 
 	ret = uio_get_minor(idev);
 	if (ret)
@@ -852,7 +939,7 @@ err_get_minor:
 err_kzalloc:
 	return ret;
 }
-EXPORT_SYMBOL_GPL(__uio_register_device);
+EXPORT_SYMBOL_GPL(uio_register_device);
 
 /**
  * uio_unregister_device - unregister a industrial IO device
@@ -862,12 +949,54 @@ EXPORT_SYMBOL_GPL(__uio_register_device);
 void uio_unregister_device(struct uio_info *info)
 {
 	struct uio_device *idev;
+	struct unload_file *ufile;
+	struct hlist_node *n1, *n2;
 
 	if (!info || !info->uio_dev)
 		return;
 
 	idev = info->uio_dev;
 
+	/*
+	 * Stop accepting new callers into the uio functions, and wait
+	 * until all existing callers into the uio functions are done.
+	 */
+	unload_barrier(&idev->unload);
+
+	/* Notify listeners that the uio devices has gone. */
+	wake_up_interruptible_all(&idev->wait);
+	kill_fasync(&idev->async_queue, SIGIO, POLL_ERR);
+
+	/*
+	 * unload_barrier froze the idev->unload.ufiles list and grabbed
+	 * a reference to every file on that list.  Now cleanup those files
+	 * and drop the extra reference.
+	 */
+	hlist_for_each_entry_safe(ufile, n1, n2, &idev->unload.ufiles, list) {
+		struct file *file = ufile->file;
+		struct inode *inode = file->f_dentry->d_inode;
+
+		/* Cause all mappings to trigger a SIGBUS */
+		unmap_mapping_range(inode->i_mapping, 0, 0, 1);
+
+		/* Remove the file from the fasync list because any
+		 * future attempts to add/remove this file from this
+		 * list will return -EIO.
+		 */
+		fasync_helper(-1, file, 0, &idev->async_queue);
+
+		/* Now that userspace is totally blocked off run the
+		 * release code to clean up, the uio devices data
+		 * structures.
+		 */
+		if (info->release)
+			info->release(info, inode);
+
+		/* Forget about this file */
+		unload_file_detach(ufile);
+		fput(file);
+	}
+
 	uio_free_minor(idev);
 
 	if (info->irq && (info->irq != UIO_IRQ_CUSTOM))
@@ -875,6 +1004,7 @@ void uio_unregister_device(struct uio_info *info)
 
 	uio_dev_del_attributes(idev);
 
+	idev->info = NULL;
 	device_unregister(&idev->device);
 
 	return;
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 33789d4..2551737 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -94,14 +94,8 @@ struct uio_info {
 };
 
 extern int __must_check
-	__uio_register_device(struct module *owner,
-			      struct device *parent,
-			      struct uio_info *info);
-static inline int __must_check
-	uio_register_device(struct device *parent, struct uio_info *info)
-{
-	return __uio_register_device(THIS_MODULE, parent, info);
-}
+	uio_register_device(struct device *parent, struct uio_info *info);
+
 extern void uio_unregister_device(struct uio_info *info);
 extern void uio_event_notify(struct uio_info *info);
 
-- 
1.7.2.2

[PATCH 6/5] uio: Fix accidentally changed return value in uio_read

[Eric W. Biederman]

Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>
---
 drivers/uio/uio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index b0032e3..20743aa 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -557,7 +557,7 @@ static ssize_t uio_read(struct file *filep, char __user *buf,
 	if (!idev->info->irq)
 		goto out;
 
-	retval = -EIO;
+	retval = -EINVAL;
 	if (count != sizeof(s32))
 		goto out;
 
-- 
1.7.2.3

Re: [PATCH 1/5] uio: Simplify the lifetime logic of struct uio_device.

[Hans J. Koch]

On Sun, Sep 26, 2010 at 03:46:56PM -0700, Eric W. Biederman wrote:
> 
> Embed struct device into struct uio_device, and use
> the refcounting and the release method of struct device
> to control struct uio_device.
> 
> This allows device_create and device_destroy to be replaced
> with the more standard device_register and device_unregister,
> and allows the struct device reference count to act as a
> reference count on struct idev as well.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@aristanetworks.com>

Eric,
just to give you a feedback: I simply didn't find the time to have a look
at your patches. I have a 200% workload ATM in my daily job. I hope I can
have a look at the weekend.

Sorry for the delay, but you're not forgotten ;-)

Thanks,
Hans

Re: [PATCH 3/5] uio: Remove unused uio_info mmap method.

[Hans J. Koch]

On Sun, Sep 26, 2010 at 03:48:19PM -0700, Eric W. Biederman wrote:
> 
> There are no drivers in the kernel that implement the uio_info mmap
> method there is no point in keeping it.
> 
> Further keeping the mmap method would necessiate wrapping all of the
> methods in vm_operations_struct to successfully implement suppport
> for hotunplugable hardware, and it I have yet to find a correct way to
> wrap the the vm_operations_struct close method.

I have no objections, but please update Documentation/DocBook/uio-howto.tmpl
accordingly.

Thanks,
Hans

Re: [PATCH 4/5] libunload: A library to help remove open files

[Hans J. Koch]

On Sun, Sep 26, 2010 at 03:48:44PM -0700, Eric W. Biederman wrote:

This doesn't build, see below.

Thanks,
Hans

[...]
> +}
> +EXPORT_SYMBOL_GPL(unload_release_unlock);
> +
> +
> +void unload_barrier(struct unload *unload)
> +{
> +	struct unload_barrier barrier;
> +	struct unload_file *ufile;
> +	struct hlist_node *pos;
> +
> +	/* Guarantee that when this function returns I am not
> +	 * executing any code protected by the unload_lock or
> +	 * unload_releas_lock, and that I will never again execute
> +	 * code protected by those locks.
> +	 *
> +	 * Also guarantee the file count for every file remaining on
> +	 * the unload ufiles list has been incremented.  The increment
> +	 * of the file count guarantees __fput will not be called.
> +	 */
> +	init_completion(&barrier.completion);
> +	barrier.releasers = 0;
> +
> +	spin_lock(&unload->lock);
> +	unload->barrier = &barrier;
> +
> +	hlist_for_each_entry(ufile, pos, &unload->ufiles, list)
> +		if (!atomic_long_inc_not_zero(&ufile->file->f_count))
> +			barrier.releasers++;
> +	unload->active--;
> +	if (unload->active || barrier.releasers) {
> +		spin_unlock(&unload->lock);
> +		wait_for_completion(&barrier.completion);
> +		spin_lock(&unload->lock);
> +	}
> +	spin_unlock(&unload->lock);
> +}

There's an EXPORT_SYMBOL_GPL(unload_barrier) missing here...

> diff --git a/include/linux/unload.h b/include/linux/unload.h
> new file mode 100644
> index 0000000..fc1b4f6
> --- /dev/null
> +++ b/include/linux/unload.h
> @@ -0,0 +1,33 @@
> +#ifndef _LINUX_UNLOAD_H
> +#define _LINUX_UNLOAD_H
> +
> +#include <linux/list.h>
> +
> +struct file;
> +struct vm_operations_struct;
> +struct unload_barrier;
> +
> +struct unload {
> +	struct hlist_head	ufiles;
> +	struct unload_barrier	*barrier;
> +	spinlock_t		lock;
> +	int			active;
> +};
> +
> +struct unload_file {
> +	struct unload		*unload;
> +	struct hlist_node	list;
> +	struct file 		*file;
> +};
> +
> +void unload_init(struct unload *unload);
> +void unload_file_init(struct unload_file *ufile, struct file *file, struct unload *unload);
> +bool unload_trylock(struct unload *unload);
> +void unload_unlock(struct unload *unload);
> +bool unload_release_trylock(struct unload_file *ufile);
> +void unload_release_unlock(struct unload_file *ufile);
> +void unload_file_attach(struct unload_file *ufile, struct unload *unload);
> +void unload_file_detach(struct unload_file *ufile);
> +struct unload_file *find_unload_file(struct unload *unload, struct file *file);
> +void unload_barrier(struct unload *unload);
> +#endif /* _LINUX_UNLOAD_H */
> -- 
> 1.7.2.2

Re: [PATCH 5/5] uio: Implement hotunplug support, using libunload

[Hans J. Koch]

On Sun, Sep 26, 2010 at 03:49:03PM -0700, Eric W. Biederman wrote:

Checkpatch reports one error, see below.

Thanks,
Hans

> 
> +	/*
> +	 * Take the users lock before opening the file to ensure the
> +	 * file is not unregistered while it is being opened.
> +	 */
> +	ret = -EIO;
> +	if (!unload_trylock(&idev->unload))
> +		goto err_out;
> +
> +	ret = 0;
> +	if (idev->info->open)
>  		ret = idev->info->open(idev->info, inode);
> -		if (ret)
> -			goto err_infoopen;
> +	

The above line adds trailing whitespace.

> +	/*
> +	 * Add to the listener list only if the open succeeds.
> +	 * This ensures that uio_unregister_device won't call
> +	 * release unless open has succeeded.
> +	 */

Re: [PATCH 5/5] uio: Implement hotunplug support, using libunload

[Hans J. Koch]

On Sun, Sep 26, 2010 at 03:49:03PM -0700, Eric W. Biederman wrote:
> 
> With this change it is possible to remove a module that implements
> a uio device, or to remove the underlying hardware device of a uio
> device withot crashing the kernel, or causing user space more problems
> than just an I/O error.

Well, that I/O error can also be a segfault if userspace accesses
memory previously mmap'ed. So a userspace program needs to properly
handle -EIO from read(), and has to handle SIG_SEGV. This should also
be mentioned in Documentation/DocBook/uio-howto.tmpl.
Or do you have a better solution?

Thanks,
Hans

Re: [PATCH 5/5] uio: Implement hotunplug support, using libunload

[Eric W. Biederman]

"Hans J. Koch" <hjk@linutronix.de> writes:

> On Sun, Sep 26, 2010 at 03:49:03PM -0700, Eric W. Biederman wrote:
>> 
>> With this change it is possible to remove a module that implements
>> a uio device, or to remove the underlying hardware device of a uio
>> device withot crashing the kernel, or causing user space more problems
>> than just an I/O error.
>
> Well, that I/O error can also be a segfault if userspace accesses
> memory previously mmap'ed. So a userspace program needs to properly
> handle -EIO from read(), and has to handle SIG_SEGV. 

It is SIG_BUS (just like truncate), but yes.  If userspace doesn't
handle there error it can go down.  All of which is a better state
then the current version where the kernel crash if you hotunplug
a uio device.

> This should also
> be mentioned in Documentation/DocBook/uio-howto.tmpl.
> Or do you have a better solution?

A better solution for?

Eric

Re: [PATCH 5/5] uio: Implement hotunplug support, using libunload

[Hans J. Koch]

On Mon, Oct 04, 2010 at 11:19:34AM -0700, Eric W. Biederman wrote:
> "Hans J. Koch" <hjk@linutronix.de> writes:
> 
> > On Sun, Sep 26, 2010 at 03:49:03PM -0700, Eric W. Biederman wrote:
> >> 
> >> With this change it is possible to remove a module that implements
> >> a uio device, or to remove the underlying hardware device of a uio
> >> device withot crashing the kernel, or causing user space more problems
> >> than just an I/O error.
> >
> > Well, that I/O error can also be a segfault if userspace accesses
> > memory previously mmap'ed. So a userspace program needs to properly
> > handle -EIO from read(), and has to handle SIG_SEGV. 
> 
> It is SIG_BUS (just like truncate), but yes.

All I saw was "Segmentation fault" from my test app when unloading
my uio_dummy test driver. I didn't check which signal it actually was.
Doesn't matter too much as long as it can be handled.

> If userspace doesn't
> handle there error it can go down.  All of which is a better state
> then the current version where the kernel crash if you hotunplug
> a uio device.
> 
> > This should also
> > be mentioned in Documentation/DocBook/uio-howto.tmpl.
> > Or do you have a better solution?
> 
> A better solution for?

For preventing the segfault situation. But forget that, it's OK as it
is. But it should be mentioned in the documentation.

Thanks,
Hans

> 
> Eric