* [PATCH 0/7] HID: Consolidate serializing ope/close in transport drivers
@ 2017-05-31 20:59 Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 1/7] HID: hiddev: use hid_hw_open/close instead of usbhid_open/close Dmitry Torokhov
` (6 more replies)
0 siblings, 7 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2017-05-31 20:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
This originally came about as report of uhid sending duplicate open and
premature close when hidraw was used alongside of input. After looking at
the drivers I think we shoud consolidate user tracking inside of the HID
core. While implementing this, there were a few cleanups as well.
Dmitry Torokhov (7):
HID: hiddev: use hid_hw_open/close instead of usbhid_open/close
HID: hiddev: use hid_hw_power instead of usbhid_get/put_power
HID: usbhid: do not rely on hid->open when deciding to do IO
HID: serialize hid_hw_open and hid_hw_close
HID: i2c-hid: remove custom locking from i2c_hid_open/close
HID: usbhid: remove custom locking from i2c_hid_open/close
HID: remove no longer used hid->open field
drivers/hid/hid-core.c | 89 +++++++++++++++++++++++++
drivers/hid/i2c-hid/i2c-hid.c | 32 +++------
drivers/hid/usbhid/hid-core.c | 150 ++++++++++++++++++++----------------------
drivers/hid/usbhid/hiddev.c | 12 ++--
drivers/hid/usbhid/usbhid.h | 15 +++--
include/linux/hid.h | 73 +++-----------------
6 files changed, 194 insertions(+), 177 deletions(-)
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/7] HID: hiddev: use hid_hw_open/close instead of usbhid_open/close
2017-05-31 20:59 [PATCH 0/7] HID: Consolidate serializing ope/close in transport drivers Dmitry Torokhov
@ 2017-05-31 20:59 ` Dmitry Torokhov
2017-06-01 2:22 ` kbuild test robot
2017-05-31 20:59 ` [PATCH 2/7] HID: hiddev: use hid_hw_power instead of usbhid_get/put_power Dmitry Torokhov
` (5 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2017-05-31 20:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
Instead of calling into usbhid code directly, let's use the standard
accessors for the transport HID drivers.
This also allows us make usbhid_open and close static.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 4 ++--
drivers/hid/usbhid/hiddev.c | 8 ++++----
drivers/hid/usbhid/usbhid.h | 2 --
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 83772fa7d92a..fb0cf5d70504 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -677,7 +677,7 @@ static int hid_get_class_descriptor(struct usb_device *dev, int ifnum,
return result;
}
-int usbhid_open(struct hid_device *hid)
+static int usbhid_open(struct hid_device *hid)
{
struct usbhid_device *usbhid = hid->driver_data;
int res = 0;
@@ -722,7 +722,7 @@ int usbhid_open(struct hid_device *hid)
return res;
}
-void usbhid_close(struct hid_device *hid)
+static void usbhid_close(struct hid_device *hid)
{
struct usbhid_device *usbhid = hid->driver_data;
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 0e06368d1fbb..d949ac13af9e 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -237,7 +237,7 @@ static int hiddev_release(struct inode * inode, struct file * file)
mutex_lock(&list->hiddev->existancelock);
if (!--list->hiddev->open) {
if (list->hiddev->exist) {
- usbhid_close(list->hiddev->hid);
+ hid_hw_close(list->hiddev->hid);
usbhid_put_power(list->hiddev->hid);
} else {
mutex_unlock(&list->hiddev->existancelock);
@@ -282,7 +282,7 @@ static int hiddev_open(struct inode *inode, struct file *file)
*/
if (list->hiddev->exist) {
if (!list->hiddev->open++) {
- res = usbhid_open(hiddev->hid);
+ res = hid_hw_open(hiddev->hid);
if (res < 0) {
res = -EIO;
goto bail;
@@ -306,7 +306,7 @@ static int hiddev_open(struct inode *inode, struct file *file)
res = -EIO;
goto bail_unlock;
}
- usbhid_open(hid);
+ hid_hw_open(hid);
}
mutex_unlock(&hiddev->existancelock);
return 0;
@@ -935,7 +935,7 @@ void hiddev_disconnect(struct hid_device *hid)
if (hiddev->open) {
mutex_unlock(&hiddev->existancelock);
- usbhid_close(hiddev->hid);
+ hid_hw_close(hiddev->hid);
wake_up_interruptible(&hiddev->wait);
} else {
mutex_unlock(&hiddev->existancelock);
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index fa47d666cfcf..83ef5c14aa92 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -34,8 +34,6 @@
#include <linux/input.h>
/* API provided by hid-core.c for USB HID drivers */
-void usbhid_close(struct hid_device *hid);
-int usbhid_open(struct hid_device *hid);
void usbhid_init_reports(struct hid_device *hid);
int usbhid_get_power(struct hid_device *hid);
void usbhid_put_power(struct hid_device *hid);
--
2.13.0.506.g27d5fe0cd-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/7] HID: hiddev: use hid_hw_power instead of usbhid_get/put_power
2017-05-31 20:59 [PATCH 0/7] HID: Consolidate serializing ope/close in transport drivers Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 1/7] HID: hiddev: use hid_hw_open/close instead of usbhid_open/close Dmitry Torokhov
@ 2017-05-31 20:59 ` Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 3/7] HID: usbhid: do not rely on hid->open when deciding to do IO Dmitry Torokhov
` (4 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2017-05-31 20:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
Instead of calling into usbhid code directly, let's use the standard
accessors for the transport HID drivers.
This also allows us to remove usbhid_get/put_power(), leaving only
usbhid_power().
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 22 +++++-----------------
drivers/hid/usbhid/hiddev.c | 4 ++--
drivers/hid/usbhid/usbhid.h | 2 --
3 files changed, 7 insertions(+), 21 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index fb0cf5d70504..62b660622265 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1203,16 +1203,19 @@ static void usbhid_stop(struct hid_device *hid)
static int usbhid_power(struct hid_device *hid, int lvl)
{
+ struct usbhid_device *usbhid = hid->driver_data;
int r = 0;
switch (lvl) {
case PM_HINT_FULLON:
- r = usbhid_get_power(hid);
+ r = usb_autopm_get_interface(usbhid->intf);
break;
+
case PM_HINT_NORMAL:
- usbhid_put_power(hid);
+ usb_autopm_put_interface(usbhid->intf);
break;
}
+
return r;
}
@@ -1492,21 +1495,6 @@ static int hid_post_reset(struct usb_interface *intf)
return 0;
}
-int usbhid_get_power(struct hid_device *hid)
-{
- struct usbhid_device *usbhid = hid->driver_data;
-
- return usb_autopm_get_interface(usbhid->intf);
-}
-
-void usbhid_put_power(struct hid_device *hid)
-{
- struct usbhid_device *usbhid = hid->driver_data;
-
- usb_autopm_put_interface(usbhid->intf);
-}
-
-
#ifdef CONFIG_PM
static int hid_resume_common(struct hid_device *hid, bool driver_suspended)
{
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index d949ac13af9e..8b62174582b5 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -238,7 +238,7 @@ static int hiddev_release(struct inode * inode, struct file * file)
if (!--list->hiddev->open) {
if (list->hiddev->exist) {
hid_hw_close(list->hiddev->hid);
- usbhid_put_power(list->hiddev->hid);
+ hid_hw_power(list->hiddev->hid, PM_HINT_NORMAL);
} else {
mutex_unlock(&list->hiddev->existancelock);
kfree(list->hiddev);
@@ -301,7 +301,7 @@ static int hiddev_open(struct inode *inode, struct file *file)
if (!list->hiddev->open++)
if (list->hiddev->exist) {
struct hid_device *hid = hiddev->hid;
- res = usbhid_get_power(hid);
+ res = hid_hw_power(hid, PM_HINT_FULLON);
if (res < 0) {
res = -EIO;
goto bail_unlock;
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 83ef5c14aa92..ffcd329b3c3b 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -35,8 +35,6 @@
/* API provided by hid-core.c for USB HID drivers */
void usbhid_init_reports(struct hid_device *hid);
-int usbhid_get_power(struct hid_device *hid);
-void usbhid_put_power(struct hid_device *hid);
struct usb_interface *usbhid_find_interface(int minor);
/* iofl flags */
--
2.13.0.506.g27d5fe0cd-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/7] HID: usbhid: do not rely on hid->open when deciding to do IO
2017-05-31 20:59 [PATCH 0/7] HID: Consolidate serializing ope/close in transport drivers Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 1/7] HID: hiddev: use hid_hw_open/close instead of usbhid_open/close Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 2/7] HID: hiddev: use hid_hw_power instead of usbhid_get/put_power Dmitry Torokhov
@ 2017-05-31 20:59 ` Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 4/7] HID: serialize hid_hw_open and hid_hw_close Dmitry Torokhov
` (3 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2017-05-31 20:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
Instead of checking hid->open (that we plan on having HID core manage) in
hid_start_in(), let's allocate a couple of new flags: HID_IN_POLLING and
HID_OPENED, and use them to decide whether we should be submitting URBs or
not.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 25 ++++++++++++++++++-------
drivers/hid/usbhid/usbhid.h | 11 +++++++++++
2 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 62b660622265..d927fe4ba592 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -85,10 +85,10 @@ static int hid_start_in(struct hid_device *hid)
struct usbhid_device *usbhid = hid->driver_data;
spin_lock_irqsave(&usbhid->lock, flags);
- if ((hid->open > 0 || hid->quirks & HID_QUIRK_ALWAYS_POLL) &&
- !test_bit(HID_DISCONNECTED, &usbhid->iofl) &&
- !test_bit(HID_SUSPENDED, &usbhid->iofl) &&
- !test_and_set_bit(HID_IN_RUNNING, &usbhid->iofl)) {
+ if (test_bit(HID_IN_POLLING, &usbhid->iofl) &&
+ !test_bit(HID_DISCONNECTED, &usbhid->iofl) &&
+ !test_bit(HID_SUSPENDED, &usbhid->iofl) &&
+ !test_and_set_bit(HID_IN_RUNNING, &usbhid->iofl)) {
rc = usb_submit_urb(usbhid->urbin, GFP_ATOMIC);
if (rc != 0) {
clear_bit(HID_IN_RUNNING, &usbhid->iofl);
@@ -272,13 +272,13 @@ static int usbhid_restart_ctrl_queue(struct usbhid_device *usbhid)
static void hid_irq_in(struct urb *urb)
{
struct hid_device *hid = urb->context;
- struct usbhid_device *usbhid = hid->driver_data;
+ struct usbhid_device *usbhid = hid->driver_data;
int status;
switch (urb->status) {
case 0: /* success */
usbhid->retry_delay = 0;
- if ((hid->quirks & HID_QUIRK_ALWAYS_POLL) && !hid->open)
+ if (!test_bit(HID_OPENED, &usbhid->iofl))
break;
usbhid_mark_busy(usbhid);
if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) {
@@ -692,6 +692,8 @@ static int usbhid_open(struct hid_device *hid)
goto done;
}
usbhid->intf->needs_remote_wakeup = 1;
+ set_bit(HID_OPENED, &usbhid->iofl);
+ set_bit(HID_IN_POLLING, &usbhid->iofl);
set_bit(HID_RESUME_RUNNING, &usbhid->iofl);
res = hid_start_in(hid);
if (res) {
@@ -701,6 +703,9 @@ static int usbhid_open(struct hid_device *hid)
} else {
/* no use opening if resources are insufficient */
hid->open--;
+ clear_bit(HID_OPENED, &usbhid->iofl);
+ if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL))
+ clear_bit(HID_IN_POLLING, &usbhid->iofl);
res = -EBUSY;
usbhid->intf->needs_remote_wakeup = 0;
}
@@ -734,6 +739,9 @@ static void usbhid_close(struct hid_device *hid)
*/
spin_lock_irq(&usbhid->lock);
if (!--hid->open) {
+ if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL))
+ clear_bit(HID_IN_POLLING, &usbhid->iofl);
+ clear_bit(HID_OPENED, &usbhid->iofl);
spin_unlock_irq(&usbhid->lock);
hid_cancel_delayed_stuff(usbhid);
if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL)) {
@@ -1135,6 +1143,7 @@ static int usbhid_start(struct hid_device *hid)
ret = usb_autopm_get_interface(usbhid->intf);
if (ret)
goto fail;
+ set_bit(HID_IN_POLLING, &usbhid->iofl);
usbhid->intf->needs_remote_wakeup = 1;
ret = hid_start_in(hid);
if (ret) {
@@ -1176,8 +1185,10 @@ static void usbhid_stop(struct hid_device *hid)
if (WARN_ON(!usbhid))
return;
- if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
+ if (hid->quirks & HID_QUIRK_ALWAYS_POLL) {
+ clear_bit(HID_IN_POLLING, &usbhid->iofl);
usbhid->intf->needs_remote_wakeup = 0;
+ }
clear_bit(HID_STARTED, &usbhid->iofl);
spin_lock_irq(&usbhid->lock); /* Sync with error and led handlers */
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index ffcd329b3c3b..da9c61d54be6 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -49,6 +49,17 @@ struct usb_interface *usbhid_find_interface(int minor);
#define HID_KEYS_PRESSED 10
#define HID_NO_BANDWIDTH 11
#define HID_RESUME_RUNNING 12
+/*
+ * The device is opened, meaning there is a client that is interested
+ * in data coming from the device.
+ */
+#define HID_OPENED 13
+/*
+ * We are polling input endpoint by [re]submitting IN URB, because
+ * either HID device is opened or ALWAYS POLL quirk is set for the
+ * device.
+ */
+#define HID_IN_POLLING 14
/*
* USB-specific HID struct, to be pointed to
--
2.13.0.506.g27d5fe0cd-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/7] HID: serialize hid_hw_open and hid_hw_close
2017-05-31 20:59 [PATCH 0/7] HID: Consolidate serializing ope/close in transport drivers Dmitry Torokhov
` (2 preceding siblings ...)
2017-05-31 20:59 ` [PATCH 3/7] HID: usbhid: do not rely on hid->open when deciding to do IO Dmitry Torokhov
@ 2017-05-31 20:59 ` Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 5/7] HID: i2c-hid: remove custom locking from i2c_hid_open/close Dmitry Torokhov
` (2 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2017-05-31 20:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
The HID transport drivers either re-implement exactly the same logic
(usbhid, i2c-hid) or forget to implement it (usbhid) which causes issues
when the same device is accessed via multiple interfaces (for example input
device through evdev and also hidraw). Let's muve the locking logic into
HID core to make sure the serialized behavior is always enforced.
Also let's uninline and move hid_hw_start() and hid_hw_stop() into hid-core
as hid_hw_start() is somewhat large and do not believe we get any benefit
from these two being inline.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/hid/hid-core.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/hid.h | 72 +++++-----------------------------------
2 files changed, 98 insertions(+), 63 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 49249d08b041..aa8be802fb18 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1750,6 +1750,94 @@ void hid_disconnect(struct hid_device *hdev)
}
EXPORT_SYMBOL_GPL(hid_disconnect);
+/**
+ * hid_hw_start - start underlying HW
+ * @hdev: hid device
+ * @connect_mask: which outputs to connect, see HID_CONNECT_*
+ *
+ * Call this in probe function *after* hid_parse. This will setup HW
+ * buffers and start the device (if not defeirred to device open).
+ * hid_hw_stop must be called if this was successful.
+ */
+int hid_hw_start(struct hid_device *hdev, unsigned int connect_mask)
+{
+ int error;
+
+ error = hdev->ll_driver->start(hdev);
+ if (error)
+ return error;
+
+ if (connect_mask) {
+ error = hid_connect(hdev, connect_mask);
+ if (error) {
+ hdev->ll_driver->stop(hdev);
+ return error;
+ }
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(hid_hw_start);
+
+/**
+ * hid_hw_stop - stop underlying HW
+ * @hdev: hid device
+ *
+ * This is usually called from remove function or from probe when something
+ * failed and hid_hw_start was called already.
+ */
+void hid_hw_stop(struct hid_device *hdev)
+{
+ hid_disconnect(hdev);
+ hdev->ll_driver->stop(hdev);
+}
+EXPORT_SYMBOL_GPL(hid_hw_stop);
+
+/**
+ * hid_hw_open - signal underlying HW to start delivering events
+ * @hdev: hid device
+ *
+ * Tell underlying HW to start delivering events from the device.
+ * This function should be called sometime after successful call
+ * to hid_hiw_start().
+ */
+int hid_hw_open(struct hid_device *hdev)
+{
+ int ret;
+
+ ret = mutex_lock_killable(&hdev->ll_open_lock);
+ if (ret)
+ return ret;
+
+ if (!hdev->ll_open_count++) {
+ ret = hdev->ll_driver->open(hdev);
+ if (ret)
+ hdev->ll_open_count--;
+ }
+
+ mutex_unlock(&hdev->ll_open_lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(hid_hw_open);
+
+/**
+ * hid_hw_close - signal underlaying HW to stop delivering events
+ *
+ * @hdev: hid device
+ *
+ * This function indicates that we are not interested in the events
+ * from this device anymore. Delivery of events may or may not stop,
+ * depending on the number of users still outstanding.
+ */
+void hid_hw_close(struct hid_device *hdev)
+{
+ mutex_lock(&hdev->ll_open_lock);
+ if (!--hdev->ll_open_count)
+ hdev->ll_driver->close(hdev);
+ mutex_unlock(&hdev->ll_open_lock);
+}
+EXPORT_SYMBOL_GPL(hid_hw_close);
+
/*
* A list of devices for which there is a specialized driver on HID bus.
*
@@ -2745,6 +2833,7 @@ struct hid_device *hid_allocate_device(void)
spin_lock_init(&hdev->debug_list_lock);
sema_init(&hdev->driver_lock, 1);
sema_init(&hdev->driver_input_lock, 1);
+ mutex_init(&hdev->ll_open_lock);
return hdev;
}
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5be325d890d9..5501eb64dbc4 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -34,6 +34,7 @@
#include <linux/workqueue.h>
#include <linux/input.h>
#include <linux/semaphore.h>
+#include <linux/mutex.h>
#include <linux/power_supply.h>
#include <uapi/linux/hid.h>
@@ -520,7 +521,10 @@ struct hid_device { /* device report descriptor */
struct semaphore driver_input_lock; /* protects the current driver */
struct device dev; /* device */
struct hid_driver *driver;
+
struct hid_ll_driver *ll_driver;
+ struct mutex ll_open_lock;
+ unsigned int ll_open_count;
#ifdef CONFIG_HID_BATTERY_STRENGTH
/*
@@ -937,69 +941,11 @@ static inline int __must_check hid_parse(struct hid_device *hdev)
return hid_open_report(hdev);
}
-/**
- * hid_hw_start - start underlaying HW
- *
- * @hdev: hid device
- * @connect_mask: which outputs to connect, see HID_CONNECT_*
- *
- * Call this in probe function *after* hid_parse. This will setup HW buffers
- * and start the device (if not deffered to device open). hid_hw_stop must be
- * called if this was successful.
- */
-static inline int __must_check hid_hw_start(struct hid_device *hdev,
- unsigned int connect_mask)
-{
- int ret = hdev->ll_driver->start(hdev);
- if (ret || !connect_mask)
- return ret;
- ret = hid_connect(hdev, connect_mask);
- if (ret)
- hdev->ll_driver->stop(hdev);
- return ret;
-}
-
-/**
- * hid_hw_stop - stop underlaying HW
- *
- * @hdev: hid device
- *
- * This is usually called from remove function or from probe when something
- * failed and hid_hw_start was called already.
- */
-static inline void hid_hw_stop(struct hid_device *hdev)
-{
- hid_disconnect(hdev);
- hdev->ll_driver->stop(hdev);
-}
-
-/**
- * hid_hw_open - signal underlaying HW to start delivering events
- *
- * @hdev: hid device
- *
- * Tell underlying HW to start delivering events from the device.
- * This function should be called sometime after successful call
- * to hid_hiw_start().
- */
-static inline int __must_check hid_hw_open(struct hid_device *hdev)
-{
- return hdev->ll_driver->open(hdev);
-}
-
-/**
- * hid_hw_close - signal underlaying HW to stop delivering events
- *
- * @hdev: hid device
- *
- * This function indicates that we are not interested in the events
- * from this device anymore. Delivery of events may or may not stop,
- * depending on the number of users still outstanding.
- */
-static inline void hid_hw_close(struct hid_device *hdev)
-{
- hdev->ll_driver->close(hdev);
-}
+int __must_check hid_hw_start(struct hid_device *hdev,
+ unsigned int connect_mask);
+void hid_hw_stop(struct hid_device *hdev);
+int __must_check hid_hw_open(struct hid_device *hdev);
+void hid_hw_close(struct hid_device *hdev);
/**
* hid_hw_power - requests underlying HW to go into given power mode
--
2.13.0.506.g27d5fe0cd-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/7] HID: i2c-hid: remove custom locking from i2c_hid_open/close
2017-05-31 20:59 [PATCH 0/7] HID: Consolidate serializing ope/close in transport drivers Dmitry Torokhov
` (3 preceding siblings ...)
2017-05-31 20:59 ` [PATCH 4/7] HID: serialize hid_hw_open and hid_hw_close Dmitry Torokhov
@ 2017-05-31 20:59 ` Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 6/7] HID: usbhid: " Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 7/7] HID: remove no longer used hid->open field Dmitry Torokhov
6 siblings, 0 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2017-05-31 20:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
Now that HID core enforces serialization of transport driver open/close
calls we can remove custom locking from i2c-hid driver.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/hid/i2c-hid/i2c-hid.c | 32 +++++++++-----------------------
1 file changed, 9 insertions(+), 23 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 8daa8ce64ebb..735669c9777e 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -743,18 +743,12 @@ static int i2c_hid_open(struct hid_device *hid)
struct i2c_hid *ihid = i2c_get_clientdata(client);
int ret = 0;
- mutex_lock(&i2c_hid_open_mut);
- if (!hid->open++) {
- ret = pm_runtime_get_sync(&client->dev);
- if (ret < 0) {
- hid->open--;
- goto done;
- }
- set_bit(I2C_HID_STARTED, &ihid->flags);
- }
-done:
- mutex_unlock(&i2c_hid_open_mut);
- return ret < 0 ? ret : 0;
+ ret = pm_runtime_get_sync(&client->dev);
+ if (ret < 0)
+ return ret;
+
+ set_bit(I2C_HID_STARTED, &ihid->flags);
+ return 0;
}
static void i2c_hid_close(struct hid_device *hid)
@@ -762,18 +756,10 @@ static void i2c_hid_close(struct hid_device *hid)
struct i2c_client *client = hid->driver_data;
struct i2c_hid *ihid = i2c_get_clientdata(client);
- /* protecting hid->open to make sure we don't restart
- * data acquistion due to a resumption we no longer
- * care about
- */
- mutex_lock(&i2c_hid_open_mut);
- if (!--hid->open) {
- clear_bit(I2C_HID_STARTED, &ihid->flags);
+ clear_bit(I2C_HID_STARTED, &ihid->flags);
- /* Save some power */
- pm_runtime_put(&client->dev);
- }
- mutex_unlock(&i2c_hid_open_mut);
+ /* Save some power */
+ pm_runtime_put(&client->dev);
}
static int i2c_hid_power(struct hid_device *hid, int lvl)
--
2.13.0.506.g27d5fe0cd-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 6/7] HID: usbhid: remove custom locking from i2c_hid_open/close
2017-05-31 20:59 [PATCH 0/7] HID: Consolidate serializing ope/close in transport drivers Dmitry Torokhov
` (4 preceding siblings ...)
2017-05-31 20:59 ` [PATCH 5/7] HID: i2c-hid: remove custom locking from i2c_hid_open/close Dmitry Torokhov
@ 2017-05-31 20:59 ` Dmitry Torokhov
2017-06-01 13:09 ` Benjamin Tissoires
2017-05-31 20:59 ` [PATCH 7/7] HID: remove no longer used hid->open field Dmitry Torokhov
6 siblings, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2017-05-31 20:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
Now that HID core enforces serialization of transport driver open/close
calls we can remove custom locking from usbhid driver.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 115 +++++++++++++++++++-----------------------
1 file changed, 53 insertions(+), 62 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index d927fe4ba592..76013eb5cb7f 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -70,8 +70,6 @@ MODULE_PARM_DESC(quirks, "Add/modify USB HID quirks by specifying "
/*
* Input submission and I/O error handler.
*/
-static DEFINE_MUTEX(hid_open_mut);
-
static void hid_io_error(struct hid_device *hid);
static int hid_submit_out(struct hid_device *hid);
static int hid_submit_ctrl(struct hid_device *hid);
@@ -680,50 +678,48 @@ static int hid_get_class_descriptor(struct usb_device *dev, int ifnum,
static int usbhid_open(struct hid_device *hid)
{
struct usbhid_device *usbhid = hid->driver_data;
- int res = 0;
-
- mutex_lock(&hid_open_mut);
- if (!hid->open++) {
- res = usb_autopm_get_interface(usbhid->intf);
- /* the device must be awake to reliably request remote wakeup */
- if (res < 0) {
- hid->open--;
- res = -EIO;
- goto done;
- }
- usbhid->intf->needs_remote_wakeup = 1;
- set_bit(HID_OPENED, &usbhid->iofl);
- set_bit(HID_IN_POLLING, &usbhid->iofl);
- set_bit(HID_RESUME_RUNNING, &usbhid->iofl);
- res = hid_start_in(hid);
- if (res) {
- if (res != -ENOSPC) {
- hid_io_error(hid);
- res = 0;
- } else {
- /* no use opening if resources are insufficient */
- hid->open--;
- clear_bit(HID_OPENED, &usbhid->iofl);
- if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL))
- clear_bit(HID_IN_POLLING, &usbhid->iofl);
- res = -EBUSY;
- usbhid->intf->needs_remote_wakeup = 0;
- }
- }
- usb_autopm_put_interface(usbhid->intf);
+ int res;
- /*
- * In case events are generated while nobody was listening,
- * some are released when the device is re-opened.
- * Wait 50 msec for the queue to empty before allowing events
- * to go through hid.
- */
- if (res == 0 && !(hid->quirks & HID_QUIRK_ALWAYS_POLL))
- msleep(50);
- clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
+ if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
+ return 0;
+
+ res = usb_autopm_get_interface(usbhid->intf);
+ /* the device must be awake to reliably request remote wakeup */
+ if (res < 0)
+ return -EIO;
+
+ usbhid->intf->needs_remote_wakeup = 1;
+
+ set_bit(HID_RESUME_RUNNING, &usbhid->iofl);
+ set_bit(HID_OPENED, &usbhid->iofl);
+ set_bit(HID_IN_POLLING, &usbhid->iofl);
+
+ res = hid_start_in(hid);
+ if (res) {
+ if (res != -ENOSPC) {
+ hid_io_error(hid);
+ res = 0;
+ } else {
+ /* no use opening if resources are insufficient */
+ res = -EBUSY;
+ clear_bit(HID_OPENED, &usbhid->iofl);
+ clear_bit(HID_IN_POLLING, &usbhid->iofl);
+ usbhid->intf->needs_remote_wakeup = 0;
+ }
}
-done:
- mutex_unlock(&hid_open_mut);
+
+ usb_autopm_put_interface(usbhid->intf);
+
+ /*
+ * In case events are generated while nobody was listening,
+ * some are released when the device is re-opened.
+ * Wait 50 msec for the queue to empty before allowing events
+ * to go through hid.
+ */
+ if (res == 0)
+ msleep(50);
+
+ clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
return res;
}
@@ -731,27 +727,22 @@ static void usbhid_close(struct hid_device *hid)
{
struct usbhid_device *usbhid = hid->driver_data;
- mutex_lock(&hid_open_mut);
+ if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
+ return;
- /* protecting hid->open to make sure we don't restart
- * data acquistion due to a resumption we no longer
- * care about
+ /*
+ * Make sure we don't restart data acquisition due to
+ * a resumption we no longer care about by avoiding racing
+ * with hid_start_in().
*/
spin_lock_irq(&usbhid->lock);
- if (!--hid->open) {
- if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL))
- clear_bit(HID_IN_POLLING, &usbhid->iofl);
- clear_bit(HID_OPENED, &usbhid->iofl);
- spin_unlock_irq(&usbhid->lock);
- hid_cancel_delayed_stuff(usbhid);
- if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL)) {
- usb_kill_urb(usbhid->urbin);
- usbhid->intf->needs_remote_wakeup = 0;
- }
- } else {
- spin_unlock_irq(&usbhid->lock);
- }
- mutex_unlock(&hid_open_mut);
+ clear_bit(HID_IN_POLLING, &usbhid->iofl);
+ clear_bit(HID_OPENED, &usbhid->iofl);
+ spin_unlock_irq(&usbhid->lock);
+
+ hid_cancel_delayed_stuff(usbhid);
+ usb_kill_urb(usbhid->urbin);
+ usbhid->intf->needs_remote_wakeup = 0;
}
/*
--
2.13.0.506.g27d5fe0cd-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 7/7] HID: remove no longer used hid->open field
2017-05-31 20:59 [PATCH 0/7] HID: Consolidate serializing ope/close in transport drivers Dmitry Torokhov
` (5 preceding siblings ...)
2017-05-31 20:59 ` [PATCH 6/7] HID: usbhid: " Dmitry Torokhov
@ 2017-05-31 20:59 ` Dmitry Torokhov
2017-06-01 3:08 ` kbuild test robot
2017-06-01 3:22 ` kbuild test robot
6 siblings, 2 replies; 12+ messages in thread
From: Dmitry Torokhov @ 2017-05-31 20:59 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
Now that all users have migrated to use hid->ll_open_count, we can remove
hid->open field.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
include/linux/hid.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5501eb64dbc4..72e8ac667771 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -548,7 +548,6 @@ struct hid_device { /* device report descriptor */
void *hiddev; /* The hiddev structure */
void *hidraw;
- int open; /* is the device open by anyone? */
char name[128]; /* Device name */
char phys[64]; /* Device physical location */
char uniq[64]; /* Device unique identifier (serial #) */
--
2.13.0.506.g27d5fe0cd-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/7] HID: hiddev: use hid_hw_open/close instead of usbhid_open/close
2017-05-31 20:59 ` [PATCH 1/7] HID: hiddev: use hid_hw_open/close instead of usbhid_open/close Dmitry Torokhov
@ 2017-06-01 2:22 ` kbuild test robot
0 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2017-06-01 2:22 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: kbuild-all, Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]
Hi Dmitry,
[auto build test WARNING on hid/for-next]
[also build test WARNING on v4.12-rc3 next-20170531]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/HID-Consolidate-serializing-ope-close-in-transport-drivers/20170601-092350
base: https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
config: x86_64-randconfig-x016-201722 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
drivers/hid/usbhid/hiddev.c: In function 'hiddev_open':
>> drivers/hid/usbhid/hiddev.c:309:4: warning: ignoring return value of 'hid_hw_open', declared with attribute warn_unused_result [-Wunused-result]
hid_hw_open(hid);
^~~~~~~~~~~~~~~~
vim +/hid_hw_open +309 drivers/hid/usbhid/hiddev.c
293 goto bail;
294 }
295
296 spin_lock_irq(&list->hiddev->list_lock);
297 list_add_tail(&list->node, &hiddev->list);
298 spin_unlock_irq(&list->hiddev->list_lock);
299
300 mutex_lock(&hiddev->existancelock);
301 if (!list->hiddev->open++)
302 if (list->hiddev->exist) {
303 struct hid_device *hid = hiddev->hid;
304 res = usbhid_get_power(hid);
305 if (res < 0) {
306 res = -EIO;
307 goto bail_unlock;
308 }
> 309 hid_hw_open(hid);
310 }
311 mutex_unlock(&hiddev->existancelock);
312 return 0;
313 bail_unlock:
314 mutex_unlock(&hiddev->existancelock);
315 bail:
316 file->private_data = NULL;
317 vfree(list);
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24500 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 7/7] HID: remove no longer used hid->open field
2017-05-31 20:59 ` [PATCH 7/7] HID: remove no longer used hid->open field Dmitry Torokhov
@ 2017-06-01 3:08 ` kbuild test robot
2017-06-01 3:22 ` kbuild test robot
1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2017-06-01 3:08 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: kbuild-all, Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]
Hi Dmitry,
[auto build test ERROR on hid/for-next]
[also build test ERROR on v4.12-rc3 next-20170531]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/HID-Consolidate-serializing-ope-close-in-transport-drivers/20170601-092350
base: https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/staging//greybus/hid.c: In function 'gb_hid_open':
>> drivers/staging//greybus/hid.c:352:10: error: 'struct hid_device' has no member named 'open'
if (!hid->open++) {
^~
drivers/staging//greybus/hid.c:355:7: error: 'struct hid_device' has no member named 'open'
hid->open--;
^~
drivers/staging//greybus/hid.c: In function 'gb_hid_close':
drivers/staging//greybus/hid.c:374:12: error: 'struct hid_device' has no member named 'open'
if (!--hid->open) {
^~
vim +352 drivers/staging//greybus/hid.c
96eab779 Viresh Kumar 2015-03-16 346 static int gb_hid_open(struct hid_device *hid)
96eab779 Viresh Kumar 2015-03-16 347 {
96eab779 Viresh Kumar 2015-03-16 348 struct gb_hid *ghid = hid->driver_data;
96eab779 Viresh Kumar 2015-03-16 349 int ret = 0;
96eab779 Viresh Kumar 2015-03-16 350
96eab779 Viresh Kumar 2015-03-16 351 mutex_lock(&gb_hid_open_mutex);
96eab779 Viresh Kumar 2015-03-16 @352 if (!hid->open++) {
96eab779 Viresh Kumar 2015-03-16 353 ret = gb_hid_set_power(ghid, GB_HID_TYPE_PWR_ON);
96eab779 Viresh Kumar 2015-03-16 354 if (ret < 0)
96eab779 Viresh Kumar 2015-03-16 355 hid->open--;
:::::: The code at line 352 was first introduced by commit
:::::: 96eab779e1985fd0b39426d288d3af38e3bce50c greybus: hid: add HID class driver
:::::: TO: Viresh Kumar <viresh.kumar@linaro.org>
:::::: CC: Greg Kroah-Hartman <greg@kroah.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59136 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 7/7] HID: remove no longer used hid->open field
2017-05-31 20:59 ` [PATCH 7/7] HID: remove no longer used hid->open field Dmitry Torokhov
2017-06-01 3:08 ` kbuild test robot
@ 2017-06-01 3:22 ` kbuild test robot
1 sibling, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2017-06-01 3:22 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: kbuild-all, Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5557 bytes --]
Hi Dmitry,
[auto build test WARNING on hid/for-next]
[also build test WARNING on v4.12-rc3 next-20170531]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/HID-Consolidate-serializing-ope-close-in-transport-drivers/20170601-092350
base: https://git.kernel.org/pub/scm/linux/kernel/git/jikos/hid.git for-next
config: x86_64-randconfig-h0-06010835 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from arch/x86/include/asm/bitops.h:15:0,
from include/linux/bitops.h:36,
from drivers/staging/greybus/hid.c:10:
drivers/staging/greybus/hid.c: In function 'gb_hid_open':
drivers/staging/greybus/hid.c:352:10: error: 'struct hid_device' has no member named 'open'
if (!hid->open++) {
^
include/linux/compiler.h:160:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
>> drivers/staging/greybus/hid.c:352:2: note: in expansion of macro 'if'
if (!hid->open++) {
^
drivers/staging/greybus/hid.c:352:10: error: 'struct hid_device' has no member named 'open'
if (!hid->open++) {
^
include/linux/compiler.h:160:42: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
>> drivers/staging/greybus/hid.c:352:2: note: in expansion of macro 'if'
if (!hid->open++) {
^
drivers/staging/greybus/hid.c:352:10: error: 'struct hid_device' has no member named 'open'
if (!hid->open++) {
^
include/linux/compiler.h:171:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^
>> drivers/staging/greybus/hid.c:352:2: note: in expansion of macro 'if'
if (!hid->open++) {
^
drivers/staging/greybus/hid.c:355:7: error: 'struct hid_device' has no member named 'open'
hid->open--;
^
In file included from arch/x86/include/asm/bitops.h:15:0,
from include/linux/bitops.h:36,
from drivers/staging/greybus/hid.c:10:
drivers/staging/greybus/hid.c: In function 'gb_hid_close':
drivers/staging/greybus/hid.c:374:12: error: 'struct hid_device' has no member named 'open'
if (!--hid->open) {
^
include/linux/compiler.h:160:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
drivers/staging/greybus/hid.c:374:2: note: in expansion of macro 'if'
if (!--hid->open) {
^
drivers/staging/greybus/hid.c:374:12: error: 'struct hid_device' has no member named 'open'
if (!--hid->open) {
^
include/linux/compiler.h:160:42: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
drivers/staging/greybus/hid.c:374:2: note: in expansion of macro 'if'
if (!--hid->open) {
^
drivers/staging/greybus/hid.c:374:12: error: 'struct hid_device' has no member named 'open'
if (!--hid->open) {
^
include/linux/compiler.h:171:16: note: in definition of macro '__trace_if'
______r = !!(cond); \
^
drivers/staging/greybus/hid.c:374:2: note: in expansion of macro 'if'
if (!--hid->open) {
^
vim +/if +352 drivers/staging/greybus/hid.c
96eab779 Viresh Kumar 2015-03-16 336 return 0;
96eab779 Viresh Kumar 2015-03-16 337 }
96eab779 Viresh Kumar 2015-03-16 338
96eab779 Viresh Kumar 2015-03-16 339 static void gb_hid_stop(struct hid_device *hid)
96eab779 Viresh Kumar 2015-03-16 340 {
96eab779 Viresh Kumar 2015-03-16 341 struct gb_hid *ghid = hid->driver_data;
96eab779 Viresh Kumar 2015-03-16 342
96eab779 Viresh Kumar 2015-03-16 343 gb_hid_free_buffers(ghid);
96eab779 Viresh Kumar 2015-03-16 344 }
96eab779 Viresh Kumar 2015-03-16 345
96eab779 Viresh Kumar 2015-03-16 346 static int gb_hid_open(struct hid_device *hid)
96eab779 Viresh Kumar 2015-03-16 347 {
96eab779 Viresh Kumar 2015-03-16 348 struct gb_hid *ghid = hid->driver_data;
96eab779 Viresh Kumar 2015-03-16 349 int ret = 0;
96eab779 Viresh Kumar 2015-03-16 350
96eab779 Viresh Kumar 2015-03-16 351 mutex_lock(&gb_hid_open_mutex);
96eab779 Viresh Kumar 2015-03-16 @352 if (!hid->open++) {
96eab779 Viresh Kumar 2015-03-16 353 ret = gb_hid_set_power(ghid, GB_HID_TYPE_PWR_ON);
96eab779 Viresh Kumar 2015-03-16 354 if (ret < 0)
96eab779 Viresh Kumar 2015-03-16 355 hid->open--;
96eab779 Viresh Kumar 2015-03-16 356 else
96eab779 Viresh Kumar 2015-03-16 357 set_bit(GB_HID_STARTED, &ghid->flags);
96eab779 Viresh Kumar 2015-03-16 358 }
96eab779 Viresh Kumar 2015-03-16 359 mutex_unlock(&gb_hid_open_mutex);
96eab779 Viresh Kumar 2015-03-16 360
:::::: The code at line 352 was first introduced by commit
:::::: 96eab779e1985fd0b39426d288d3af38e3bce50c greybus: hid: add HID class driver
:::::: TO: Viresh Kumar <viresh.kumar@linaro.org>
:::::: CC: Greg Kroah-Hartman <greg@kroah.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27675 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 6/7] HID: usbhid: remove custom locking from i2c_hid_open/close
2017-05-31 20:59 ` [PATCH 6/7] HID: usbhid: " Dmitry Torokhov
@ 2017-06-01 13:09 ` Benjamin Tissoires
0 siblings, 0 replies; 12+ messages in thread
From: Benjamin Tissoires @ 2017-06-01 13:09 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Jiri Kosina, linux-input, linux-kernel
Hi Dmitry,
thanks for the series, it looks good at first sight. There is a nitpick
here, the subject should not be "i2c_hid_open/close" but
"usbhid_open/close" I guess.
Do you plan to send a v2 of the series to fix the greybus issues
reported by the kbuild bot or should we start reviewing carefully this
version?
Cheers,
Benjamin
On May 31 2017 or thereabouts, Dmitry Torokhov wrote:
> Now that HID core enforces serialization of transport driver open/close
> calls we can remove custom locking from usbhid driver.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/hid/usbhid/hid-core.c | 115 +++++++++++++++++++-----------------------
> 1 file changed, 53 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index d927fe4ba592..76013eb5cb7f 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -70,8 +70,6 @@ MODULE_PARM_DESC(quirks, "Add/modify USB HID quirks by specifying "
> /*
> * Input submission and I/O error handler.
> */
> -static DEFINE_MUTEX(hid_open_mut);
> -
> static void hid_io_error(struct hid_device *hid);
> static int hid_submit_out(struct hid_device *hid);
> static int hid_submit_ctrl(struct hid_device *hid);
> @@ -680,50 +678,48 @@ static int hid_get_class_descriptor(struct usb_device *dev, int ifnum,
> static int usbhid_open(struct hid_device *hid)
> {
> struct usbhid_device *usbhid = hid->driver_data;
> - int res = 0;
> -
> - mutex_lock(&hid_open_mut);
> - if (!hid->open++) {
> - res = usb_autopm_get_interface(usbhid->intf);
> - /* the device must be awake to reliably request remote wakeup */
> - if (res < 0) {
> - hid->open--;
> - res = -EIO;
> - goto done;
> - }
> - usbhid->intf->needs_remote_wakeup = 1;
> - set_bit(HID_OPENED, &usbhid->iofl);
> - set_bit(HID_IN_POLLING, &usbhid->iofl);
> - set_bit(HID_RESUME_RUNNING, &usbhid->iofl);
> - res = hid_start_in(hid);
> - if (res) {
> - if (res != -ENOSPC) {
> - hid_io_error(hid);
> - res = 0;
> - } else {
> - /* no use opening if resources are insufficient */
> - hid->open--;
> - clear_bit(HID_OPENED, &usbhid->iofl);
> - if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL))
> - clear_bit(HID_IN_POLLING, &usbhid->iofl);
> - res = -EBUSY;
> - usbhid->intf->needs_remote_wakeup = 0;
> - }
> - }
> - usb_autopm_put_interface(usbhid->intf);
> + int res;
>
> - /*
> - * In case events are generated while nobody was listening,
> - * some are released when the device is re-opened.
> - * Wait 50 msec for the queue to empty before allowing events
> - * to go through hid.
> - */
> - if (res == 0 && !(hid->quirks & HID_QUIRK_ALWAYS_POLL))
> - msleep(50);
> - clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
> + if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
> + return 0;
> +
> + res = usb_autopm_get_interface(usbhid->intf);
> + /* the device must be awake to reliably request remote wakeup */
> + if (res < 0)
> + return -EIO;
> +
> + usbhid->intf->needs_remote_wakeup = 1;
> +
> + set_bit(HID_RESUME_RUNNING, &usbhid->iofl);
> + set_bit(HID_OPENED, &usbhid->iofl);
> + set_bit(HID_IN_POLLING, &usbhid->iofl);
> +
> + res = hid_start_in(hid);
> + if (res) {
> + if (res != -ENOSPC) {
> + hid_io_error(hid);
> + res = 0;
> + } else {
> + /* no use opening if resources are insufficient */
> + res = -EBUSY;
> + clear_bit(HID_OPENED, &usbhid->iofl);
> + clear_bit(HID_IN_POLLING, &usbhid->iofl);
> + usbhid->intf->needs_remote_wakeup = 0;
> + }
> }
> -done:
> - mutex_unlock(&hid_open_mut);
> +
> + usb_autopm_put_interface(usbhid->intf);
> +
> + /*
> + * In case events are generated while nobody was listening,
> + * some are released when the device is re-opened.
> + * Wait 50 msec for the queue to empty before allowing events
> + * to go through hid.
> + */
> + if (res == 0)
> + msleep(50);
> +
> + clear_bit(HID_RESUME_RUNNING, &usbhid->iofl);
> return res;
> }
>
> @@ -731,27 +727,22 @@ static void usbhid_close(struct hid_device *hid)
> {
> struct usbhid_device *usbhid = hid->driver_data;
>
> - mutex_lock(&hid_open_mut);
> + if (hid->quirks & HID_QUIRK_ALWAYS_POLL)
> + return;
>
> - /* protecting hid->open to make sure we don't restart
> - * data acquistion due to a resumption we no longer
> - * care about
> + /*
> + * Make sure we don't restart data acquisition due to
> + * a resumption we no longer care about by avoiding racing
> + * with hid_start_in().
> */
> spin_lock_irq(&usbhid->lock);
> - if (!--hid->open) {
> - if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL))
> - clear_bit(HID_IN_POLLING, &usbhid->iofl);
> - clear_bit(HID_OPENED, &usbhid->iofl);
> - spin_unlock_irq(&usbhid->lock);
> - hid_cancel_delayed_stuff(usbhid);
> - if (!(hid->quirks & HID_QUIRK_ALWAYS_POLL)) {
> - usb_kill_urb(usbhid->urbin);
> - usbhid->intf->needs_remote_wakeup = 0;
> - }
> - } else {
> - spin_unlock_irq(&usbhid->lock);
> - }
> - mutex_unlock(&hid_open_mut);
> + clear_bit(HID_IN_POLLING, &usbhid->iofl);
> + clear_bit(HID_OPENED, &usbhid->iofl);
> + spin_unlock_irq(&usbhid->lock);
> +
> + hid_cancel_delayed_stuff(usbhid);
> + usb_kill_urb(usbhid->urbin);
> + usbhid->intf->needs_remote_wakeup = 0;
> }
>
> /*
> --
> 2.13.0.506.g27d5fe0cd-goog
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-06-01 13:10 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-31 20:59 [PATCH 0/7] HID: Consolidate serializing ope/close in transport drivers Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 1/7] HID: hiddev: use hid_hw_open/close instead of usbhid_open/close Dmitry Torokhov
2017-06-01 2:22 ` kbuild test robot
2017-05-31 20:59 ` [PATCH 2/7] HID: hiddev: use hid_hw_power instead of usbhid_get/put_power Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 3/7] HID: usbhid: do not rely on hid->open when deciding to do IO Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 4/7] HID: serialize hid_hw_open and hid_hw_close Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 5/7] HID: i2c-hid: remove custom locking from i2c_hid_open/close Dmitry Torokhov
2017-05-31 20:59 ` [PATCH 6/7] HID: usbhid: " Dmitry Torokhov
2017-06-01 13:09 ` Benjamin Tissoires
2017-05-31 20:59 ` [PATCH 7/7] HID: remove no longer used hid->open field Dmitry Torokhov
2017-06-01 3:08 ` kbuild test robot
2017-06-01 3:22 ` kbuild test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).