Input: trackpoint - force 3 buttons if 0 button is reported
diff mbox

Message ID 20180112064658.25019-1-aaron.ma@canonical.com
State Accepted
Headers show

Commit Message

Aaron Ma Jan. 12, 2018, 6:46 a.m. UTC
Lenovo introduced trackpoint compatible sticks with minimum PS/2 commands.
Some of these sticks with 3 buttons always return 0 when reading
extended button info, set it as 3 buttons to enable middle button.

Cc: stable@vger.kernel.org
Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
---
 drivers/input/mouse/trackpoint.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Dmitry Torokhov Jan. 12, 2018, 8:39 a.m. UTC | #1
On Fri, Jan 12, 2018 at 02:46:58PM +0800, Aaron Ma wrote:
> Lenovo introduced trackpoint compatible sticks with minimum PS/2 commands.
> Some of these sticks with 3 buttons always return 0 when reading
> extended button info, set it as 3 buttons to enable middle button.

Does it only happen with newer ELan/ALPS/NXP trackpoints, or also with
trackpoints advertising the original 0x010E version? What devices
respond with 0 as button info?

> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> ---
>  drivers/input/mouse/trackpoint.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> index 0871010f18d5..00c0d1706567 100644
> --- a/drivers/input/mouse/trackpoint.c
> +++ b/drivers/input/mouse/trackpoint.c
> @@ -383,6 +383,10 @@ int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
>  	if (trackpoint_read(ps2dev, TP_EXT_BTN, &button_info)) {
>  		psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons\n");
>  		button_info = 0x33;
> +	} else if (!button_info) {
> +		psmouse_warn(psmouse,
> +			"got no extended button data, assuming 3 buttons\n");
> +		button_info = 0x33;
>  	}
>  
>  	psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);
> -- 
> 2.14.3
>
Aaron Ma Jan. 12, 2018, 12:45 p.m. UTC | #2
Only this laptop had been confirmed is ALPS: 0102 – FF02 trackpoint, it
return 0 of extended button.
I saw the other device that returned 0, but don't have it now, so the ID
can not be checked.

ThinkPad always have 3 buttons installed, I suggest always set 0x33 for
button info when the button info returned 0.
--
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
Dmitry Torokhov Jan. 12, 2018, 6:18 p.m. UTC | #3
Hi Aaron,

On Fri, Jan 12, 2018 at 08:45:17PM +0800, Aaron Ma wrote:
> Only this laptop had been confirmed is ALPS: 0102 – FF02 trackpoint, it
> return 0 of extended button.
> I saw the other device that returned 0, but don't have it now, so the ID
> can not be checked.
> 
> ThinkPad always have 3 buttons installed, I suggest always set 0x33 for
> button info when the button info returned 0.

I am really uncomfortable at us chipping away all validations in
trackpont protocol. The newer trackpoints (Elan, ALPS, etc) will be
taken care of with my patch that will not even try to fetch extended
button data for 0x02 - 0x04 extended ID devices, but unless we have a
concrete example of 0x01 responding with 0 for button info I will not
apply this patch.

Thanks.
Aaron Ma Jan. 13, 2018, 2:34 a.m. UTC | #4
Will your patch go to stable kernel?
If yes, that's fine.
--
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
Dmitry Torokhov Jan. 16, 2018, 11:07 p.m. UTC | #5
On Sat, Jan 13, 2018 at 10:34:19AM +0800, Aaron Ma wrote:
> Will your patch go to stable kernel?
> If yes, that's fine.

Hmm, it looks like we need this patch after all as we do have screwy
Lenovos with apparently 0x01 devices reporting 0 buttons. From
https://bugzilla.kernel.org/show_bug.cgi?id=196253

 thinkpad_acpi: ThinkPad BIOS R0DET87W (1.87 ), EC unknown
 thinkpad_acpi: Lenovo ThinkPad E470, model 20H1004SGE

 psmouse serio2: trackpoint: IBM TrackPoint firmware: 0x0e, buttons: 0/0

I'll ask them to try my patch and if it is indeed 0x01 device I'll apply
your patch.

