Re: [PATCH] HID: hiddev: fix mess in hiddev_open()

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

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
>