diff mbox

[1/1] HID: Fix missing Unifying device issue

Message ID 1348222894-6767-1-git-send-email-nlopezcasad@logitech.com (mailing list archive)
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

Nestor Lopez Casado Sept. 21, 2012, 10:21 a.m. UTC
This patch fixes an issue introduced after commit 4ea5454203d991ec

After that commit, hid-core silently discards any incoming packet
that arrives while any hid driver's probe function is being executed.

This broke the enumeration process of hid-logitech-dj, that must
receive control packets in-band with the mouse and keyboard
packets. Discarding mouse or keyboard data at the very begining is
usually fine, but it is not the case for control packets.

This patch forces a re-enumeration of the paired devices when a packet
arrives that comes from an unknown device.

Based on a patch originally written by Benjamin Tissoires.

Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com>
---

Hello Jiri, David,

      I suggest we include this patch as this solves the issue with the 
      Unifying devices while we decide the future of the lock in hid-core.

      Cheers,
      Nestor

 drivers/hid/hid-logitech-dj.c |   45 +++++++++++++++++++++++++++++++++++++++++
 drivers/hid/hid-logitech-dj.h |    1 +
 2 files changed, 46 insertions(+), 0 deletions(-)

Comments

Jiri Kosina Sept. 22, 2012, 7:50 a.m. UTC | #1
On Fri, 21 Sep 2012, Nestor Lopez Casado wrote:

> This patch fixes an issue introduced after commit 4ea5454203d991ec
> 
> After that commit, hid-core silently discards any incoming packet
> that arrives while any hid driver's probe function is being executed.
> 
> This broke the enumeration process of hid-logitech-dj, that must
> receive control packets in-band with the mouse and keyboard
> packets. Discarding mouse or keyboard data at the very begining is
> usually fine, but it is not the case for control packets.
> 
> This patch forces a re-enumeration of the paired devices when a packet
> arrives that comes from an unknown device.
> 
> Based on a patch originally written by Benjamin Tissoires.
> 
> Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com>

I am now applying it, will be pushing to Linus very soon, and am also 
marking it for stable 3.2+

Thanks to everyone involved.
Jiri Kosina Sept. 22, 2012, 8:47 p.m. UTC | #2
On Sat, 22 Sep 2012, Jiri Kosina wrote:

> > This patch fixes an issue introduced after commit 4ea5454203d991ec
> > 
> > After that commit, hid-core silently discards any incoming packet
> > that arrives while any hid driver's probe function is being executed.
> > 
> > This broke the enumeration process of hid-logitech-dj, that must
> > receive control packets in-band with the mouse and keyboard
> > packets. Discarding mouse or keyboard data at the very begining is
> > usually fine, but it is not the case for control packets.
> > 
> > This patch forces a re-enumeration of the paired devices when a packet
> > arrives that comes from an unknown device.
> > 
> > Based on a patch originally written by Benjamin Tissoires.
> > 
> > Signed-off-by: Nestor Lopez Casado <nlopezcasad@logitech.com>
> 
> I am now applying it, will be pushing to Linus very soon

Now in Linus' tree and marked for stable.

Thanks,
Josip Rodin Sept. 23, 2012, 10:23 a.m. UTC | #3
On Fri, Sep 21, 2012 at 12:21:34PM +0200, Nestor Lopez Casado wrote:
> This patch fixes an issue introduced after commit 4ea5454203d991ec
> 
> After that commit, hid-core silently discards any incoming packet
> that arrives while any hid driver's probe function is being executed.

I managed to test this now, on top of Linux 3.5, but it didn't fix my
keyboard. I still get the same sequence of messages with hid.debug=1:

