HID: hiddev: fix mess in hiddev_open()

BugLink: https://bugs.launchpad.net/bugs/1861934
commit 18a1b06e5b91d47dc86c0a66a762646ea7c5d141 upstream.

The open method of hiddev handler fails to bring the device out of
autosuspend state as was promised in 0361a28d3f9a, as it actually has 2
blocks that try to start the transport (call hid_hw_open()) with both
being guarded by the "open" counter, so the 2nd block is never executed as
the first block increments the counter so it is never at 0 when we check
it for the second block.

Additionally hiddev_open() was leaving counter incremented on errors,
causing the device to never be reopened properly if there was ever an
error.

Let's fix all of this by factoring out code that creates client structure
and powers up the device into a separate function that is being called
from usbhid_open() with the "existancelock" being held.

Fixes: 0361a28d3f9a ("HID: autosuspend support for USB HID")
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com>

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index ce342fd0457e9e81f11f631a0b54af3691896a5a..bccd97cdc53f9fc5bb8052a978b73f7035717e1e 100644 (file)
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -254,12 +254,51 @@ static int hiddev_release(struct inode * inode, struct file * file)
        return 0;
 }
 
+static int __hiddev_open(struct hiddev *hiddev, struct file *file)
+{
+       struct hiddev_list *list;
+       int error;
+
+       lockdep_assert_held(&hiddev->existancelock);
+
+       list = vzalloc(sizeof(*list));
+       if (!list)
+               return -ENOMEM;
+
+       mutex_init(&list->thread_lock);
+       list->hiddev = hiddev;
+
+       if (!hiddev->open++) {
+               error = hid_hw_power(hiddev->hid, PM_HINT_FULLON);
+               if (error < 0)
+                       goto err_drop_count;
+
+               error = hid_hw_open(hiddev->hid);
+               if (error < 0)
+                       goto err_normal_power;
+       }
+
+       spin_lock_irq(&hiddev->list_lock);
+       list_add_tail(&list->node, &hiddev->list);
+       spin_unlock_irq(&hiddev->list_lock);
+
+       file->private_data = list;
+
+       return 0;
+
+err_normal_power:
+       hid_hw_power(hiddev->hid, PM_HINT_NORMAL);
+err_drop_count:
+       hiddev->open--;
+       vfree(list);
+       return error;
+}
+
 /*
  * open file op
  */
 static int hiddev_open(struct inode *inode, struct file *file)
 {
-       struct hiddev_list *list;
        struct usb_interface *intf;
        struct hid_device *hid;
        struct hiddev *hiddev;
@@ -268,66 +307,14 @@ static int hiddev_open(struct inode *inode, struct file *file)
        intf = usbhid_find_interface(iminor(inode));
        if (!intf)
                return -ENODEV;
+
        hid = usb_get_intfdata(intf);
        hiddev = hid->hiddev;
 
-       if (!(list = vzalloc(sizeof(struct hiddev_list))))
-               return -ENOMEM;
-       mutex_init(&list->thread_lock);
-       list->hiddev = hiddev;
-       file->private_data = list;
-
-       /*
-        * no need for locking because the USB major number
-        * is shared which usbcore guards against disconnect
-        */
-       if (list->hiddev->exist) {
-               if (!list->hiddev->open++) {
-                       res = hid_hw_open(hiddev->hid);
-                       if (res < 0)
-                               goto bail;
-               }
-       } else {
-               res = -ENODEV;
-               goto bail;
-       }
-
-       spin_lock_irq(&list->hiddev->list_lock);
-       list_add_tail(&list->node, &hiddev->list);
-       spin_unlock_irq(&list->hiddev->list_lock);
-
        mutex_lock(&hiddev->existancelock);
-       /*
-        * recheck exist with existance lock held to
-        * avoid opening a disconnected device
-        */
-       if (!list->hiddev->exist) {
-               res = -ENODEV;
-               goto bail_unlock;
-       }
-       if (!list->hiddev->open++)
-               if (list->hiddev->exist) {
-                       struct hid_device *hid = hiddev->hid;
-                       res = hid_hw_power(hid, PM_HINT_FULLON);
-                       if (res < 0)
-                               goto bail_unlock;
-                       res = hid_hw_open(hid);
-                       if (res < 0)
-                               goto bail_normal_power;
-               }
-       mutex_unlock(&hiddev->existancelock);
-       return 0;
-bail_normal_power:
-       hid_hw_power(hid, PM_HINT_NORMAL);
-bail_unlock:
+       res = hiddev->exist ? __hiddev_open(hiddev, file) : -ENODEV;
        mutex_unlock(&hiddev->existancelock);
 
-       spin_lock_irq(&list->hiddev->list_lock);
-       list_del(&list->node);
-       spin_unlock_irq(&list->hiddev->list_lock);
-bail:
-       file->private_data = NULL;
-       vfree(list);
        return res;
 }