From 1363e4fb4f460a903f3d62d7c1466ce76e31adb0 Mon Sep 17 00:00:00 2001 From: Ian Abbott Date: Wed, 11 Dec 2013 14:51:03 +0000 Subject: [PATCH] staging/comedi: bug fix for module usage count on device removal When a dynamically created comedi device is being automatically removed by a call to `comedi_auto_unconfig()` from the lower level driver, `comedi_device_cleanup()` is called to perform the detachment from the lower level driver. If the comedi device is open at the time, `dev->use_count` will be the the number of outstanding opens. The function currently decrements the the module counts of the "comedi" module and the low-level driver module by this amount and reduces `dev->use_count` to zero. There are various problems with this as the `release` file operation handler `comedi_close()` also decrements `dev->use_count` and decrements the module usage counts. This means that `dev->use_count` and the module counts can end up negative. Also, the assumed one-to-one relationship between the file open count and the low-level module usage count is invalid and can get screwed up. We only want to stop the low-level module being unloaded while a comedi device using the module has an open file object. Also, there is no need to manipulate the module count of the core "comedi" module at all since the comedi module is the owner of the file operations structure and the system will not unload the module while there are open file objects using it. Correct the bugs and simplify as follows: 1. Get rid of the module count manipulations of the core "comedi" module (`THIS_MODULE`) altogether. 2. Don't alter `dev->use_count` in `comedi_device_cleanup()` as it should only be altered by the `open` and `release` file operation handlers `comedi_open()` and `comedi_close()`. 3. Increment the low-level module count for the following reasons: a) In `comedi_open()` if the open count was zero and the comedi device is attached to the low-level driver. b) When the `COMEDI_DEVCONFIG` ioctl is used to manually attach an unattached comedi device to a low-level driver. The open count will be greater than zero at this time. The actual increment of the low-level module count is already done by `comedi_device_attach()`. 4. Decrement the low-level module count for the following reasons: a) In `comedi_close()` if the open count was 1 and the comedi device is attached to the low-level driver. b) In `comedi_device_cleanup()` (called via `comedi_auto_unconfig()` --> `comedi_release_hardware_device()` --> `comedi_free_board_dev()` when the comedi device is automatically unconfigured due to action by the low-level driver) if the device was attached (which it should be) and open count was non-zero (greater than zero). c) When the `COMEDI_DEVCONFIG` ioctl is used to manually detach the comedi device from the low-level driver. The open count will be greater than zero at this time. The open count should never go negative. Parts 3 and 4 ensure that the low-level module usage count is incremented on entering the state where the comedi device is attached to the low-level driver AND the open count is greater than zero, and is decremented on leaving that state. Signed-off-by: Ian Abbott Signed-off-by: Greg Kroah-Hartman --- drivers/staging/comedi/comedi_fops.c | 36 ++++++++++------------------ 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 6e5f538023c1..fc3404882ae8 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -120,12 +120,8 @@ static void comedi_device_cleanup(struct comedi_device *dev) if (dev->attached) driver_module = dev->driver->module; comedi_device_detach(dev); - while (dev->use_count > 0) { - if (driver_module) - module_put(driver_module); - module_put(THIS_MODULE); - dev->use_count--; - } + if (driver_module && dev->use_count) + module_put(driver_module); mutex_unlock(&dev->mutex); } @@ -2356,22 +2352,17 @@ static int comedi_open(struct inode *inode, struct file *file) goto out; } ok: - __module_get(THIS_MODULE); - - if (dev->attached) { + if (dev->attached && dev->use_count == 0) { if (!try_module_get(dev->driver->module)) { - module_put(THIS_MODULE); rc = -ENOSYS; goto out; } - } - - if (dev->attached && dev->use_count == 0 && dev->open) { - rc = dev->open(dev); - if (rc < 0) { - module_put(dev->driver->module); - module_put(THIS_MODULE); - goto out; + if (dev->open) { + rc = dev->open(dev); + if (rc < 0) { + module_put(dev->driver->module); + goto out; + } } } @@ -2411,12 +2402,11 @@ static int comedi_close(struct inode *inode, struct file *file) s->lock = NULL; } } - if (dev->attached && dev->use_count == 1 && dev->close) - dev->close(dev); - - module_put(THIS_MODULE); - if (dev->attached) + if (dev->attached && dev->use_count == 1) { + if (dev->close) + dev->close(dev); module_put(dev->driver->module); + } dev->use_count--; -- 2.20.1