+usb 5-2: new full-speed USB device number 3 using ohci_hcd
+drivers/hid/usbhid/hid-core.c: HID probe called for ifnum 0
+drivers/hid/hid-logitech-dj.c: logi_dj_probe called for ifnum 0
+drivers/hid/hid-logitech-dj.c: logi_dj_probe: ignoring ifnum 0
+drivers/hid/usbhid/hid-core.c: HID probe called for ifnum 1
+drivers/hid/hid-logitech-dj.c: logi_dj_probe called for ifnum 1
+drivers/hid/hid-logitech-dj.c: logi_dj_probe: ignoring ifnum 1
+drivers/hid/usbhid/hid-core.c: HID probe called for ifnum 2
+drivers/hid/hid-logitech-dj.c: logi_dj_probe called for ifnum 2
+drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report wValue=0x0110 wIndex=0x0002 wLength=7
+drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report wValue=0x0111 wIndex=0x0002 wLength=20
+drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report wValue=0x0120 wIndex=0x0002 wLength=15
+drivers/hid/usbhid/hid-core.c: submitting ctrl urb: Get_Report wValue=0x0121 wIndex=0x0002 wLength=32
+logitech-djreceiver 0003:046D:C52B.0005: claimed by neither input, hiddev nor hidraw
+logitech-djreceiver 0003:046D:C52B.0005: logi_dj_probe:hid_hw_start returned error
Josip Rodin Sept. 27, 2012, 7:04 a.m. UTC | #4
On Mon, Sep 24, 2012 at 11:30:28AM +0200, Nestor Lopez Casado wrote:
> Josip, this is a different issue from the one addressed with the patch.
> 
> 1) Can you try it on a 3.2 kernel ?

I can try that too, I'll let you know how it went.

(Unfortunately the machine is in the same room with a crib, so I don't
get a lot of time slots for testing. *shrug* :)

> 2) The problem you describe, does it happen all the time ?

Yes. The keyboard simply stopped working after I upgraded to Linux 3.2.
It works fine under 3.1 and earlier, and under Windows.
Josip Rodin Sept. 27, 2012, 6:56 p.m. UTC | #5
On Thu, Sep 27, 2012 at 09:04:26AM +0200, Josip Rodin wrote:
> On Mon, Sep 24, 2012 at 11:30:28AM +0200, Nestor Lopez Casado wrote:
> > Josip, this is a different issue from the one addressed with the patch.
> > 
> > 1) Can you try it on a 3.2 kernel ?
> 
> I can try that too, I'll let you know how it went.

Same thing, it doesn't work.
Nestor Lopez Casado Oct. 9, 2012, 10:15 a.m. UTC | #6
Hello Apostolos,

Your problem is unrelated to hid-logitech-dj. This seems an usb driver
issue. It seems that your motherboard has different usb controllers
for the rear and front ports. There may be an issue with the usb
drivers for the xhci controllers.

Cheers,
Nestor.

