[PATCH] HID: Separate struct hid_device's driver_lock into two locks.

[PATCH] HID: Separate struct hid_device's driver_lock into two locks.

[Andrew de los Reyes]

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

Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.

[Jiri Kosina]

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

[PATCH 0/3] logitech-dj: Successfully detect attached devices during probe()

[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

[PATCH 1/3] HID: Separate struct hid_device's driver_lock into two locks.

[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

[PATCH 2/3] HID: Fix logitech-dj: Allow incoming packets during probe().

[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

[PATCH 3/3] Revert "HID: Fix logitech-dj: missing Unifying device issue"

[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

Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.

[Jiri Kosina]

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

Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.

[David Herrmann]

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

Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.

[Jiri Kosina]

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

Re: [PATCH] HID: Separate struct hid_device's driver_lock into two locks.

[Andrew de los Reyes]

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

Re: [PATCH 0/3] logitech-dj: Successfully detect attached devices during probe()

[Jiri Kosina]

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