Bug 120781

It looks like byd_detect() routine is not reliable enough; I wonder if
we need to combine byd_detect and byd_init into one. It would be
interesting to know at what step byd_reset_touchpad() fails with this
HP mouse.

On Thu, Jun 23, 2016 at 12:08:52AM +0100, Chris Diamand wrote:
> Hi,
> 
> > I'll take a look at this tonight.
> 
> Thanks Richard.
> 
> >> It looks like byd_detect() routine is not reliable enough; I wonder if
> >> we need to combine byd_detect and byd_init into one. It would be
> >> interesting to know at what step byd_reset_touchpad() fails with this
> >> HP mouse.
> 
> If I've read through psmouse_extensions() correctly, one of two things is
> happening, depending on what the error actually is here:
> 
> - The BYD driver is being used instead of the correct one, meaning that
>   the whole of byd_init() is passing for some other device.
> 
> - OR: byd_detect() passes, but byd_init() fails. This means it defaults to
>   PSMOUSE_PS2, but the ps2bare_detect() doesn't overwrite the previously-set
>   vendor and name. This would mean that the name is wrong, and the HP mouse
>   would get the "PS/2 bare" driver.
> 
> Hopefully it's case (2), in which case moving byd_reset_touchpad() into
> byd_detect() sounds like a good plan as Dmitry says. If case (1), then we'd
> be in more trouble...

Yes, it is case 2: if you look at how the device is identified it says
"ImPS/2 BYD Touchpad" which means that byd_detect() succeeded,
byd_init() failed and we limited the protocol selection to PSMOUSE_IMEX
and then settled on Intellimouse protocol.

Thanks.

Hi,

So I've spelunked through the same code and have come to the same 
conclusion as Dmitry.  Presumably the bug filer would have mentioned 
that the mouse doesn't work at all along with the name being wrong.

So, looking at the 'secret handshake' used (set res x4, get info), it 
really isn't that surprising that byd_detect should also be giving false 
positives, seeings how any generic PS/2 mouse should respond in exactly 
the same way.

The 'Status Request' command just returns 3 bytes:

0 : button states and some other flags (we disregard this byte)
1 : resolution (we expect 0x03 here, which is also to be expected since 
we just set resolution to 0x03 four times)
2 : sample rate (we expect 0x64 here, which coincides with 100 
samples/sec which seems like a reasonable number for a generic mouse).

