On Tue, Dec 17, 2019 at 11:50 PM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx>
> ---
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
>
