[PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes

[PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes

[Jaikumar Ganesh]

From: Jiri Kosina <jkosina@suse.cz>

23746a introduced this commit but was reverted in c3a492.
The trackpad uses report Ids which are not present in the
report descriptor. These reports ids are not documented
anywhere. There are devices in the market (Apple magic tracpkad,
BT version 2.0 is one such device) with the same device id,
which fail when we use 0xd7 as the report id. So we need the EIO
change of 23746a as a failsafe to work with these devices.

Original Author: Jiri Kosina <jkosina@suse.cz>

Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com>
---
 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;
 	}
-- 
1.7.3.1

Re: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes

[Chase Douglas]

On 08/17/2011 12:59 PM, Jaikumar Ganesh wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> 
> 23746a introduced this commit but was reverted in c3a492.
> The trackpad uses report Ids which are not present in the
> report descriptor. These reports ids are not documented
> anywhere. There are devices in the market (Apple magic tracpkad,
> BT version 2.0 is one such device) with the same device id,
> which fail when we use 0xd7 as the report id. So we need the EIO
> change of 23746a as a failsafe to work with these devices.
> 
> Original Author: Jiri Kosina <jkosina@suse.cz>
> 
> Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com>

I worry this may just be papering over a bug elsewhere in the system
again. I'm going to try to git bisect this today.

-- Chase

Re: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes

[Chase Douglas]

On 08/18/2011 09:36 AM, Jaikumar Ganesh wrote:
> Hi Chase
> 
> On Thu, Aug 18, 2011 at 9:25 AM, Chase Douglas
> <chase.douglas@canonical.com <mailto:chase.douglas@canonical.com>> wrote:
> 
>     On 08/17/2011 12:59 PM, Jaikumar Ganesh wrote:
>     > From: Jiri Kosina <jkosina@suse.cz <mailto:jkosina@suse.cz>>
>     >
>     > 23746a introduced this commit but was reverted in c3a492.
>     > The trackpad uses report Ids which are not present in the
>     > report descriptor. These reports ids are not documented
>     > anywhere. There are devices in the market (Apple magic tracpkad,
>     > BT version 2.0 is one such device) with the same device id,
>     > which fail when we use 0xd7 as the report id. So we need the EIO
>     > change of 23746a as a failsafe to work with these devices.
>     >
>     > Original Author: Jiri Kosina <jkosina@suse.cz
>     <mailto:jkosina@suse.cz>>
>     >
>     > Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com
>     <mailto:jaikumarg@android.com>>
> 
>     I worry this may just be papering over a bug elsewhere in the system
>     again. I'm going to try to git bisect this today.
> 
>  
> The original commit explains in detail why it used to work before, so
> its not a regression in that sense. The commit was present in 2.6.39
> stable tree too but was reverted in 3.0 tree. 

It was reverted because it was papering over a bug in the bluetooth
stack, and when that bug was fixed this commit broke things again.

I've determined that there's a bug between Ubuntu's 3.0-1.2 and 3.0-2.3
kernels. There does not appear to be any Ubuntu specific patches that
went in between those versions that would affect this, but there was a
rebase from upstream 3.0-rc3 to 3.0-rc4. There is likely a bug
introduced sometime in there.

I'll be digging deeper...

-- Chase

Re: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes

[Chase Douglas]

On 08/18/2011 09:48 AM, Chase Douglas wrote:
> On 08/18/2011 09:36 AM, Jaikumar Ganesh wrote:
>> Hi Chase
>>
>> On Thu, Aug 18, 2011 at 9:25 AM, Chase Douglas
>> <chase.douglas@canonical.com <mailto:chase.douglas@canonical.com>> wrote:
>>
>>     On 08/17/2011 12:59 PM, Jaikumar Ganesh wrote:
>>     > From: Jiri Kosina <jkosina@suse.cz <mailto:jkosina@suse.cz>>
>>     >
>>     > 23746a introduced this commit but was reverted in c3a492.
>>     > The trackpad uses report Ids which are not present in the
>>     > report descriptor. These reports ids are not documented
>>     > anywhere. There are devices in the market (Apple magic tracpkad,
>>     > BT version 2.0 is one such device) with the same device id,
>>     > which fail when we use 0xd7 as the report id. So we need the EIO
>>     > change of 23746a as a failsafe to work with these devices.
>>     >
>>     > Original Author: Jiri Kosina <jkosina@suse.cz
>>     <mailto:jkosina@suse.cz>>
>>     >
>>     > Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com
>>     <mailto:jaikumarg@android.com>>
>>
>>     I worry this may just be papering over a bug elsewhere in the system
>>     again. I'm going to try to git bisect this today.
>>
>>  
>> The original commit explains in detail why it used to work before, so
>> its not a regression in that sense. The commit was present in 2.6.39
>> stable tree too but was reverted in 3.0 tree. 
> 
> It was reverted because it was papering over a bug in the bluetooth
> stack, and when that bug was fixed this commit broke things again.
> 
> I've determined that there's a bug between Ubuntu's 3.0-1.2 and 3.0-2.3
> kernels. There does not appear to be any Ubuntu specific patches that
> went in between those versions that would affect this, but there was a
> rebase from upstream 3.0-rc3 to 3.0-rc4. There is likely a bug
> introduced sometime in there.
> 
> I'll be digging deeper...

After some more digging I realized that the bluetooth fix for some
devices went in at 3.0-rc4, just as the reversion of this patch went in.
Thus, my bisect of ubuntu kernels was unhelpful.

I decided to try a few tests:

3.0-rc0 kernel with the bluetooth fix and this patch reverted
2.6.39 kernel with the bluetooth fix (this patch was not committed yet)

Both failed to properly initialize the magic trackpad. The only
conclusion I can come up with is that there are some hardware
differences between the machine I am currently testing with, and the
machine I previously had the magicmouse working with.

If that's the case, the safest thing to do would be to apply this patch
with a tweak. Don't check only that -EIO is returned. Check for either
success or -EIO is returned. This should allow good and bad behaving
hardware to still function.

Jiri, do you want me to send a rework of the patch, or do you want to do
it yourself?

-- Chase

Re: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes

[Jaikumar Ganesh]

Chase:

On Thu, Aug 18, 2011 at 12:32 PM, Chase Douglas
<chase.douglas@canonical.com> wrote:
>
> On 08/18/2011 09:48 AM, Chase Douglas wrote:
> > On 08/18/2011 09:36 AM, Jaikumar Ganesh wrote:
> >> Hi Chase
> >>
> >> On Thu, Aug 18, 2011 at 9:25 AM, Chase Douglas
> >> <chase.douglas@canonical.com <mailto:chase.douglas@canonical.com>> wrote:
> >>
> >>     On 08/17/2011 12:59 PM, Jaikumar Ganesh wrote:
> >>     > From: Jiri Kosina <jkosina@suse.cz <mailto:jkosina@suse.cz>>
> >>     >
> >>     > 23746a introduced this commit but was reverted in c3a492.
> >>     > The trackpad uses report Ids which are not present in the
> >>     > report descriptor. These reports ids are not documented
> >>     > anywhere. There are devices in the market (Apple magic tracpkad,
> >>     > BT version 2.0 is one such device) with the same device id,
> >>     > which fail when we use 0xd7 as the report id. So we need the EIO
> >>     > change of 23746a as a failsafe to work with these devices.
> >>     >
> >>     > Original Author: Jiri Kosina <jkosina@suse.cz
> >>     <mailto:jkosina@suse.cz>>
> >>     >
> >>     > Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com
> >>     <mailto:jaikumarg@android.com>>
> >>
> >>     I worry this may just be papering over a bug elsewhere in the system
> >>     again. I'm going to try to git bisect this today.
> >>
> >>
> >> The original commit explains in detail why it used to work before, so
> >> its not a regression in that sense. The commit was present in 2.6.39
> >> stable tree too but was reverted in 3.0 tree.
> >
> > It was reverted because it was papering over a bug in the bluetooth
> > stack, and when that bug was fixed this commit broke things again.
> >
> > I've determined that there's a bug between Ubuntu's 3.0-1.2 and 3.0-2.3
> > kernels. There does not appear to be any Ubuntu specific patches that
> > went in between those versions that would affect this, but there was a
> > rebase from upstream 3.0-rc3 to 3.0-rc4. There is likely a bug
> > introduced sometime in there.
> >
> > I'll be digging deeper...
>
> After some more digging I realized that the bluetooth fix for some
> devices went in at 3.0-rc4, just as the reversion of this patch went in.
> Thus, my bisect of ubuntu kernels was unhelpful.
>
> I decided to try a few tests:
>
> 3.0-rc0 kernel with the bluetooth fix and this patch reverted
> 2.6.39 kernel with the bluetooth fix (this patch was not committed yet)
>
> Both failed to properly initialize the magic trackpad. The only
> conclusion I can come up with is that there are some hardware
> differences between the machine I am currently testing with, and the
> machine I previously had the magicmouse working with.
>
> If that's the case, the safest thing to do would be to apply this patch
> with a tweak. Don't check only that -EIO is returned. Check for either
> success or -EIO is returned. This should allow good and bad behaving
> hardware to still function.

I also tracked down two Apple magic trackpads - both with the same
Bluetooth version.
One which needs this patch (as it returns an invalid report id) and
the other which doesn't need this patch.
Both use the same device id.

>
> Jiri, do you want me to send a rework of the patch, or do you want to do
> it yourself?
>
> -- Chase
--
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] HID: magicmouse: ignore 'ivalid report id' while switching modes

