[SRU,B/C/D/OEM-OSP1-B/E,1/1] Input: alps - don't handle ALPS cs19 trackpoint-only device

BugLink: https://bugs.launchpad.net/bugs/1836752

On a latest Lenovo laptop, the trackpoint and 3 buttons below it
don't work at all, when we move the trackpoint or press those 3
buttons, the kernel will print out:
"Rejected trackstick packet from non DualPoint device"

This device is identified as an alps touchpad but the packet has
trackpoint format, so the alps.c drops the packet and prints out
the message above.

According to XiaoXiao's explanation, this device is named cs19 and
is trackpoint-only device, its firmware is only for trackpoint, it
is independent of touchpad and is a device completely different from
DualPoint ones.

To drive this device with mininal changes to the existing driver, we
just let the alps driver not handle this device, then the trackpoint.c
will be the driver of this device if the trackpoint driver is enabled.
(if not, this device will fallback to a bare PS/2 device)

With the trackpoint.c, this trackpoint and 3 buttons all work well,
they have all features that the trackpoint should have, like
scrolling-screen, drag-and-drop and frame-selection.

Signed-off-by: XiaoXiao Liu <sliuuxiaonxiao@gmail.com>
Signed-off-by: Hui Wang <hui.wang@canonical.com>
Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
(cherry picked from commit 7e4935ccc3236751e5fe4bd6846f86e46bb2e427
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git)
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 drivers/input/mouse/alps.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Cherry-picked from maintainer's tree, with positive test result.

Acked-by: Aaron Ma <aaron.ma@canonical.com>

osp1 doesn't have the luxury to wait for regression testing, so applied
there to meet the project deadline

On Tue, Jul 16, 2019 at 10:12:57PM +0800, Hui Wang wrote:
> BugLink: https://bugs.launchpad.net/bugs/1836752
> 
> On a latest Lenovo laptop, the trackpoint and 3 buttons below it
> don't work at all, when we move the trackpoint or press those 3
> buttons, the kernel will print out:
> "Rejected trackstick packet from non DualPoint device"
> 
> This device is identified as an alps touchpad but the packet has
> trackpoint format, so the alps.c drops the packet and prints out
> the message above.
> 
> According to XiaoXiao's explanation, this device is named cs19 and
> is trackpoint-only device, its firmware is only for trackpoint, it
> is independent of touchpad and is a device completely different from
> DualPoint ones.
> 
> To drive this device with mininal changes to the existing driver, we
> just let the alps driver not handle this device, then the trackpoint.c
> will be the driver of this device if the trackpoint driver is enabled.
> (if not, this device will fallback to a bare PS/2 device)
> 
> With the trackpoint.c, this trackpoint and 3 buttons all work well,
> they have all features that the trackpoint should have, like
> scrolling-screen, drag-and-drop and frame-selection.
> 
> Signed-off-by: XiaoXiao Liu <sliuuxiaonxiao@gmail.com>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> (cherry picked from commit 7e4935ccc3236751e5fe4bd6846f86e46bb2e427
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git)
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>  drivers/input/mouse/alps.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 0a6f7ca883e7..11a4363c3b60 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -24,6 +24,7 @@
>  
>  #include "psmouse.h"
>  #include "alps.h"
> +#include "trackpoint.h"
>  
>  /*
>   * Definitions for ALPS version 3 and 4 command mode protocol
> @@ -2864,6 +2865,23 @@ static const struct alps_protocol_info *alps_match_table(unsigned char *e7,
>  	return NULL;
>  }
>  
> +static bool alps_is_cs19_trackpoint(struct psmouse *psmouse)
> +{
> +	u8 param[2] = { 0 };
> +
> +	if (ps2_command(&psmouse->ps2dev,
> +			param, MAKE_PS2_CMD(0, 2, TP_READ_ID)))
> +		return false;
> +
> +	/*
> +	 * param[0] contains the trackpoint device variant_id while
> +	 * param[1] contains the firmware_id. So far all alps
> +	 * trackpoint-only devices have their variant_ids equal

> +	 * TP_VARIANT_ALPS and their firmware_ids are in 0x20~0x2f range.
> +	 */
> +	return param[0] == TP_VARIANT_ALPS && (param[1] & 0x20);

I don't think the comment is consistent with the code.  I think the last
conditional should be something like the below to match the comment:

    ((param[1] | 0x0f) == 0x2f)

I have no idea if the comment is right, or the code is right.

