35022: hid-magicmouse broken
diff mbox

Message ID alpine.LNX.2.00.1105191801540.28291@pobox.suse.cz
State Accepted
Commit 23746a66d7d9e73402c68ef00d708796b97ebd72
Headers show

Commit Message

Jiri Kosina May 19, 2011, 4:03 p.m. UTC
On Thu, 19 May 2011, Chase Douglas wrote:

> >> [  195.491735] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
> >> [  222.693947] input: cndougla’s trackpad as
> >> /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.3/1-1.3.3/1-1.3.3:1.0/bluetooth/hci0/hci0:12/input16
> >> [  222.694119] magicmouse 0005:05AC:030E.0005: input,hidraw3: BLUETOOTH
> >> HID v1.60 Mouse [cndougla’s trackpad] on 50:63:13:90:AF:A4
> >> [  222.694128] hidp_output_raw_report, report_type: 2
> >> [  222.808111] session ffff88012d9b1200 param 0x02
> >> [  222.808198] hidp: returnign -EIO because of !output_report_success
> >> [  222.808209] magicmouse 0005:05AC:030E.0005: unable to request touch
> >> data (-5)
> >> [  222.809358] session ffff88012d9b1200 param 0x02
> >> [  222.810606] session ffff88012d9b1200 param 0x02
> >> [  222.813113] session ffff88012d9b1200 param 0x00
> >> [  223.045363] magicmouse: probe of 0005:05AC:030E.0005 failed with error -5
> > 
> > Ok, so what is happening here is that magicmouse_probe() sends the { 0xd7, 
> > 0x01 } feature report to the device, and it responds with 
> > HIDP_HSHK_ERR_INVALID_REPORT_ID.
> > 
> > That's why the _raw callback (correctly) returns error.
> > 
> > So the question to the driver authors is -- what is the point behind the { 
> > 0xd7, 0x01 } report? Clearly the device doesn't like it during probe 
> > because of invalid report ID.
> > And it never did, we just silently ignored the error with the older 
> > kernels.
> 
> The feature report is what flips the devices (magic mouse, magic
> trackpad) into multitouch mode. Without the report, the mouse will not
> emit the location of any touches on its surface, and the trackpad will
> only emit single touch data.
> 
> What do you think we should do? We can't get rid of the report. Should
> we silently ignore the error?

Hmm, I am afraid that it's the only option (apart from thowing the whole 
'waiting for ack' infrastructure away, which would be sad, as it's a good 
thing in principle).

So how about the patch below? :/ Could you please verify that it makes 
hid-magicmouse work again? Thanks.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes

The device reponds with 'invalid report id' when feature report switching it
into multitouch mode is sent to it.