On Mon, Oct 8, 2012 at 9:42 PM, Apostolos Bartziokas <barz621@gmail.com> wrote:
>
> Nestor hello
>
> Sorry for bringing this up again but i tested the patch now that it
> reached the stable kernels and when i connect it to the rear panel of my
> motherboard i get:
>
> Oct 08 22:11:43 mainland kernel: usb 3-2: >USB disconnect, device number 2
> Oct 08 22:12:04 mainland kernel: usb 1-2: >new full-speed USB device
> number 3 using xhci_hcd
> Oct 08 22:12:05 mainland kernel: logitech-djreceiver 0003:046D:C52B.0009:
> >hiddev0,hidraw2: USB HID v1.11 Device [Logitech USB Receiver] on
> usb-0000:00:14.0-2/input2
> Oct 08 22:12:05 mainland kernel: logitech-djreceiver 0003:046D:C52B.0009:
> >logi_dj_probe:logi_dj_recv_query_paired_devices error:-32
> Oct 08 22:12:05 mainland kernel: logitech-djreceiver: probe of
> 0003:046D:C52B.0009 failed with error -32
> Oct 08 22:13:50 mainland kernel: usb 1-2: >USB disconnect, device number 3
> Oct 08 22:13:57 mainland kernel: usb 5-1.6: >new full-speed USB device
> number 3 using ehci_hcd
> Oct 08 22:13:58 mainland kernel: logitech-djreceiver 0003:046D:C52B.000C:
> >hiddev0,hidraw2: USB HID v1.11 Device [Logitech USB Receiver] on
> usb-0000:00:1a.0-1.6/input2
> Oct 08 22:14:01 mainland kernel: input: Logitech Unifying Device. Wireless
> PID:101a as
> /devices/pci0000:00/0000:00:1a.0/usb5/5-1/5-1.6/5-1.6:1.2/0003:046D:C52B.000C/input/input15
> Oct 08 22:14:01 mainland kernel: logitech-djdevice 0003:046D:C52B.000D:
> >input,hidraw3: USB HID v1.11 Mouse [Logitech Unifying Device. Wireless
> PID:101a] on usb-0000:00:1a.0-1.6:1
> Oct 08 22:14:17 mainland kernel: usb 5-1.6: >USB disconnect, device number
> 3
> Oct 08 22:14:20 mainland kernel: usb 1-2: >new full-speed USB device
> number 4 using xhci_hcd
> Oct 08 22:14:20 mainland kernel: logitech-djreceiver 0003:046D:C52B.0010:
> >hiddev0,hidraw2: USB HID v1.11 Device [Logitech USB Receiver] on
> usb-0000:00:14.0-2/input2
> Oct 08 22:14:20 mainland kernel: logitech-djreceiver 0003:046D:C52B.0010:
> >logi_dj_probe:logi_dj_recv_query_paired_devices error:-32
> Oct 08 22:14:20 mainland kernel: logitech-djreceiver: probe of
> 0003:046D:C52B.0010 failed with error -32
>
> When i use a usb3 port it doesn't move at all and if i use a usb2 port its
> slow. When i use the front panel it works flawlessly. Could this be related
> to my POS Intel motherboard?
>
> Kind regards and apologies if i raise a false alarm.
>
> Apostolos B.
> On 09/28/2012 10:57 AM, Nestor Lopez Casado wrote:
>
> Hello Josip,
>
> This really looks like a different problem from the one addressed with
> this patch.
>
> I'll try to work on it as soon as I find some time for that.
>
> Thanks,
> Nestor.
>
> On Thu, Sep 27, 2012 at 8:56 PM, Josip Rodin <joy@debbugs.entuzijast.net>
> wrote:
>>
>> On Thu, Sep 27, 2012 at 09:04:26AM +0200, Josip Rodin wrote:
>> > On Mon, Sep 24, 2012 at 11:30:28AM +0200, Nestor Lopez Casado wrote:
>> > > Josip, this is a different issue from the one addressed with the
>> > > patch.
>> > >
>> > > 1) Can you try it on a 3.2 kernel ?
>> >
>> > I can try that too, I'll let you know how it went.
>>
>> Same thing, it doesn't work.
>>
>> --
>>      2. That which causes joy or happiness.
>> --
>> 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
diff mbox

Patch

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 4d524b5..9500f2f 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -193,6 +193,7 @@  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)
@@ -233,6 +234,7 @@  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;
 	}
 
@@ -243,6 +245,12 @@  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",
@@ -306,6 +314,7 @@  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__);
 
@@ -338,6 +347,25 @@  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__);
 	}
 }
@@ -368,6 +396,12 @@  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;
 	}
 
@@ -398,6 +432,12 @@  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;
 	}
 
@@ -439,6 +479,10 @@  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;
@@ -450,6 +494,7 @@  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 fd28a5e..4a40003 100644
--- a/drivers/hid/hid-logitech-dj.h
+++ b/drivers/hid/hid-logitech-dj.h
@@ -101,6 +101,7 @@  struct dj_receiver_dev {
 	struct work_struct work;
 	struct kfifo notif_fifo;
 	spinlock_t lock;
+	bool querying_devices;
 };
 
 struct dj_device {