[regression] focaltech touchpad driver misbehaves due to "kbuild: treat char as always unsigned"

[regression] focaltech touchpad driver misbehaves due to "kbuild: treat char as always unsigned"

[Linux regression tracking (Thorsten Leemhuis)]

Hi, Thorsten here, the Linux kernel's regression tracker.

I noticed a regression report in bugzilla.kernel.org. Jason, apparently
it's caused by a change of yours (3bc753c06dd0 ("kbuild: treat char as
always unsigned")), which apparently caused a problem in
drivers/input/mouse/focaltech.c to surface. Someone provided a patch
already to fix it here: https://bugs.archlinux.org/task/77733?getfile=22498

Back to the bug. As many (most?) kernel developer don't keep an eye on
bugzilla, I decided to forward it by mail. Quoting from
https://bugzilla.kernel.org/show_bug.cgi?id=217211 :

> Barry 2023-03-17 13:51:10 UTC
> 
> Created attachment 303972 [details] Kernel bisect result
> 
> O/S: Archlinux.
> 
> On any kernel release from 6.2 onwards I have found that the touchpad
> doesn't respond to multi finger touches properly. The pad works fine for
> single finger movement and single finger tap to click. If I click and
> hold the pad button and then use another finger to move such as for text
> selection, drag and drop, moving or resizing a window etc. Or if I try
> to use 2 finger scrolling then the mouse pointer jumps to the top or
> right or into the top right of the screen. All of this functionality
> worked as expected up to kernel 6.1.19.
> 
> I have bisected the kernel and got the attached result.
> 
> 
> I have checked out kernel 6.2.6 and removed the `-funsigned-char` from
> the Makefile. Kernel 6.2.6 built with the modified Makefile restores the
> correct functionality. I believe the touchpad uses the psmouse driver so
> maybe the new build option has broken this driver.>
> I have bisected the kernel and got the attached result.
> 
> 
> I have checked out kernel 6.2.6 and removed the `-funsigned-char`
> from the Makefile. Kernel 6.2.6 built with the modified Makefile
> restores the correct functionality. I believe the touchpad uses the
> psmouse driver so maybe the new build option has broken this driver.
> 
> [...]
> 
> barry@messagefor.me.uk 2023-03-18 11:49:27 UTC
> 
> Hi. If you check this link which is my report of the same bug on the
> arch bug tracker there is a patch attached which fixes the issue.
> 
> https://bugs.archlinux.org/task/77733#comment216336

See the ticket for more details.


[TLDR for the rest of this mail: I'm adding this report to the list of
tracked Linux kernel regressions; the text you find below is based on a
few templates paragraphs you might have encountered already in similar
form.]

BTW, let me use this mail to also add the report to the list of tracked
regressions to ensure it's doesn't fall through the cracks:

#regzbot introduced: 3bc753c06dd0
https://bugzilla.kernel.org/show_bug.cgi?id=217211
#regzbot title: kbuild/input: focaltech touchpad driver misbehaves due
to a checke how to treat char
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (e.g. the buzgzilla ticket and maybe this mail as well, if
this thread sees some discussion). See page linked in footer for details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

[PATCH] Input: focaltech - use explicitly signed char type

[Jason A. Donenfeld]

The recent change of -funsigned-char causes additions of negative
numbers to become additions of large positive numbers, leading to wrong
calculations of mouse movement. Change these casts to be explictly
signed, to take into account negative offsets.