[Jiri Kosina]

On Thu, 18 Aug 2011, Jaikumar Ganesh wrote:

> I also tracked down two Apple magic trackpads - both with the same
> Bluetooth version.
> One which needs this patch (as it returns an invalid report id) and
> the other which doesn't need this patch.
> Both use the same device id.

Gotta love hardware vendors indeed.

So how about the patch below?



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

This is basically a more generic respin of 23746a6 ("HID: magicmouse: ignore
'ivalid report id' while switching modes") which got reverted later by
c3a492.

It turns out that on some configurations, this is actually still the case
and we are not able to detect in runtime.

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

Reported-by: Chase Douglas <chase.douglas@canonical.com>
Reported-by: Jaikumar Ganesh <jaikumarg@android.com>
Tested-by: Chase Douglas <chase.douglas@canonical.com>
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 b5bdab3..f0fbd7b 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -537,9 +537,17 @@ static int magicmouse_probe(struct hid_device *hdev,
 	}
 	report->size = 6;
 
+	/*
+	 * Some devices repond 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 && ret != sizeof(feature)) {
 		hid_err(hdev, "unable to request touch data (%d)\n", ret);
 		goto err_stop_hw;
 	}
-- 
1.7.3.1

Re: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes

[Jaikumar Ganesh]

Hi Jiri,

On Tue, Aug 23, 2011 at 1:53 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 18 Aug 2011, Jaikumar Ganesh wrote:
>
>> I also tracked down two Apple magic trackpads - both with the same
>> Bluetooth version.
>> One which needs this patch (as it returns an invalid report id) and
>> the other which doesn't need this patch.
>> Both use the same device id.
>
> Gotta love hardware vendors indeed.
>
> So how about the patch below?
>
>
>
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes, v2
>
> This is basically a more generic respin of 23746a6 ("HID: magicmouse: ignore
> 'ivalid report id' while switching modes") which got reverted later by
> c3a492.
>
> It turns out that on some configurations, this is actually still the case
> and we are not able to detect in runtime.
>
> 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
>
> Reported-by: Chase Douglas <chase.douglas@canonical.com>
> Reported-by: Jaikumar Ganesh <jaikumarg@android.com>
> Tested-by: Chase Douglas <chase.douglas@canonical.com>
> 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 b5bdab3..f0fbd7b 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -537,9 +537,17 @@ static int magicmouse_probe(struct hid_device *hdev,
>        }
>        report->size = 6;
>
> +       /*
> +        * Some devices repond 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 && ret != sizeof(feature)) {
>                hid_err(hdev, "unable to request touch data (%d)\n", ret);
>                goto err_stop_hw;
>        }
> --
> 1.7.3.1
>
>

Tested. Please push.

Thanks
Jaikumar
--
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] HID: magicmouse: ignore 'ivalid report id' while switching modes

[Jiri Kosina]

On Tue, 23 Aug 2011, Jaikumar Ganesh wrote:

> > From: Jiri Kosina <jkosina@suse.cz>
> > Subject: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes, v2
> >
> > This is basically a more generic respin of 23746a6 ("HID: magicmouse: ignore
> > 'ivalid report id' while switching modes") which got reverted later by
> > c3a492.
> >
> > It turns out that on some configurations, this is actually still the case
> > and we are not able to detect in runtime.
> >
> > 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
> >
> > Reported-by: Chase Douglas <chase.douglas@canonical.com>
> > Reported-by: Jaikumar Ganesh <jaikumarg@android.com>
> > Tested-by: Chase Douglas <chase.douglas@canonical.com>
> > 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 b5bdab3..f0fbd7b 100644
> > --- a/drivers/hid/hid-magicmouse.c
> > +++ b/drivers/hid/hid-magicmouse.c
> > @@ -537,9 +537,17 @@ static int magicmouse_probe(struct hid_device *hdev,
> >        }
> >        report->size = 6;
> >
> > +       /*
> > +        * Some devices repond 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 && ret != sizeof(feature)) {
> >                hid_err(hdev, "unable to request touch data (%d)\n", ret);
> >                goto err_stop_hw;
> >        }
> > --
> > 1.7.3.1
> 
> Tested. Please push.

Okay, I have queued it with your Tested-by. Thanks,

-- 
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] HID: magicmouse: ignore 'ivalid report id' while switching modes

[Chase Douglas]

On 08/23/2011 01:53 AM, Jiri Kosina wrote:
> On Thu, 18 Aug 2011, Jaikumar Ganesh wrote:
> 
>> I also tracked down two Apple magic trackpads - both with the same
>> Bluetooth version.
>> One which needs this patch (as it returns an invalid report id) and
>> the other which doesn't need this patch.
>> Both use the same device id.
> 
> Gotta love hardware vendors indeed.
> 
> So how about the patch below?
> 
> 
> 
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] HID: magicmouse: ignore 'ivalid report id' while switching modes, v2

                                    spelling: ^

> 
> This is basically a more generic respin of 23746a6 ("HID: magicmouse: ignore
> 'ivalid report id' while switching modes") which got reverted later by
> c3a492.
> 
> It turns out that on some configurations, this is actually still the case
> and we are not able to detect in runtime.
> 
> 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
> 
> Reported-by: Chase Douglas <chase.douglas@canonical.com>
> Reported-by: Jaikumar Ganesh <jaikumarg@android.com>
> Tested-by: Chase Douglas <chase.douglas@canonical.com>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>

Looks good to me. I tested it and it fixed things here.

Thanks!

-- Chase