> +}
> +
>  static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>  {
>  	const struct alps_protocol_info *protocol;
> @@ -3164,6 +3182,20 @@ int alps_detect(struct psmouse *psmouse, bool set_properties)
>  	if (error)
>  		return error;
>  
> +	/*
> +	 * ALPS cs19 is a trackpoint-only device, and uses different
> +	 * protocol than DualPoint ones, so we return -EINVAL here and let
> +	 * trackpoint.c drive this device. If the trackpoint driver is not
> +	 * enabled, the device will fall back to a bare PS/2 mouse.
> +	 * If ps2_command() fails here, we depend on the immediately
> +	 * followed psmouse_reset() to reset the device to normal state.
> +	 */
> +	if (alps_is_cs19_trackpoint(psmouse)) {
> +		psmouse_dbg(psmouse,
> +			    "ALPS CS19 trackpoint-only device detected, ignoring\n");
> +		return -EINVAL;
> +	}
> +
>  	/*
>  	 * Reset the device to make sure it is fully operational:
>  	 * on some laptops, like certain Dell Latitudes, we may
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

On 2019/7/19 上午5:01, Andy Whitcroft wrote:
> On Tue, Jul 16, 2019 at 10:12:57PM +0800, Hui Wang wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1836752
>>
>> On a latest Lenovo laptop, the trackpoint and 3 buttons below it
>> don't work at all, when we move the trackpoint or press those 3
>> buttons, the kernel will print out:
>> "Rejected trackstick packet from non DualPoint device"
>>
>> This device is identified as an alps touchpad but the packet has
>> trackpoint format, so the alps.c drops the packet and prints out
>> the message above.
>>
>> According to XiaoXiao's explanation, this device is named cs19 and
>> is trackpoint-only device, its firmware is only for trackpoint, it
>> is independent of touchpad and is a device completely different from
>> DualPoint ones.
>>
>> To drive this device with mininal changes to the existing driver, we
>> just let the alps driver not handle this device, then the trackpoint.c
>> will be the driver of this device if the trackpoint driver is enabled.
>> (if not, this device will fallback to a bare PS/2 device)
>>
>> With the trackpoint.c, this trackpoint and 3 buttons all work well,
>> they have all features that the trackpoint should have, like
>> scrolling-screen, drag-and-drop and frame-selection.
>>
>> Signed-off-by: XiaoXiao Liu <sliuuxiaonxiao@gmail.com>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> Reviewed-by: Pali Rohár <pali.rohar@gmail.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> (cherry picked from commit 7e4935ccc3236751e5fe4bd6846f86e46bb2e427
>> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git)
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>>   drivers/input/mouse/alps.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
>> index 0a6f7ca883e7..11a4363c3b60 100644
>> --- a/drivers/input/mouse/alps.c
>> +++ b/drivers/input/mouse/alps.c
>> @@ -24,6 +24,7 @@
>>   
>>   #include "psmouse.h"
>>   #include "alps.h"
>> +#include "trackpoint.h"
>>   
>>   /*
>>    * Definitions for ALPS version 3 and 4 command mode protocol
>> @@ -2864,6 +2865,23 @@ static const struct alps_protocol_info *alps_match_table(unsigned char *e7,
>>   	return NULL;
>>   }
>>   
>> +static bool alps_is_cs19_trackpoint(struct psmouse *psmouse)
>> +{
>> +	u8 param[2] = { 0 };
>> +
>> +	if (ps2_command(&psmouse->ps2dev,
>> +			param, MAKE_PS2_CMD(0, 2, TP_READ_ID)))
>> +		return false;
>> +
>> +	/*
>> +	 * param[0] contains the trackpoint device variant_id while
>> +	 * param[1] contains the firmware_id. So far all alps
>> +	 * trackpoint-only devices have their variant_ids equal
>> +	 * TP_VARIANT_ALPS and their firmware_ids are in 0x20~0x2f range.
>> +	 */
>> +	return param[0] == TP_VARIANT_ALPS && (param[1] & 0x20);
> I don't think the comment is consistent with the code.  I think the last
> conditional should be something like the below to match the comment:
>
>      ((param[1] | 0x0f) == 0x2f)
>
> I have no idea if the comment is right, or the code is right.

You are right,  if param[1] is 0x30, 0x60..., the condition will be true 
unexpectedly. This is an issue, and I will send an incremental patch to 
fix it.

And in practice, it will not introduce any issue since the alps told me 
the param[1]  only has 0x0~0x1f in the old devices, and in the 
trackpoint-only device, it has 0x20~0x2f, the value higher than 0x2f has 
not been used yet. so it is safe to merge this patch temporarily. And I 
will submit an incremental SRU after the incremental patch is upstreamed.


Thanks.



>
>> +}
>> +
>>   static int alps_identify(struct psmouse *psmouse, struct alps_data *priv)
>>   {
>>   	const struct alps_protocol_info *protocol;
>> @@ -3164,6 +3182,20 @@ int alps_detect(struct psmouse *psmouse, bool set_properties)
>>   	if (error)
>>   		return error;
>>   
>> +	/*
>> +	 * ALPS cs19 is a trackpoint-only device, and uses different
>> +	 * protocol than DualPoint ones, so we return -EINVAL here and let
>> +	 * trackpoint.c drive this device. If the trackpoint driver is not
>> +	 * enabled, the device will fall back to a bare PS/2 mouse.
>> +	 * If ps2_command() fails here, we depend on the immediately
>> +	 * followed psmouse_reset() to reset the device to normal state.
>> +	 */
>> +	if (alps_is_cs19_trackpoint(psmouse)) {
>> +		psmouse_dbg(psmouse,
>> +			    "ALPS CS19 trackpoint-only device detected, ignoring\n");
>> +		return -EINVAL;
>> +	}
>> +
>>   	/*
>>   	 * Reset the device to make sure it is fully operational:
>>   	 * on some laptops, like certain Dell Latitudes, we may
>> -- 
>> 2.17.1
>>
>>
>> -- 
>> kernel-team mailing list
>> kernel-team@lists.ubuntu.com
>> https://lists.ubuntu.com/mailman/listinfo/kernel-team