[PATCH 0/5] HID logitech DJ fixes and preps for enabling extended features

[PATCH 0/5] HID logitech DJ fixes and preps for enabling extended features

[Benjamin Tissoires]

Hi Jiri,

Well, this work is _not_ for 3.14 (except maybe patch 1), especially since it is 
missing the biggest part where we enable the capabilities of Logitech devices.

Long story short:
This work is based on the work I did back in Summer 2011. I worked at Logitech
for a few weeks to show up a demo of a driver for the Logitech Wireless Touchpad.
At that time, a first draft has been done, but due to a lack of resources, noone
upstreamed it.
Since then, the code marinated a little at Logitech and in the ChromeOS kernel
tree, but nobody tried to push it upstream. So here I am, trying to push this stuff
upstream.

I can not send the full series right now because I am lacking most of the
testing hardware (I mean I only have the oldest Wireless Touchpad).
Hopefully, I should receive some other soon, and I'll be able to send the second
part then.

Now, let me roughly explain the patches:
- patch 1 can be applied right now I think, but it's entirely up to you Jiri.
  This patch should fix the missing notifications with some USB 3.0 boards.
- patches 2 to 5 allows to forward in both direction the proprietary protocol
  used by Logitech (HID++ [1]) between the driver and the hardware.
- later patches will introduce a transport layer for HID++ and also a driver
  to support full multitouch on various Logitech touchpads.

Nestor, Andrew, feel free to add your "Signed-off-by" whereever you want, I lost
a little bit the track of who added what.

Cheers,
Benjamin

[1] HID++: Documentation is provided at
https://drive.google.com/a/logitech.com/?tab=mo#folders/0BxbRzx7vEV7eWmgwazJ3NUFfQ28

Benjamin Tisssoires (5):
  HID: logitech-dj: Fix USB 3.0 issue
  HID: core: do not scan reports if the group is already set
  HID: logitech-dj: rely on hid groups to separate receivers from dj
    devices
  HID: logitech-dj: forward incoming HID++ reports to the correct dj
    device
  HID: logitech-dj: add .request callback

 drivers/hid/hid-core.c        |   3 +-
 drivers/hid/hid-logitech-dj.c | 161 +++++++++++++++++++++++++++++++++---------
 drivers/hid/hid-logitech-dj.h |  16 ++---
 include/linux/hid.h           |   1 +
 4 files changed, 136 insertions(+), 45 deletions(-)

-- 
1.8.4.2

[PATCH 1/5] HID: logitech-dj: Fix USB 3.0 issue

[Benjamin Tissoires]

From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>

This fix (not very clean though) should fix the long time USB3
issue that was spotted last year. The rational has been given by
Hans de Goede:

 ----

I think the most likely cause for this is a firmware bug
in the unifying receiver, likely a race condition.

The most prominent difference between having a USB-2 device
plugged into an EHCI (so USB-2 only) port versus an XHCI
port will be inter packet timing. Specifically if you
send packets (ie hid reports) one at a time, then with
the EHCI controller their will be a significant pause
between them, where with XHCI they will be very close
together in time.

The reason for this is the difference in EHCI / XHCI
controller OS <-> driver interfaces.

For non periodic endpoints (control, bulk) the EHCI uses a
circular linked-list of commands in dma-memory, which it
follows to execute commands, if the list is empty, it
will go into an idle state and re-check periodically.

The XHCI uses a ring of commands per endpoint, and if the OS
places anything new on the ring it will do an ioport write,
waking up the XHCI making it send the new packet immediately.

For periodic transfers (isoc, interrupt) the delay between
packets when sending one at a time (rather then queuing them
up) will be even larger, because they need to be inserted into
the EHCI schedule 2 ms in the future so the OS driver can be
sure that the EHCI driver does not try to start executing the
time slot in question before the insertion has completed.

So a possible fix may be to insert a delay between packets
being send to the receiver.

 ----

I tested this on a buggy Haswell USB 3.0 motherboard, and I always
get the notification after adding the msleep.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-dj.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index a7947d8..f45279c 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -516,6 +516,14 @@ static int logi_dj_recv_switch_to_dj_mode(struct dj_receiver_dev *djrcv_dev,
 	dj_report->report_params[CMD_SWITCH_PARAM_TIMEOUT_SECONDS] = (u8)timeout;
 	retval = logi_dj_recv_send_report(djrcv_dev, dj_report);
 	kfree(dj_report);
+
+	/*
+	 * Ugly sleep to work around a USB 3.0 bug when the receiver is still
+	 * processing the "switch-to-dj" command while we send an other command.
+	 * 50 msec should gives enough time to the receiver to be ready.
+	 */
+	msleep(50);
+
 	return retval;
 }
 
-- 
1.8.4.2

[PATCH 2/5] HID: core: do not scan reports if the group is already set

[Benjamin Tissoires]

From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>

This allows the transport layer (I have in mind hid-logitech-dj and uhid)
to set the group before it is added to the hid bus. This way, it can
bypass the hid_scan_report() call, and choose in advance which driver
will handle the newly created hid device.