Fixes: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
Cc: stable@vger.kernel.org
Cc: regressions@leemhuis.info
Cc: barry@messagefor.me.uk
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Wrote this patch from my phone, untested, so it would be nice if
somebody with hardware could confirm it works.

 drivers/input/mouse/focaltech.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index 6fd5fff0cbff..3dbad0d8e8c9 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -202,8 +202,8 @@ static void focaltech_process_rel_packet(struct psmouse *psmouse,
 	state->pressed = packet[0] >> 7;
 	finger1 = ((packet[0] >> 4) & 0x7) - 1;
 	if (finger1 < FOC_MAX_FINGERS) {
-		state->fingers[finger1].x += (char)packet[1];
-		state->fingers[finger1].y += (char)packet[2];
+		state->fingers[finger1].x += (signed char)packet[1];
+		state->fingers[finger1].y += (signed char)packet[2];
 	} else {
 		psmouse_err(psmouse, "First finger in rel packet invalid: %d\n",
 			    finger1);
@@ -218,8 +218,8 @@ static void focaltech_process_rel_packet(struct psmouse *psmouse,
 	 */
 	finger2 = ((packet[3] >> 4) & 0x7) - 1;
 	if (finger2 < FOC_MAX_FINGERS) {
-		state->fingers[finger2].x += (char)packet[4];
-		state->fingers[finger2].y += (char)packet[5];
+		state->fingers[finger2].x += (signed char)packet[4];
+		state->fingers[finger2].y += (signed char)packet[5];
 	}
 }
 
-- 
2.40.0

Re: [regression] focaltech touchpad driver misbehaves due to "kbuild: treat char as always unsigned"

[msizanoen]

[-- Attachment #1: Type: text/plain, Size: 4099 bytes --]

Seems like the AlpsPS/2 driver also got hit with the exact same issue: 
https://gitlab.freedesktop.org/libinput/libinput/-/issues/871#note_1829296

I submitted a patch for that: 
https://lore.kernel.org/linux-input/20230318144206.14309-1-msizanoen@qtmlabs.xyz/

On 3/18/23 19:08, Linux regression tracking (Thorsten Leemhuis) wrote:
> Hi, Thorsten here, the Linux kernel's regression tracker.
>
> I noticed a regression report in bugzilla.kernel.org. Jason, apparently
> it's caused by a change of yours (3bc753c06dd0 ("kbuild: treat char as
> always unsigned")), which apparently caused a problem in
> drivers/input/mouse/focaltech.c to surface. Someone provided a patch
> already to fix it here: https://bugs.archlinux.org/task/77733?getfile=22498
>
> Back to the bug. As many (most?) kernel developer don't keep an eye on
> bugzilla, I decided to forward it by mail. Quoting from
> https://bugzilla.kernel.org/show_bug.cgi?id=217211 :
>
>> Barry 2023-03-17 13:51:10 UTC
>>
>> Created attachment 303972 [details] Kernel bisect result
>>
>> O/S: Archlinux.
>>
>> On any kernel release from 6.2 onwards I have found that the touchpad
>> doesn't respond to multi finger touches properly. The pad works fine for
>> single finger movement and single finger tap to click. If I click and
>> hold the pad button and then use another finger to move such as for text
>> selection, drag and drop, moving or resizing a window etc. Or if I try
>> to use 2 finger scrolling then the mouse pointer jumps to the top or
>> right or into the top right of the screen. All of this functionality
>> worked as expected up to kernel 6.1.19.
>>
>> I have bisected the kernel and got the attached result.
>>
>>
>> I have checked out kernel 6.2.6 and removed the `-funsigned-char` from
>> the Makefile. Kernel 6.2.6 built with the modified Makefile restores the
>> correct functionality. I believe the touchpad uses the psmouse driver so
>> maybe the new build option has broken this driver.>
>> I have bisected the kernel and got the attached result.
>>
>>
>> I have checked out kernel 6.2.6 and removed the `-funsigned-char`
>> from the Makefile. Kernel 6.2.6 built with the modified Makefile
>> restores the correct functionality. I believe the touchpad uses the
>> psmouse driver so maybe the new build option has broken this driver.
>>
>> [...]
>>
>> barry@messagefor.me.uk 2023-03-18 11:49:27 UTC
>>
>> Hi. If you check this link which is my report of the same bug on the
>> arch bug tracker there is a patch attached which fixes the issue.
>>
>> https://bugs.archlinux.org/task/77733#comment216336
> See the ticket for more details.
>
>
> [TLDR for the rest of this mail: I'm adding this report to the list of
> tracked Linux kernel regressions; the text you find below is based on a
> few templates paragraphs you might have encountered already in similar
> form.]
>
> BTW, let me use this mail to also add the report to the list of tracked
> regressions to ensure it's doesn't fall through the cracks:
>
> #regzbot introduced: 3bc753c06dd0
> https://bugzilla.kernel.org/show_bug.cgi?id=217211
> #regzbot title: kbuild/input: focaltech touchpad driver misbehaves due
> to a checke how to treat char
> #regzbot ignore-activity
>
> This isn't a regression? This issue or a fix for it are already
> discussed somewhere else? It was fixed already? You want to clarify when
> the regression started to happen? Or point out I got the title or
> something else totally wrong? Then just reply and tell me -- ideally
> while also telling regzbot about it, as explained by the page listed in
> the footer of this mail.
>
> Developers: When fixing the issue, remember to add 'Link:' tags pointing
> to the report (e.g. the buzgzilla ticket and maybe this mail as well, if
> this thread sees some discussion). See page linked in footer for details.
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4494 bytes --]

Re: [PATCH] Input: focaltech - use explicitly signed char type

[Hans de Goede]

Hi,

On 3/18/23 14:30, Jason A. Donenfeld wrote:
> The recent change of -funsigned-char causes additions of negative
> numbers to become additions of large positive numbers, leading to wrong
> calculations of mouse movement. Change these casts to be explictly
> signed, to take into account negative offsets.
> 
> Fixes: 3bc753c06dd0 ("kbuild: treat char as always unsigned")
> Cc: stable@vger.kernel.org
> Cc: regressions@leemhuis.info
> Cc: barry@messagefor.me.uk
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Wrote this patch from my phone, untested, so it would be nice if
> somebody with hardware could confirm it works.

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> 
>  drivers/input/mouse/focaltech.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
> index 6fd5fff0cbff..3dbad0d8e8c9 100644
> --- a/drivers/input/mouse/focaltech.c
> +++ b/drivers/input/mouse/focaltech.c
> @@ -202,8 +202,8 @@ static void focaltech_process_rel_packet(struct psmouse *psmouse,
>  	state->pressed = packet[0] >> 7;
>  	finger1 = ((packet[0] >> 4) & 0x7) - 1;
>  	if (finger1 < FOC_MAX_FINGERS) {
> -		state->fingers[finger1].x += (char)packet[1];
> -		state->fingers[finger1].y += (char)packet[2];
> +		state->fingers[finger1].x += (signed char)packet[1];
> +		state->fingers[finger1].y += (signed char)packet[2];
>  	} else {
>  		psmouse_err(psmouse, "First finger in rel packet invalid: %d\n",
>  			    finger1);
> @@ -218,8 +218,8 @@ static void focaltech_process_rel_packet(struct psmouse *psmouse,
>  	 */
>  	finger2 = ((packet[3] >> 4) & 0x7) - 1;
>  	if (finger2 < FOC_MAX_FINGERS) {
> -		state->fingers[finger2].x += (char)packet[4];
> -		state->fingers[finger2].y += (char)packet[5];
> +		state->fingers[finger2].x += (signed char)packet[4];
> +		state->fingers[finger2].y += (signed char)packet[5];
>  	}
>  }
>