All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
@ 2013-02-14  5:07 Andrew de los Reyes
  2013-02-18 13:08 ` Jiri Kosina
  0 siblings, 1 reply; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-14  5:07 UTC (permalink / raw)
  To: David Herrmann, Linux Input, Nestor Lopez Casado,
	Benjamin Tissoires, Jiri Kosina, Bruno Prémont

Here's the latest version of the patch. I believe it addresses all
outstanding comments.

Thanks!
-andrew

This patch separates struct hid_device's driver_lock into two. The
goal is to allow hid device drivers to receive input during their
probe() or remove() function calls. This is necessary because some
drivers need to communicate with the device to determine parameters
needed during probe (e.g., size of a multi-touch surface), and if
possible, may perfer to communicate with a device on host-initiated
disconnect (e.g., to put it into a low-power state).

Historically, three functions used driver_lock:

- hid_device_probe: blocks to acquire lock
- hid_device_remove: blocks to acquire lock
- hid_input_report: if locked returns -EBUSY, else acquires lock

This patch adds another lock (driver_input_lock) which is used to
block input from occurring. The lock behavior is now:

- hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
- hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
- hid_input_report: if driver_input_lock locked returns -EBUSY, else
  acquires driver_input_lock

This patch also adds two helper functions to be called during probe()
or remove(): hid_device_io_start() and hid_device_io_stop(). These
functions lock and unlock, respectively, driver_input_lock; they also
make a note of whether they did so that hid-core knows if a driver has
changed the lock state.

This patch results in no behavior change for existing devices and
drivers. However, during a probe() or remove() function call in a
driver, that driver may now selectively call hid_device_io_start() to
let input events come through, then optionally call
hid_device_io_stop() to stop them.

Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
---
 drivers/hid/hid-core.c | 24 +++++++++++++++++++++---
 include/linux/hid.h    | 38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 4da66b4..714d8b7 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int
type, u8 *data, int size, int i
 	if (!hid)
 		return -ENODEV;

-	if (down_trylock(&hid->driver_lock))
+	if (down_trylock(&hid->driver_input_lock))
 		return -EBUSY;

 	if (!hid->driver) {
@@ -1150,7 +1150,7 @@ nomem:
 	hid_report_raw_event(hid, type, data, size, interrupt);

 unlock:
-	up(&hid->driver_lock);
+	up(&hid->driver_input_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(hid_input_report);
@@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev)

 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		ret = -EINTR;
+		goto unlock_driver_lock;
+	}
+	hdev->io_started = false;

 	if (!hdev->driver) {
 		id = hid_match_device(hdev, hdrv);
@@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev)
 			hdev->driver = NULL;
 	}
 unlock:
+	if (!hdev->io_started)
+		up(&hdev->driver_input_lock);
+unlock_driver_lock:
 	up(&hdev->driver_lock);
 	return ret;
 }
@@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
 	struct hid_driver *hdrv;
+	int ret = 0;

 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		ret = -EINTR;
+		goto unlock_driver_lock;
+	}
+	hdev->io_started = false;

 	hdrv = hdev->driver;
 	if (hdrv) {
@@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device *dev)
 		hdev->driver = NULL;
 	}

+	if (!hdev->io_started)
+		up(&hdev->driver_input_lock);
+unlock_driver_lock:
 	up(&hdev->driver_lock);
-	return 0;
+	return ret;
 }

 static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
@@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void)
 	init_waitqueue_head(&hdev->debug_wait);
 	INIT_LIST_HEAD(&hdev->debug_list);
 	sema_init(&hdev->driver_lock, 1);
+	sema_init(&hdev->driver_input_lock, 1);

 	return hdev;
 err:
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 3a95da6..27282a1 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -481,7 +481,8 @@ struct hid_device {							/* device report descriptor */
 	unsigned country;						/* HID country */
 	struct hid_report_enum report_enum[HID_REPORT_TYPES];

-	struct semaphore driver_lock;					/* protects the current driver */
+	struct semaphore driver_lock;					/* protects the current driver,
except during input */
+	struct semaphore driver_input_lock;				/* protects the current driver */
 	struct device dev;						/* device */
 	struct hid_driver *driver;
 	struct hid_ll_driver *ll_driver;
@@ -502,6 +503,7 @@ struct hid_device {							/* device report descriptor */
 	unsigned int status;						/* see STAT flags above */
 	unsigned claimed;						/* Claimed by hidinput, hiddev? */
 	unsigned quirks;						/* Various quirks the device can pull on us */
+	bool io_started;						/* Protected by driver_lock. If IO has started */

 	struct list_head inputs;					/* The list of inputs */
 	void *hiddev;							/* The hiddev structure */
@@ -622,6 +624,10 @@ struct hid_usage_id {
  * @resume: invoked on resume if device was not reset (NULL means nop)
  * @reset_resume: invoked on resume if device was reset (NULL means nop)
  *
+ * probe should return -errno on error, or 0 on success. During probe,
+ * input will not be passed to raw_event unless hid_device_io_start is
+ * called.
+ *
  * raw_event and event should return 0 on no action performed, 1 when no
  * further processing should be done and negative on error
  *
@@ -742,6 +748,36 @@ const struct hid_device_id *hid_match_id(struct
hid_device *hdev,
 					 const struct hid_device_id *id);

 /**
+ * hid_device_io_start - enable HID input during probe, remove
+ *
+ * @hid - the device
+ *
+ * This should only be called during probe or remove and only be
+ * called by the thread calling probe or remove. It will allow
+ * incoming packets to be delivered to the driver.
+ */
+static inline void hid_device_io_start(struct hid_device *hid) {
+  hid->io_started = true;
+  up(&hid->driver_input_lock);
+}
+
+/**
+ * hid_device_io_stop - disable HID input during probe, remove
+ *
+ * @hid - the device
+ *
+ * Should only be called after hid_device_io_start. It will prevent
+ * incoming packets from going to the driver for the duration of
+ * probe, remove. If called during probe, packets will still go to the
+ * driver after probe is complete. This function should only be called
+ * by the thread calling probe or remove.
+ */
+static inline void hid_device_io_stop(struct hid_device *hid) {
+  hid->io_started = false;
+  down(&hid->driver_input_lock);
+}
+
+/**
  * hid_map_usage - map usage input bits
  *
  * @hidinput: hidinput which we are interested in
-- 
1.8.1

^ permalink raw reply related	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-14  5:07 [PATCH] HID: Separate struct hid_device's driver_lock into two locks Andrew de los Reyes
@ 2013-02-18 13:08 ` Jiri Kosina
  2013-02-18 17:20   ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Andrew de los Reyes
  2013-02-25 13:03   ` [PATCH] HID: Separate struct hid_device's driver_lock into two locks Jiri Kosina
  0 siblings, 2 replies; 33+ messages in thread
From: Jiri Kosina @ 2013-02-18 13:08 UTC (permalink / raw)
  To: Andrew de los Reyes
  Cc: David Herrmann, Linux Input, Nestor Lopez Casado,
	Benjamin Tissoires, Bruno Prémont

On Wed, 13 Feb 2013, Andrew de los Reyes wrote:

> Here's the latest version of the patch. I believe it addresses all
> outstanding comments.
> 
> Thanks!
> -andrew
> 
> This patch separates struct hid_device's driver_lock into two. The
> goal is to allow hid device drivers to receive input during their
> probe() or remove() function calls. This is necessary because some
> drivers need to communicate with the device to determine parameters
> needed during probe (e.g., size of a multi-touch surface), and if
> possible, may perfer to communicate with a device on host-initiated
> disconnect (e.g., to put it into a low-power state).
> 
> Historically, three functions used driver_lock:
> 
> - hid_device_probe: blocks to acquire lock
> - hid_device_remove: blocks to acquire lock
> - hid_input_report: if locked returns -EBUSY, else acquires lock
> 
> This patch adds another lock (driver_input_lock) which is used to
> block input from occurring. The lock behavior is now:
> 
> - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
> - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
> - hid_input_report: if driver_input_lock locked returns -EBUSY, else
>   acquires driver_input_lock
> 
> This patch also adds two helper functions to be called during probe()
> or remove(): hid_device_io_start() and hid_device_io_stop(). These
> functions lock and unlock, respectively, driver_input_lock; they also
> make a note of whether they did so that hid-core knows if a driver has
> changed the lock state.
> 
> This patch results in no behavior change for existing devices and
> drivers. However, during a probe() or remove() function call in a
> driver, that driver may now selectively call hid_device_io_start() to
> let input events come through, then optionally call
> hid_device_io_stop() to stop them.
> 
> Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85

Hi,

thanks for finally proceeding on this front.

Please provide your Signed-off-by line; without that, the patch can't be 
accepted.

> ---
>  drivers/hid/hid-core.c | 24 +++++++++++++++++++++---
>  include/linux/hid.h    | 38 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 4da66b4..714d8b7 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int
> type, u8 *data, int size, int i
>  	if (!hid)
>  		return -ENODEV;
> 
> -	if (down_trylock(&hid->driver_lock))
> +	if (down_trylock(&hid->driver_input_lock))
>  		return -EBUSY;
> 
>  	if (!hid->driver) {
> @@ -1150,7 +1150,7 @@ nomem:
>  	hid_report_raw_event(hid, type, data, size, interrupt);
> 
>  unlock:
> -	up(&hid->driver_lock);
> +	up(&hid->driver_input_lock);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(hid_input_report);
> @@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev)
> 
>  	if (down_interruptible(&hdev->driver_lock))
>  		return -EINTR;
> +	if (down_interruptible(&hdev->driver_input_lock)) {
> +		ret = -EINTR;
> +		goto unlock_driver_lock;
> +	}
> +	hdev->io_started = false;
> 
>  	if (!hdev->driver) {
>  		id = hid_match_device(hdev, hdrv);
> @@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev)
>  			hdev->driver = NULL;
>  	}
>  unlock:
> +	if (!hdev->io_started)
> +		up(&hdev->driver_input_lock);
> +unlock_driver_lock:
>  	up(&hdev->driver_lock);
>  	return ret;
>  }
> @@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
>  {
>  	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>  	struct hid_driver *hdrv;
> +	int ret = 0;
> 
>  	if (down_interruptible(&hdev->driver_lock))
>  		return -EINTR;
> +	if (down_interruptible(&hdev->driver_input_lock)) {
> +		ret = -EINTR;
> +		goto unlock_driver_lock;
> +	}
> +	hdev->io_started = false;
> 
>  	hdrv = hdev->driver;
>  	if (hdrv) {
> @@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device *dev)
>  		hdev->driver = NULL;
>  	}
> 
> +	if (!hdev->io_started)
> +		up(&hdev->driver_input_lock);
> +unlock_driver_lock:
>  	up(&hdev->driver_lock);
> -	return 0;
> +	return ret;
>  }
> 
>  static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
> @@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void)
>  	init_waitqueue_head(&hdev->debug_wait);
>  	INIT_LIST_HEAD(&hdev->debug_list);
>  	sema_init(&hdev->driver_lock, 1);
> +	sema_init(&hdev->driver_input_lock, 1);
> 
>  	return hdev;
>  err:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 3a95da6..27282a1 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -481,7 +481,8 @@ struct hid_device {							/* device report descriptor */
>  	unsigned country;						/* HID country */
>  	struct hid_report_enum report_enum[HID_REPORT_TYPES];
> 
> -	struct semaphore driver_lock;					/* protects the current driver */
> +	struct semaphore driver_lock;					/* protects the current driver,
> except during input */
> +	struct semaphore driver_input_lock;				/* protects the current driver */
>  	struct device dev;						/* device */
>  	struct hid_driver *driver;
>  	struct hid_ll_driver *ll_driver;
> @@ -502,6 +503,7 @@ struct hid_device {							/* device report descriptor */
>  	unsigned int status;						/* see STAT flags above */
>  	unsigned claimed;						/* Claimed by hidinput, hiddev? */
>  	unsigned quirks;						/* Various quirks the device can pull on us */
> +	bool io_started;						/* Protected by driver_lock. If IO has started */
> 
>  	struct list_head inputs;					/* The list of inputs */
>  	void *hiddev;							/* The hiddev structure */
> @@ -622,6 +624,10 @@ struct hid_usage_id {
>   * @resume: invoked on resume if device was not reset (NULL means nop)
>   * @reset_resume: invoked on resume if device was reset (NULL means nop)
>   *
> + * probe should return -errno on error, or 0 on success. During probe,
> + * input will not be passed to raw_event unless hid_device_io_start is
> + * called.
> + *
>   * raw_event and event should return 0 on no action performed, 1 when no
>   * further processing should be done and negative on error
>   *
> @@ -742,6 +748,36 @@ const struct hid_device_id *hid_match_id(struct
> hid_device *hdev,
>  					 const struct hid_device_id *id);
> 
>  /**
> + * hid_device_io_start - enable HID input during probe, remove
> + *
> + * @hid - the device
> + *
> + * This should only be called during probe or remove and only be
> + * called by the thread calling probe or remove. It will allow
> + * incoming packets to be delivered to the driver.
> + */
> +static inline void hid_device_io_start(struct hid_device *hid) {
> +  hid->io_started = true;
> +  up(&hid->driver_input_lock);
> +}
> +
> +/**
> + * hid_device_io_stop - disable HID input during probe, remove
> + *
> + * @hid - the device
> + *
> + * Should only be called after hid_device_io_start. It will prevent
> + * incoming packets from going to the driver for the duration of
> + * probe, remove. If called during probe, packets will still go to the
> + * driver after probe is complete. This function should only be called
> + * by the thread calling probe or remove.
> + */
> +static inline void hid_device_io_stop(struct hid_device *hid) {
> +  hid->io_started = false;
> +  down(&hid->driver_input_lock);
> +}

Is there any particular reason to have hid_device_io_start() and 
hid_device_io_stop() indentation not use tabs?

Also, the functions are currently unused, so I'd rather suggest adding 
them together when individual device driver(s) are converted to using it.

Thanks again,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe()
  2013-02-18 13:08 ` Jiri Kosina
@ 2013-02-18 17:20   ` Andrew de los Reyes
  2013-02-18 17:20     ` [PATCH 1/3] HID: Separate struct hid_device's driver_lock into two locks Andrew de los Reyes
                       ` (3 more replies)
  2013-02-25 13:03   ` [PATCH] HID: Separate struct hid_device's driver_lock into two locks Jiri Kosina
  1 sibling, 4 replies; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-18 17:20 UTC (permalink / raw)
  To: Linux Input; +Cc: Andrew de los Reyes

From: Andrew de los Reyes <adlr@chromium.org>

This patch series includes the latest version of the patch to separate
the HID driver lock into two. It also includes the first use of the
new API provided by that patch: a change to the Logitech Unifying
Receiver driver to detect attached peripherals during probe(). With
that in, we can also revert a previous workound, which was originally
introduced when probe() stopped being able to communicate with the
device.

Andrew de los Reyes (3):
  HID: Separate struct hid_device's driver_lock into two locks.
  HID: Fix logitech-dj: Allow incoming packets during probe().
  Revert "HID: Fix logitech-dj: missing Unifying device issue"

 drivers/hid/hid-core.c        | 24 +++++++++++++++++++---
 drivers/hid/hid-logitech-dj.c | 48 +++----------------------------------------
 drivers/hid/hid-logitech-dj.h |  1 -
 include/linux/hid.h           | 46 ++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 69 insertions(+), 50 deletions(-)

--
1.8.1.3


^ permalink raw reply	[flat|nested] 33+ messages in thread
* [PATCH 1/3] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-18 17:20   ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Andrew de los Reyes
@ 2013-02-18 17:20     ` Andrew de los Reyes
  2013-02-18 17:20     ` [PATCH 2/3] HID: Fix logitech-dj: Allow incoming packets during probe() Andrew de los Reyes
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-18 17:20 UTC (permalink / raw)
  To: Linux Input; +Cc: Andrew de los Reyes

From: Andrew de los Reyes <adlr@chromium.org>

This patch separates struct hid_device's driver_lock into two. The
goal is to allow hid device drivers to receive input during their
probe() or remove() function calls. This is necessary because some
drivers need to communicate with the device to determine parameters
needed during probe (e.g., size of a multi-touch surface), and if
possible, may perfer to communicate with a device on host-initiated
disconnect (e.g., to put it into a low-power state).

Historically, three functions used driver_lock:

- hid_device_probe: blocks to acquire lock
- hid_device_remove: blocks to acquire lock
- hid_input_report: if locked returns -EBUSY, else acquires lock

This patch adds another lock (driver_input_lock) which is used to
block input from occurring. The lock behavior is now:

- hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
- hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
- hid_input_report: if driver_input_lock locked returns -EBUSY, else
  acquires driver_input_lock

This patch also adds two helper functions to be called during probe()
or remove(): hid_device_io_start() and hid_device_io_stop(). These
functions lock and unlock, respectively, driver_input_lock; they also
make a note of whether they did so that hid-core knows if a driver has
changed the lock state.

This patch results in no behavior change for existing devices and
drivers. However, during a probe() or remove() function call in a
driver, that driver may now selectively call hid_device_io_start() to
let input events come through, then optionally call
hid_device_io_stop() to stop them.

Signed-off-by: Andrew de los Reyes <adlr@chromium.org>

Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
---
 drivers/hid/hid-core.c | 24 +++++++++++++++++++++---
 include/linux/hid.h    | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 4da66b4..714d8b7 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
 	if (!hid)
 		return -ENODEV;
 