Signed-off-by: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index a5a8c6a..200118a 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2438,7 +2438,8 @@ int hid_add_device(struct hid_device *hdev)
 	 * Scan generic devices for group information
 	 */
 	if (hid_ignore_special_drivers ||
-	    !hid_match_id(hdev, hid_have_special_driver)) {
+	    (!hdev->group &&
+	     !hid_match_id(hdev, hid_have_special_driver))) {
 		ret = hid_scan_report(hdev);
 		if (ret)
 			hid_warn(hdev, "bad device descriptor (%d)\n", ret);
-- 
1.8.4.2

[PATCH 3/5] HID: logitech-dj: rely on hid groups to separate receivers from dj devices

[Benjamin Tissoires]

From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>

Several benefits here:
- we can drop the macro is_dj_device: I never been really conviced by
  this macro as we could fall into a null pointer anytime. Anyway time
  showed that this never happened.
- we can simplify the hid driver logitech-djdevice, and make it aware
  of any new receiver VID/PID.
- we can use the Wireless PID of the DJ device as the product id of the
  hid device, this way the sysfs will differentiate between different
  DJ devices.

Signed-off-by: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-dj.c | 37 +++++++++----------------------------
 drivers/hid/hid-logitech-dj.h | 10 ----------
 include/linux/hid.h           |  1 +
 3 files changed, 10 insertions(+), 38 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index f45279c..a4b3cee 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -263,11 +263,14 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
 	dj_hiddev->dev.parent = &djrcv_hdev->dev;
 	dj_hiddev->bus = BUS_USB;
 	dj_hiddev->vendor = le16_to_cpu(usbdev->descriptor.idVendor);
-	dj_hiddev->product = le16_to_cpu(usbdev->descriptor.idProduct);
+	dj_hiddev->product =
+	    (dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_MSB] << 8) |
+	     dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_LSB];
 	snprintf(dj_hiddev->name, sizeof(dj_hiddev->name),
-		"Logitech Unifying Device. Wireless PID:%02x%02x",
-		dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_MSB],
-		dj_report->report_params[DEVICE_PAIRED_PARAM_EQUAD_ID_LSB]);
+		"Logitech Unifying Device. Wireless PID:%04x",
+		dj_hiddev->product);
+
+	dj_hiddev->group = HID_GROUP_LOGITECH_DJ_DEVICE_GENERIC;
 
 	usb_make_path(usbdev, dj_hiddev->phys, sizeof(dj_hiddev->phys));
 	snprintf(tmpstr, sizeof(tmpstr), ":%d", dj_report->device_index);
@@ -752,9 +755,6 @@ static int logi_dj_probe(struct hid_device *hdev,
 	struct dj_receiver_dev *djrcv_dev;
 	int retval;
 
-	if (is_dj_device((struct dj_device *)hdev->driver_data))
-		return -ENODEV;
-
 	dbg_hid("%s called for ifnum %d\n", __func__,
 		intf->cur_altsetting->desc.bInterfaceNumber);
 
@@ -907,22 +907,6 @@ static void logi_dj_remove(struct hid_device *hdev)
 	hid_set_drvdata(hdev, NULL);
 }
 
-static int logi_djdevice_probe(struct hid_device *hdev,
-			 const struct hid_device_id *id)
-{
-	int ret;
-	struct dj_device *dj_dev = hdev->driver_data;
-
-	if (!is_dj_device(dj_dev))
-		return -ENODEV;
-
-	ret = hid_parse(hdev);
-	if (!ret)
-		ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
-
-	return ret;
-}
-
 static const struct hid_device_id logi_dj_receivers[] = {
 	{HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
 		USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER)},
@@ -946,17 +930,14 @@ static struct hid_driver logi_djreceiver_driver = {
 
 
 static const struct hid_device_id logi_dj_devices[] = {
-	{HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
-		USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER)},
-	{HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
-		USB_DEVICE_ID_LOGITECH_UNIFYING_RECEIVER_2)},
+	{ HID_DEVICE(BUS_USB, HID_GROUP_LOGITECH_DJ_DEVICE_GENERIC,
+		USB_VENDOR_ID_LOGITECH, HID_ANY_ID)},
 	{}
 };
 
 static struct hid_driver logi_djdevice_driver = {
 	.name = "logitech-djdevice",
 	.id_table = logi_dj_devices,
-	.probe = logi_djdevice_probe,
 };
 
 
diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
index 4a40003..2e52167 100644
--- a/drivers/hid/hid-logitech-dj.h
+++ b/drivers/hid/hid-logitech-dj.h
@@ -111,14 +111,4 @@ struct dj_device {
 	u8 device_index;
 };
 
-/**
- * is_dj_device - know if the given dj_device is not the receiver.
- * @dj_dev: the dj device to test
- *
- * This macro tests if a struct dj_device pointer is a device created
- * by the bus enumarator.
- */
-#define is_dj_device(dj_dev) \
-	(&(dj_dev)->dj_receiver_dev->hdev->dev == (dj_dev)->hdev->dev.parent)
-
 #endif
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 31b9d29..eacf556 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -299,6 +299,7 @@ struct hid_item {
 #define HID_GROUP_MULTITOUCH			0x0002
 #define HID_GROUP_SENSOR_HUB			0x0003
 #define HID_GROUP_MULTITOUCH_WIN_8		0x0004
+#define HID_GROUP_LOGITECH_DJ_DEVICE_GENERIC	0x0005
 
 /*
  * This is the global environment of the parser. This information is
-- 
1.8.4.2

[PATCH 4/5] HID: logitech-dj: forward incoming HID++ reports to the correct dj device

[Benjamin Tissoires]

From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>

HID++ is a Logitech-specific protocol for communicating with HID
devices. DJ devices implement HID++, and so we can add the HID++
collection in the report descriptor and forward the incoming
reports from the receiver to the appropriate DJ device.

Signed-off-by: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-dj.c | 99 ++++++++++++++++++++++++++++++++++++++++---
 drivers/hid/hid-logitech-dj.h |  6 +++
 2 files changed, 99 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index a4b3cee..3444feb 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -152,6 +152,57 @@ static const char media_descriptor[] = {
 	0xc0,			/* EndCollection                       */
 };				/*                                     */
 
