| Message ID | 20191217225021.GA34258@dtor-ws (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 18a1b06e5b91d47dc86c0a66a762646ea7c5d141 |
| Delegated to: | Benjamin Tissoires |
| Headers | show |
| Series | HID: hiddev: fix mess in hiddev_open() | expand |
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 >
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; }
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(-)