This has been silently ignored before 0825411ade ("HID: bt: Wait for ACK
on Sent Reports"), but since this commit, it propagates -EIO from the _raw
callback .

So let the driver ignore -EIO as response to 0xd7,0x01 report, as that's
how the device reacts in normal mode.

Sad, but following reality.

This fixes https://bugzilla.kernel.org/show_bug.cgi?id=35022

Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/hid/hid-magicmouse.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

Comments

Chase Douglas May 19, 2011, 6:03 p.m. UTC | #1
On 05/19/2011 12:03 PM, Jiri Kosina wrote:
> On Thu, 19 May 2011, Chase Douglas wrote:
> 
>>>> [  195.491735] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
>>>> [  222.693947] input: cndougla’s trackpad as
>>>> /devices/pci0000:00/0000:00:1a.0/usb1/1-1/1-1.3/1-1.3.3/1-1.3.3:1.0/bluetooth/hci0/hci0:12/input16
>>>> [  222.694119] magicmouse 0005:05AC:030E.0005: input,hidraw3: BLUETOOTH
>>>> HID v1.60 Mouse [cndougla’s trackpad] on 50:63:13:90:AF:A4
>>>> [  222.694128] hidp_output_raw_report, report_type: 2
>>>> [  222.808111] session ffff88012d9b1200 param 0x02
>>>> [  222.808198] hidp: returnign -EIO because of !output_report_success
>>>> [  222.808209] magicmouse 0005:05AC:030E.0005: unable to request touch
>>>> data (-5)
>>>> [  222.809358] session ffff88012d9b1200 param 0x02
>>>> [  222.810606] session ffff88012d9b1200 param 0x02
>>>> [  222.813113] session ffff88012d9b1200 param 0x00
>>>> [  223.045363] magicmouse: probe of 0005:05AC:030E.0005 failed with error -5
>>>
>>> Ok, so what is happening here is that magicmouse_probe() sends the { 0xd7, 
>>> 0x01 } feature report to the device, and it responds with 
>>> HIDP_HSHK_ERR_INVALID_REPORT_ID.
>>>
>>> That's why the _raw callback (correctly) returns error.
>>>
>>> So the question to the driver authors is -- what is the point behind the { 
>>> 0xd7, 0x01 } report? Clearly the device doesn't like it during probe 
>>> because of invalid report ID.
>>> And it never did, we just silently ignored the error with the older 
>>> kernels.
>>
>> The feature report is what flips the devices (magic mouse, magic
>> trackpad) into multitouch mode. Without the report, the mouse will not
>> emit the location of any touches on its surface, and the trackpad will
>> only emit single touch data.
>>
>> What do you think we should do? We can't get rid of the report. Should
>> we silently ignore the error?
> 
> Hmm, I am afraid that it's the only option (apart from thowing the whole 
> 'waiting for ack' infrastructure away, which would be sad, as it's a good 
> thing in principle).
> 
> So how about the patch below? :/ Could you please verify that it makes 
> hid-magicmouse work again? Thanks.
> 
> 
> 
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes
> 
> The device reponds with 'invalid report id' when feature report switching it
> into multitouch mode is sent to it.
> 
> This has been silently ignored before 0825411ade ("HID: bt: Wait for ACK
> on Sent Reports"), but since this commit, it propagates -EIO from the _raw
> callback .
> 
> So let the driver ignore -EIO as response to 0xd7,0x01 report, as that's
> how the device reacts in normal mode.
> 
> Sad, but following reality.
> 
> This fixes https://bugzilla.kernel.org/show_bug.cgi?id=35022
> 
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
>  drivers/hid/hid-magicmouse.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 0ec91c1..a5eda4c 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -501,9 +501,17 @@ static int magicmouse_probe(struct hid_device *hdev,
>  	}
>  	report->size = 6;
>  
> +	/*
> +	 * The device reponds with 'invalid report id' when feature
> +	 * report switching it into multitouch mode is sent to it.
> +	 *
> +	 * This results in -EIO from the _raw low-level transport callback,
> +	 * but there seems to be no other way of switching the mode.
> +	 * Thus the super-ugly hacky success check below.
> +	 */
>  	ret = hdev->hid_output_raw_report(hdev, feature, sizeof(feature),
>  			HID_FEATURE_REPORT);
> -	if (ret != sizeof(feature)) {
> +	if (ret != -EIO) {
>  		hid_err(hdev, "unable to request touch data (%d)\n", ret);
>  		goto err_stop_hw;
>  	}

Seems to work for me :).

Reviewed-by: Chase Douglas <chase.douglas@canonical.com>

Thanks!
--
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
Jiri Kosina May 20, 2011, 8:28 a.m. UTC | #2
On Thu, 19 May 2011, Chase Douglas wrote:

> Seems to work for me :).

I have applied the patch with your Tested-by. Thanks,
Chase Douglas May 20, 2011, 1:04 p.m. UTC | #3
On 05/20/2011 04:28 AM, Jiri Kosina wrote:
> On Thu, 19 May 2011, Chase Douglas wrote:
> 
>> Seems to work for me :).
> 
> I have applied the patch with your Tested-by. Thanks,

Thanks Jiri!

Could you add 'Cc: stable@kernel.org' if it hasn't been merged yet? It
looks like .39 was just released without this patch...

--
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
Jiri Kosina May 23, 2011, 9:04 a.m. UTC | #4
On Fri, 20 May 2011, Chase Douglas wrote:

> >> Seems to work for me :).
> > 
> > I have applied the patch with your Tested-by. Thanks,
> 
> Thanks Jiri!
> 
> Could you add 'Cc: stable@kernel.org' if it hasn't been merged yet? It
> looks like .39 was just released without this patch...

I will be sending this patch to stable once it's in Linus tree (I will be 
sending pull request likely today).

Patch
diff mbox

diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 0ec91c1..a5eda4c 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -501,9 +501,17 @@  static int magicmouse_probe(struct hid_device *hdev,
 	}
 	report->size = 6;
 
+	/*
+	 * The device reponds with 'invalid report id' when feature
+	 * report switching it into multitouch mode is sent to it.
+	 *
+	 * This results in -EIO from the _raw low-level transport callback,
+	 * but there seems to be no other way of switching the mode.
+	 * Thus the super-ugly hacky success check below.
+	 */
 	ret = hdev->hid_output_raw_report(hdev, feature, sizeof(feature),
 			HID_FEATURE_REPORT);
-	if (ret != sizeof(feature)) {
+	if (ret != -EIO) {
 		hid_err(hdev, "unable to request touch data (%d)\n", ret);
 		goto err_stop_hw;
 	}