+/* HIDPP descriptor */
+static const char hidpp_descriptor[] = {
+	0x06, 0x00, 0xff,	/* Usage Page (Vendor Defined Page 1)  */
+	0x09, 0x01,		/* Usage (Vendor Usage 1)              */
+	0xa1, 0x01,		/* Collection (Application)            */
+	0x85, 0x10,		/*   Report ID (16)                    */
+	0x75, 0x08,		/*   Report Size (8)                   */
+	0x95, 0x06,		/*   Report Count (6)                  */
+	0x15, 0x00,		/*   Logical Minimum (0)               */
+	0x26, 0xff, 0x00,	/*   Logical Maximum (255)             */
+	0x09, 0x01,		/*   Usage (Vendor Usage 1)            */
+	0x81, 0x00,		/*   Input (Data,Arr,Abs)              */
+	0x09, 0x01,		/*   Usage (Vendor Usage 1)            */
+	0x91, 0x00,		/*   Output (Data,Arr,Abs)             */
+	0xc0,			/* End Collection                      */
+	0x06, 0x00, 0xff,	/* Usage Page (Vendor Defined Page 1)  */
+	0x09, 0x02,		/* Usage (Vendor Usage 2)              */
+	0xa1, 0x01,		/* Collection (Application)            */
+	0x85, 0x11,		/*   Report ID (17)                    */
+	0x75, 0x08,		/*   Report Size (8)                   */
+	0x95, 0x13,		/*   Report Count (19)                 */
+	0x15, 0x00,		/*   Logical Minimum (0)               */
+	0x26, 0xff, 0x00,	/*   Logical Maximum (255)             */
+	0x09, 0x02,		/*   Usage (Vendor Usage 2)            */
+	0x81, 0x00,		/*   Input (Data,Arr,Abs)              */
+	0x09, 0x02,		/*   Usage (Vendor Usage 2)            */
+	0x91, 0x00,		/*   Output (Data,Arr,Abs)             */
+	0xc0,			/* End Collection                      */
+	0x06, 0x00, 0xff,	/* Usage Page (Vendor Defined Page 1)  */
+	0x09, 0x04,		/* Usage (Vendor Usage 0x04)           */
+	0xa1, 0x01,		/* Collection (Application)            */
+	0x85, 0x20,		/*   Report ID (32)                    */
+	0x75, 0x08,		/*   Report Size (8)                   */
+	0x95, 0x0e,		/*   Report Count (14)                 */
+	0x15, 0x00,		/*   Logical Minimum (0)               */
+	0x26, 0xff, 0x00,	/*   Logical Maximum (255)             */
+	0x09, 0x41,		/*   Usage (Vendor Usage 0x41)         */
+	0x81, 0x00,		/*   Input (Data,Arr,Abs)              */
+	0x09, 0x41,		/*   Usage (Vendor Usage 0x41)         */
+	0x91, 0x00,		/*   Output (Data,Arr,Abs)             */
+	0x85, 0x21,		/*   Report ID (33)                    */
+	0x95, 0x1f,		/*   Report Count (31)                 */
+	0x15, 0x00,		/*   Logical Minimum (0)               */
+	0x26, 0xff, 0x00,	/*   Logical Maximum (255)             */
+	0x09, 0x42,		/*   Usage (Vendor Usage 0x42)         */
+	0x81, 0x00,		/*   Input (Data,Arr,Abs)              */
+	0x09, 0x42,		/*   Usage (Vendor Usage 0x42)         */
+	0x91, 0x00,		/*   Output (Data,Arr,Abs)             */
+	0xc0,			/* End Collection                      */
+};
+
 /* Maximum size of all defined hid reports in bytes (including report id) */
 #define MAX_REPORT_SIZE 8
 
@@ -161,7 +212,8 @@ static const char media_descriptor[] = {
 	 sizeof(mse_descriptor) +		\
 	 sizeof(consumer_descriptor) +		\
 	 sizeof(syscontrol_descriptor) +	\
-	 sizeof(media_descriptor))
+	 sizeof(media_descriptor) +	\
+	 sizeof(hidpp_descriptor))
 
 /* Number of possible hid report types that can be created by this driver.
  *
@@ -456,6 +508,25 @@ static void logi_dj_recv_forward_report(struct dj_receiver_dev *djrcv_dev,
 	}
 }
 
+static void logi_dj_recv_forward_hidpp(struct dj_receiver_dev *djrcv_dev,
+			u8 *data, int size)
+{
+	/* We are called from atomic context (tasklet && djrcv->lock held) */
+
+	struct dj_device *dj_dev = NULL;
+	u8 device_index = data[1];
+
+	if ((device_index < DJ_DEVICE_INDEX_MIN) ||
+	    (device_index > DJ_DEVICE_INDEX_MAX))
+		return;
+
+	dj_dev = djrcv_dev->paired_dj_devices[device_index];
+
+	if (!dj_dev)
+		return;
+
+	hid_input_report(dj_dev->hdev, HID_INPUT_REPORT, data, size, 1);
+}
 
 static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
 				    struct dj_report *dj_report)
@@ -610,6 +681,8 @@ static int logi_dj_ll_parse(struct hid_device *hid)
 			__func__, djdev->reports_supported);
 	}
 
+	rdcat(rdesc, &rsize, hidpp_descriptor, sizeof(hidpp_descriptor));
+
 	retval = hid_parse_report(hid, rdesc, rsize);
 	kfree(rdesc);
 
@@ -701,12 +774,12 @@ static int logi_dj_raw_event(struct hid_device *hdev,
 
 	dbg_hid("%s, size:%d\n", __func__, size);
 
-	/* Here we receive all data coming from iface 2, there are 4 cases:
+	/* Here we receive all data coming from iface 2, there are 5 cases:
 	 *
 	 * 1) Data should continue its normal processing i.e. data does not
-	 * come from the DJ collection, in which case we do nothing and
-	 * return 0, so hid-core can continue normal processing (will forward
-	 * to associated hidraw device)
+	 * come from the DJ or the HID++ collection, in which case we do nothing
+	 * and return 0, so hid-core can continue normal processing (will
+	 * forward to associated hidraw device)
 	 *
 	 * 2) Data is from DJ collection, and is intended for this driver i. e.
 	 * data contains arrival, departure, etc notifications, in which case
@@ -723,10 +796,17 @@ static int logi_dj_raw_event(struct hid_device *hdev,
 	 * a paired DJ device in which case we forward it to the correct hid
 	 * device (via hid_input_report() ) and return 1 so hid-core does not do
 	 * anything else with it.
+	 *
+	 * 5) Data is from the HID++ collection, in this case, we forward the
+	 * data to the corresponding child dj device and return 0 to hid-core
+	 * so he data also goes to the hidraw device of the receiver. This
+	 * allows a user space application to implement the full HID++ routing
+	 * via the receiver.
 	 */
 
 	spin_lock_irqsave(&djrcv_dev->lock, flags);
-	if (dj_report->report_id == REPORT_ID_DJ_SHORT) {
+	switch (data[0]) {
+	case REPORT_ID_DJ_SHORT:
 		switch (dj_report->report_type) {
 		case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
 		case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
@@ -742,6 +822,13 @@ static int logi_dj_raw_event(struct hid_device *hdev,
 			logi_dj_recv_forward_report(djrcv_dev, dj_report);
 		}
 		report_processed = true;
+		break;
+	case REPORT_ID_HIDPP_SHORT:
+		/* intentional fallthrough */
+	case REPORT_ID_HIDPP_LONG:
+		logi_dj_recv_forward_hidpp(djrcv_dev, data, size);
+		report_processed = false;
+		break;
 	}
 	spin_unlock_irqrestore(&djrcv_dev->lock, flags);
 
diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
index 2e52167..a805d44 100644
--- a/drivers/hid/hid-logitech-dj.h
+++ b/drivers/hid/hid-logitech-dj.h
@@ -36,6 +36,12 @@
 #define REPORT_ID_DJ_SHORT			0x20
 #define REPORT_ID_DJ_LONG			0x21
 
+#define REPORT_ID_HIDPP_SHORT			0x10
+#define REPORT_ID_HIDPP_LONG			0x11
+
+#define HIDPP_REPORT_SHORT_LENGTH		7
+#define HIDPP_REPORT_LONG_LENGTH		20
+
 #define REPORT_TYPE_RFREPORT_FIRST		0x01
 #define REPORT_TYPE_RFREPORT_LAST		0x1F
 
-- 
1.8.4.2

[PATCH 5/5] HID: logitech-dj: add .request callback

[Benjamin Tissoires]

From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>

The .request callback allows to send data from the dj device driver to the
device. Because of the DJ protocol, we only authorize HID++ communication
through this request.

The device index has to be overwritten by the receiver. All communication pass
through it, and the receiver is the only one to know which dj device has which
device index.
Furthermore, this allows to use the same calls from the driver point of view:
if a device is connected through a DJ interface, the receiver will overwrite
the device index, if it is connected through another bus (like bluetooth),
then the transport layer will not change the report, and it will be correctly
forwarded to the device.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-logitech-dj.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 3444feb..4335d21 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -624,6 +624,22 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
 	return 0;
 }
 
+static void logi_dj_ll_request(struct hid_device *hid, struct hid_report *rep,
+		int reqtype)
+{
+	struct dj_device *djdev = hid->driver_data;
+	struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
+
+	if ((rep->id != REPORT_ID_HIDPP_LONG) &&
+	    (rep->id != REPORT_ID_HIDPP_SHORT))
+		/* prevent forwarding of non acceptable reports */
+		return;
+
+	hid_set_field(rep->field[0], 0, djdev->device_index);
+
+	hid_hw_request(djrcv_dev->hdev, rep, reqtype);
+}
+
 static void rdcat(char *rdesc, unsigned int *rsize, const char *data, unsigned int size)
 {
 	memcpy(rdesc + *rsize, data, size);
@@ -759,6 +775,7 @@ static struct hid_ll_driver logi_dj_ll_driver = {
 	.stop = logi_dj_ll_stop,
 	.open = logi_dj_ll_open,
 	.close = logi_dj_ll_close,
+	.request = logi_dj_ll_request,
 	.hidinput_input_event = logi_dj_ll_input_event,
 };
 
-- 
1.8.4.2

Re: [PATCH 0/5] HID logitech DJ fixes and preps for enabling extended features

[Andrew de los Reyes]

In general, I'm positive on the change to fix the USB3 issue (yay!),
and for the others I'm happy it's going upstream. It seem to open up
the possibility of user-space drivers, which is great, even though we
don't need this on our team.

One thing I want to double-check: on some devices (T651, at least),
the raw data comes in not via HID++, but tacked onto the end of the
normal mouse reports. Will a driver for this device be able to get all
packets, not just HID++ ones? Sorry if this was clear and I missed it.

-andrew

On Wed, Jan 8, 2014 at 2:18 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Hi Jiri,
>
> Well, this work is _not_ for 3.14 (except maybe patch 1), especially since it is
> missing the biggest part where we enable the capabilities of Logitech devices.
>
> Long story short:
> This work is based on the work I did back in Summer 2011. I worked at Logitech
> for a few weeks to show up a demo of a driver for the Logitech Wireless Touchpad.
> At that time, a first draft has been done, but due to a lack of resources, noone
> upstreamed it.
> Since then, the code marinated a little at Logitech and in the ChromeOS kernel
> tree, but nobody tried to push it upstream. So here I am, trying to push this stuff
> upstream.
>
> I can not send the full series right now because I am lacking most of the
> testing hardware (I mean I only have the oldest Wireless Touchpad).
> Hopefully, I should receive some other soon, and I'll be able to send the second
> part then.
>
> Now, let me roughly explain the patches:
> - patch 1 can be applied right now I think, but it's entirely up to you Jiri.
>   This patch should fix the missing notifications with some USB 3.0 boards.
> - patches 2 to 5 allows to forward in both direction the proprietary protocol
>   used by Logitech (HID++ [1]) between the driver and the hardware.
> - later patches will introduce a transport layer for HID++ and also a driver
>   to support full multitouch on various Logitech touchpads.
>
> Nestor, Andrew, feel free to add your "Signed-off-by" whereever you want, I lost
> a little bit the track of who added what.
>
> Cheers,
> Benjamin
>
> [1] HID++: Documentation is provided at
> https://drive.google.com/a/logitech.com/?tab=mo#folders/0BxbRzx7vEV7eWmgwazJ3NUFfQ28
>
> Benjamin Tisssoires (5):
>   HID: logitech-dj: Fix USB 3.0 issue
>   HID: core: do not scan reports if the group is already set
>   HID: logitech-dj: rely on hid groups to separate receivers from dj
>     devices
>   HID: logitech-dj: forward incoming HID++ reports to the correct dj
>     device
>   HID: logitech-dj: add .request callback
>
>  drivers/hid/hid-core.c        |   3 +-
>  drivers/hid/hid-logitech-dj.c | 161 +++++++++++++++++++++++++++++++++---------
>  drivers/hid/hid-logitech-dj.h |  16 ++---
>  include/linux/hid.h           |   1 +
>  4 files changed, 136 insertions(+), 45 deletions(-)
>
> --
> 1.8.4.2
>
> --
> 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/5] HID logitech DJ fixes and preps for enabling extended features

[Benjamin Tissoires]

On 09/01/14 16:08, Andrew de los Reyes wrote:
> In general, I'm positive on the change to fix the USB3 issue (yay!),
> and for the others I'm happy it's going upstream. It seem to open up
> the possibility of user-space drivers, which is great, even though we
> don't need this on our team.
> 
> One thing I want to double-check: on some devices (T651, at least),
> the raw data comes in not via HID++, but tacked onto the end of the
> normal mouse reports. Will a driver for this device be able to get all
> packets, not just HID++ ones? Sorry if this was clear and I missed it.

Yeah, that is partly why I can not send the rest of the series right
now. Nestor already warned me about those funny devices, so I need to
double check how to implement HID++.

On a technical point of view, a driver connected through the unifying
receiver currently only get the regular input reports, and this series
adds the HID++ reports to these ones. So yes, a device will receive all
reports dedicated to it.

Cheers,
Benjamin

> 
> -andrew
> 
> On Wed, Jan 8, 2014 at 2:18 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Hi Jiri,
>>
>> Well, this work is _not_ for 3.14 (except maybe patch 1), especially since it is
>> missing the biggest part where we enable the capabilities of Logitech devices.
>>
>> Long story short:
>> This work is based on the work I did back in Summer 2011. I worked at Logitech
>> for a few weeks to show up a demo of a driver for the Logitech Wireless Touchpad.
>> At that time, a first draft has been done, but due to a lack of resources, noone
>> upstreamed it.
>> Since then, the code marinated a little at Logitech and in the ChromeOS kernel
>> tree, but nobody tried to push it upstream. So here I am, trying to push this stuff
>> upstream.
>>
>> I can not send the full series right now because I am lacking most of the
>> testing hardware (I mean I only have the oldest Wireless Touchpad).
>> Hopefully, I should receive some other soon, and I'll be able to send the second
>> part then.
>>
>> Now, let me roughly explain the patches:
>> - patch 1 can be applied right now I think, but it's entirely up to you Jiri.
>>   This patch should fix the missing notifications with some USB 3.0 boards.
>> - patches 2 to 5 allows to forward in both direction the proprietary protocol
>>   used by Logitech (HID++ [1]) between the driver and the hardware.
>> - later patches will introduce a transport layer for HID++ and also a driver
>>   to support full multitouch on various Logitech touchpads.
>>
>> Nestor, Andrew, feel free to add your "Signed-off-by" whereever you want, I lost
>> a little bit the track of who added what.
>>
>> Cheers,
>> Benjamin
>>
>> [1] HID++: Documentation is provided at
>> https://drive.google.com/a/logitech.com/?tab=mo#folders/0BxbRzx7vEV7eWmgwazJ3NUFfQ28
>>
>> Benjamin Tisssoires (5):
>>   HID: logitech-dj: Fix USB 3.0 issue
>>   HID: core: do not scan reports if the group is already set
>>   HID: logitech-dj: rely on hid groups to separate receivers from dj
>>     devices
>>   HID: logitech-dj: forward incoming HID++ reports to the correct dj
>>     device
>>   HID: logitech-dj: add .request callback
>>
>>  drivers/hid/hid-core.c        |   3 +-
>>  drivers/hid/hid-logitech-dj.c | 161 +++++++++++++++++++++++++++++++++---------
>>  drivers/hid/hid-logitech-dj.h |  16 ++---
>>  include/linux/hid.h           |   1 +
>>  4 files changed, 136 insertions(+), 45 deletions(-)
>>
>> --
>> 1.8.4.2
>>
>> --
>> 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 1/5] HID: logitech-dj: Fix USB 3.0 issue

[Jiri Kosina]

On Wed, 8 Jan 2014, Benjamin Tissoires wrote:

> From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
> 
> This fix (not very clean though) should fix the long time USB3
> issue that was spotted last year. The rational has been given by
> Hans de Goede:

Man this is so ugly ... makes one wonder how come that other OSes don't 
suffer from this. Are they really *that much* slower? :)

I have applied this (and just this) one for 3.14.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/5] HID: logitech-dj: Fix USB 3.0 issue

[Nestor Lopez Casado]

On Thu, Jan 16, 2014 at 10:50 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 8 Jan 2014, Benjamin Tissoires wrote:
>
>> From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
>>
>> This fix (not very clean though) should fix the long time USB3
>> issue that was spotted last year. The rational has been given by
>> Hans de Goede:
>
> Man this is so ugly ... makes one wonder how come that other OSes don't
> suffer from this. Are they really *that much* slower? :)
>
> I have applied this (and just this) one for 3.14.

This is clearly a workaround for an underlying bug.
Most probably the issue is in the receiver firmware, and the bug
happens to become apparent easier when it is plugged in a usb 3.0
port.
We are experiencing some issues when the receiver is plugged to usb
3.0 ports in other OSes as well, these could also be explained by the
same phenomenon of vendor requests being missed by the receiver. But
we dont have a clear understanding of the problem yet.

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
Cheers,
-nestor

Re: [PATCH 0/5] HID logitech DJ fixes and preps for enabling extended features

[Andrew de los Reyes]

Just curious, what is the status of these patches?

Benjamin, should I rebase Chrome OS's Logitech patches on top of these
and look at sending them upstream as well?

On Thu, Jan 9, 2014 at 1:22 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> On 09/01/14 16:08, Andrew de los Reyes wrote:
>> In general, I'm positive on the change to fix the USB3 issue (yay!),
>> and for the others I'm happy it's going upstream. It seem to open up
>> the possibility of user-space drivers, which is great, even though we
>> don't need this on our team.
>>
>> One thing I want to double-check: on some devices (T651, at least),
>> the raw data comes in not via HID++, but tacked onto the end of the
>> normal mouse reports. Will a driver for this device be able to get all
>> packets, not just HID++ ones? Sorry if this was clear and I missed it.
>
> Yeah, that is partly why I can not send the rest of the series right
> now. Nestor already warned me about those funny devices, so I need to
> double check how to implement HID++.
>
> On a technical point of view, a driver connected through the unifying
> receiver currently only get the regular input reports, and this series
> adds the HID++ reports to these ones. So yes, a device will receive all
> reports dedicated to it.
>
> Cheers,
> Benjamin
>
>>
>> -andrew
>>
>> On Wed, Jan 8, 2014 at 2:18 PM, Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:
>>> Hi Jiri,
>>>
>>> Well, this work is _not_ for 3.14 (except maybe patch 1), especially since it is
>>> missing the biggest part where we enable the capabilities of Logitech devices.
>>>
>>> Long story short:
>>> This work is based on the work I did back in Summer 2011. I worked at Logitech
>>> for a few weeks to show up a demo of a driver for the Logitech Wireless Touchpad.
>>> At that time, a first draft has been done, but due to a lack of resources, noone
>>> upstreamed it.
>>> Since then, the code marinated a little at Logitech and in the ChromeOS kernel
>>> tree, but nobody tried to push it upstream. So here I am, trying to push this stuff
>>> upstream.
>>>
>>> I can not send the full series right now because I am lacking most of the
>>> testing hardware (I mean I only have the oldest Wireless Touchpad).
>>> Hopefully, I should receive some other soon, and I'll be able to send the second
>>> part then.
>>>
>>> Now, let me roughly explain the patches:
>>> - patch 1 can be applied right now I think, but it's entirely up to you Jiri.
>>>   This patch should fix the missing notifications with some USB 3.0 boards.
>>> - patches 2 to 5 allows to forward in both direction the proprietary protocol
>>>   used by Logitech (HID++ [1]) between the driver and the hardware.
>>> - later patches will introduce a transport layer for HID++ and also a driver
>>>   to support full multitouch on various Logitech touchpads.
>>>
>>> Nestor, Andrew, feel free to add your "Signed-off-by" whereever you want, I lost
>>> a little bit the track of who added what.
>>>
>>> Cheers,
>>> Benjamin
>>>
>>> [1] HID++: Documentation is provided at
>>> https://drive.google.com/a/logitech.com/?tab=mo#folders/0BxbRzx7vEV7eWmgwazJ3NUFfQ28
>>>
>>> Benjamin Tisssoires (5):
>>>   HID: logitech-dj: Fix USB 3.0 issue
>>>   HID: core: do not scan reports if the group is already set
>>>   HID: logitech-dj: rely on hid groups to separate receivers from dj
>>>     devices
>>>   HID: logitech-dj: forward incoming HID++ reports to the correct dj
>>>     device
>>>   HID: logitech-dj: add .request callback
>>>
>>>  drivers/hid/hid-core.c        |   3 +-
>>>  drivers/hid/hid-logitech-dj.c | 161 +++++++++++++++++++++++++++++++++---------
>>>  drivers/hid/hid-logitech-dj.h |  16 ++---
>>>  include/linux/hid.h           |   1 +
>>>  4 files changed, 136 insertions(+), 45 deletions(-)
>>>
>>> --
>>> 1.8.4.2
>>>
>>> --
>>> 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/5] HID logitech DJ fixes and preps for enabling extended features

[Benjamin Tissoires]

Hi Andrew,

On Wed, Jan 29, 2014 at 1:21 PM, Andrew de los Reyes <adlr@chromium.org> wrote:
> Just curious, what is the status of these patches?

Currently, Jiri had taken only the USB3 fix. The rest is still under
review/testing.

>
> Benjamin, should I rebase Chrome OS's Logitech patches on top of these
> and look at sending them upstream as well?

I would say wait a little bit more :)
I received the testing devices 2 weeks ago, and I managed to get the
Wireless Touchpad, T650, T651 and TK820 working with them.
I still don't know the fate regarding the touch mice.

I put a WIP on my github repo here (these are not the final patches though):
https://github.com/bentiss/hid-logitech-dj/tree/for-whot (branch for-whot).

Still, I need to rework on the patch 5/5 of this series, because it's
not working with bluetooth devices. I will also have to check if I can
use David's latest patches regarding the transport layer.

So until it has been accepted upstream by Jiri, do not consider this
better than what you have in ChromeOS, it may still change, and you
will have to redo the work again and again :)

Cheers,
Benjamin

>
> On Thu, Jan 9, 2014 at 1:22 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> On 09/01/14 16:08, Andrew de los Reyes wrote:
>>> In general, I'm positive on the change to fix the USB3 issue (yay!),
>>> and for the others I'm happy it's going upstream. It seem to open up
>>> the possibility of user-space drivers, which is great, even though we
>>> don't need this on our team.
>>>
>>> One thing I want to double-check: on some devices (T651, at least),
>>> the raw data comes in not via HID++, but tacked onto the end of the
>>> normal mouse reports. Will a driver for this device be able to get all
>>> packets, not just HID++ ones? Sorry if this was clear and I missed it.
>>
>> Yeah, that is partly why I can not send the rest of the series right
>> now. Nestor already warned me about those funny devices, so I need to
>> double check how to implement HID++.
>>
>> On a technical point of view, a driver connected through the unifying
>> receiver currently only get the regular input reports, and this series
>> adds the HID++ reports to these ones. So yes, a device will receive all
>> reports dedicated to it.
>>
>> Cheers,
>> Benjamin
>>
>>>
>>> -andrew
>>>
>>> On Wed, Jan 8, 2014 at 2:18 PM, Benjamin Tissoires
>>> <benjamin.tissoires@redhat.com> wrote:
>>>> Hi Jiri,
>>>>
>>>> Well, this work is _not_ for 3.14 (except maybe patch 1), especially since it is
>>>> missing the biggest part where we enable the capabilities of Logitech devices.
>>>>
>>>> Long story short:
>>>> This work is based on the work I did back in Summer 2011. I worked at Logitech
>>>> for a few weeks to show up a demo of a driver for the Logitech Wireless Touchpad.
>>>> At that time, a first draft has been done, but due to a lack of resources, noone
>>>> upstreamed it.
>>>> Since then, the code marinated a little at Logitech and in the ChromeOS kernel
>>>> tree, but nobody tried to push it upstream. So here I am, trying to push this stuff
>>>> upstream.
>>>>
>>>> I can not send the full series right now because I am lacking most of the
>>>> testing hardware (I mean I only have the oldest Wireless Touchpad).
>>>> Hopefully, I should receive some other soon, and I'll be able to send the second
>>>> part then.
>>>>
>>>> Now, let me roughly explain the patches:
>>>> - patch 1 can be applied right now I think, but it's entirely up to you Jiri.
>>>>   This patch should fix the missing notifications with some USB 3.0 boards.
>>>> - patches 2 to 5 allows to forward in both direction the proprietary protocol
>>>>   used by Logitech (HID++ [1]) between the driver and the hardware.
>>>> - later patches will introduce a transport layer for HID++ and also a driver
>>>>   to support full multitouch on various Logitech touchpads.
>>>>
>>>> Nestor, Andrew, feel free to add your "Signed-off-by" whereever you want, I lost
>>>> a little bit the track of who added what.
>>>>
>>>> Cheers,
>>>> Benjamin
>>>>
>>>> [1] HID++: Documentation is provided at
>>>> https://drive.google.com/a/logitech.com/?tab=mo#folders/0BxbRzx7vEV7eWmgwazJ3NUFfQ28
>>>>
>>>> Benjamin Tisssoires (5):
>>>>   HID: logitech-dj: Fix USB 3.0 issue
>>>>   HID: core: do not scan reports if the group is already set
>>>>   HID: logitech-dj: rely on hid groups to separate receivers from dj
>>>>     devices
>>>>   HID: logitech-dj: forward incoming HID++ reports to the correct dj
>>>>     device
>>>>   HID: logitech-dj: add .request callback
>>>>
>>>>  drivers/hid/hid-core.c        |   3 +-
>>>>  drivers/hid/hid-logitech-dj.c | 161 +++++++++++++++++++++++++++++++++---------
>>>>  drivers/hid/hid-logitech-dj.h |  16 ++---
>>>>  include/linux/hid.h           |   1 +
>>>>  4 files changed, 136 insertions(+), 45 deletions(-)
>>>>
>>>> --
>>>> 1.8.4.2
>>>>
>>>> --
>>>> 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 1/5] HID: logitech-dj: Fix USB 3.0 issue

[Nestor Lopez Casado]

We discussed this issue with one of the receiver firmware developers today.

We do see a clear issue when more than one DJ command (those with
report id 0x20, 0x21) are sent to the receiver in quick sequence. The
second command can be trashed if the first is still under processing,
and there is no way for the host to know when the receiver has
actually finished processing the first DJ command.

Both the first and the second commands are ACKed at the USB level.

For hidpp commands (those with report id 0x10, 0x11) there is a
response sent from the receiver to the host, and the host should wait
for the response before sending a new command.

For some DJ commands (those with report id 0x20, 0x21) there is no
response at all, as is the case for the 0x80 (switch to dj mode with
keep alive)

So the ugly sleep accounts to workaround this issue on the receiver.

There may be a cleaner workaround that could be tested (I did not test
myself, but looking at the receiver code it does seem as a plausible
solution)

Invert the order in which the commands are sent to the receiver:

First send the CMD_GET_PAIRED_DEVICES (0x81) to the receiver, and wait
to get all the device paired notifications, (the last notification is
specially tagged), then send the CMD_SWITCH (0x80).
Using this mechanism we guarantee that the receiver never sees a
second command while still processing the previous one and no explicit
sleep will be required.

For me, the ugly sleep suffices, as long as we understand the root
cause of the problem.

Cheers,
-nestor

On Fri, Feb 14, 2014 at 2:32 PM, Nestor Lopez Casado
<nlopezcasad@logitech.com> wrote:
> We discussed this issue with one of the receiver firmware developers today.
>
> We do see a clear issue when more than one DJ command (those with report id
> 0x20, 0x21) are sent to the receiver in quick sequence. The second command
> can be trashed if the first is still under processing, and there is no way
> for the host to know when the receiver has actually finished processing the
> first DJ command.
>
> Both the first and the second commands are ACKed at the USB level.
>
> For hidpp commands (those with report id 0x10, 0x11) there is a response
> sent from the receiver to the host, and the host should wait for the
> response before sending a new command.
>
> For some DJ commands (those with report id 0x20, 0x21) there is no response
> at all, as is the case for the 0x80 (switch to dj mode with keep alive)
>
> So the ugly sleep accounts to workaround this issue on the receiver.
>
> There may be a cleaner workaround that could be tested (I did not test
> myself, but looking at the receiver code it does seem as a plausible
> solution)
>
> Invert the order in which the commands are sent to the receiver:
>
> First send the CMD_GET_PAIRED_DEVICES (0x81) to the receiver, and wait to
> get all the device paired notifications, (the last notification is specially
> tagged), then send the CMD_SWITCH (0x80).
> Using this mechanism we guarantee that the receiver never sees a second
> command while still processing the previous one and no explicit sleep will
> be required.
>
> For me, the ugly sleep suffices, as long as we understand the root cause of
> the problem.
>
> Cheers,
> -nestor
>
>
> On Fri, Jan 17, 2014 at 9:48 AM, Nestor Lopez Casado
> <nlopezcasad@logitech.com> wrote:
>>
>> On Thu, Jan 16, 2014 at 10:50 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>> > On Wed, 8 Jan 2014, Benjamin Tissoires wrote:
>> >
>> >> From: Benjamin Tisssoires <benjamin.tissoires@redhat.com>
>> >>
>> >> This fix (not very clean though) should fix the long time USB3
>> >> issue that was spotted last year. The rational has been given by
>> >> Hans de Goede:
>> >
>> > Man this is so ugly ... makes one wonder how come that other OSes don't
>> > suffer from this. Are they really *that much* slower? :)
>> >
>> > I have applied this (and just this) one for 3.14.
>>
>> This is clearly a workaround for an underlying bug.
>> Most probably the issue is in the receiver firmware, and the bug
>> happens to become apparent easier when it is plugged in a usb 3.0
>> port.
>> We are experiencing some issues when the receiver is plugged to usb
>> 3.0 ports in other OSes as well, these could also be explained by the
>> same phenomenon of vendor requests being missed by the receiver. But
>> we dont have a clear understanding of the problem yet.
>>
>> >
>> > Thanks,
>> >
>> > --
>> > Jiri Kosina
>> > SUSE Labs
>> Cheers,
>> -nestor
>
>