-	if (down_trylock(&hid->driver_lock))
+	if (down_trylock(&hid->driver_input_lock))
 		return -EBUSY;
 
 	if (!hid->driver) {
@@ -1150,7 +1150,7 @@ nomem:
 	hid_report_raw_event(hid, type, data, size, interrupt);
 
 unlock:
-	up(&hid->driver_lock);
+	up(&hid->driver_input_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(hid_input_report);
@@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev)
 
 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		ret = -EINTR;
+		goto unlock_driver_lock;
+	}
+	hdev->io_started = false;
 
 	if (!hdev->driver) {
 		id = hid_match_device(hdev, hdrv);
@@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev)
 			hdev->driver = NULL;
 	}
 unlock:
+	if (!hdev->io_started)
+		up(&hdev->driver_input_lock);
+unlock_driver_lock:
 	up(&hdev->driver_lock);
 	return ret;
 }
@@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
 	struct hid_driver *hdrv;
+	int ret = 0;
 
 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		ret = -EINTR;
+		goto unlock_driver_lock;
+	}
+	hdev->io_started = false;
 
 	hdrv = hdev->driver;
 	if (hdrv) {
@@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device *dev)
 		hdev->driver = NULL;
 	}
 
+	if (!hdev->io_started)
+		up(&hdev->driver_input_lock);
+unlock_driver_lock:
 	up(&hdev->driver_lock);
-	return 0;
+	return ret;
 }
 
 static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
@@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void)
 	init_waitqueue_head(&hdev->debug_wait);
 	INIT_LIST_HEAD(&hdev->debug_list);
 	sema_init(&hdev->driver_lock, 1);
+	sema_init(&hdev->driver_input_lock, 1);
 
 	return hdev;
 err:
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 3a95da6..1e88371 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -481,7 +481,8 @@ struct hid_device {							/* device report descriptor */
 	unsigned country;						/* HID country */
 	struct hid_report_enum report_enum[HID_REPORT_TYPES];
 
-	struct semaphore driver_lock;					/* protects the current driver */
+	struct semaphore driver_lock;					/* protects the current driver, except during input */
+	struct semaphore driver_input_lock;				/* protects the current driver */
 	struct device dev;						/* device */
 	struct hid_driver *driver;
 	struct hid_ll_driver *ll_driver;
@@ -502,6 +503,7 @@ struct hid_device {							/* device report descriptor */
 	unsigned int status;						/* see STAT flags above */
 	unsigned claimed;						/* Claimed by hidinput, hiddev? */
 	unsigned quirks;						/* Various quirks the device can pull on us */
+	bool io_started;						/* Protected by driver_lock. If IO has started */
 
 	struct list_head inputs;					/* The list of inputs */
 	void *hiddev;							/* The hiddev structure */
@@ -622,6 +624,10 @@ struct hid_usage_id {
  * @resume: invoked on resume if device was not reset (NULL means nop)
  * @reset_resume: invoked on resume if device was reset (NULL means nop)
  *
+ * probe should return -errno on error, or 0 on success. During probe,
+ * input will not be passed to raw_event unless hid_device_io_start is
+ * called.
+ *
  * raw_event and event should return 0 on no action performed, 1 when no
  * further processing should be done and negative on error
  *
@@ -742,6 +748,44 @@ const struct hid_device_id *hid_match_id(struct hid_device *hdev,
 					 const struct hid_device_id *id);
 
 /**
+ * hid_device_io_start - enable HID input during probe, remove
+ *
+ * @hid - the device
+ *
+ * This should only be called during probe or remove and only be
+ * called by the thread calling probe or remove. It will allow
+ * incoming packets to be delivered to the driver.
+ */
+static inline void hid_device_io_start(struct hid_device *hid) {
+	if (hid->io_started) {
+		dev_warn(&hid->dev, "io already started");
+		return;
+	}
+	hid->io_started = true;
+	up(&hid->driver_input_lock);
+}
+
+/**
+ * hid_device_io_stop - disable HID input during probe, remove
+ *
+ * @hid - the device
+ *
+ * Should only be called after hid_device_io_start. It will prevent
+ * incoming packets from going to the driver for the duration of
+ * probe, remove. If called during probe, packets will still go to the
+ * driver after probe is complete. This function should only be called
+ * by the thread calling probe or remove.
+ */
+static inline void hid_device_io_stop(struct hid_device *hid) {
+	if (!hid->io_started) {
+		dev_warn(&hid->dev, "io already stopped");
+		return;
+	}
+	hid->io_started = false;
+	down(&hid->driver_input_lock);
+}
+
+/**
  * hid_map_usage - map usage input bits
  *
  * @hidinput: hidinput which we are interested in
-- 
1.8.1.3


^ permalink raw reply related	[flat|nested] 33+ messages in thread
* [PATCH 2/3] HID: Fix logitech-dj: Allow incoming packets during probe().
  2013-02-18 17:20   ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Andrew de los Reyes
  2013-02-18 17:20     ` [PATCH 1/3] HID: Separate struct hid_device's driver_lock into two locks Andrew de los Reyes
@ 2013-02-18 17:20     ` Andrew de los Reyes
  2013-02-18 17:20     ` [PATCH 3/3] Revert "HID: Fix logitech-dj: missing Unifying device issue" Andrew de los Reyes
  2013-03-01 13:15     ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Jiri Kosina
  3 siblings, 0 replies; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-18 17:20 UTC (permalink / raw)
  To: Linux Input; +Cc: Andrew de los Reyes

From: Andrew de los Reyes <adlr@chromium.org>

Historically, logitech-dj communicated with the device during probe()
to query the list of devices attached. Later, a change was introduced
to hid-core that prevented incoming packets for a device during
probe(), as many drivers are unable to handle such input. That change
broke the device enumeration in logitech-dj, so commit
596264082f10dd4a56 was introduced to workaround that by waiting for
normal input before enumerating devices.

Now that drivers can opt-in to receive input during probe, this patch
changes logitech-dj to do that, so that it can successfully complete
enumeration of devices during probe().

Signed-off-by: Andrew de los Reyes <adlr@chromium.org>

Change-Id: I2dca10a5c64f6c3e90ffb00547b47eaf94546eb6
---
 drivers/hid/hid-logitech-dj.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 9500f2f..bf647ef 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -803,6 +803,9 @@ static int logi_dj_probe(struct hid_device *hdev,
 		goto llopen_failed;
 	}
 
+	/* Allow incoming packets to arrive: */
+	hid_device_io_start(hdev);
+
 	retval = logi_dj_recv_query_paired_devices(djrcv_dev);
 	if (retval < 0) {
 		dev_err(&hdev->dev, "%s:logi_dj_recv_query_paired_devices "
-- 
1.8.1.3


^ permalink raw reply related	[flat|nested] 33+ messages in thread
* [PATCH 3/3] Revert "HID: Fix logitech-dj: missing Unifying device issue"
  2013-02-18 17:20   ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Andrew de los Reyes
  2013-02-18 17:20     ` [PATCH 1/3] HID: Separate struct hid_device's driver_lock into two locks Andrew de los Reyes
  2013-02-18 17:20     ` [PATCH 2/3] HID: Fix logitech-dj: Allow incoming packets during probe() Andrew de los Reyes
@ 2013-02-18 17:20     ` Andrew de los Reyes
  2013-03-01 13:15     ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Jiri Kosina
  3 siblings, 0 replies; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-18 17:20 UTC (permalink / raw)
  To: Linux Input; +Cc: Andrew de los Reyes

From: Andrew de los Reyes <adlr@chromium.org>

This reverts commit 596264082f10dd4a567c43d4526b2f54ac5520bc.

The reverted commit was a workaround needed when drivers became unable
to communicate with devices during probe(). Now that such
communication is possible, the workaround is not needed.

Change-Id: I75e27afbd1acfcb2fea2a7a645b4fcec3e31c417
Signed-off-by: Andrew de los Reyes <adlr@chromium.org>
---
 drivers/hid/hid-logitech-dj.c | 45 -------------------------------------------
 drivers/hid/hid-logitech-dj.h |  1 -
 2 files changed, 46 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index bf647ef..199b78c 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -193,7 +193,6 @@ static struct hid_ll_driver logi_dj_ll_driver;
 static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
 					size_t count,
 					unsigned char report_type);
-static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev);
 
 static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
 						struct dj_report *dj_report)
@@ -234,7 +233,6 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
 	if (dj_report->report_params[DEVICE_PAIRED_PARAM_SPFUNCTION] &
 	    SPFUNCTION_DEVICE_LIST_EMPTY) {
 		dbg_hid("%s: device list is empty\n", __func__);
-		djrcv_dev->querying_devices = false;
 		return;
 	}
 
@@ -245,12 +243,6 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
 		return;
 	}
 
-	if (djrcv_dev->paired_dj_devices[dj_report->device_index]) {
-		/* The device is already known. No need to reallocate it. */
-		dbg_hid("%s: device is already known\n", __func__);
-		return;
-	}
-
 	dj_hiddev = hid_allocate_device();
 	if (IS_ERR(dj_hiddev)) {
 		dev_err(&djrcv_hdev->dev, "%s: hid_allocate_device failed\n",
@@ -314,7 +306,6 @@ static void delayedwork_callback(struct work_struct *work)
 	struct dj_report dj_report;
 	unsigned long flags;
 	int count;
-	int retval;
 
 	dbg_hid("%s\n", __func__);
 
@@ -347,25 +338,6 @@ static void delayedwork_callback(struct work_struct *work)
 		logi_dj_recv_destroy_djhid_device(djrcv_dev, &dj_report);
 		break;
 	default:
-	/* A normal report (i. e. not belonging to a pair/unpair notification)
-	 * arriving here, means that the report arrived but we did not have a
-	 * paired dj_device associated to the report's device_index, this
-	 * means that the original "device paired" notification corresponding
-	 * to this dj_device never arrived to this driver. The reason is that
-	 * hid-core discards all packets coming from a device while probe() is
-	 * executing. */
-	if (!djrcv_dev->paired_dj_devices[dj_report.device_index]) {
-		/* ok, we don't know the device, just re-ask the
-		 * receiver for the list of connected devices. */
-		retval = logi_dj_recv_query_paired_devices(djrcv_dev);
-		if (!retval) {
-			/* everything went fine, so just leave */
-			break;
-		}
-		dev_err(&djrcv_dev->hdev->dev,
-			"%s:logi_dj_recv_query_paired_devices "
-			"error:%d\n", __func__, retval);
-		}
 		dbg_hid("%s: unexpected report type\n", __func__);
 	}
 }
@@ -396,12 +368,6 @@ static void logi_dj_recv_forward_null_report(struct dj_receiver_dev *djrcv_dev,
 	if (!djdev) {
 		dbg_hid("djrcv_dev->paired_dj_devices[dj_report->device_index]"
 			" is NULL, index %d\n", dj_report->device_index);
-		kfifo_in(&djrcv_dev->notif_fifo, dj_report, sizeof(struct dj_report));
-
-		if (schedule_work(&djrcv_dev->work) == 0) {
-			dbg_hid("%s: did not schedule the work item, was already "
-			"queued\n", __func__);
-		}
 		return;
 	}
 
@@ -432,12 +398,6 @@ static void logi_dj_recv_forward_report(struct dj_receiver_dev *djrcv_dev,
 	if (dj_device == NULL) {
 		dbg_hid("djrcv_dev->paired_dj_devices[dj_report->device_index]"
 			" is NULL, index %d\n", dj_report->device_index);
-		kfifo_in(&djrcv_dev->notif_fifo, dj_report, sizeof(struct dj_report));
-
-		if (schedule_work(&djrcv_dev->work) == 0) {
-			dbg_hid("%s: did not schedule the work item, was already "
-			"queued\n", __func__);
-		}
 		return;
 	}
 
@@ -479,10 +439,6 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev)
 	struct dj_report *dj_report;
 	int retval;
 
-	/* no need to protect djrcv_dev->querying_devices */
-	if (djrcv_dev->querying_devices)
-		return 0;
-
 	dj_report = kzalloc(sizeof(struct dj_report), GFP_KERNEL);
 	if (!dj_report)
 		return -ENOMEM;
@@ -494,7 +450,6 @@ static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev)
 	return retval;
 }
 
-
 static int logi_dj_recv_switch_to_dj_mode(struct dj_receiver_dev *djrcv_dev,
 					  unsigned timeout)
 {
diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
index 4a40003..fd28a5e 100644
--- a/drivers/hid/hid-logitech-dj.h
+++ b/drivers/hid/hid-logitech-dj.h
@@ -101,7 +101,6 @@ struct dj_receiver_dev {
 	struct work_struct work;
 	struct kfifo notif_fifo;
 	spinlock_t lock;
-	bool querying_devices;
 };
 
 struct dj_device {
-- 
1.8.1.3


^ permalink raw reply related	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-18 13:08 ` Jiri Kosina
  2013-02-18 17:20   ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Andrew de los Reyes
@ 2013-02-25 13:03   ` Jiri Kosina
  2013-02-25 13:09     ` David Herrmann
  1 sibling, 1 reply; 33+ messages in thread
From: Jiri Kosina @ 2013-02-25 13:03 UTC (permalink / raw)
  To: Andrew de los Reyes
  Cc: David Herrmann, Linux Input, Nestor Lopez Casado,
	Benjamin Tissoires, Bruno Prémont

On Mon, 18 Feb 2013, Jiri Kosina wrote:

> > Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
> 
> Hi,
> 
> thanks for finally proceeding on this front.
> 
> Please provide your Signed-off-by line; without that, the patch can't be 
> accepted.

Andrew, please ... ?

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-25 13:03   ` [PATCH] HID: Separate struct hid_device's driver_lock into two locks Jiri Kosina
@ 2013-02-25 13:09     ` David Herrmann
  2013-02-25 13:15       ` Jiri Kosina
  0 siblings, 1 reply; 33+ messages in thread
From: David Herrmann @ 2013-02-25 13:09 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Andrew de los Reyes, David Herrmann, Linux Input,
	Nestor Lopez Casado, Benjamin Tissoires, Bruno Prémont

Hi Jiri

On Mon, Feb 25, 2013 at 2:03 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 18 Feb 2013, Jiri Kosina wrote:
>
>> > Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
>>
>> Hi,
>>
>> thanks for finally proceeding on this front.
>>
>> Please provide your Signed-off-by line; without that, the patch can't be
>> accepted.
>
> Andrew, please ... ?

I got the new PATCH with the sign-off. Maybe you missed it?

Subject: [PATCH 1/3] HID: Separate struct hid_device's driver_lock
into two locks.
Date:	Mon, 18 Feb 2013 09:20:21 -0800
Message-Id: <1361208023-11500-2-git-send-email-andrew-vger@gizmolabs.org>

Cheers
David

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-25 13:09     ` David Herrmann
@ 2013-02-25 13:15       ` Jiri Kosina
  2013-02-25 17:16         ` Andrew de los Reyes
  0 siblings, 1 reply; 33+ messages in thread
From: Jiri Kosina @ 2013-02-25 13:15 UTC (permalink / raw)
  To: David Herrmann
  Cc: Andrew de los Reyes, David Herrmann, Linux Input,
	Nestor Lopez Casado, Benjamin Tissoires, Bruno Prémont

On Mon, 25 Feb 2013, David Herrmann wrote:

> >> Please provide your Signed-off-by line; without that, the patch can't be
> >> accepted.
> >
> > Andrew, please ... ?
> 
> I got the new PATCH with the sign-off. Maybe you missed it?
> 
> Subject: [PATCH 1/3] HID: Separate struct hid_device's driver_lock
> into two locks.
> Date:	Mon, 18 Feb 2013 09:20:21 -0800
> Message-Id: <1361208023-11500-2-git-send-email-andrew-vger@gizmolabs.org>

Ah, thanks for the pointer, apparently I wasn't CCed, which increases the 
likelyhood of patches getting lost significantly.

I'll take them from linux-input@ and add to my TODO.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-25 13:15       ` Jiri Kosina
@ 2013-02-25 17:16         ` Andrew de los Reyes
  0 siblings, 0 replies; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-25 17:16 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: David Herrmann, David Herrmann, Linux Input, Nestor Lopez Casado,
	Benjamin Tissoires, Bruno Prémont

Sorry, I'll make sure to CC you in the future.

If you look at linux-input@, you'll see I updated the patch and also
included a use of this patch.

-andrew

On Mon, Feb 25, 2013 at 5:15 AM, Jiri Kosina <jkosina@suse.cz> wrote:
>
> On Mon, 25 Feb 2013, David Herrmann wrote:
>
> > >> Please provide your Signed-off-by line; without that, the patch can't be
> > >> accepted.
> > >
> > > Andrew, please ... ?
> >
> > I got the new PATCH with the sign-off. Maybe you missed it?
> >
> > Subject: [PATCH 1/3] HID: Separate struct hid_device's driver_lock
> > into two locks.
> > Date: Mon, 18 Feb 2013 09:20:21 -0800
> > Message-Id: <1361208023-11500-2-git-send-email-andrew-vger@gizmolabs.org>
>
> Ah, thanks for the pointer, apparently I wasn't CCed, which increases the
> likelyhood of patches getting lost significantly.
>
> I'll take them from linux-input@ and add to my TODO.
>
> --
> Jiri Kosina
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe()
  2013-02-18 17:20   ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Andrew de los Reyes
                       ` (2 preceding siblings ...)
  2013-02-18 17:20     ` [PATCH 3/3] Revert "HID: Fix logitech-dj: missing Unifying device issue" Andrew de los Reyes
@ 2013-03-01 13:15     ` Jiri Kosina
  3 siblings, 0 replies; 33+ messages in thread
From: Jiri Kosina @ 2013-03-01 13:15 UTC (permalink / raw)
  To: Andrew de los Reyes; +Cc: Linux Input, Andrew de los Reyes

On Mon, 18 Feb 2013, Andrew de los Reyes wrote:

> From: Andrew de los Reyes <adlr@chromium.org>
> 
> This patch series includes the latest version of the patch to separate
> the HID driver lock into two. It also includes the first use of the
> new API provided by that patch: a change to the Logitech Unifying
> Receiver driver to detect attached peripherals during probe(). With
> that in, we can also revert a previous workound, which was originally
> introduced when probe() stopped being able to communicate with the
> device.
> 
> Andrew de los Reyes (3):
>   HID: Separate struct hid_device's driver_lock into two locks.
>   HID: Fix logitech-dj: Allow incoming packets during probe().
>   Revert "HID: Fix logitech-dj: missing Unifying device issue"
> 
>  drivers/hid/hid-core.c        | 24 +++++++++++++++++++---
>  drivers/hid/hid-logitech-dj.c | 48 +++----------------------------------------
>  drivers/hid/hid-logitech-dj.h |  1 -
>  include/linux/hid.h           | 46 ++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 69 insertions(+), 50 deletions(-)

Now queued in my tree. Thanks,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-14  7:06       ` Bruno Prémont
@ 2013-02-17  1:43         ` Andrew de los Reyes
  0 siblings, 0 replies; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-17  1:43 UTC (permalink / raw)
  To: Bruno Prémont
  Cc: David Herrmann, Linux Input, Nestor Lopez Casado,
	Benjamin Tissoires, Jiri Kosina

This version incorporates the warning for drivers that use
io_start()/stop() in an unbalanced way. It is otherwise unchanged from
the previous version.

Let me know if there are any other comments/issues.

Thanks,
-andrew

This patch separates struct hid_device's driver_lock into two. The
goal is to allow hid device drivers to receive input during their
probe() or remove() function calls. This is necessary because some
drivers need to communicate with the device to determine parameters
needed during probe (e.g., size of a multi-touch surface), and if
possible, may perfer to communicate with a device on host-initiated
disconnect (e.g., to put it into a low-power state).

Historically, three functions used driver_lock:

- hid_device_probe: blocks to acquire lock
- hid_device_remove: blocks to acquire lock
- hid_input_report: if locked returns -EBUSY, else acquires lock

This patch adds another lock (driver_input_lock) which is used to
block input from occurring. The lock behavior is now:

- hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
- hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
- hid_input_report: if driver_input_lock locked returns -EBUSY, else
  acquires driver_input_lock

This patch also adds two helper functions to be called during probe()
or remove(): hid_device_io_start() and hid_device_io_stop(). These
functions lock and unlock, respectively, driver_input_lock; they also
make a note of whether they did so that hid-core knows if a driver has
changed the lock state.

This patch results in no behavior change for existing devices and
drivers. However, during a probe() or remove() function call in a
driver, that driver may now selectively call hid_device_io_start() to
let input events come through, then optionally call
hid_device_io_stop() to stop them.

Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
---
 drivers/hid/hid-core.c | 24 +++++++++++++++++++++---
 include/linux/hid.h    | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 4da66b4..714d8b7 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int
type, u8 *data, int size, int i
 	if (!hid)
 		return -ENODEV;

-	if (down_trylock(&hid->driver_lock))
+	if (down_trylock(&hid->driver_input_lock))
 		return -EBUSY;

 	if (!hid->driver) {
@@ -1150,7 +1150,7 @@ nomem:
 	hid_report_raw_event(hid, type, data, size, interrupt);

 unlock:
-	up(&hid->driver_lock);
+	up(&hid->driver_input_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(hid_input_report);
@@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev)

 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		ret = -EINTR;
+		goto unlock_driver_lock;
+	}
+	hdev->io_started = false;

 	if (!hdev->driver) {
 		id = hid_match_device(hdev, hdrv);
@@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev)
 			hdev->driver = NULL;
 	}
 unlock:
+	if (!hdev->io_started)
+		up(&hdev->driver_input_lock);
+unlock_driver_lock:
 	up(&hdev->driver_lock);
 	return ret;
 }
@@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
 	struct hid_driver *hdrv;
+	int ret = 0;

 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		ret = -EINTR;
+		goto unlock_driver_lock;
+	}
+	hdev->io_started = false;

 	hdrv = hdev->driver;
 	if (hdrv) {
@@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device *dev)
 		hdev->driver = NULL;
 	}

+	if (!hdev->io_started)
+		up(&hdev->driver_input_lock);
+unlock_driver_lock:
 	up(&hdev->driver_lock);
-	return 0;
+	return ret;
 }

 static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