Thanks.
Ulrik De Bie Jan. 21, 2018, 8:10 p.m. UTC | #6
On Tue, Jan 16, 2018 at 03:07:13PM -0800, Dmitry Torokhov wrote:
Hi Dmitry,

> 
> On Sat, Jan 13, 2018 at 10:34:19AM +0800, Aaron Ma wrote:
> > Will your patch go to stable kernel?
> > If yes, that's fine.
> 
> Hmm, it looks like we need this patch after all as we do have screwy
> Lenovos with apparently 0x01 devices reporting 0 buttons. From
> https://bugzilla.kernel.org/show_bug.cgi?id=196253
> 
>  thinkpad_acpi: ThinkPad BIOS R0DET87W (1.87 ), EC unknown
>  thinkpad_acpi: Lenovo ThinkPad E470, model 20H1004SGE
> 
>  psmouse serio2: trackpoint: IBM TrackPoint firmware: 0x0e, buttons: 0/0
> 
> I'll ask them to try my patch and if it is indeed 0x01 device I'll apply
> your patch.

It is quite strange. In the past,for commit 293b915fd9be 
"Input: trackpoint - assume 3 buttons when buttons detection fails" there was
success reported for lenovo e470 and I see the same success report in that
bugzilla ticket.  I can also report success on lenovo e570.

But some seem to have problems, could it be that they are not including that
commit in the kernel they are running ? Or did a bios upgrade change the
behaviour ?
It would be best when we have a confirmation from a e470 owner that without the
proposed patch from Aaron the middle button does not work on his e470, and it
works with it.


Kind regards,
Ulrik
--
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
Dmitry Torokhov Jan. 21, 2018, 9:14 p.m. UTC | #7
On January 21, 2018 12:10:50 PM PST, ulrik.debie-os@e2big.org wrote:
>On Tue, Jan 16, 2018 at 03:07:13PM -0800, Dmitry Torokhov wrote:
>Hi Dmitry,
>
>> 
>> On Sat, Jan 13, 2018 at 10:34:19AM +0800, Aaron Ma wrote:
>> > Will your patch go to stable kernel?
>> > If yes, that's fine.
>> 
>> Hmm, it looks like we need this patch after all as we do have screwy
>> Lenovos with apparently 0x01 devices reporting 0 buttons. From
>> https://bugzilla.kernel.org/show_bug.cgi?id=196253
>> 
>>  thinkpad_acpi: ThinkPad BIOS R0DET87W (1.87 ), EC unknown
>>  thinkpad_acpi: Lenovo ThinkPad E470, model 20H1004SGE
>> 
>>  psmouse serio2: trackpoint: IBM TrackPoint firmware: 0x0e, buttons:
>0/0
>> 
>> I'll ask them to try my patch and if it is indeed 0x01 device I'll
>apply
>> your patch.
>
>It is quite strange. In the past,for commit 293b915fd9be 
>"Input: trackpoint - assume 3 buttons when buttons detection fails"
>there was
>success reported for lenovo e470 and I see the same success report in
>that
>bugzilla ticket.  I can also report success on lenovo e570.
>
>But some seem to have problems, could it be that they are not including
>that
>commit in the kernel they are running ? Or did a bios upgrade change
>the
>behaviour ?
>It would be best when we have a confirmation from a e470 owner that
>without the
>proposed patch from Aaron the middle button does not work on his e470,
>and it
>works with it.

I suppose Lenovo changed the firmware, because if you take a look at bugzilla link I provided there is a box that does not fail the command but indeed responds with 0 as number of buttons. It still reports device ID 0x010e.

Hi Ulrik,
Thanks.

Patch
diff mbox

diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index 0871010f18d5..00c0d1706567 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -383,6 +383,10 @@  int trackpoint_detect(struct psmouse *psmouse, bool set_properties)
 	if (trackpoint_read(ps2dev, TP_EXT_BTN, &button_info)) {
 		psmouse_warn(psmouse, "failed to get extended button data, assuming 3 buttons\n");
 		button_info = 0x33;
+	} else if (!button_info) {
+		psmouse_warn(psmouse,
+			"got no extended button data, assuming 3 buttons\n");
+		button_info = 0x33;
 	}
 
 	psmouse->private = kzalloc(sizeof(struct trackpoint_data), GFP_KERNEL);