The BYD documentation itself ( 
http://bydit.com/userfiles/file/BTP10463-XXX.pdf page 4) doesn't even 
say those bytes are useful.

I think the best fix here is to replace that 'secret handshake' with a 
single call to byd_reset_touchpad.

Let me know what you think, and I'll post a patch to the list asap.

thanks,
-Richard


On 06/22/2016 04:24 PM, Dmitry Torokhov wrote:
> On Thu, Jun 23, 2016 at 12:08:52AM +0100, Chris Diamand wrote:
>> Hi,
>>
>>> I'll take a look at this tonight.
>> Thanks Richard.
>>
>>>> It looks like byd_detect() routine is not reliable enough; I wonder if
>>>> we need to combine byd_detect and byd_init into one. It would be
>>>> interesting to know at what step byd_reset_touchpad() fails with this
>>>> HP mouse.
>> If I've read through psmouse_extensions() correctly, one of two things is
>> happening, depending on what the error actually is here:
>>
>> - The BYD driver is being used instead of the correct one, meaning that
>>    the whole of byd_init() is passing for some other device.
>>
>> - OR: byd_detect() passes, but byd_init() fails. This means it defaults to
>>    PSMOUSE_PS2, but the ps2bare_detect() doesn't overwrite the
>>    previously-set
>>    vendor and name. This would mean that the name is wrong, and the HP mouse
>>    would get the "PS/2 bare" driver.
>>
>> Hopefully it's case (2), in which case moving byd_reset_touchpad() into
>> byd_detect() sounds like a good plan as Dmitry says. If case (1), then we'd
>> be in more trouble...
> Yes, it is case 2: if you look at how the device is identified it says
> "ImPS/2 BYD Touchpad" which means that byd_detect() succeeded,
> byd_init() failed and we limited the protocol selection to PSMOUSE_IMEX
> and then settled on Intellimouse protocol.
>
> Thanks.
>

On Wed, Jun 22, 2016 at 08:21:17PM -0700, Richard Pospesel wrote:
> The 'Status Request' command just returns 3 bytes:
> 
> 0 : button states and some other flags (we disregard this byte)
> 1 : resolution (we expect 0x03 here, which is also to be expected
> since we just set resolution to 0x03 four times)
> 2 : sample rate (we expect 0x64 here, which coincides with 100
> samples/sec which seems like a reasonable number for a generic
> mouse).
> 
> The BYD documentation itself (
> http://bydit.com/userfiles/file/BTP10463-XXX.pdf page 4) doesn't
> even say those bytes are useful.

Actually, it does say they are useful, we simply not using the right
ones:

"It is noteworthy that it is expected the get the value of “01” in the
first byte data when using BYD TouchPad if the above handshaking
protocol is executed, and the first byte data is the only data needed to
be checked."

> 
> I think the best fix here is to replace that 'secret handshake' with
> a single call to byd_reset_touchpad.

I'd prefer we kept handshake still. Maybe we should start by trying to
add test for 01 in byd_detect():

	if (param[0] != 0x01 || param[1] != 0x03 || param[2] != 0x64)
		return -ENODEV;

Thanks.

Right sorry I neglected to mention that the comments in the init method
mention that the 1st byte isn't a reliable indicator.

Also, a value of 0x01 could also be received if, during the init
sequence, the 'right' mouse button is pressed then the value will also
be returned as 0x01 by a generic PS/2 mouse.

thanks,
-Richard

On 06/23/2016 10:42 AM, Dmitry Torokhov wrote:
> On Wed, Jun 22, 2016 at 08:21:17PM -0700, Richard Pospesel wrote:
>> The 'Status Request' command just returns 3 bytes:
>>
>> 0 : button states and some other flags (we disregard this byte)
>> 1 : resolution (we expect 0x03 here, which is also to be expected
>> since we just set resolution to 0x03 four times)
>> 2 : sample rate (we expect 0x64 here, which coincides with 100
>> samples/sec which seems like a reasonable number for a generic
>> mouse).
>>
>> The BYD documentation itself (
>> http://bydit.com/userfiles/file/BTP10463-XXX.pdf page 4) doesn't
>> even say those bytes are useful.
>
> Actually, it does say they are useful, we simply not using the right
> ones:
>
> "It is noteworthy that it is expected the get the value of “01” in the
> first byte data when using BYD TouchPad if the above handshaking
> protocol is executed, and the first byte data is the only data needed to
> be checked."
>
>>
>> I think the best fix here is to replace that 'secret handshake' with
>> a single call to byd_reset_touchpad.
>
> I'd prefer we kept handshake still. Maybe we should start by trying to
> add test for 01 in byd_detect():
>
>       if (param[0] != 0x01 || param[1] != 0x03 || param[2] != 0x64)
>               return -ENODEV;
>
> Thanks.
>

Well I've just confirmed that the first byte the BYD touchpad is not a 
set value, and varies exactly as hypothesized (ie 0x01 with right button 
is pressed, 0x04 when left button is pressed, 0x00 otherwise).  The 
'secret' knock provides no value and behaves exactly as any generic PS/2.

Replacing with an attempt to reset seems likes the best option to 
guarantee correct identification.

-Richard

On 06/23/2016 06:47 PM, Richard Pospesel wrote:
> Right sorry I neglected to mention that the comments in the init method
> mention that the 1st byte isn't a reliable indicator.
>
> Also, a value of 0x01 could also be received if, during the init
> sequence, the 'right' mouse button is pressed then the value will also
> be returned as 0x01 by a generic PS/2 mouse.
>
> thanks,
> -Richard
>
> On 06/23/2016 10:42 AM, Dmitry Torokhov wrote:
>> On Wed, Jun 22, 2016 at 08:21:17PM -0700, Richard Pospesel wrote:
>>> The 'Status Request' command just returns 3 bytes:
>>>
>>> 0 : button states and some other flags (we disregard this byte)
>>> 1 : resolution (we expect 0x03 here, which is also to be expected
>>> since we just set resolution to 0x03 four times)
>>> 2 : sample rate (we expect 0x64 here, which coincides with 100
>>> samples/sec which seems like a reasonable number for a generic
>>> mouse).
>>>
>>> The BYD documentation itself (
>>> http://bydit.com/userfiles/file/BTP10463-XXX.pdf page 4) doesn't
>>> even say those bytes are useful.
>>
>> Actually, it does say they are useful, we simply not using the right
>> ones:
>>
>> "It is noteworthy that it is expected the get the value of “01” in the
>> first byte data when using BYD TouchPad if the above handshaking
>> protocol is executed, and the first byte data is the only data needed to
>> be checked."
>>
>>>
>>> I think the best fix here is to replace that 'secret handshake' with
>>> a single call to byd_reset_touchpad.
>>
>> I'd prefer we kept handshake still. Maybe we should start by trying to
>> add test for 01 in byd_detect():
>>
>>     if (param[0] != 0x01 || param[1] != 0x03 || param[2] != 0x64)
>>         return -ENODEV;
>>
>> Thanks.
>>

*** Bug 121281 has been marked as a duplicate of this bug. ***

Linking to a patch lingering in Patchwork: https://patchwork.kernel.org/patch/9204273/

Didn't see this mentioned in a list of regressions here:
https://lkml.org/lkml/2016/9/18/117

(In reply to Mikhail Zabaluev from comment #9)
> Didn't see this mentioned in a list of regressions here:
> https://lkml.org/lkml/2016/9/18/117

BTW, should there be a tag or a metabug to track regressions _in_ Bugzilla?

(In reply to Mikhail Zabaluev from comment #9)
> Didn't see this mentioned in a list of regressions here:
> https://lkml.org/lkml/2016/9/18/117

That one is only for regressions from 4.7->4.8, hence this problems afaics will never make the list; nobody tracks regressions for older kernels. I hope we can change this sooner or later, but for now that's how it is, sorry.

(In reply to Mikhail Zabaluev from comment #10)
> BTW, should there be a tag or a metabug to track regressions _in_ Bugzilla?
That's possible, but not done right now, as it would need someone to keep an eye on things. And a lot (most?) regressions are not tracked in bugzilla, so mayb bugzilla is the wrong place/tool. I plan to thing more about this sooner or later and do something about it, but one step at a time...

Could anyone with a BYD touchpad try 00 as the setres byte during the handshake?

There's 5 specs for 5 touchpads on the BYD website.

Based on the BYD documents:
BTP6034 and BTP6740 have the handshake E8/00/E8/00/E8/00/E9
The other 3 BTP8644, BTP10463 and BTP11484 have the handshake E8/03/E8/03/E8/03/E9.

The driver only tries the handshake with 03, trying with 00 you might get a successful actual handshake then work from there.

The regression appears to be gone as of kernel 4.8.14.
Observed in Fedora 25, kernel 4.8.14-300.fc25.x86_64 on VirtualBox 5.1.10.

We are no longer automatically probe BYD protocol, so the mouse should stay detected as mouse now.

And my ALPS Touchpad now detects properly as 

input: AlpsPS/2 ALPS DualPoint Stick as /devices/platform/i8042/serio1/input/input7
input: AlpsPS/2 ALPS DualPoint TouchPad as /devices/platform/i8042/serio1/input/input6

I can confirm Linux 4.9.x now detects the touchpad properly. This has had some unfortunate effects:

* The touchpad now randomly stops working for a few seconds every now and then. (No relation to typing on the keyboard.)
* The sensitivity is strange.
* The trackpoint is so extremely oversensitive that a slightly slant table causes the pointer to move in one direction constantly. It jumps even if somebody just walks around me.

Oh god, what did I wish for? :-(