@@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void)
 	init_waitqueue_head(&hdev->debug_wait);
 	INIT_LIST_HEAD(&hdev->debug_list);
 	sema_init(&hdev->driver_lock, 1);
+	sema_init(&hdev->driver_input_lock, 1);

 	return hdev;
 err:
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 3a95da6..41e0e7b 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -481,7 +481,8 @@ struct hid_device {							/* device report descriptor */
 	unsigned country;						/* HID country */
 	struct hid_report_enum report_enum[HID_REPORT_TYPES];

-	struct semaphore driver_lock;					/* protects the current driver */
+	struct semaphore driver_lock;					/* protects the current driver,
except during input */
+	struct semaphore driver_input_lock;				/* protects the current driver */
 	struct device dev;						/* device */
 	struct hid_driver *driver;
 	struct hid_ll_driver *ll_driver;
@@ -502,6 +503,7 @@ struct hid_device {							/* device report descriptor */
 	unsigned int status;						/* see STAT flags above */
 	unsigned claimed;						/* Claimed by hidinput, hiddev? */
 	unsigned quirks;						/* Various quirks the device can pull on us */
+	bool io_started;						/* Protected by driver_lock. If IO has started */

 	struct list_head inputs;					/* The list of inputs */
 	void *hiddev;							/* The hiddev structure */
@@ -622,6 +624,10 @@ struct hid_usage_id {
  * @resume: invoked on resume if device was not reset (NULL means nop)
  * @reset_resume: invoked on resume if device was reset (NULL means nop)
  *
+ * probe should return -errno on error, or 0 on success. During probe,
+ * input will not be passed to raw_event unless hid_device_io_start is
+ * called.
+ *
  * raw_event and event should return 0 on no action performed, 1 when no
  * further processing should be done and negative on error
  *
@@ -742,6 +748,44 @@ const struct hid_device_id *hid_match_id(struct
hid_device *hdev,
 					 const struct hid_device_id *id);

 /**
+ * hid_device_io_start - enable HID input during probe, remove
+ *
+ * @hid - the device
+ *
+ * This should only be called during probe or remove and only be
+ * called by the thread calling probe or remove. It will allow
+ * incoming packets to be delivered to the driver.
+ */
+static inline void hid_device_io_start(struct hid_device *hid) {
+  if (hid->io_started) {
+    dev_warn(&hid->dev, "io already started");
+    return;
+  }
+  hid->io_started = true;
+  up(&hid->driver_input_lock);
+}
+
+/**
+ * hid_device_io_stop - disable HID input during probe, remove
+ *
+ * @hid - the device
+ *
+ * Should only be called after hid_device_io_start. It will prevent
+ * incoming packets from going to the driver for the duration of
+ * probe, remove. If called during probe, packets will still go to the
+ * driver after probe is complete. This function should only be called
+ * by the thread calling probe or remove.
+ */
+static inline void hid_device_io_stop(struct hid_device *hid) {
+  if (!hid->io_started) {
+    dev_warn(&hid->dev, "io already stopped");
+    return;
+  }
+  hid->io_started = false;
+  down(&hid->driver_input_lock);
+}
+
+/**
  * hid_map_usage - map usage input bits
  *
  * @hidinput: hidinput which we are interested in
-- 
1.8.1.3

^ permalink raw reply related	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-14  5:08     ` Andrew de los Reyes
@ 2013-02-14  7:06       ` Bruno Prémont
  2013-02-17  1:43         ` Andrew de los Reyes
  0 siblings, 1 reply; 33+ messages in thread
From: Bruno Prémont @ 2013-02-14  7:06 UTC (permalink / raw)
  To: Andrew de los Reyes
  Cc: David Herrmann, Linux Input, Nestor Lopez Casado,
	Benjamin Tissoires, Jiri Kosina

On Wed, 13 Feb 2013 21:08:20 -0800 Andrew de los Reyes wrote:
> > > >  /**
> > > > + * hid_device_io_start - enable HID input during probe, remove
> > > > + *
> > > > + * @hid - the device
> > > > + *
> > > > + * This should only be called during probe or remove. It will allow
> > > > + * incoming packets to be delivered to the driver.
> > > > + */
> > > > +static inline void hid_device_io_start(struct hid_device *hid) {
> > > > +  up(&hid->driver_input_lock);
> > > > +  hid->io_started = true;
> > >
> > > Shouldn't these lines be swapped? Doesn't matter but it looks weird to
> > > me this way.
> > >
> > > But more importantly, we must go sure this is called from the same
> > > thread that probe() is called on. Other use-cases are not supported by
> > > semaphores and might break due to missing barriers. So maybe the
> > > comment could include that?
> >
> > Maybe even check what value hid->io_started has and WARN() [+skip
> > up()/down()]
> > in case hid_device_io_start()/stop() was not called in a balanced way.
> 
> It is a goal to support hid_device_io_start()/stop() not being called
> in a balanced way. This is specifically needed by a change I'm making
> to hid-logitech-dj.c: During probe(), the driver will send out a
> request to the USB dongle to list any attached mice/keyboards. The
> reply packets aren't needed during probe(), so probe will return, but
> the driver wants packets to flow in, and it doesn't mind if they come
> in while probe() is still running or not. In this case, during
> probe(), the driver will call hid_device_io_start() to allow incoming
> packets, but then not call hid_device_io_stop().

With unbalanced I meant calling hid_device_io_start()/stop() twice or
more in a row in _probe() (or _remove()) without corresponding
_stop()/_start() call.
That kind of cases might happen (mostly) with error paths.

Like the following (simplified) example when both if() match:

int driver_probe(...)
{
   ...
   if (...) {
      ...
      hid_device_io_start();
      ...
   }
   ...
   if (...) {
      ...
      hid_device_io_start();
      ...
   }
   ...
}


Bruno

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-11 17:55   ` Bruno Prémont
  2013-02-11 19:13     ` Andrew de los Reyes
@ 2013-02-14  5:08     ` Andrew de los Reyes
  2013-02-14  7:06       ` Bruno Prémont
  1 sibling, 1 reply; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-14  5:08 UTC (permalink / raw)
  To: Bruno Prémont
  Cc: David Herrmann, Linux Input, Nestor Lopez Casado,
	Benjamin Tissoires, Jiri Kosina

I'm sending another version of this patch now, but I wanted to give a
couple quick comments:

On Mon, Feb 11, 2013 at 9:55 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
>
> Hi Andrew, David,
>
> On Mon, 11 February 2013 David Herrmann wrote:
> > Thanks a lot for the patch. I think it's fine and I would like to see
> > it applied upstream:
> >   Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Reviewed-by: Bruno Prémont <bonbons@linux-vserver.org>
>
> > Some small things below. They're just my personal opinion so I am also
> > ok with this being applied right away.
>
> Same from me, with the idea of making possible driver-bugs more visible.
>
> Thanks,
> Bruno
>
>
> > On Sun, Feb 10, 2013 at 6:47 PM, Andrew de los Reyes
> > <andrew-vger@gizmolabs.org> wrote:
> > > Hi Linux-Input and others,
> > >
> > > This is the latest version of the patch to allow device drivers to
> > > communicate with the corresponding device during probe(). I've
> > > incorporated many of David Herrmann's suggestions into this revision.
> > > The most notable change is that by using helper functions, we no
> > > longer need to have a special magic number return value from probe().
> > >
> > > This patch is part of a patch series to support Logitech touch devices
> > > (e.g., Wireless Touchpad). The rest of the series is not yet ready for
> > > discussion here, but those curious can see it here:
> > > https://github.com/adlr/linux/commits/logitech7
> > >
> > > Thanks for your comments,
> > > -andrew
> > >
> > > This patch separates struct hid_device's driver_lock into two. The
> > > goal is to allow hid device drivers to receive input during their
> > > probe() or remove() function calls. This is necessary because some
> > > drivers need to communicate with the device to determine parameters
> > > needed during probe (e.g., size of a multi-touch surface), and if
> > > possible, may perfer to communicate with a device on host-initiated
> > > disconnect (e.g., to put it into a low-power state).
> > >
> > > Historically, three functions used driver_lock:
> > >
> > > - hid_device_probe: blocks to acquire lock
> > > - hid_device_remove: blocks to acquire lock
> > > - hid_input_report: if locked returns -EBUSY, else acquires lock
> > >
> > > This patch adds another lock (driver_input_lock) which is used to
> > > block input from occurring. The lock behavior is now:
> > >
> > > - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
> > > - hid_device_remove: blocks to acq. driver_lock, then
> > > driver_input_lock
> > > - hid_input_report: if driver_input_lock locked returns -EBUSY, else
> > >   acquires driver_input_lock
> > >
> > > This patch also adds two helper functions to be called during probe()
> > > or remove(): hid_device_io_start() and hid_device_io_stop(). These
> > > functions lock and unlock, respectively, driver_input_lock; they also
> > > make a note of whether they did so that hid-core knows if a driver has
> > > changed the lock state.
> > >
> > > This patch results in no behavior change for existing devices and
> > > drivers. However, during a probe() or remove() function call in a
> > > driver, that driver may now selectively call hid_device_io_start() to
> > > let input events come through, then optionally call
> > > hid_device_io_stop() to stop them.
> > >
> > > Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
> > > ---
> > >  drivers/hid/hid-core.c | 24 +++++++++++++++++++++---
> > >  include/linux/hid.h    | 36 +++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 56 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 4da66b4..6a04b72 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int
> > > type, u8 *data, int size, int i
> > >         if (!hid)
> > >                 return -ENODEV;
> > >
> > > -       if (down_trylock(&hid->driver_lock))
> > > +       if (down_trylock(&hid->driver_input_lock))
> > >                 return -EBUSY;
> > >
> > >         if (!hid->driver) {
> > > @@ -1150,7 +1150,7 @@ nomem:
> > >         hid_report_raw_event(hid, type, data, size, interrupt);
> > >
> > >  unlock:
> > > -       up(&hid->driver_lock);
> > > +       up(&hid->driver_input_lock);
> > >         return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(hid_input_report);
> > > @@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev)
> > >
> > >         if (down_interruptible(&hdev->driver_lock))
> > >                 return -EINTR;
> > > +       if (down_interruptible(&hdev->driver_input_lock)) {
> > > +               ret = -EINTR;
> > > +               goto unlock_driver_lock;
> > > +       }
> > > +       hdev->io_started = false;
> > >
> > >         if (!hdev->driver) {
> > >                 id = hid_match_device(hdev, hdrv);
> > > @@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev)
> > >                         hdev->driver = NULL;
> > >         }
> > >  unlock:
> > > +       if (!hdev->io_started)
> > > +               hid_device_io_start(hdev);
> >
> > For symmetry you might use up() here and use the wrapper-functions
> > only in drivers? I don't know. Jiri should say what he prefers.
>
> Then same for the _remove() case, see below
>
> > > +unlock_driver_lock:
> > >         up(&hdev->driver_lock);
> > >         return ret;
> > >  }
> > > @@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device
> > > *dev)
> > >  {
> > >         struct hid_device *hdev = container_of(dev, struct hid_device,
> > > dev);
> > >         struct hid_driver *hdrv;
> > > +       int ret = 0;
> > >
> > >         if (down_interruptible(&hdev->driver_lock))
> > >                 return -EINTR;
> > > +       if (down_interruptible(&hdev->driver_input_lock)) {
> > > +               ret = -EINTR;
> > > +               goto unlock_driver_lock;
> > > +       }
> > > +       hdev->io_started = false;
> >
> > If we lock driver_input_lock during remove, we might lose important
> > packages here because the input handler drops them.
> >
> > Drivers that require this should deal with it properly, but you might
> > get annoying timeouts during remove if you do I/O and lose an
> > important packet here.
> >
> > But I think the proper way to fix this is to move I/O handling into
> > workqueues instead of interrupt-context so we can actually sleep in
> > handle_input. So I think this is fine.
>
> Though the driver would suffer the same timeout if it does not shortcut
> them when the device is being physically unplugged.
> So in both cases the driver does need to take explicit action in
> ->remove()
> probably even before reenabling I/O in order to "cancel" pending activity
> prior to doing the removal reconfiguration on logical unplug.
>
> > >         hdrv = hdev->driver;
> > >         if (hdrv) {
> > > @@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device
> > > *dev)
> > >                 hdev->driver = NULL;
> > >         }
> > >
> > > +       if (!hdev->io_started)
> > > +               hid_device_io_start(hdev);
>
> If withing probe we switch to up() we should go for down() here

It's up(), not down(), here.

>
> > > +unlock_driver_lock:
> > >         up(&hdev->driver_lock);
> > > -       return 0;
> > > +       return ret;
> > >  }
> > >
> > >  static int hid_uevent(struct device *dev, struct kobj_uevent_env
> > > *env)
> > > @@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void)
> > >         init_waitqueue_head(&hdev->debug_wait);
> > >         INIT_LIST_HEAD(&hdev->debug_list);
> > >         sema_init(&hdev->driver_lock, 1);
> > > +       sema_init(&hdev->driver_input_lock, 1);
> > >
> > >         return hdev;
> > >  err:
> > > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > > index 3a95da6..ae7d32d 100644
> > > --- a/include/linux/hid.h
> > > +++ b/include/linux/hid.h
> > > @@ -481,7 +481,8 @@ struct hid_device {
> > > /* device report descriptor */
> > >         unsigned country;
> > > /* HID country */
> > >         struct hid_report_enum report_enum[HID_REPORT_TYPES];
> > >
> > > -       struct semaphore driver_lock;
> > > /* protects the current driver */
> > > +       struct semaphore driver_lock;
> > > /* protects the current driver,
> > > except during input */
> > > +       struct semaphore driver_input_lock;
> > > /* protects the current driver */
> > >         struct device dev;
> > > /* device */
> > >         struct hid_driver *driver;
> > >         struct hid_ll_driver *ll_driver;
> > > @@ -502,6 +503,7 @@ struct hid_device {
> > > /* device report descriptor */
> > >         unsigned int status;
> > > /* see STAT flags above */
> > >         unsigned claimed;
> > > /* Claimed by hidinput, hiddev? */
> > >         unsigned quirks;
> > > /* Various quirks the device can pull on us */
> > > +       bool io_started;
> > > /* Protected by driver_lock. If IO has started */
> > >
> > >         struct list_head inputs;
> > > /* The list of inputs */
> > >         void *hiddev;
> > > /* The hiddev structure */
> > > @@ -622,6 +624,10 @@ struct hid_usage_id {
> > >   * @resume: invoked on resume if device was not reset (NULL means
> > > nop)
> > >   * @reset_resume: invoked on resume if device was reset (NULL means
> > > nop)
> > >   *
> > > + * probe should return -errno on error, or 0 on success. During
> > > probe,
> > > + * input will not be passed to raw_event unless hid_device_io_start
> > > is
> > > + * called.
> > > + *
> > >   * raw_event and event should return 0 on no action performed, 1 when
> > > no
> > >   * further processing should be done and negative on error
> > >   *
> > > @@ -742,6 +748,34 @@ const struct hid_device_id *hid_match_id(struct
> > > hid_device *hdev,
> > >                                          const struct hid_device_id
> > > *id);
> > >
> > >  /**
> > > + * hid_device_io_start - enable HID input during probe, remove
> > > + *
> > > + * @hid - the device
> > > + *
> > > + * This should only be called during probe or remove. It will allow
> > > + * incoming packets to be delivered to the driver.
> > > + */
> > > +static inline void hid_device_io_start(struct hid_device *hid) {
> > > +  up(&hid->driver_input_lock);
> > > +  hid->io_started = true;
> >
> > Shouldn't these lines be swapped? Doesn't matter but it looks weird to
> > me this way.
> >
> > But more importantly, we must go sure this is called from the same
> > thread that probe() is called on. Other use-cases are not supported by
> > semaphores and might break due to missing barriers. So maybe the
> > comment could include that?
>
> Maybe even check what value hid->io_started has and WARN() [+skip
> up()/down()]
> in case hid_device_io_start()/stop() was not called in a balanced way.

It is a goal to support hid_device_io_start()/stop() not being called
in a balanced way. This is specifically needed by a change I'm making
to hid-logitech-dj.c: During probe(), the driver will send out a
request to the USB dongle to list any attached mice/keyboards. The
reply packets aren't needed during probe(), so probe will return, but
the driver wants packets to flow in, and it doesn't mind if they come
in while probe() is still running or not. In this case, during
probe(), the driver will call hid_device_io_start() to allow incoming
packets, but then not call hid_device_io_stop().

>
> > > +}
> > > +
> > > +/**
> > > + * hid_device_io_stop - disable HID input during probe, remove
> > > + *
> > > + * @hid - the device
> > > + *
> > > + * Should only be called after hid_device_io_start. It will prevent
> > > + * incoming packets from going to the driver for the duration of
> > > + * probe, remove. If called during probe, packets will still go to
> > > the
> > > + * driver after probe is complete.
> > > + */
> > > +static inline void hid_device_io_stop(struct hid_device *hid) {
> > > +  hid->io_started = false;
> > > +  down(&hid->driver_input_lock);
> >
> > Same.
>
> Same as _start() here
>
> > > +}
> > > +
> > > +/**
> > >   * hid_map_usage - map usage input bits
> > >   *
> > >   * @hidinput: hidinput which we are interested in
> > > --
> > > 1.8.1
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-11 19:13     ` Andrew de los Reyes
@ 2013-02-11 21:05       ` Bruno Prémont
  0 siblings, 0 replies; 33+ messages in thread
From: Bruno Prémont @ 2013-02-11 21:05 UTC (permalink / raw)
  To: Andrew de los Reyes
  Cc: David Herrmann, Linux Input, Nestor Lopez Casado,
	Benjamin Tissoires, Jiri Kosina

On Mon, 11 February 2013 Andrew de los Reyes wrote:
> > > > @@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
> > > >  {
> > > >         struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> > > >         struct hid_driver *hdrv;
> > > > +       int ret = 0;
> > > >
> > > >         if (down_interruptible(&hdev->driver_lock))
> > > >                 return -EINTR;
> > > > +       if (down_interruptible(&hdev->driver_input_lock)) {
> > > > +               ret = -EINTR;
> > > > +               goto unlock_driver_lock;
> > > > +       }
> > > > +       hdev->io_started = false;
> > >
> > > If we lock driver_input_lock during remove, we might lose important
> > > packages here because the input handler drops them.
> > >
> > > Drivers that require this should deal with it properly, but you might
> > > get annoying timeouts during remove if you do I/O and lose an
> > > important packet here.
> > >
> > > But I think the proper way to fix this is to move I/O handling into
> > > workqueues instead of interrupt-context so we can actually sleep in
> > > handle_input. So I think this is fine.
> >
> > Though the driver would suffer the same timeout if it does not shortcut
> > them when the device is being physically unplugged.
> > So in both cases the driver does need to take explicit action in ->remove()
> > probably even before reenabling I/O in order to "cancel" pending activity
> > prior to doing the removal reconfiguration on logical unplug.
> 
> The kernel already blocks input during remove(); this patch doesn't
> change that behavior. We could have another API that allows drivers to
> opt-out of this behavior, but maybe that should be another patch.

I was mostly replying to David on the timeout part.
But sure it is for another patch.

There are two case here anyhow:
- physical unplug

  Here it will never make sense to restart I/O as there device is gone

- logical unplug

  In this case, maybe driver could tell which variant it prefers, stopping
  I/O itself during remove() or having I/O stopped by HID (with option to
  temporarily restart it as needed)

  In the case driver specifies "don't stop I/O on logical unplug" driver
  needs to be able to check current state of I/O. Either using a
  hid_device_io_can_start() or having hid_device_io_start() having a return
  value of 0 for successful start (even if I/O had never been stopped) and
  return -ENODEV if the device is gone.

Bruno

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-11 17:55   ` Bruno Prémont
@ 2013-02-11 19:13     ` Andrew de los Reyes
  2013-02-11 21:05       ` Bruno Prémont
  2013-02-14  5:08     ` Andrew de los Reyes
  1 sibling, 1 reply; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-11 19:13 UTC (permalink / raw)
  To: Bruno Prémont
  Cc: David Herrmann, Linux Input, Nestor Lopez Casado,
	Benjamin Tissoires, Jiri Kosina

On Mon, Feb 11, 2013 at 9:55 AM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
>
> Hi Andrew, David,
>
> On Mon, 11 February 2013 David Herrmann wrote:
> > Thanks a lot for the patch. I think it's fine and I would like to see
> > it applied upstream:
> >   Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Reviewed-by: Bruno Prémont <bonbons@linux-vserver.org>
>
> > Some small things below. They're just my personal opinion so I am also
> > ok with this being applied right away.
>
> Same from me, with the idea of making possible driver-bugs more visible.
>
> Thanks,
> Bruno
>
>
> > On Sun, Feb 10, 2013 at 6:47 PM, Andrew de los Reyes
> > <andrew-vger@gizmolabs.org> wrote:
> > > Hi Linux-Input and others,
> > >
> > > This is the latest version of the patch to allow device drivers to
> > > communicate with the corresponding device during probe(). I've
> > > incorporated many of David Herrmann's suggestions into this revision.
> > > The most notable change is that by using helper functions, we no
> > > longer need to have a special magic number return value from probe().
> > >
> > > This patch is part of a patch series to support Logitech touch devices
> > > (e.g., Wireless Touchpad). The rest of the series is not yet ready for
> > > discussion here, but those curious can see it here:
> > > https://github.com/adlr/linux/commits/logitech7
> > >
> > > Thanks for your comments,
> > > -andrew
> > >
> > > This patch separates struct hid_device's driver_lock into two. The
> > > goal is to allow hid device drivers to receive input during their
> > > probe() or remove() function calls. This is necessary because some
> > > drivers need to communicate with the device to determine parameters
> > > needed during probe (e.g., size of a multi-touch surface), and if
> > > possible, may perfer to communicate with a device on host-initiated
> > > disconnect (e.g., to put it into a low-power state).
> > >
> > > Historically, three functions used driver_lock:
> > >
> > > - hid_device_probe: blocks to acquire lock
> > > - hid_device_remove: blocks to acquire lock
> > > - hid_input_report: if locked returns -EBUSY, else acquires lock
> > >
> > > This patch adds another lock (driver_input_lock) which is used to
> > > block input from occurring. The lock behavior is now:
> > >
> > > - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
> > > - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
> > > - hid_input_report: if driver_input_lock locked returns -EBUSY, else
> > >   acquires driver_input_lock
> > >
> > > This patch also adds two helper functions to be called during probe()
> > > or remove(): hid_device_io_start() and hid_device_io_stop(). These
> > > functions lock and unlock, respectively, driver_input_lock; they also
> > > make a note of whether they did so that hid-core knows if a driver has
> > > changed the lock state.
> > >
> > > This patch results in no behavior change for existing devices and
> > > drivers. However, during a probe() or remove() function call in a
> > > driver, that driver may now selectively call hid_device_io_start() to
> > > let input events come through, then optionally call
> > > hid_device_io_stop() to stop them.
> > >
> > > Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
> > > ---
> > >  drivers/hid/hid-core.c | 24 +++++++++++++++++++++---
> > >  include/linux/hid.h    | 36 +++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 56 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 4da66b4..6a04b72 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int
> > > type, u8 *data, int size, int i
> > >         if (!hid)
> > >                 return -ENODEV;
> > >
> > > -       if (down_trylock(&hid->driver_lock))
> > > +       if (down_trylock(&hid->driver_input_lock))
> > >                 return -EBUSY;
> > >
> > >         if (!hid->driver) {
> > > @@ -1150,7 +1150,7 @@ nomem:
> > >         hid_report_raw_event(hid, type, data, size, interrupt);
> > >
> > >  unlock:
> > > -       up(&hid->driver_lock);
> > > +       up(&hid->driver_input_lock);
> > >         return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(hid_input_report);
> > > @@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev)
> > >
> > >         if (down_interruptible(&hdev->driver_lock))
> > >                 return -EINTR;
> > > +       if (down_interruptible(&hdev->driver_input_lock)) {
> > > +               ret = -EINTR;
> > > +               goto unlock_driver_lock;
> > > +       }
> > > +       hdev->io_started = false;
> > >
> > >         if (!hdev->driver) {
> > >                 id = hid_match_device(hdev, hdrv);
> > > @@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev)
> > >                         hdev->driver = NULL;
> > >         }
> > >  unlock:
> > > +       if (!hdev->io_started)
> > > +               hid_device_io_start(hdev);
> >
> > For symmetry you might use up() here and use the wrapper-functions
> > only in drivers? I don't know. Jiri should say what he prefers.
>
> Then same for the _remove() case, see below

Sounds good. I'll make this change for both.

>
> > > +unlock_driver_lock:
> > >         up(&hdev->driver_lock);
> > >         return ret;
> > >  }
> > > @@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
> > >  {
> > >         struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> > >         struct hid_driver *hdrv;
> > > +       int ret = 0;
> > >
> > >         if (down_interruptible(&hdev->driver_lock))
> > >                 return -EINTR;
> > > +       if (down_interruptible(&hdev->driver_input_lock)) {
> > > +               ret = -EINTR;
> > > +               goto unlock_driver_lock;
> > > +       }
> > > +       hdev->io_started = false;
> >
> > If we lock driver_input_lock during remove, we might lose important
> > packages here because the input handler drops them.
> >
> > Drivers that require this should deal with it properly, but you might
> > get annoying timeouts during remove if you do I/O and lose an
> > important packet here.
> >
> > But I think the proper way to fix this is to move I/O handling into
> > workqueues instead of interrupt-context so we can actually sleep in
> > handle_input. So I think this is fine.
>
> Though the driver would suffer the same timeout if it does not shortcut
> them when the device is being physically unplugged.
> So in both cases the driver does need to take explicit action in ->remove()
> probably even before reenabling I/O in order to "cancel" pending activity
> prior to doing the removal reconfiguration on logical unplug.

The kernel already blocks input during remove(); this patch doesn't
change that behavior. We could have another API that allows drivers to
opt-out of this behavior, but maybe that should be another patch.

-andrew

>
> > >         hdrv = hdev->driver;
> > >         if (hdrv) {
> > > @@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device *dev)
> > >                 hdev->driver = NULL;
> > >         }
> > >
> > > +       if (!hdev->io_started)
> > > +               hid_device_io_start(hdev);
>
> If withing probe we switch to up() we should go for down() here
>
> > > +unlock_driver_lock:
> > >         up(&hdev->driver_lock);
> > > -       return 0;
> > > +       return ret;
> > >  }
> > >
> > >  static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
> > > @@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void)
> > >         init_waitqueue_head(&hdev->debug_wait);
> > >         INIT_LIST_HEAD(&hdev->debug_list);
> > >         sema_init(&hdev->driver_lock, 1);
> > > +       sema_init(&hdev->driver_input_lock, 1);
> > >
> > >         return hdev;
> > >  err:
> > > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > > index 3a95da6..ae7d32d 100644
> > > --- a/include/linux/hid.h
> > > +++ b/include/linux/hid.h
> > > @@ -481,7 +481,8 @@ struct hid_device {                                                 /* device report descriptor */
> > >         unsigned country;                                               /* HID country */
> > >         struct hid_report_enum report_enum[HID_REPORT_TYPES];
> > >
> > > -       struct semaphore driver_lock;                                   /* protects the current driver */
> > > +       struct semaphore driver_lock;                                   /* protects the current driver,
> > > except during input */
> > > +       struct semaphore driver_input_lock;                             /* protects the current driver */
> > >         struct device dev;                                              /* device */
> > >         struct hid_driver *driver;
> > >         struct hid_ll_driver *ll_driver;
> > > @@ -502,6 +503,7 @@ struct hid_device {                                                 /* device report descriptor */
> > >         unsigned int status;                                            /* see STAT flags above */
> > >         unsigned claimed;                                               /* Claimed by hidinput, hiddev? */
> > >         unsigned quirks;                                                /* Various quirks the device can pull on us */
> > > +       bool io_started;                                                /* Protected by driver_lock. If IO has started */
> > >
> > >         struct list_head inputs;                                        /* The list of inputs */
> > >         void *hiddev;                                                   /* The hiddev structure */
> > > @@ -622,6 +624,10 @@ struct hid_usage_id {
> > >   * @resume: invoked on resume if device was not reset (NULL means nop)
> > >   * @reset_resume: invoked on resume if device was reset (NULL means nop)
> > >   *
> > > + * probe should return -errno on error, or 0 on success. During probe,
> > > + * input will not be passed to raw_event unless hid_device_io_start is
> > > + * called.
> > > + *
> > >   * raw_event and event should return 0 on no action performed, 1 when no
> > >   * further processing should be done and negative on error
> > >   *
> > > @@ -742,6 +748,34 @@ const struct hid_device_id *hid_match_id(struct
> > > hid_device *hdev,
> > >                                          const struct hid_device_id *id);
> > >
> > >  /**
> > > + * hid_device_io_start - enable HID input during probe, remove
> > > + *
> > > + * @hid - the device
> > > + *
> > > + * This should only be called during probe or remove. It will allow
> > > + * incoming packets to be delivered to the driver.
> > > + */
> > > +static inline void hid_device_io_start(struct hid_device *hid) {
> > > +  up(&hid->driver_input_lock);
> > > +  hid->io_started = true;
> >
> > Shouldn't these lines be swapped? Doesn't matter but it looks weird to
> > me this way.
> >
> > But more importantly, we must go sure this is called from the same
> > thread that probe() is called on. Other use-cases are not supported by
> > semaphores and might break due to missing barriers. So maybe the
> > comment could include that?
>
> Maybe even check what value hid->io_started has and WARN() [+skip up()/down()]
> in case hid_device_io_start()/stop() was not called in a balanced way.
>
> > > +}
> > > +
> > > +/**
> > > + * hid_device_io_stop - disable HID input during probe, remove
> > > + *
> > > + * @hid - the device
> > > + *
> > > + * Should only be called after hid_device_io_start. It will prevent
> > > + * incoming packets from going to the driver for the duration of
> > > + * probe, remove. If called during probe, packets will still go to the
> > > + * driver after probe is complete.
> > > + */
> > > +static inline void hid_device_io_stop(struct hid_device *hid) {
> > > +  hid->io_started = false;
> > > +  down(&hid->driver_input_lock);
> >
> > Same.
>
> Same as _start() here
>
> > > +}
> > > +
> > > +/**
> > >   * hid_map_usage - map usage input bits
> > >   *
> > >   * @hidinput: hidinput which we are interested in
> > > --
> > > 1.8.1
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-11 15:42 ` David Herrmann
@ 2013-02-11 17:55   ` Bruno Prémont
  2013-02-11 19:13     ` Andrew de los Reyes
  2013-02-14  5:08     ` Andrew de los Reyes
  0 siblings, 2 replies; 33+ messages in thread
From: Bruno Prémont @ 2013-02-11 17:55 UTC (permalink / raw)
  To: David Herrmann, Andrew de los Reyes
  Cc: Linux Input, Nestor Lopez Casado, Benjamin Tissoires, Jiri Kosina

Hi Andrew, David,

On Mon, 11 February 2013 David Herrmann wrote:
> Thanks a lot for the patch. I think it's fine and I would like to see
> it applied upstream:
>   Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Reviewed-by: Bruno Prémont <bonbons@linux-vserver.org>

> Some small things below. They're just my personal opinion so I am also
> ok with this being applied right away.

Same from me, with the idea of making possible driver-bugs more visible.

Thanks,
Bruno


> On Sun, Feb 10, 2013 at 6:47 PM, Andrew de los Reyes
> <andrew-vger@gizmolabs.org> wrote:
> > Hi Linux-Input and others,
> >
> > This is the latest version of the patch to allow device drivers to
> > communicate with the corresponding device during probe(). I've
> > incorporated many of David Herrmann's suggestions into this revision.
> > The most notable change is that by using helper functions, we no
> > longer need to have a special magic number return value from probe().
> >
> > This patch is part of a patch series to support Logitech touch devices
> > (e.g., Wireless Touchpad). The rest of the series is not yet ready for
> > discussion here, but those curious can see it here:
> > https://github.com/adlr/linux/commits/logitech7
> >
> > Thanks for your comments,
> > -andrew
> >
> > This patch separates struct hid_device's driver_lock into two. The
> > goal is to allow hid device drivers to receive input during their
> > probe() or remove() function calls. This is necessary because some
> > drivers need to communicate with the device to determine parameters
> > needed during probe (e.g., size of a multi-touch surface), and if
> > possible, may perfer to communicate with a device on host-initiated
> > disconnect (e.g., to put it into a low-power state).
> >
> > Historically, three functions used driver_lock:
> >
> > - hid_device_probe: blocks to acquire lock
> > - hid_device_remove: blocks to acquire lock
> > - hid_input_report: if locked returns -EBUSY, else acquires lock
> >
> > This patch adds another lock (driver_input_lock) which is used to
> > block input from occurring. The lock behavior is now:
> >
> > - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
> > - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
> > - hid_input_report: if driver_input_lock locked returns -EBUSY, else
> >   acquires driver_input_lock
> >
> > This patch also adds two helper functions to be called during probe()
> > or remove(): hid_device_io_start() and hid_device_io_stop(). These
> > functions lock and unlock, respectively, driver_input_lock; they also
> > make a note of whether they did so that hid-core knows if a driver has
> > changed the lock state.
> >
> > This patch results in no behavior change for existing devices and
> > drivers. However, during a probe() or remove() function call in a
> > driver, that driver may now selectively call hid_device_io_start() to
> > let input events come through, then optionally call
> > hid_device_io_stop() to stop them.
> >
> > Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
> > ---
> >  drivers/hid/hid-core.c | 24 +++++++++++++++++++++---
> >  include/linux/hid.h    | 36 +++++++++++++++++++++++++++++++++++-
> >  2 files changed, 56 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index 4da66b4..6a04b72 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int
> > type, u8 *data, int size, int i
> >         if (!hid)
> >                 return -ENODEV;
> >
> > -       if (down_trylock(&hid->driver_lock))
> > +       if (down_trylock(&hid->driver_input_lock))
> >                 return -EBUSY;
> >
> >         if (!hid->driver) {
> > @@ -1150,7 +1150,7 @@ nomem:
> >         hid_report_raw_event(hid, type, data, size, interrupt);
> >
> >  unlock:
> > -       up(&hid->driver_lock);
> > +       up(&hid->driver_input_lock);
> >         return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(hid_input_report);
> > @@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev)
> >
> >         if (down_interruptible(&hdev->driver_lock))
> >                 return -EINTR;
> > +       if (down_interruptible(&hdev->driver_input_lock)) {
> > +               ret = -EINTR;
> > +               goto unlock_driver_lock;
> > +       }
> > +       hdev->io_started = false;
> >
> >         if (!hdev->driver) {
> >                 id = hid_match_device(hdev, hdrv);
> > @@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev)
> >                         hdev->driver = NULL;
> >         }
> >  unlock:
> > +       if (!hdev->io_started)
> > +               hid_device_io_start(hdev);
> 
> For symmetry you might use up() here and use the wrapper-functions
> only in drivers? I don't know. Jiri should say what he prefers.

Then same for the _remove() case, see below

> > +unlock_driver_lock:
> >         up(&hdev->driver_lock);
> >         return ret;
> >  }
> > @@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
> >  {
> >         struct hid_device *hdev = container_of(dev, struct hid_device, dev);
> >         struct hid_driver *hdrv;
> > +       int ret = 0;
> >
> >         if (down_interruptible(&hdev->driver_lock))
> >                 return -EINTR;
> > +       if (down_interruptible(&hdev->driver_input_lock)) {
> > +               ret = -EINTR;
> > +               goto unlock_driver_lock;
> > +       }
> > +       hdev->io_started = false;
> 
> If we lock driver_input_lock during remove, we might lose important
> packages here because the input handler drops them.
> 
> Drivers that require this should deal with it properly, but you might
> get annoying timeouts during remove if you do I/O and lose an
> important packet here.
> 
> But I think the proper way to fix this is to move I/O handling into
> workqueues instead of interrupt-context so we can actually sleep in
> handle_input. So I think this is fine.

Though the driver would suffer the same timeout if it does not shortcut
them when the device is being physically unplugged.
So in both cases the driver does need to take explicit action in ->remove()
probably even before reenabling I/O in order to "cancel" pending activity
prior to doing the removal reconfiguration on logical unplug.

> >         hdrv = hdev->driver;
> >         if (hdrv) {
> > @@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device *dev)
> >                 hdev->driver = NULL;
> >         }
> >
> > +       if (!hdev->io_started)
> > +               hid_device_io_start(hdev);

If withing probe we switch to up() we should go for down() here

> > +unlock_driver_lock:
> >         up(&hdev->driver_lock);
> > -       return 0;
> > +       return ret;
> >  }
> >
> >  static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
> > @@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void)
> >         init_waitqueue_head(&hdev->debug_wait);
> >         INIT_LIST_HEAD(&hdev->debug_list);
> >         sema_init(&hdev->driver_lock, 1);
> > +       sema_init(&hdev->driver_input_lock, 1);
> >
> >         return hdev;
> >  err:
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index 3a95da6..ae7d32d 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -481,7 +481,8 @@ struct hid_device {                                                 /* device report descriptor */
> >         unsigned country;                                               /* HID country */
> >         struct hid_report_enum report_enum[HID_REPORT_TYPES];
> >
> > -       struct semaphore driver_lock;                                   /* protects the current driver */
> > +       struct semaphore driver_lock;                                   /* protects the current driver,
> > except during input */
> > +       struct semaphore driver_input_lock;                             /* protects the current driver */
> >         struct device dev;                                              /* device */
> >         struct hid_driver *driver;
> >         struct hid_ll_driver *ll_driver;
> > @@ -502,6 +503,7 @@ struct hid_device {                                                 /* device report descriptor */
> >         unsigned int status;                                            /* see STAT flags above */
> >         unsigned claimed;                                               /* Claimed by hidinput, hiddev? */
> >         unsigned quirks;                                                /* Various quirks the device can pull on us */
> > +       bool io_started;                                                /* Protected by driver_lock. If IO has started */
> >
> >         struct list_head inputs;                                        /* The list of inputs */
> >         void *hiddev;                                                   /* The hiddev structure */
> > @@ -622,6 +624,10 @@ struct hid_usage_id {
> >   * @resume: invoked on resume if device was not reset (NULL means nop)
> >   * @reset_resume: invoked on resume if device was reset (NULL means nop)
> >   *
> > + * probe should return -errno on error, or 0 on success. During probe,
> > + * input will not be passed to raw_event unless hid_device_io_start is
> > + * called.
> > + *
> >   * raw_event and event should return 0 on no action performed, 1 when no
> >   * further processing should be done and negative on error
> >   *
> > @@ -742,6 +748,34 @@ const struct hid_device_id *hid_match_id(struct
> > hid_device *hdev,
> >                                          const struct hid_device_id *id);
> >
> >  /**
> > + * hid_device_io_start - enable HID input during probe, remove
> > + *
> > + * @hid - the device
> > + *
> > + * This should only be called during probe or remove. It will allow
> > + * incoming packets to be delivered to the driver.
> > + */
> > +static inline void hid_device_io_start(struct hid_device *hid) {
> > +  up(&hid->driver_input_lock);
> > +  hid->io_started = true;
> 
> Shouldn't these lines be swapped? Doesn't matter but it looks weird to
> me this way.
> 
> But more importantly, we must go sure this is called from the same
> thread that probe() is called on. Other use-cases are not supported by
> semaphores and might break due to missing barriers. So maybe the
> comment could include that?

Maybe even check what value hid->io_started has and WARN() [+skip up()/down()]
in case hid_device_io_start()/stop() was not called in a balanced way.

> > +}
> > +
> > +/**
> > + * hid_device_io_stop - disable HID input during probe, remove
> > + *
> > + * @hid - the device
> > + *
> > + * Should only be called after hid_device_io_start. It will prevent
> > + * incoming packets from going to the driver for the duration of
> > + * probe, remove. If called during probe, packets will still go to the
> > + * driver after probe is complete.
> > + */
> > +static inline void hid_device_io_stop(struct hid_device *hid) {
> > +  hid->io_started = false;
> > +  down(&hid->driver_input_lock);
> 
> Same.

Same as _start() here

> > +}
> > +
> > +/**
> >   * hid_map_usage - map usage input bits
> >   *
> >   * @hidinput: hidinput which we are interested in
> > --
> > 1.8.1
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-10 17:47 Andrew de los Reyes
@ 2013-02-11 15:42 ` David Herrmann
  2013-02-11 17:55   ` Bruno Prémont
  0 siblings, 1 reply; 33+ messages in thread
From: David Herrmann @ 2013-02-11 15:42 UTC (permalink / raw)
  To: Andrew de los Reyes
  Cc: Linux Input, Nestor Lopez Casado, Benjamin Tissoires,
	Bruno Prémont, Jiri Kosina

Hi Andrew

Thanks a lot for the patch. I think it's fine and I would like to see
it applied upstream:
  Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Some small things below. They're just my personal opinion so I am also
ok with this being applied right away.

Thanks
David

On Sun, Feb 10, 2013 at 6:47 PM, Andrew de los Reyes
<andrew-vger@gizmolabs.org> wrote:
> Hi Linux-Input and others,
>
> This is the latest version of the patch to allow device drivers to
> communicate with the corresponding device during probe(). I've
> incorporated many of David Herrmann's suggestions into this revision.
> The most notable change is that by using helper functions, we no
> longer need to have a special magic number return value from probe().
>
> This patch is part of a patch series to support Logitech touch devices
> (e.g., Wireless Touchpad). The rest of the series is not yet ready for
> discussion here, but those curious can see it here:
> https://github.com/adlr/linux/commits/logitech7
>
> Thanks for your comments,
> -andrew
>
> This patch separates struct hid_device's driver_lock into two. The
> goal is to allow hid device drivers to receive input during their
> probe() or remove() function calls. This is necessary because some
> drivers need to communicate with the device to determine parameters
> needed during probe (e.g., size of a multi-touch surface), and if
> possible, may perfer to communicate with a device on host-initiated
> disconnect (e.g., to put it into a low-power state).
>
> Historically, three functions used driver_lock:
>
> - hid_device_probe: blocks to acquire lock
> - hid_device_remove: blocks to acquire lock
> - hid_input_report: if locked returns -EBUSY, else acquires lock
>
> This patch adds another lock (driver_input_lock) which is used to
> block input from occurring. The lock behavior is now:
>
> - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
> - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
> - hid_input_report: if driver_input_lock locked returns -EBUSY, else
>   acquires driver_input_lock
>
> This patch also adds two helper functions to be called during probe()
> or remove(): hid_device_io_start() and hid_device_io_stop(). These
> functions lock and unlock, respectively, driver_input_lock; they also
> make a note of whether they did so that hid-core knows if a driver has
> changed the lock state.
>
> This patch results in no behavior change for existing devices and
> drivers. However, during a probe() or remove() function call in a
> driver, that driver may now selectively call hid_device_io_start() to
> let input events come through, then optionally call
> hid_device_io_stop() to stop them.
>
> Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
> ---
>  drivers/hid/hid-core.c | 24 +++++++++++++++++++++---
>  include/linux/hid.h    | 36 +++++++++++++++++++++++++++++++++++-
>  2 files changed, 56 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 4da66b4..6a04b72 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int
> type, u8 *data, int size, int i
>         if (!hid)
>                 return -ENODEV;
>
> -       if (down_trylock(&hid->driver_lock))
> +       if (down_trylock(&hid->driver_input_lock))
>                 return -EBUSY;
>
>         if (!hid->driver) {
> @@ -1150,7 +1150,7 @@ nomem:
>         hid_report_raw_event(hid, type, data, size, interrupt);
>
>  unlock:
> -       up(&hid->driver_lock);
> +       up(&hid->driver_input_lock);
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(hid_input_report);
> @@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev)
>
>         if (down_interruptible(&hdev->driver_lock))
>                 return -EINTR;
> +       if (down_interruptible(&hdev->driver_input_lock)) {
> +               ret = -EINTR;
> +               goto unlock_driver_lock;
> +       }
> +       hdev->io_started = false;
>
>         if (!hdev->driver) {
>                 id = hid_match_device(hdev, hdrv);
> @@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev)
>                         hdev->driver = NULL;
>         }
>  unlock:
> +       if (!hdev->io_started)
> +               hid_device_io_start(hdev);

For symmetry you might use up() here and use the wrapper-functions
only in drivers? I don't know. Jiri should say what he prefers.

> +unlock_driver_lock:
>         up(&hdev->driver_lock);
>         return ret;
>  }
> @@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
>  {
>         struct hid_device *hdev = container_of(dev, struct hid_device, dev);
>         struct hid_driver *hdrv;
> +       int ret = 0;
>
>         if (down_interruptible(&hdev->driver_lock))
>                 return -EINTR;
> +       if (down_interruptible(&hdev->driver_input_lock)) {
> +               ret = -EINTR;
> +               goto unlock_driver_lock;
> +       }
> +       hdev->io_started = false;

If we lock driver_input_lock during remove, we might lose important
packages here because the input handler drops them.

Drivers that require this should deal with it properly, but you might
get annoying timeouts during remove if you do I/O and lose an
important packet here.

But I think the proper way to fix this is to move I/O handling into
workqueues instead of interrupt-context so we can actually sleep in
handle_input. So I think this is fine.

>         hdrv = hdev->driver;
>         if (hdrv) {
> @@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device *dev)
>                 hdev->driver = NULL;
>         }
>
> +       if (!hdev->io_started)
> +               hid_device_io_start(hdev);
> +unlock_driver_lock:
>         up(&hdev->driver_lock);
> -       return 0;
> +       return ret;
>  }
>
>  static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
> @@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void)
>         init_waitqueue_head(&hdev->debug_wait);
>         INIT_LIST_HEAD(&hdev->debug_list);
>         sema_init(&hdev->driver_lock, 1);
> +       sema_init(&hdev->driver_input_lock, 1);
>
>         return hdev;
>  err:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 3a95da6..ae7d32d 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -481,7 +481,8 @@ struct hid_device {                                                 /* device report descriptor */
>         unsigned country;                                               /* HID country */
>         struct hid_report_enum report_enum[HID_REPORT_TYPES];
>
> -       struct semaphore driver_lock;                                   /* protects the current driver */
> +       struct semaphore driver_lock;                                   /* protects the current driver,
> except during input */
> +       struct semaphore driver_input_lock;                             /* protects the current driver */
>         struct device dev;                                              /* device */
>         struct hid_driver *driver;
>         struct hid_ll_driver *ll_driver;
> @@ -502,6 +503,7 @@ struct hid_device {                                                 /* device report descriptor */
>         unsigned int status;                                            /* see STAT flags above */
>         unsigned claimed;                                               /* Claimed by hidinput, hiddev? */
>         unsigned quirks;                                                /* Various quirks the device can pull on us */
> +       bool io_started;                                                /* Protected by driver_lock. If IO has started */
>
>         struct list_head inputs;                                        /* The list of inputs */
>         void *hiddev;                                                   /* The hiddev structure */
> @@ -622,6 +624,10 @@ struct hid_usage_id {
>   * @resume: invoked on resume if device was not reset (NULL means nop)
>   * @reset_resume: invoked on resume if device was reset (NULL means nop)
>   *
> + * probe should return -errno on error, or 0 on success. During probe,
> + * input will not be passed to raw_event unless hid_device_io_start is
> + * called.
> + *
>   * raw_event and event should return 0 on no action performed, 1 when no
>   * further processing should be done and negative on error
>   *
> @@ -742,6 +748,34 @@ const struct hid_device_id *hid_match_id(struct
> hid_device *hdev,
>                                          const struct hid_device_id *id);
>
>  /**
> + * hid_device_io_start - enable HID input during probe, remove
> + *
> + * @hid - the device
> + *
> + * This should only be called during probe or remove. It will allow
> + * incoming packets to be delivered to the driver.
> + */
> +static inline void hid_device_io_start(struct hid_device *hid) {
> +  up(&hid->driver_input_lock);
> +  hid->io_started = true;

Shouldn't these lines be swapped? Doesn't matter but it looks weird to
me this way.

But more importantly, we must go sure this is called from the same
thread that probe() is called on. Other use-cases are not supported by
semaphores and might break due to missing barriers. So maybe the
comment could include that?

> +}
> +
> +/**
> + * hid_device_io_stop - disable HID input during probe, remove
> + *
> + * @hid - the device
> + *
> + * Should only be called after hid_device_io_start. It will prevent
> + * incoming packets from going to the driver for the duration of
> + * probe, remove. If called during probe, packets will still go to the
> + * driver after probe is complete.
> + */
> +static inline void hid_device_io_stop(struct hid_device *hid) {
> +  hid->io_started = false;
> +  down(&hid->driver_input_lock);

Same.

> +}
> +
> +/**
>   * hid_map_usage - map usage input bits
>   *
>   * @hidinput: hidinput which we are interested in
> --
> 1.8.1

^ permalink raw reply	[flat|nested] 33+ messages in thread
* [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
@ 2013-02-10 17:47 Andrew de los Reyes
  2013-02-11 15:42 ` David Herrmann
  0 siblings, 1 reply; 33+ messages in thread
From: Andrew de los Reyes @ 2013-02-10 17:47 UTC (permalink / raw)
  To: Linux Input, Nestor Lopez Casado, Benjamin Tissoires,
	Bruno Prémont, David Herrmann
  Cc: Jiri Kosina

Hi Linux-Input and others,

This is the latest version of the patch to allow device drivers to
communicate with the corresponding device during probe(). I've
incorporated many of David Herrmann's suggestions into this revision.
The most notable change is that by using helper functions, we no
longer need to have a special magic number return value from probe().

This patch is part of a patch series to support Logitech touch devices
(e.g., Wireless Touchpad). The rest of the series is not yet ready for
discussion here, but those curious can see it here:
https://github.com/adlr/linux/commits/logitech7

Thanks for your comments,
-andrew

This patch separates struct hid_device's driver_lock into two. The
goal is to allow hid device drivers to receive input during their
probe() or remove() function calls. This is necessary because some
drivers need to communicate with the device to determine parameters
needed during probe (e.g., size of a multi-touch surface), and if
possible, may perfer to communicate with a device on host-initiated
disconnect (e.g., to put it into a low-power state).

Historically, three functions used driver_lock:

- hid_device_probe: blocks to acquire lock
- hid_device_remove: blocks to acquire lock
- hid_input_report: if locked returns -EBUSY, else acquires lock

This patch adds another lock (driver_input_lock) which is used to
block input from occurring. The lock behavior is now:

- hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
- hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
- hid_input_report: if driver_input_lock locked returns -EBUSY, else
  acquires driver_input_lock

This patch also adds two helper functions to be called during probe()
or remove(): hid_device_io_start() and hid_device_io_stop(). These
functions lock and unlock, respectively, driver_input_lock; they also
make a note of whether they did so that hid-core knows if a driver has
changed the lock state.

This patch results in no behavior change for existing devices and
drivers. However, during a probe() or remove() function call in a
driver, that driver may now selectively call hid_device_io_start() to
let input events come through, then optionally call
hid_device_io_stop() to stop them.

Change-Id: I737f6fc15911134b51273acf8d3de92fa5cc0f85
---
 drivers/hid/hid-core.c | 24 +++++++++++++++++++++---
 include/linux/hid.h    | 36 +++++++++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 4da66b4..6a04b72 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int
type, u8 *data, int size, int i
 	if (!hid)
 		return -ENODEV;

-	if (down_trylock(&hid->driver_lock))
+	if (down_trylock(&hid->driver_input_lock))
 		return -EBUSY;

 	if (!hid->driver) {
@@ -1150,7 +1150,7 @@ nomem:
 	hid_report_raw_event(hid, type, data, size, interrupt);

 unlock:
-	up(&hid->driver_lock);
+	up(&hid->driver_input_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(hid_input_report);
@@ -1703,6 +1703,11 @@ static int hid_device_probe(struct device *dev)

 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		ret = -EINTR;
+		goto unlock_driver_lock;
+	}
+	hdev->io_started = false;

 	if (!hdev->driver) {
 		id = hid_match_device(hdev, hdrv);
@@ -1726,6 +1731,9 @@ static int hid_device_probe(struct device *dev)
 			hdev->driver = NULL;
 	}
 unlock:
+	if (!hdev->io_started)
+		hid_device_io_start(hdev);
+unlock_driver_lock:
 	up(&hdev->driver_lock);
 	return ret;
 }
@@ -1734,9 +1742,15 @@ static int hid_device_remove(struct device *dev)
 {
 	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
 	struct hid_driver *hdrv;
+	int ret = 0;

 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		ret = -EINTR;
+		goto unlock_driver_lock;
+	}
+	hdev->io_started = false;

 	hdrv = hdev->driver;
 	if (hdrv) {
@@ -1747,8 +1761,11 @@ static int hid_device_remove(struct device *dev)
 		hdev->driver = NULL;
 	}

+	if (!hdev->io_started)
+		hid_device_io_start(hdev);
+unlock_driver_lock:
 	up(&hdev->driver_lock);
-	return 0;
+	return ret;
 }

 static int hid_uevent(struct device *dev, struct kobj_uevent_env *env)
@@ -2126,6 +2143,7 @@ struct hid_device *hid_allocate_device(void)
 	init_waitqueue_head(&hdev->debug_wait);
 	INIT_LIST_HEAD(&hdev->debug_list);
 	sema_init(&hdev->driver_lock, 1);
+	sema_init(&hdev->driver_input_lock, 1);

 	return hdev;
 err:
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 3a95da6..ae7d32d 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -481,7 +481,8 @@ struct hid_device {							/* device report descriptor */
 	unsigned country;						/* HID country */
 	struct hid_report_enum report_enum[HID_REPORT_TYPES];

-	struct semaphore driver_lock;					/* protects the current driver */
+	struct semaphore driver_lock;					/* protects the current driver,
except during input */
+	struct semaphore driver_input_lock;				/* protects the current driver */
 	struct device dev;						/* device */
 	struct hid_driver *driver;
 	struct hid_ll_driver *ll_driver;
@@ -502,6 +503,7 @@ struct hid_device {							/* device report descriptor */
 	unsigned int status;						/* see STAT flags above */
 	unsigned claimed;						/* Claimed by hidinput, hiddev? */
 	unsigned quirks;						/* Various quirks the device can pull on us */
+	bool io_started;						/* Protected by driver_lock. If IO has started */

 	struct list_head inputs;					/* The list of inputs */
 	void *hiddev;							/* The hiddev structure */
@@ -622,6 +624,10 @@ struct hid_usage_id {
  * @resume: invoked on resume if device was not reset (NULL means nop)
  * @reset_resume: invoked on resume if device was reset (NULL means nop)
  *
+ * probe should return -errno on error, or 0 on success. During probe,
+ * input will not be passed to raw_event unless hid_device_io_start is
+ * called.
+ *
  * raw_event and event should return 0 on no action performed, 1 when no
  * further processing should be done and negative on error
  *
@@ -742,6 +748,34 @@ const struct hid_device_id *hid_match_id(struct
hid_device *hdev,
 					 const struct hid_device_id *id);

 /**
+ * hid_device_io_start - enable HID input during probe, remove
+ *
+ * @hid - the device
+ *
+ * This should only be called during probe or remove. It will allow
+ * incoming packets to be delivered to the driver.
+ */
+static inline void hid_device_io_start(struct hid_device *hid) {
+  up(&hid->driver_input_lock);
+  hid->io_started = true;
+}
+
+/**
+ * hid_device_io_stop - disable HID input during probe, remove
+ *
+ * @hid - the device
+ *
+ * Should only be called after hid_device_io_start. It will prevent
+ * incoming packets from going to the driver for the duration of
+ * probe, remove. If called during probe, packets will still go to the
+ * driver after probe is complete.
+ */
+static inline void hid_device_io_stop(struct hid_device *hid) {
+  hid->io_started = false;
+  down(&hid->driver_input_lock);
+}
+
+/**
  * hid_map_usage - map usage input bits
  *
  * @hidinput: hidinput which we are interested in
-- 
1.8.1

^ permalink raw reply related	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-06 15:10                 ` Jiri Kosina
@ 2013-02-06 15:21                   ` David Herrmann
  0 siblings, 0 replies; 33+ messages in thread
From: David Herrmann @ 2013-02-06 15:21 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Andrew de los Reyes, Nestor Lopez Casado, Benjamin Tissoires,
	Bruno Prémont, Linux Input

Hi Jiri

On Wed, Feb 6, 2013 at 4:10 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Tue, 5 Feb 2013, David Herrmann wrote:
>
>> > Yes, I'm hoping to upstream soon as part of a patch series to get WTP
>> > support in. The current version of the commit you are asking about is
>> > specifically here:
>> > https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c
>> >
>> > The main difference from before is that there is a new return value (1)
>> > which signifies that the event lock has been been up()ed, so the hid-core
>> > doesn't need to call up() itself. This way during probe(), a driver can
>> > allow incoming events continuously from midway through probe() indefinitely.
>>
>> The patch looks nice. Some comments:
>>
>> 1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or
>> similar instead of 1. This makes the code much more readable than
>> returning 0 or 1. Maybe you can find better names than my suggestions?
>>
>> 2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;"
>> instead of "up(input_lock); return -EINTR;". I think this is more
>> consistent.
>>
>> 3) in hid_device_probe() maybe check that hid_hw_start() doesn't
>> return "1" accidentally? The current code deadlocks if hid_hw_start()
>> would be changed to return 1 (which would be bad as is, but maybe be
>> rather safe than sorry?).
>>
>> 4) Maybe we can add a similar logic to hid_device_remove? That is,
>> hid_device_remove() shouldn't lock input_lock if the driver wants to
>> perform I/O during remove, too. Because this may cause the driver to
>> drop input events while the lock is held.
>> At least the hid-wiimote driver could make use of this feature during removal.
>
> David,
>
> what use-case exactly are you thinking of here, please?

If you unbind a driver via sysfs, hid-wiimote could suspend several
peripherals of a Wii Remote to safe energy. I don't know whether
that's a use-case that we actively support, but I thought it would be
nice to have if we are on it, anyway.

I know that during normal device removal no I/O is possible
(obviously), but during manual probe/remove this might happen.

Regards
David

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-05 17:22               ` David Herrmann
  2013-02-05 17:50                 ` Nestor Lopez Casado
@ 2013-02-06 15:10                 ` Jiri Kosina
  2013-02-06 15:21                   ` David Herrmann
  1 sibling, 1 reply; 33+ messages in thread
From: Jiri Kosina @ 2013-02-06 15:10 UTC (permalink / raw)
  To: David Herrmann
  Cc: Andrew de los Reyes, Nestor Lopez Casado, Benjamin Tissoires,
	Bruno Prémont, Linux Input

On Tue, 5 Feb 2013, David Herrmann wrote:

> > Yes, I'm hoping to upstream soon as part of a patch series to get WTP
> > support in. The current version of the commit you are asking about is
> > specifically here:
> > https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c
> >
> > The main difference from before is that there is a new return value (1)
> > which signifies that the event lock has been been up()ed, so the hid-core
> > doesn't need to call up() itself. This way during probe(), a driver can
> > allow incoming events continuously from midway through probe() indefinitely.
> 
> The patch looks nice. Some comments:
> 
> 1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or
> similar instead of 1. This makes the code much more readable than
> returning 0 or 1. Maybe you can find better names than my suggestions?
> 
> 2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;"
> instead of "up(input_lock); return -EINTR;". I think this is more
> consistent.
> 
> 3) in hid_device_probe() maybe check that hid_hw_start() doesn't
> return "1" accidentally? The current code deadlocks if hid_hw_start()
> would be changed to return 1 (which would be bad as is, but maybe be
> rather safe than sorry?).
> 
> 4) Maybe we can add a similar logic to hid_device_remove? That is,
> hid_device_remove() shouldn't lock input_lock if the driver wants to
> perform I/O during remove, too. Because this may cause the driver to
> drop input events while the lock is held.
> At least the hid-wiimote driver could make use of this feature during removal.

David,

what use-case exactly are you thinking of here, please?

Thanks,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-05 17:22               ` David Herrmann
@ 2013-02-05 17:50                 ` Nestor Lopez Casado
  2013-02-06 15:10                 ` Jiri Kosina
  1 sibling, 0 replies; 33+ messages in thread
From: Nestor Lopez Casado @ 2013-02-05 17:50 UTC (permalink / raw)
  To: David Herrmann
  Cc: Andrew de los Reyes, Jiri Kosina, Benjamin Tissoires,
	Bruno Prémont, Linux Input

Hi David,

I agree with your suggestions.

Nestor

On Tue, Feb 5, 2013 at 6:22 PM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> Hi Nestor
>
> On Tue, Feb 5, 2013 at 5:38 PM, Andrew de los Reyes
> <andrew-vger@gizmolabs.org> wrote:
>> Yes, I'm hoping to upstream soon as part of a patch series to get WTP
>> support in. The current version of the commit you are asking about is
>> specifically here:
>> https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c
>>
>> The main difference from before is that there is a new return value (1)
>> which signifies that the event lock has been been up()ed, so the hid-core
>> doesn't need to call up() itself. This way during probe(), a driver can
>> allow incoming events continuously from midway through probe() indefinitely.
>
> The patch looks nice. Some comments:
>
> 1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or
> similar instead of 1. This makes the code much more readable than
> returning 0 or 1. Maybe you can find better names than my suggestions?
>
> 2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;"
> instead of "up(input_lock); return -EINTR;". I think this is more
> consistent.
>
> 3) in hid_device_probe() maybe check that hid_hw_start() doesn't
> return "1" accidentally? The current code deadlocks if hid_hw_start()
> would be changed to return 1 (which would be bad as is, but maybe be
> rather safe than sorry?).
>
> 4) Maybe we can add a similar logic to hid_device_remove? That is,
> hid_device_remove() shouldn't lock input_lock if the driver wants to
> perform I/O during remove, too. Because this may cause the driver to
> drop input events while the lock is held.
> At least the hid-wiimote driver could make use of this feature during removal.
>
> 5) In current code, if a driver unlocks input_lock but still fails, it
> needs to re-lock input_lock and then return the appropriate error
> code. Is that the intended behavior?
>
> 6) Documentation! At least add some comments to
> hid_driver_probe/remove so others can make use of this feature without
> looking at other drivers.
>
>> If you are interested, I can send just that one commit to the linux-input
>> list now, instead of waiting on WTP to be ready for posting to linux-input
>> (which may be a couple weeks).
>
> I would prefer if you send an RFC patch to the list so we can all comment on it.
>
> btw. how about two helpers:
>
> static inline void hid_device_io_start(struct hid_device *d)
> {
>     up(&d->driver_input_lock);
> }
>
> static inline void hid_device_io_stop(struct hid_device *d)
> {
>     down(&d->driver_input_lock);
> }
>
> This would guarantee that drivers don't use down_interruptible()
> (which would be wrong in error-paths in ->probe()) and it would be
> more readable. The return-value could then be called something like
> HID_START_IO & HID_IO_RUNNING.. Or we can even set a flag on
> hid_device in io_start/io_stop so the return-value isn't needed at
> all.
>
> Thanks for the patch!
> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


On Tue, Feb 5, 2013 at 6:22 PM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> Hi Nestor
>
> On Tue, Feb 5, 2013 at 5:38 PM, Andrew de los Reyes
> <andrew-vger@gizmolabs.org> wrote:
>> Yes, I'm hoping to upstream soon as part of a patch series to get WTP
>> support in. The current version of the commit you are asking about is
>> specifically here:
>> https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c
>>
>> The main difference from before is that there is a new return value (1)
>> which signifies that the event lock has been been up()ed, so the hid-core
>> doesn't need to call up() itself. This way during probe(), a driver can
>> allow incoming events continuously from midway through probe() indefinitely.
>
> The patch looks nice. Some comments:
>
> 1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or
> similar instead of 1. This makes the code much more readable than
> returning 0 or 1. Maybe you can find better names than my suggestions?
>
> 2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;"
> instead of "up(input_lock); return -EINTR;". I think this is more
> consistent.
>
> 3) in hid_device_probe() maybe check that hid_hw_start() doesn't
> return "1" accidentally? The current code deadlocks if hid_hw_start()
> would be changed to return 1 (which would be bad as is, but maybe be
> rather safe than sorry?).
>
> 4) Maybe we can add a similar logic to hid_device_remove? That is,
> hid_device_remove() shouldn't lock input_lock if the driver wants to
> perform I/O during remove, too. Because this may cause the driver to
> drop input events while the lock is held.
> At least the hid-wiimote driver could make use of this feature during removal.
>
> 5) In current code, if a driver unlocks input_lock but still fails, it
> needs to re-lock input_lock and then return the appropriate error
> code. Is that the intended behavior?
>
> 6) Documentation! At least add some comments to
> hid_driver_probe/remove so others can make use of this feature without
> looking at other drivers.
>
>> If you are interested, I can send just that one commit to the linux-input
>> list now, instead of waiting on WTP to be ready for posting to linux-input
>> (which may be a couple weeks).
>
> I would prefer if you send an RFC patch to the list so we can all comment on it.
>
> btw. how about two helpers:
>
> static inline void hid_device_io_start(struct hid_device *d)
> {
>     up(&d->driver_input_lock);
> }
>
> static inline void hid_device_io_stop(struct hid_device *d)
> {
>     down(&d->driver_input_lock);
> }
>
> This would guarantee that drivers don't use down_interruptible()
> (which would be wrong in error-paths in ->probe()) and it would be
> more readable. The return-value could then be called something like
> HID_START_IO & HID_IO_RUNNING.. Or we can even set a flag on
> hid_device in io_start/io_stop so the return-value isn't needed at
> all.
>
> Thanks for the patch!
> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
       [not found]             ` <CAG_cf+ev2y1MHNJFYZycbjQfNgk4hYYRaEoFsKkZyhzAzMgc1A@mail.gmail.com>
@ 2013-02-05 17:22               ` David Herrmann
  2013-02-05 17:50                 ` Nestor Lopez Casado
  2013-02-06 15:10                 ` Jiri Kosina
  0 siblings, 2 replies; 33+ messages in thread
From: David Herrmann @ 2013-02-05 17:22 UTC (permalink / raw)
  To: Andrew de los Reyes
  Cc: Nestor Lopez Casado, Jiri Kosina, Benjamin Tissoires,
	Bruno Prémont, Linux Input

Hi Nestor

On Tue, Feb 5, 2013 at 5:38 PM, Andrew de los Reyes
<andrew-vger@gizmolabs.org> wrote:
> Yes, I'm hoping to upstream soon as part of a patch series to get WTP
> support in. The current version of the commit you are asking about is
> specifically here:
> https://github.com/adlr/linux/commit/2a4fae572bd88462c4a558a77dc53d9eb3f9e91c
>
> The main difference from before is that there is a new return value (1)
> which signifies that the event lock has been been up()ed, so the hid-core
> doesn't need to call up() itself. This way during probe(), a driver can
> allow incoming events continuously from midway through probe() indefinitely.

The patch looks nice. Some comments:

1) I would prefer constants HID_IS_LOCKED and HID_IS_UNLOCKED or
similar instead of 1. This makes the code much more readable than
returning 0 or 1. Maybe you can find better names than my suggestions?

2) in hid_device_probe() you can use "ret = -EINTR; goto unlock;"
instead of "up(input_lock); return -EINTR;". I think this is more
consistent.

3) in hid_device_probe() maybe check that hid_hw_start() doesn't
return "1" accidentally? The current code deadlocks if hid_hw_start()
would be changed to return 1 (which would be bad as is, but maybe be
rather safe than sorry?).

4) Maybe we can add a similar logic to hid_device_remove? That is,
hid_device_remove() shouldn't lock input_lock if the driver wants to
perform I/O during remove, too. Because this may cause the driver to
drop input events while the lock is held.
At least the hid-wiimote driver could make use of this feature during removal.

5) In current code, if a driver unlocks input_lock but still fails, it
needs to re-lock input_lock and then return the appropriate error
code. Is that the intended behavior?

6) Documentation! At least add some comments to
hid_driver_probe/remove so others can make use of this feature without
looking at other drivers.

> If you are interested, I can send just that one commit to the linux-input
> list now, instead of waiting on WTP to be ready for posting to linux-input
> (which may be a couple weeks).

I would prefer if you send an RFC patch to the list so we can all comment on it.

btw. how about two helpers:

static inline void hid_device_io_start(struct hid_device *d)
{
    up(&d->driver_input_lock);
}

static inline void hid_device_io_stop(struct hid_device *d)
{
    down(&d->driver_input_lock);
}

This would guarantee that drivers don't use down_interruptible()
(which would be wrong in error-paths in ->probe()) and it would be
more readable. The return-value could then be called something like
HID_START_IO & HID_IO_RUNNING.. Or we can even set a flag on
hid_device in io_start/io_stop so the return-value isn't needed at
all.

Thanks for the patch!
David

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2013-02-05 15:07         ` Jiri Kosina
@ 2013-02-05 16:19           ` Nestor Lopez Casado
       [not found]             ` <CAG_cf+ev2y1MHNJFYZycbjQfNgk4hYYRaEoFsKkZyhzAzMgc1A@mail.gmail.com>
  0 siblings, 1 reply; 33+ messages in thread
From: Nestor Lopez Casado @ 2013-02-05 16:19 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Benjamin Tissoires, Bruno Prémont, Andrew de los Reyes,
	Linux Input, David Herrmann

Hi Jiri,

Andrew has been progressing on a patchset that he will soon post.

for info: https://github.com/adlr/linux/commits/logitech6

I've been following his updates to hid-logitech-dj as well as
hid-logitech-wtp. We will soon get back to you.

Thanks for asking!

Cheers,
Nestor

On Tue, Feb 5, 2013 at 4:07 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 3 Dec 2012, Nestor Lopez Casado wrote:
>
>> >> It is clear that the goal is to make commit
>> >> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive.
>> >> The problem is that this commit stops any communication with the
>> >> device, even configuration communication.
>> >>
>> >> Logitech devices use their own protocol on top of the HID standard
>> >> protocol. For touch devices, this proprietary protocol requires to ask
>> >> the device for axis ranges, etc...
>> >>
>> >> So here, the idea is not to open the can of worm for every hid devices
>> >> through hw_start() / hw_stop() calls, I think the idea of Andrew is
>> >> just to allow hid-logitech-dj to get rid of this restriction in some
>> >> particular circumstances.
>> >> Consider this as a controlled backdoor of the can of worms :)
>> >
>> > I have been thinking for this for a while, and I wasn't able to come up
>> > with anything I'd personally consider substantially better aproach than
>> > splitting the lock. So I don't have strong objections at the very moment.
>> >
>> > This will also allow us to get rid of what commit 596264082f introduced in
>> > unifying driver, right? (adding Nestor to CC).
>>
>> That is correct. Moreover, we are planning additional changes to
>> hid-logitech-dj that will somehow revert that commit. As Andrew
>> mentioned earlier, the drivers for Unifying devices need to query
>> several parameters during their probe() execution, and besides this
>> lock issue, there is still another problem with the current
>> hid-logitech-dj strategy.  Unifying devices can only be queried when
>> the radio link is active, and as such, we are planning to postpone
>> calling hid_add_device() until we are sure that the device has an
>> active radio link.
>>
>> With both issues resolved, the path will be clear to add native
>> multitouch support for several Unifying devices like t650, t620 and
>> t400.
>
> Nestor,
>
> is there anything happening in this area, please?
>
> --
> Jiri Kosina
> SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2012-12-03 17:53       ` Nestor Lopez Casado
@ 2013-02-05 15:07         ` Jiri Kosina
  2013-02-05 16:19           ` Nestor Lopez Casado
  0 siblings, 1 reply; 33+ messages in thread
From: Jiri Kosina @ 2013-02-05 15:07 UTC (permalink / raw)
  To: Nestor Lopez Casado
  Cc: Benjamin Tissoires, Bruno Prémont, Andrew de los Reyes,
	Linux Input, David Herrmann

On Mon, 3 Dec 2012, Nestor Lopez Casado wrote:

> >> It is clear that the goal is to make commit
> >> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive.
> >> The problem is that this commit stops any communication with the
> >> device, even configuration communication.
> >>
> >> Logitech devices use their own protocol on top of the HID standard
> >> protocol. For touch devices, this proprietary protocol requires to ask
> >> the device for axis ranges, etc...
> >>
> >> So here, the idea is not to open the can of worm for every hid devices
> >> through hw_start() / hw_stop() calls, I think the idea of Andrew is
> >> just to allow hid-logitech-dj to get rid of this restriction in some
> >> particular circumstances.
> >> Consider this as a controlled backdoor of the can of worms :)
> >
> > I have been thinking for this for a while, and I wasn't able to come up
> > with anything I'd personally consider substantially better aproach than
> > splitting the lock. So I don't have strong objections at the very moment.
> >
> > This will also allow us to get rid of what commit 596264082f introduced in
> > unifying driver, right? (adding Nestor to CC).
> 
> That is correct. Moreover, we are planning additional changes to
> hid-logitech-dj that will somehow revert that commit. As Andrew
> mentioned earlier, the drivers for Unifying devices need to query
> several parameters during their probe() execution, and besides this
> lock issue, there is still another problem with the current
> hid-logitech-dj strategy.  Unifying devices can only be queried when
> the radio link is active, and as such, we are planning to postpone
> calling hid_add_device() until we are sure that the device has an
> active radio link.
> 
> With both issues resolved, the path will be clear to add native
> multitouch support for several Unifying devices like t650, t620 and
> t400.

Nestor,

is there anything happening in this area, please?

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2012-11-26 18:54     ` David Herrmann
  2012-11-26 20:50       ` Andrew de los Reyes
@ 2012-12-04  8:04       ` Jiri Kosina
  1 sibling, 0 replies; 33+ messages in thread
From: Jiri Kosina @ 2012-12-04  8:04 UTC (permalink / raw)
  To: David Herrmann
  Cc: Benjamin Tissoires, Bruno Prémont, Andrew de los Reyes, Linux Input

On Mon, 26 Nov 2012, David Herrmann wrote:

> The fix you are proposing actually looks pretty nice, although it puts
> the burden of locking to the hid-driver instead of the hid-core. So
> every .probe function must go sure that the lock is held when
> returning. This means if you unlock the input-lock, you need to lock
> it before calling return so hid-core can unlock it again. This opens a
> small race-condition where the hid-core might drop important
> input-events as input-lock is held during "return". Any ideas here?

But why should we care deeply?

The lost events wouldn't be the ones that are needed for initialization of 
the device, as all the probing as been finished already.

So they would be 'regular' events sent by the device. And as the 
initialization hasn't been finished yet, it really can't be predicted 
which events make it and which don't.

Thanks,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2012-12-03 13:17     ` Jiri Kosina
@ 2012-12-03 17:53       ` Nestor Lopez Casado
  2013-02-05 15:07         ` Jiri Kosina
  0 siblings, 1 reply; 33+ messages in thread
From: Nestor Lopez Casado @ 2012-12-03 17:53 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Benjamin Tissoires, Bruno Prémont, Andrew de los Reyes,
	Linux Input, David Herrmann

No html now ...
Cheers,
Nestor

On Mon, Dec 3, 2012 at 5:17 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 26 Nov 2012, Benjamin Tissoires wrote:
>
>> It is clear that the goal is to make commit
>> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive.
>> The problem is that this commit stops any communication with the
>> device, even configuration communication.
>>
>> Logitech devices use their own protocol on top of the HID standard
>> protocol. For touch devices, this proprietary protocol requires to ask
>> the device for axis ranges, etc...
>>
>> So here, the idea is not to open the can of worm for every hid devices
>> through hw_start() / hw_stop() calls, I think the idea of Andrew is
>> just to allow hid-logitech-dj to get rid of this restriction in some
>> particular circumstances.
>> Consider this as a controlled backdoor of the can of worms :)
>
> I have been thinking for this for a while, and I wasn't able to come up
> with anything I'd personally consider substantially better aproach than
> splitting the lock. So I don't have strong objections at the very moment.
>
> This will also allow us to get rid of what commit 596264082f introduced in
> unifying driver, right? (adding Nestor to CC).

That is correct. Moreover, we are planning additional changes to
hid-logitech-dj that will somehow revert that commit. As Andrew
mentioned earlier, the drivers for Unifying devices need to query
several parameters during their probe() execution, and besides this
lock issue, there is still another problem with the current
hid-logitech-dj strategy.  Unifying devices can only be queried when
the radio link is active, and as such, we are planning to postpone
calling hid_add_device() until we are sure that the device has an
active radio link.

With both issues resolved, the path will be clear to add native
multitouch support for several Unifying devices like t650, t620 and
t400.

> --
> Jiri Kosina
> SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2012-11-26 18:34   ` Benjamin Tissoires
  2012-11-26 18:54     ` David Herrmann
@ 2012-12-03 13:17     ` Jiri Kosina
  2012-12-03 17:53       ` Nestor Lopez Casado
  1 sibling, 1 reply; 33+ messages in thread
From: Jiri Kosina @ 2012-12-03 13:17 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Bruno Prémont, Andrew de los Reyes, Linux Input,
	David Herrmann, Nestor Lopez Casado

On Mon, 26 Nov 2012, Benjamin Tissoires wrote:

> It is clear that the goal is to make commit
> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive.
> The problem is that this commit stops any communication with the
> device, even configuration communication.
> 
> Logitech devices use their own protocol on top of the HID standard
> protocol. For touch devices, this proprietary protocol requires to ask
> the device for axis ranges, etc...
> 
> So here, the idea is not to open the can of worm for every hid devices
> through hw_start() / hw_stop() calls, I think the idea of Andrew is
> just to allow hid-logitech-dj to get rid of this restriction in some
> particular circumstances.
> Consider this as a controlled backdoor of the can of worms :)

I have been thinking for this for a while, and I wasn't able to come up 
with anything I'd personally consider substantially better aproach than 
splitting the lock. So I don't have strong objections at the very moment.

This will also allow us to get rid of what commit 596264082f introduced in 
unifying driver, right? (adding Nestor to CC).

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2012-11-26 18:54     ` David Herrmann
@ 2012-11-26 20:50       ` Andrew de los Reyes
  2012-12-04  8:04       ` Jiri Kosina
  1 sibling, 0 replies; 33+ messages in thread
From: Andrew de los Reyes @ 2012-11-26 20:50 UTC (permalink / raw)
  To: David Herrmann
  Cc: Benjamin Tissoires, Bruno Prémont, Linux Input, Jiri Kosina

On Mon, Nov 26, 2012 at 10:54 AM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> Hi
>
> On Mon, Nov 26, 2012 at 7:34 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> Hi Bruno,
>>
>> On Mon, Nov 26, 2012 at 6:56 PM, Bruno Prémont
>> <bonbons@linux-vserver.org> wrote:
>>> Hi Andrew,
>>>
>>> [CCing David Herrmann]
>>>
>>>
>>> On Sun, 25 November 2012 Andrew de los Reyes wrote:
>>>> Benjamin Tissoires and Nestor Lopez Casado have been helping me to add
>>>> Linux support for new Logitech Touch Mice (T620, T400). After running
>>>> into a road-block in hid-core, and solving it with this patch, we
>>>> thought it was good to show the community and see if this is okay, or
>>>> if there's a better solution that we've missed.
>>>>
>>>> Thanks for your comments,
>>>> -andrew
>>>>
>>>> This patch separates struct hid_device's driver_lock into two. The
>>>> goal is to allow hid device drivers to receive input during their
>>>> probe() function call. This is necessary because some drivers need to
>>>> communicate with the device to determine parameters needed during
>>>> probe (e.g., size of a multi-touch surface).
>>>>
>>>> Historically, three functions used driver_lock:
>>>>
>>>> - hid_device_probe: blocks to acquire lock
>>>> - hid_device_remove: blocks to acquire lock
>>>> - hid_input_report: if locked returns -EBUSY, else acquires lock
>>>>
>>>> This patch adds another lock (driver_input_lock) which is used to
>>>> block input from occurring. The lock behavior is now:
>>>>
>>>> - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
>>>> - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
>>>> - hid_input_report: if driver_input_lock locked returns -EBUSY, else
>>>>   acquires driver_input_lock
>>>>
>>>> This results in no behavior change for existing devices and
>>>> drivers. However, during a probe() function call in a driver, that
>>>> driver may now selectively unlock driver_input_lock to let input
>>>> events come through, then re-lock.
>>>
>>> That is more or less a new approach to release the restriction added in
>>> commit 4ea5454203d991ec85264f64f89ca8855fce69b0
>>> (HID: Fix race condition between driver core and ll-driver).
>>>
>>> From your suggested change the question could be if releasing the input
>>> lock should be connected to hw_start() / hw_stop() calls...
>>>
>>> Though connecting those together would certainly require some review of
>>> existing drivers in order not to reopen the can of worms closed by above
>>> mentioned commit.
>>>
>>
>> It is clear that the goal is to make commit
>> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive.
>> The problem is that this commit stops any communication with the
>> device, even configuration communication.
>>
>> Logitech devices use their own protocol on top of the HID standard
>> protocol. For touch devices, this proprietary protocol requires to ask
>> the device for axis ranges, etc...
>>
>> So here, the idea is not to open the can of worm for every hid devices
>> through hw_start() / hw_stop() calls, I think the idea of Andrew is
>> just to allow hid-logitech-dj to get rid of this restriction in some
>> particular circumstances.
>> Consider this as a controlled backdoor of the can of worms :)
>
> The problems I was facing back then when writing the patch was that a
> driver wasn't able to properly set itself up before input events might
> be received. Hence, I locked down the whole .probe function. We should
> make sure that this is guaranteed for all drivers when fixing this as
> most of them silently depend on this behavior.
> Releasing the lock with hw_start() would actually be the most logical
> thing to do, but the implementation is ugly as we would release the
> lock inside of the callback that ought to be protected by the lock.
> Therefore, I didn't do this. Furthermore, it is unclear whether this
> fixes all the race-conditions of all hid-drivers.
>
> The fix you are proposing actually looks pretty nice, although it puts
> the burden of locking to the hid-driver instead of the hid-core. So
> every .probe function must go sure that the lock is held when
> returning. This means if you unlock the input-lock, you need to lock
> it before calling return so hid-core can unlock it again. This opens a
> small race-condition where the hid-core might drop important
> input-events as input-lock is held during "return". Any ideas here?

Good point. I hadn't considered that some drivers may want to do this.
In the case that probe() is going to fail and return error, it seems
okay to lock the lock within probe() and stop events from coming in,
as this driver will cease to be used anyway. So that leaves us with
the case that a driver's probe() is successful and wants to not
interrupt the flow of incoming events. In that case, the simplest
solution seems like a new return value. Existing code returns 0 on
success or -errno on failure, it seems. Perhaps a return value of 0x1
could signify that the lock has been already unlocked?

-andrew

>
> Regards
> David
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2012-11-26 18:34   ` Benjamin Tissoires
@ 2012-11-26 18:54     ` David Herrmann
  2012-11-26 20:50       ` Andrew de los Reyes
  2012-12-04  8:04       ` Jiri Kosina
  2012-12-03 13:17     ` Jiri Kosina
  1 sibling, 2 replies; 33+ messages in thread
From: David Herrmann @ 2012-11-26 18:54 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Bruno Prémont, Andrew de los Reyes, Linux Input, Jiri Kosina

Hi

On Mon, Nov 26, 2012 at 7:34 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi Bruno,
>
> On Mon, Nov 26, 2012 at 6:56 PM, Bruno Prémont
> <bonbons@linux-vserver.org> wrote:
>> Hi Andrew,
>>
>> [CCing David Herrmann]
>>
>>
>> On Sun, 25 November 2012 Andrew de los Reyes wrote:
>>> Benjamin Tissoires and Nestor Lopez Casado have been helping me to add
>>> Linux support for new Logitech Touch Mice (T620, T400). After running
>>> into a road-block in hid-core, and solving it with this patch, we
>>> thought it was good to show the community and see if this is okay, or
>>> if there's a better solution that we've missed.
>>>
>>> Thanks for your comments,
>>> -andrew
>>>
>>> This patch separates struct hid_device's driver_lock into two. The
>>> goal is to allow hid device drivers to receive input during their
>>> probe() function call. This is necessary because some drivers need to
>>> communicate with the device to determine parameters needed during
>>> probe (e.g., size of a multi-touch surface).
>>>
>>> Historically, three functions used driver_lock:
>>>
>>> - hid_device_probe: blocks to acquire lock
>>> - hid_device_remove: blocks to acquire lock
>>> - hid_input_report: if locked returns -EBUSY, else acquires lock
>>>
>>> This patch adds another lock (driver_input_lock) which is used to
>>> block input from occurring. The lock behavior is now:
>>>
>>> - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
>>> - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
>>> - hid_input_report: if driver_input_lock locked returns -EBUSY, else
>>>   acquires driver_input_lock
>>>
>>> This results in no behavior change for existing devices and
>>> drivers. However, during a probe() function call in a driver, that
>>> driver may now selectively unlock driver_input_lock to let input
>>> events come through, then re-lock.
>>
>> That is more or less a new approach to release the restriction added in
>> commit 4ea5454203d991ec85264f64f89ca8855fce69b0
>> (HID: Fix race condition between driver core and ll-driver).
>>
>> From your suggested change the question could be if releasing the input
>> lock should be connected to hw_start() / hw_stop() calls...
>>
>> Though connecting those together would certainly require some review of
>> existing drivers in order not to reopen the can of worms closed by above
>> mentioned commit.
>>
>
> It is clear that the goal is to make commit
> 4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive.
> The problem is that this commit stops any communication with the
> device, even configuration communication.
>
> Logitech devices use their own protocol on top of the HID standard
> protocol. For touch devices, this proprietary protocol requires to ask
> the device for axis ranges, etc...
>
> So here, the idea is not to open the can of worm for every hid devices
> through hw_start() / hw_stop() calls, I think the idea of Andrew is
> just to allow hid-logitech-dj to get rid of this restriction in some
> particular circumstances.
> Consider this as a controlled backdoor of the can of worms :)

The problems I was facing back then when writing the patch was that a
driver wasn't able to properly set itself up before input events might
be received. Hence, I locked down the whole .probe function. We should
make sure that this is guaranteed for all drivers when fixing this as
most of them silently depend on this behavior.
Releasing the lock with hw_start() would actually be the most logical
thing to do, but the implementation is ugly as we would release the
lock inside of the callback that ought to be protected by the lock.
Therefore, I didn't do this. Furthermore, it is unclear whether this
fixes all the race-conditions of all hid-drivers.

The fix you are proposing actually looks pretty nice, although it puts
the burden of locking to the hid-driver instead of the hid-core. So
every .probe function must go sure that the lock is held when
returning. This means if you unlock the input-lock, you need to lock
it before calling return so hid-core can unlock it again. This opens a
small race-condition where the hid-core might drop important
input-events as input-lock is held during "return". Any ideas here?

Regards
David
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2012-11-26 17:56 ` Bruno Prémont
@ 2012-11-26 18:34   ` Benjamin Tissoires
  2012-11-26 18:54     ` David Herrmann
  2012-12-03 13:17     ` Jiri Kosina
  0 siblings, 2 replies; 33+ messages in thread
From: Benjamin Tissoires @ 2012-11-26 18:34 UTC (permalink / raw)
  To: Bruno Prémont
  Cc: Andrew de los Reyes, Linux Input, Jiri Kosina, David Herrmann

Hi Bruno,

On Mon, Nov 26, 2012 at 6:56 PM, Bruno Prémont
<bonbons@linux-vserver.org> wrote:
> Hi Andrew,
>
> [CCing David Herrmann]
>
>
> On Sun, 25 November 2012 Andrew de los Reyes wrote:
>> Benjamin Tissoires and Nestor Lopez Casado have been helping me to add
>> Linux support for new Logitech Touch Mice (T620, T400). After running
>> into a road-block in hid-core, and solving it with this patch, we
>> thought it was good to show the community and see if this is okay, or
>> if there's a better solution that we've missed.
>>
>> Thanks for your comments,
>> -andrew
>>
>> This patch separates struct hid_device's driver_lock into two. The
>> goal is to allow hid device drivers to receive input during their
>> probe() function call. This is necessary because some drivers need to
>> communicate with the device to determine parameters needed during
>> probe (e.g., size of a multi-touch surface).
>>
>> Historically, three functions used driver_lock:
>>
>> - hid_device_probe: blocks to acquire lock
>> - hid_device_remove: blocks to acquire lock
>> - hid_input_report: if locked returns -EBUSY, else acquires lock
>>
>> This patch adds another lock (driver_input_lock) which is used to
>> block input from occurring. The lock behavior is now:
>>
>> - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
>> - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
>> - hid_input_report: if driver_input_lock locked returns -EBUSY, else
>>   acquires driver_input_lock
>>
>> This results in no behavior change for existing devices and
>> drivers. However, during a probe() function call in a driver, that
>> driver may now selectively unlock driver_input_lock to let input
>> events come through, then re-lock.
>
> That is more or less a new approach to release the restriction added in
> commit 4ea5454203d991ec85264f64f89ca8855fce69b0
> (HID: Fix race condition between driver core and ll-driver).
>
> From your suggested change the question could be if releasing the input
> lock should be connected to hw_start() / hw_stop() calls...
>
> Though connecting those together would certainly require some review of
> existing drivers in order not to reopen the can of worms closed by above
> mentioned commit.
>

It is clear that the goal is to make commit
4ea5454203d991ec85264f64f89ca8855fce69b0 less restrictive.
The problem is that this commit stops any communication with the
device, even configuration communication.

Logitech devices use their own protocol on top of the HID standard
protocol. For touch devices, this proprietary protocol requires to ask
the device for axis ranges, etc...

So here, the idea is not to open the can of worm for every hid devices
through hw_start() / hw_stop() calls, I think the idea of Andrew is
just to allow hid-logitech-dj to get rid of this restriction in some
particular circumstances.
Consider this as a controlled backdoor of the can of worms :)

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 33+ messages in thread
* Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
  2012-11-25 18:48 Andrew de los Reyes
@ 2012-11-26 17:56 ` Bruno Prémont
  2012-11-26 18:34   ` Benjamin Tissoires
  0 siblings, 1 reply; 33+ messages in thread
From: Bruno Prémont @ 2012-11-26 17:56 UTC (permalink / raw)
  To: Andrew de los Reyes; +Cc: Linux Input, Jiri Kosina

Hi Andrew,

[CCing David Herrmann]


On Sun, 25 November 2012 Andrew de los Reyes wrote:
> Benjamin Tissoires and Nestor Lopez Casado have been helping me to add
> Linux support for new Logitech Touch Mice (T620, T400). After running
> into a road-block in hid-core, and solving it with this patch, we
> thought it was good to show the community and see if this is okay, or
> if there's a better solution that we've missed.
> 
> Thanks for your comments,
> -andrew
> 
> This patch separates struct hid_device's driver_lock into two. The
> goal is to allow hid device drivers to receive input during their
> probe() function call. This is necessary because some drivers need to
> communicate with the device to determine parameters needed during
> probe (e.g., size of a multi-touch surface).
> 
> Historically, three functions used driver_lock:
> 
> - hid_device_probe: blocks to acquire lock
> - hid_device_remove: blocks to acquire lock
> - hid_input_report: if locked returns -EBUSY, else acquires lock
> 
> This patch adds another lock (driver_input_lock) which is used to
> block input from occurring. The lock behavior is now:
> 
> - hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
> - hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
> - hid_input_report: if driver_input_lock locked returns -EBUSY, else
>   acquires driver_input_lock
> 
> This results in no behavior change for existing devices and
> drivers. However, during a probe() function call in a driver, that
> driver may now selectively unlock driver_input_lock to let input
> events come through, then re-lock.

That is more or less a new approach to release the restriction added in
commit 4ea5454203d991ec85264f64f89ca8855fce69b0
(HID: Fix race condition between driver core and ll-driver).

>From your suggested change the question could be if releasing the input
lock should be connected to hw_start() / hw_stop() calls...

Though connecting those together would certainly require some review of
existing drivers in order not to reopen the can of worms closed by above
mentioned commit.

Bruno

^ permalink raw reply	[flat|nested] 33+ messages in thread
* [PATCH] HID: Separate struct hid_device's driver_lock into two locks.
@ 2012-11-25 18:48 Andrew de los Reyes
  2012-11-26 17:56 ` Bruno Prémont
  0 siblings, 1 reply; 33+ messages in thread
From: Andrew de los Reyes @ 2012-11-25 18:48 UTC (permalink / raw)
  To: Linux Input, Jiri Kosina

Hi Linux-Input, Jiri,

Benjamin Tissoires and Nestor Lopez Casado have been helping me to add
Linux support for new Logitech Touch Mice (T620, T400). After running
into a road-block in hid-core, and solving it with this patch, we
thought it was good to show the community and see if this is okay, or
if there's a better solution that we've missed.

Thanks for your comments,
-andrew

This patch separates struct hid_device's driver_lock into two. The
goal is to allow hid device drivers to receive input during their
probe() function call. This is necessary because some drivers need to
communicate with the device to determine parameters needed during
probe (e.g., size of a multi-touch surface).

Historically, three functions used driver_lock:

- hid_device_probe: blocks to acquire lock
- hid_device_remove: blocks to acquire lock
- hid_input_report: if locked returns -EBUSY, else acquires lock

This patch adds another lock (driver_input_lock) which is used to
block input from occurring. The lock behavior is now:

- hid_device_probe: blocks to acq. driver_lock, then driver_input_lock
- hid_device_remove: blocks to acq. driver_lock, then driver_input_lock
- hid_input_report: if driver_input_lock locked returns -EBUSY, else
  acquires driver_input_lock

This results in no behavior change for existing devices and
drivers. However, during a probe() function call in a driver, that
driver may now selectively unlock driver_input_lock to let input
events come through, then re-lock.

---
 drivers/hid/hid-core.c |   15 +++++++++++++--
 include/linux/hid.h    |    3 ++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 4da66b4..3eb4398 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1097,7 +1097,7 @@ int hid_input_report(struct hid_device *hid, int
type, u8 *data, int size, int i
 	if (!hid)
 		return -ENODEV;

-	if (down_trylock(&hid->driver_lock))
+	if (down_trylock(&hid->driver_input_lock))
 		return -EBUSY;

 	if (!hid->driver) {
@@ -1150,7 +1150,7 @@ nomem:
 	hid_report_raw_event(hid, type, data, size, interrupt);

 unlock:
-	up(&hid->driver_lock);
+	up(&hid->driver_input_lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(hid_input_report);
@@ -1703,6 +1703,10 @@ static int hid_device_probe(struct device *dev)

 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		up(&hdev->driver_lock);
+		return -EINTR;
+	}

 	if (!hdev->driver) {
 		id = hid_match_device(hdev, hdrv);
@@ -1726,6 +1730,7 @@ static int hid_device_probe(struct device *dev)
 			hdev->driver = NULL;
 	}
 unlock:
+	up(&hdev->driver_input_lock);
 	up(&hdev->driver_lock);
 	return ret;
 }
@@ -1737,6 +1742,10 @@ static int hid_device_remove(struct device *dev)

 	if (down_interruptible(&hdev->driver_lock))
 		return -EINTR;
+	if (down_interruptible(&hdev->driver_input_lock)) {
+		up(&hdev->driver_lock);
+		return -EINTR;
+	}

 	hdrv = hdev->driver;
 	if (hdrv) {
@@ -1747,6 +1756,7 @@ static int hid_device_remove(struct device *dev)
 		hdev->driver = NULL;
 	}

+	up(&hdev->driver_input_lock);
 	up(&hdev->driver_lock);
 	return 0;
 }
@@ -2126,6 +2136,7 @@ struct hid_device *hid_allocate_device(void)
 	init_waitqueue_head(&hdev->debug_wait);
 	INIT_LIST_HEAD(&hdev->debug_list);
 	sema_init(&hdev->driver_lock, 1);
+	sema_init(&hdev->driver_input_lock, 1);

 	return hdev;
 err:
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 3a95da6..7e9d235 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -481,7 +481,8 @@ struct hid_device {							/* device report descriptor */
 	unsigned country;						/* HID country */
 	struct hid_report_enum report_enum[HID_REPORT_TYPES];

-	struct semaphore driver_lock;					/* protects the current driver */
+	struct semaphore driver_lock;					/* protects the current driver,
except during input */
+	struct semaphore driver_input_lock;				/* protects the current driver */
 	struct device dev;						/* device */
 	struct hid_driver *driver;
 	struct hid_ll_driver *ll_driver;
-- 
1.7.7.3

^ permalink raw reply related	[flat|nested] 33+ messages in thread
end of thread, other threads:[~2013-03-01 13:15 UTC | newest]

Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-14  5:07 [PATCH] HID: Separate struct hid_device's driver_lock into two locks Andrew de los Reyes
2013-02-18 13:08 ` Jiri Kosina
2013-02-18 17:20   ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Andrew de los Reyes
2013-02-18 17:20     ` [PATCH 1/3] HID: Separate struct hid_device's driver_lock into two locks Andrew de los Reyes
2013-02-18 17:20     ` [PATCH 2/3] HID: Fix logitech-dj: Allow incoming packets during probe() Andrew de los Reyes
2013-02-18 17:20     ` [PATCH 3/3] Revert "HID: Fix logitech-dj: missing Unifying device issue" Andrew de los Reyes
2013-03-01 13:15     ` [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe() Jiri Kosina
2013-02-25 13:03   ` [PATCH] HID: Separate struct hid_device's driver_lock into two locks Jiri Kosina
2013-02-25 13:09     ` David Herrmann
2013-02-25 13:15       ` Jiri Kosina
2013-02-25 17:16         ` Andrew de los Reyes
  -- strict thread matches above, loose matches on Subject: below --
2013-02-10 17:47 Andrew de los Reyes
2013-02-11 15:42 ` David Herrmann
2013-02-11 17:55   ` Bruno Prémont
2013-02-11 19:13     ` Andrew de los Reyes
2013-02-11 21:05       ` Bruno Prémont
2013-02-14  5:08     ` Andrew de los Reyes
2013-02-14  7:06       ` Bruno Prémont
2013-02-17  1:43         ` Andrew de los Reyes
2012-11-25 18:48 Andrew de los Reyes
2012-11-26 17:56 ` Bruno Prémont
2012-11-26 18:34   ` Benjamin Tissoires
2012-11-26 18:54     ` David Herrmann
2012-11-26 20:50       ` Andrew de los Reyes
2012-12-04  8:04       ` Jiri Kosina
2012-12-03 13:17     ` Jiri Kosina
2012-12-03 17:53       ` Nestor Lopez Casado
2013-02-05 15:07         ` Jiri Kosina
2013-02-05 16:19           ` Nestor Lopez Casado
     [not found]             ` <CAG_cf+ev2y1MHNJFYZycbjQfNgk4hYYRaEoFsKkZyhzAzMgc1A@mail.gmail.com>
2013-02-05 17:22               ` David Herrmann
2013-02-05 17:50                 ` Nestor Lopez Casado
2013-02-06 15:10                 ` Jiri Kosina
2013-02-06 15:21                   ` David Herrmann

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.