HID: hiddev: fix mess in hiddev_open()
diff mbox series

Message ID 20191217225021.GA34258@dtor-ws
State Mainlined
Commit 18a1b06e5b91d47dc86c0a66a762646ea7c5d141
Delegated to: Benjamin Tissoires
Headers show
Series
  • HID: hiddev: fix mess in hiddev_open()
Related show

Commit Message

Dmitry Torokhov Dec. 17, 2019, 10:50 p.m. UTC
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>
---
 drivers/hid/usbhid/hiddev.c | 97 ++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 55 deletions(-)

Comments

Benjamin Tissoires Dec. 18, 2019, 3:53 p.m. UTC | #1
On Tue, Dec 17, 2019 at 11:50 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> 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>
> ---

Thanks!

Applied to for-5.5/upstream-fixes

Cheers,
Benjamin

>  drivers/hid/usbhid/hiddev.c | 97 ++++++++++++++++---------------------
>  1 file changed, 42 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 1f9bc4483465..c879b214a479 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -241,12 +241,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;
> @@ -255,66 +294,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;
>  }
>
> --
> 2.24.1.735.g03f4e72817-goog
>
>
> --
> Dmitry
>

Patch
diff mbox series

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 1f9bc4483465..c879b214a479 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -241,12 +241,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;
@@ -255,66 +294,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;
 }