* [PATCH 1/4] uio: Simplify the lifetime logic of struct uio_device.
2015-01-16 18:49 [PATCH 0/4] uio hotplug support Mandeep Sandhu
@ 2015-01-16 18:49 ` Mandeep Sandhu
2015-01-25 12:33 ` Greg KH
2015-01-16 18:49 ` [PATCH 2/4] uio: Remove unused uio_info mmap method Mandeep Sandhu
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Mandeep Sandhu @ 2015-01-16 18:49 UTC (permalink / raw)
To: hjk, gregkh; +Cc: linux-kernel, viro, linux-fsdevel, Mandeep Sandhu
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.
Tested-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
Signed-off-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
---
drivers/uio/uio.c | 41 ++++++++++++++++++++++++++---------------
include/linux/uio_driver.h | 3 ++-
2 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 6276f13..350b81b 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -16,7 +16,6 @@
#include <linux/module.h>
#include <linux/init.h>
#include <linux/poll.h>
-#include <linux/device.h>
#include <linux/slab.h>
#include <linux/mm.h>
#include <linux/idr.h>
@@ -270,7 +269,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;
}
@@ -295,7 +294,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;
}
@@ -334,7 +333,7 @@ err_map_kobj:
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;
}
@@ -371,7 +370,7 @@ static int uio_get_minor(struct uio_device *idev)
idev->minor = retval;
retval = 0;
} else if (retval == -ENOSPC) {
- dev_err(idev->dev, "too many uio devices\n");
+ dev_err(&idev->device, "too many uio devices\n");
retval = -EINVAL;
}
mutex_unlock(&minor_lock);
@@ -785,6 +784,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);
+
+ devm_kfree(dev->parent, idev);
+}
+
/**
* uio_register_device - register a new userspace IO device
* @owner: module that creates the new device
@@ -819,14 +825,19 @@ int __uio_register_device(struct module *owner,
if (ret)
return ret;
- 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)
@@ -835,7 +846,7 @@ int __uio_register_device(struct module *owner,
info->uio_dev = idev;
if (info->irq && (info->irq != UIO_IRQ_CUSTOM)) {
- ret = devm_request_irq(idev->dev, info->irq, uio_interrupt,
+ ret = devm_request_irq(&idev->device, info->irq, uio_interrupt,
info->irq_flags, info->name, idev);
if (ret)
goto err_request_irq;
@@ -846,7 +857,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_unregister(&idev->device);
err_device_create:
uio_free_minor(idev);
return ret;
@@ -871,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_unregister(&idev->device);
return;
}
diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
index 32c0e83..e8f7f82 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -14,6 +14,7 @@
#ifndef _UIO_DRIVER_H_
#define _UIO_DRIVER_H_
+#include <linux/device.h>
#include <linux/fs.h>
#include <linux/interrupt.h>
@@ -65,7 +66,7 @@ struct uio_port {
struct uio_device {
struct module *owner;
- struct device *dev;
+ struct device device;
int minor;
atomic_t event;
struct fasync_struct *async_queue;
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/4] uio: Simplify the lifetime logic of struct uio_device.
2015-01-16 18:49 ` [PATCH 1/4] uio: Simplify the lifetime logic of struct uio_device Mandeep Sandhu
@ 2015-01-25 12:33 ` Greg KH
2015-01-26 19:57 ` Mandeep Sandhu
0 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2015-01-25 12:33 UTC (permalink / raw)
To: Mandeep Sandhu; +Cc: hjk, linux-kernel, viro, linux-fsdevel
On Fri, Jan 16, 2015 at 10:49:36AM -0800, Mandeep Sandhu 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.
>
> Tested-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
> Signed-off-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
> ---
> drivers/uio/uio.c | 41 ++++++++++++++++++++++++++---------------
> include/linux/uio_driver.h | 3 ++-
> 2 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 6276f13..350b81b 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -16,7 +16,6 @@
> #include <linux/module.h>
> #include <linux/init.h>
> #include <linux/poll.h>
> -#include <linux/device.h>
> #include <linux/slab.h>
> #include <linux/mm.h>
> #include <linux/idr.h>
> @@ -270,7 +269,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;
> }
> @@ -295,7 +294,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;
> }
> @@ -334,7 +333,7 @@ err_map_kobj:
> 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;
> }
>
> @@ -371,7 +370,7 @@ static int uio_get_minor(struct uio_device *idev)
> idev->minor = retval;
> retval = 0;
> } else if (retval == -ENOSPC) {
> - dev_err(idev->dev, "too many uio devices\n");
> + dev_err(&idev->device, "too many uio devices\n");
> retval = -EINVAL;
> }
> mutex_unlock(&minor_lock);
> @@ -785,6 +784,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);
No, calculate the offset of, then you don't need to mess with drvdata,
as that shouldn't be used by the "core" of a subsystem.
> +
> + devm_kfree(dev->parent, idev);
> +}
> +
> /**
> * uio_register_device - register a new userspace IO device
> * @owner: module that creates the new device
> @@ -819,14 +825,19 @@ int __uio_register_device(struct module *owner,
> if (ret)
> return ret;
>
> - 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);
This will not be needed if you fix up the release function.
> +
> + ret = kobject_set_name(&idev->device.kobj, "uio%d", idev->minor);
dev_set_name()?
> + 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)
> @@ -835,7 +846,7 @@ int __uio_register_device(struct module *owner,
> info->uio_dev = idev;
>
> if (info->irq && (info->irq != UIO_IRQ_CUSTOM)) {
> - ret = devm_request_irq(idev->dev, info->irq, uio_interrupt,
> + ret = devm_request_irq(&idev->device, info->irq, uio_interrupt,
> info->irq_flags, info->name, idev);
> if (ret)
> goto err_request_irq;
> @@ -846,7 +857,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_unregister(&idev->device);
> err_device_create:
> uio_free_minor(idev);
> return ret;
> @@ -871,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_unregister(&idev->device);
>
> return;
> }
> diff --git a/include/linux/uio_driver.h b/include/linux/uio_driver.h
> index 32c0e83..e8f7f82 100644
> --- a/include/linux/uio_driver.h
> +++ b/include/linux/uio_driver.h
> @@ -14,6 +14,7 @@
> #ifndef _UIO_DRIVER_H_
> #define _UIO_DRIVER_H_
>
> +#include <linux/device.h>
> #include <linux/fs.h>
> #include <linux/interrupt.h>
>
> @@ -65,7 +66,7 @@ struct uio_port {
>
> struct uio_device {
> struct module *owner;
> - struct device *dev;
> + struct device device;
No need to rename the variable, just change it from being a pointer.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/4] uio: Simplify the lifetime logic of struct uio_device.
2015-01-25 12:33 ` Greg KH
@ 2015-01-26 19:57 ` Mandeep Sandhu
2015-01-26 20:04 ` Greg KH
0 siblings, 1 reply; 9+ messages in thread
From: Mandeep Sandhu @ 2015-01-26 19:57 UTC (permalink / raw)
To: Greg KH; +Cc: hjk, linux-kernel, viro, linux-fsdevel
>> +
>> + ret = kobject_set_name(&idev->device.kobj, "uio%d", idev->minor);
>
> dev_set_name()?
There's another instance of use of kobject_set_name in
uio_major_init(). Should I change that too ,or that should be done in
a new (unrelated) patch?
Thanks,
-mandeep
>
>
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] uio: Simplify the lifetime logic of struct uio_device.
2015-01-26 19:57 ` Mandeep Sandhu
@ 2015-01-26 20:04 ` Greg KH
0 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2015-01-26 20:04 UTC (permalink / raw)
To: Mandeep Sandhu; +Cc: hjk, linux-kernel, viro, linux-fsdevel
On Mon, Jan 26, 2015 at 11:57:30AM -0800, Mandeep Sandhu wrote:
> >> +
> >> + ret = kobject_set_name(&idev->device.kobj, "uio%d", idev->minor);
> >
> > dev_set_name()?
>
> There's another instance of use of kobject_set_name in
> uio_major_init(). Should I change that too ,or that should be done in
> a new (unrelated) patch?
Unrelated please, one logical change per patch is the rules.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/4] uio: Remove unused uio_info mmap method.
2015-01-16 18:49 [PATCH 0/4] uio hotplug support Mandeep Sandhu
2015-01-16 18:49 ` [PATCH 1/4] uio: Simplify the lifetime logic of struct uio_device Mandeep Sandhu
@ 2015-01-16 18:49 ` Mandeep Sandhu
2015-01-16 18:49 ` [PATCH 3/4] libunload: A library to help remove open files Mandeep Sandhu
2015-01-16 18:49 ` [PATCH 4/4] uio: Implement hotunplug support, using libunload Mandeep Sandhu
3 siblings, 0 replies; 9+ messages in thread
From: Mandeep Sandhu @ 2015-01-16 18:49 UTC (permalink / raw)
To: hjk, gregkh; +Cc: linux-kernel, viro, linux-fsdevel, Mandeep Sandhu
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.
Tested-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
Signed-off-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
---
drivers/uio/uio.c | 6 ------
include/linux/uio_driver.h | 2 --
2 files changed, 8 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 350b81b..4b57c6b 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -670,7 +670,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;
@@ -687,11 +686,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 e8f7f82..3d906e0 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -87,7 +87,6 @@ struct uio_device {
* @irq_flags: flags for request_irq()
* @priv: optional private data
* @handler: the device's irq handler
- * @mmap: mmap operation for this uio device
* @open: open operation for this uio device
* @release: release operation for this uio device
* @irqcontrol: disable/enable irqs when 0/1 is written to /dev/uioX
@@ -102,7 +101,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.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/4] libunload: A library to help remove open files
2015-01-16 18:49 [PATCH 0/4] uio hotplug support Mandeep Sandhu
2015-01-16 18:49 ` [PATCH 1/4] uio: Simplify the lifetime logic of struct uio_device Mandeep Sandhu
2015-01-16 18:49 ` [PATCH 2/4] uio: Remove unused uio_info mmap method Mandeep Sandhu
@ 2015-01-16 18:49 ` Mandeep Sandhu
2015-01-25 12:36 ` Greg KH
2015-01-16 18:49 ` [PATCH 4/4] uio: Implement hotunplug support, using libunload Mandeep Sandhu
3 siblings, 1 reply; 9+ messages in thread
From: Mandeep Sandhu @ 2015-01-16 18:49 UTC (permalink / raw)
To: hjk, gregkh; +Cc: linux-kernel, viro, linux-fsdevel, Mandeep Sandhu
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 conjunction with the code implementiong an object to make it
safe to 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
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.
Tested-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
Signed-off-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
---
fs/Makefile | 2 +-
fs/libunload.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/unload.h | 35 ++++++++++
3 files changed, 205 insertions(+), 1 deletion(-)
create mode 100644 fs/libunload.c
create mode 100644 include/linux/unload.h
diff --git a/fs/Makefile b/fs/Makefile
index bedff48..165bcfa 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 splice.o sync.o utimes.o \
- stack.o fs_struct.o statfs.o fs_pin.o nsfs.o
+ stack.o fs_struct.o statfs.o fs_pin.o nsfs.o libunload.o
ifeq ($(CONFIG_BLOCK),y)
obj-y += buffer.o block_dev.o direct-io.o mpage.o
diff --git a/fs/libunload.c b/fs/libunload.c
new file mode 100644
index 0000000..0a365bb
--- /dev/null
+++ b/fs/libunload.c
@@ -0,0 +1,169 @@
+#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;
+
+ spin_lock(&unload->lock);
+ hlist_for_each_entry(ufile, &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;
+
+ /* 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, &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);
+}
+EXPORT_SYMBOL_GPL(unload_barrier);
diff --git a/include/linux/unload.h b/include/linux/unload.h
new file mode 100644
index 0000000..83d378f
--- /dev/null
+++ b/include/linux/unload.h
@@ -0,0 +1,35 @@
+#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.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/4] libunload: A library to help remove open files
2015-01-16 18:49 ` [PATCH 3/4] libunload: A library to help remove open files Mandeep Sandhu
@ 2015-01-25 12:36 ` Greg KH
0 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2015-01-25 12:36 UTC (permalink / raw)
To: Mandeep Sandhu; +Cc: hjk, linux-kernel, viro, linux-fsdevel
On Fri, Jan 16, 2015 at 10:49:38AM -0800, Mandeep Sandhu wrote:
> 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 conjunction with the code implementiong an object to make it
> safe to 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
> 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.
Can you provide some documentation for how to use this, kerneldoc at the
very least.
Also, can you port the above mentioned interfaces (proc, sysfs, tun,
etc.) that need this type of logic to this api, so we can see how well
it works out? Just providing one user of the code (uio), doesn't really
show if this is "generic" enough for everyone to use.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 4/4] uio: Implement hotunplug support, using libunload
2015-01-16 18:49 [PATCH 0/4] uio hotplug support Mandeep Sandhu
` (2 preceding siblings ...)
2015-01-16 18:49 ` [PATCH 3/4] libunload: A library to help remove open files Mandeep Sandhu
@ 2015-01-16 18:49 ` Mandeep Sandhu
3 siblings, 0 replies; 9+ messages in thread
From: Mandeep Sandhu @ 2015-01-16 18:49 UTC (permalink / raw)
To: hjk, gregkh; +Cc: linux-kernel, viro, linux-fsdevel, Mandeep Sandhu
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 passed 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 carefully 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.
Tested-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
Signed-off-by: Mandeep Sandhu <mandeep.sandhu@cyaninc.com>
---
drivers/uio/uio.c | 258 +++++++++++++++++++++++++++++++++------------
include/linux/uio_driver.h | 11 +-
2 files changed, 194 insertions(+), 75 deletions(-)
diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 4b57c6b..50548fb 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -23,7 +23,9 @@
#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)
@@ -415,60 +417,73 @@ 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;
- }
- return 0;
-err_infoopen:
- kfree(listener);
+ /*
+ * 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);
-err_alloc_listener:
- module_put(idev->owner);
+ unload_unlock(&idev->unload);
-out:
+err_out:
+ if (ret) {
+ if (idev)
+ put_device(&idev->device);
+ kfree(listener);
+ }
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);
return fasync_helper(fd, filep, on, &idev->async_queue);
}
@@ -477,44 +492,61 @@ 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 (idev->info->release)
- ret = idev->info->release(idev->info, inode);
+ if (unload_release_trylock(&listener->ufile)) {
+ 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 = -EINVAL;
if (count != sizeof(s32))
- return -EINVAL;
+ goto out;
add_wait_queue(&idev->wait, &wait);
@@ -541,12 +573,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;
}
@@ -554,24 +595,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);
}
@@ -587,17 +637,23 @@ 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;
void *addr;
+ 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].
@@ -611,26 +667,38 @@ static int uio_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
page = vmalloc_to_page(addr);
get_page(page);
vmf->page = page;
- return 0;
+ ret = 0;
+out:
+ unload_unlock(&idev->unload);
+ return ret;
}
static const struct vm_operations_struct uio_logical_vm_ops = {
- .fault = uio_vma_fault,
+ .fault = uio_logical_fault,
};
-static int uio_mmap_logical(struct vm_area_struct *vma)
+static int uio_physical_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
{
- vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
- vma->vm_ops = &uio_logical_vm_ops;
- return 0;
+ /* 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,
#ifdef CONFIG_HAVE_IOREMAP_PROT
.access = generic_access_phys,
#endif
};
+static int uio_mmap_logical(struct vm_area_struct *vma)
+{
+ vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
+ vma->vm_ops = &uio_logical_vm_ops;
+ return 0;
+}
+
static int uio_mmap_physical(struct vm_area_struct *vma)
{
struct uio_device *idev = vma->vm_private_data;
@@ -647,6 +715,7 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
vma->vm_ops = &uio_physical_vm_ops;
vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+ vma->vm_ops = &uio_physical_vm_ops;
/*
* We cannot use the vm_iomap_memory() helper here,
@@ -667,34 +736,48 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
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_pages(vma);
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);
- case UIO_MEM_LOGICAL:
- case UIO_MEM_VIRTUAL:
- return uio_mmap_logical(vma);
- default:
- return -EINVAL;
+ case UIO_MEM_PHYS:
+ ret = uio_mmap_physical(vma);
+ break;
+ case UIO_MEM_LOGICAL:
+ case UIO_MEM_VIRTUAL:
+ ret = uio_mmap_logical(vma);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
}
+out:
+ unload_unlock(&idev->unload);
+ return ret;
}
static const struct file_operations uio_fops = {
@@ -793,9 +876,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;
@@ -810,10 +891,10 @@ int __uio_register_device(struct module *owner,
return -ENOMEM;
}
- 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)
@@ -856,7 +937,7 @@ err_device_create:
uio_free_minor(idev);
return ret;
}
-EXPORT_SYMBOL_GPL(__uio_register_device);
+EXPORT_SYMBOL_GPL(uio_register_device);
/**
* uio_unregister_device - unregister a industrial IO device
@@ -866,16 +947,59 @@ 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 *n;
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, n, &idev->unload.ufiles, list) {
+ struct file *file = ufile->file;
+ struct inode *inode = file->f_path.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);
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 3d906e0..81674ef 100644
--- a/include/linux/uio_driver.h
+++ b/include/linux/uio_driver.h
@@ -17,6 +17,7 @@
#include <linux/device.h>
#include <linux/fs.h>
#include <linux/interrupt.h>
+#include <linux/unload.h>
struct module;
struct uio_map;
@@ -65,7 +66,6 @@ struct uio_port {
#define MAX_UIO_PORT_REGIONS 5
struct uio_device {
- struct module *owner;
struct device device;
int minor;
atomic_t event;
@@ -74,6 +74,7 @@ struct uio_device {
struct uio_info *info;
struct kobject *map_dir;
struct kobject *portio_dir;
+ struct unload unload;
};
/**
@@ -107,13 +108,7 @@ struct uio_info {
};
extern int __must_check
- __uio_register_device(struct module *owner,
- struct device *parent,
- struct uio_info *info);
-
-/* use a define to avoid include chaining to get THIS_MODULE */
-#define uio_register_device(parent, info) \
- __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.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread