Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1659438 > unrolled thread

[PATCH v2 0/8] HID: Consolidate serializing ope/close in transport drivers

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/8] HID: Consolidate serializing ope/close in transport drivers Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-06-07 09:10 +0200
    [PATCH v2 6/8] HID: usbhid: remove custom locking from usbhid_open/close Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-06-07 09:10 +0200
    [PATCH v2 4/8] HID: serialize hid_hw_open and hid_hw_close Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2017-06-07 09:10 +0200
    Re: [PATCH v2 0/8] HID: Consolidate serializing ope/close in  transport drivers Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-06-07 10:50 +0200
    Re: [PATCH v2 0/8] HID: Consolidate serializing ope/close in  transport drivers Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-06-07 15:50 +0200

#1659438 — [PATCH v2 0/8] HID: Consolidate serializing ope/close in transport drivers

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 should consolidate user tracking inside of the HID
core. While implementing this, there were a few cleanups as well.

V2:

- added greybus hid changes
- added error handling in hiddev around hid_hw_open which is __much_check

Dmitry Torokhov (8):
  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 usbhid_open/close
  greybus: hid: remove custom locking from gb_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   |  24 +++----
 drivers/hid/usbhid/usbhid.h   |  15 +++--
 drivers/staging/greybus/hid.c |  43 ++++--------
 include/linux/hid.h           |  73 +++-----------------
 7 files changed, 214 insertions(+), 212 deletions(-)

Thanks.

-- 
Dmitry

[toc] | [next] | [standalone]

#1659441 — [PATCH v2 6/8] HID: usbhid: remove custom locking from usbhid_open/close

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

[toc] | [prev] | [next] | [standalone]

#1659445 — [PATCH v2 4/8] HID: serialize hid_hw_open and hid_hw_close

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 ca173801d2be..f2a2cd522e46 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.
  *
@@ -2748,6 +2836,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

[toc] | [prev] | [next] | [standalone]

#1659544 — Re: [PATCH v2 0/8] HID: Consolidate serializing ope/close in transport drivers

On Wed, Jun 7, 2017 at 9:59 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> 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 should consolidate user tracking inside of the HID
> core. While implementing this, there were a few cleanups as well.
>
> V2:
>
> - added greybus hid changes
> - added error handling in hiddev around hid_hw_open which is __much_check

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> Dmitry Torokhov (8):
>   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 usbhid_open/close
>   greybus: hid: remove custom locking from gb_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   |  24 +++----
>  drivers/hid/usbhid/usbhid.h   |  15 +++--
>  drivers/staging/greybus/hid.c |  43 ++++--------
>  include/linux/hid.h           |  73 +++-----------------
>  7 files changed, 214 insertions(+), 212 deletions(-)
>
> Thanks.
>
> --
> Dmitry
>



-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1659813 — Re: [PATCH v2 0/8] HID: Consolidate serializing ope/close in transport drivers

On Jun 06 2017 or thereabouts, Dmitry Torokhov wrote:
> 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 should consolidate user tracking inside of the HID
> core. While implementing this, there were a few cleanups as well.
> 
> V2:
> 
> - added greybus hid changes
> - added error handling in hiddev around hid_hw_open which is __much_check
> 

Looks good to me:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

> Dmitry Torokhov (8):
>   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 usbhid_open/close
>   greybus: hid: remove custom locking from gb_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   |  24 +++----
>  drivers/hid/usbhid/usbhid.h   |  15 +++--
>  drivers/staging/greybus/hid.c |  43 ++++--------
>  include/linux/hid.h           |  73 +++-----------------
>  7 files changed, 214 insertions(+), 212 deletions(-)
> 
> Thanks.
> 
> -- 
> Dmitry
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web