* INFO: trying to register non-static key in usbtouch_reset_resume
From: syzbot @ 2019-05-27 11:38 UTC (permalink / raw)
To: andreyknvl, dmitry.torokhov, linux-input, linux-kernel, linux-usb,
rydberg, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 43151d6c usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=14a2b45ca00000
kernel config: https://syzkaller.appspot.com/x/.config?x=95aff7278e7ff25e
dashboard link: https://syzkaller.appspot.com/bug?extid=933daad9be4e67ba91a9
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1776669aa00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+933daad9be4e67ba91a9@syzkaller.appspotmail.com
usb 1-1: config 0 descriptor??
usb 1-1: string descriptor 0 read error: -71
hub 1-1:0.189: ignoring external hub
input: USB Touchscreen 08f2:007f as
/devices/platform/dummy_hcd.0/usb1/1-1/1-1:0.189/input/input6
usb 1-1: reset high-speed USB device number 3 using dummy_hcd
usb 1-1: Using ep0 maxpacket: 8
INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.
CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.1.0-rc3+ #8
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xca/0x13e lib/dump_stack.c:113
assign_lock_key kernel/locking/lockdep.c:786 [inline]
register_lock_class+0x11ae/0x1240 kernel/locking/lockdep.c:1095
__lock_acquire+0xfb/0x37c0 kernel/locking/lockdep.c:3582
lock_acquire+0x100/0x2b0 kernel/locking/lockdep.c:4211
__mutex_lock_common kernel/locking/mutex.c:925 [inline]
__mutex_lock+0xf9/0x12b0 kernel/locking/mutex.c:1072
usbtouch_reset_resume+0xb3/0x170
drivers/input/touchscreen/usbtouchscreen.c:1624
usb_resume_interface drivers/usb/core/driver.c:1242 [inline]
usb_resume_interface.isra.0+0x187/0x3a0 drivers/usb/core/driver.c:1210
usb_resume_both+0x23d/0x780 drivers/usb/core/driver.c:1412
__rpm_callback+0x287/0x3c0 drivers/base/power/runtime.c:357
rpm_callback+0x18f/0x230 drivers/base/power/runtime.c:487
rpm_resume+0x10c2/0x1840 drivers/base/power/runtime.c:851
__pm_runtime_resume+0x103/0x180 drivers/base/power/runtime.c:1078
pm_runtime_get_sync include/linux/pm_runtime.h:227 [inline]
usb_autoresume_device+0x1e/0x60 drivers/usb/core/driver.c:1599
usb_remote_wakeup+0x7b/0xb0 drivers/usb/core/hub.c:3600
hub_port_connect_change drivers/usb/core/hub.c:5190 [inline]
port_event drivers/usb/core/hub.c:5350 [inline]
hub_event+0x23c9/0x35a0 drivers/usb/core/hub.c:5432
process_one_work+0x90a/0x1580 kernel/workqueue.c:2269
process_scheduled_works kernel/workqueue.c:2331 [inline]
worker_thread+0x7ab/0xe20 kernel/workqueue.c:2417
kthread+0x30e/0x420 kernel/kthread.c:253
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
dummy_hcd dummy_hcd.0: port status 0x00010100 has changes
usb 1-1: USB disconnect, device number 3
usb usb1: dummy_bus_suspend
usb usb1: dummy_bus_resume
dummy_hcd dummy_hcd.0: port status 0x00010101 has changes
dummy_hcd dummy_hcd.0: port status 0x00100503 has changes
usb 1-1: new high-speed USB device number 4 using dummy_hcd
dummy_hcd dummy_hcd.0: port status 0x00100503 has changes
usb 1-1: Using ep0 maxpacket: 8
usb 1-1: config 0 has an invalid interface number: 189 but max is 0
usb 1-1: config 0 has an invalid descriptor of length 0, skipping remainder
of the config
usb 1-1: config 0 has no interface number 0
usb 1-1: config 0 interface 189 altsetting 0 endpoint 0x82 has an invalid
bInterval 0, changing to 7
usb 1-1: New USB device found, idVendor=08f2, idProduct=007f,
bcdDevice=97.17
usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
usb 1-1: config 0 descriptor??
usb 1-1: string descriptor 0 read error: -71
hub 1-1:0.189: ignoring external hub
input: USB Touchscreen 08f2:007f as
/devices/platform/dummy_hcd.0/usb1/1-1/1-1:0.189/input/input7
dummy_hcd dummy_hcd.0: port status 0x00010101 has changes
dummy_hcd dummy_hcd.0: port status 0x00010101 has changes
dummy_hcd dummy_hcd.0: port status 0x00100503 has changes
usb 1-1: reset high-speed USB device number 4 using dummy_hcd
dummy_hcd dummy_hcd.0: port status 0x00100503 has changes
usb 1-1: Using ep0 maxpacket: 8
usb usb1: dummy_bus_suspend
usb usb1: dummy_bus_resume
dummy_hcd dummy_hcd.0: port status 0x00010100 has changes
usb 1-1: USB disconnect, device number 6
usb usb1: dummy_bus_suspend
usb usb1: dummy_bus_resume
dummy_hcd dummy_hcd.0: port status 0x00010101 has changes
dummy_hcd dummy_hcd.0: port status 0x00100503 has changes
usb 1-1: new high-speed USB device number 7 using dummy_hcd
dummy_hcd dummy_hcd.0: port status 0x00100503 has changes
usb 1-1: Using ep0 maxpacket: 8
usb 1-1: config 0 has an invalid interface number: 189 but max is 0
usb 1-1: config 0 has an invalid descriptor of length 0, skipping remainder
of the config
usb 1-1: config 0 has no interface number 0
usb 1-1: config 0 interface 189 altsetting 0 endpoint 0x82 has an invalid
bInterval 0, changing to 7
usb 1-1: New USB device found, idVendor=08f2, idProduct=007f,
bcdDevice=97.17
usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
usb 1-1: config 0 descriptor??
usb 1-1: string descriptor 0 read error: -71
hub 1-1:0.189: ignoring external hub
input: USB Touchscreen 08f2:007f as
/devices/platform/dummy_hcd.0/usb1/1-1/1-1:0.189/input/input10
dummy_hcd dummy_hcd.0: port status 0x00010101 has changes
dummy_hcd dummy_hcd.0: port status 0x00010101 has changes
dummy_hcd dummy_hcd.0: port status 0x00100503 has changes
usb 1-1: reset high-speed USB device number 7 using dummy_hcd
dummy_hcd dummy_hcd.0: port status 0x00100503 has changes
usb 1-1: Using ep0 maxpacket: 8
usb usb1: dummy_bus_suspend
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* [PATCH] [trivial] HID: Typo s/to back 0/back to 0/
From: Geert Uytterhoeven @ 2019-05-27 12:25 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Andrej Shadura
Cc: linux-input, linux-kernel, Geert Uytterhoeven
Fix a silly word ordering typo.
Fixes: 42337b9d4d958daa ("HID: add driver for U2F Zero built-in LED and RNG")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
drivers/hid/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index c3c390ca369022f0..735223f90035b2bf 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1028,7 +1028,7 @@ config HID_U2FZERO
U2F Zero only supports blinking its LED, so this driver doesn't
allow setting the brightness to anything but 1, which will
- trigger a single blink and immediately reset to back 0.
+ trigger a single blink and immediately reset back to 0.
config HID_WACOM
tristate "Wacom Intuos/Graphire tablet support (USB)"
--
2.17.1
^ permalink raw reply related
* [PATCH] Input: qt1050 - remove unnecessary comparison of unsigned integer with < 0
From: Gustavo A. R. Silva @ 2019-05-27 14:10 UTC (permalink / raw)
To: Dmitry Torokhov, Marco Felsch
Cc: linux-input, linux-kernel, Gustavo A. R. Silva
There is no need to compare button.num with < 0 because such comparison
of an unsigned value is always false.
Fix this by removing such comparison.
Addresses-Coverity-ID: 1445492 ("Unsigned compared against 0")
Fixes: cbebf5addec1 ("Input: qt1050 - add Microchip AT42QT1050 support")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/input/keyboard/qt1050.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/qt1050.c b/drivers/input/keyboard/qt1050.c
index 403060d05c3b..a9ac99f62e39 100644
--- a/drivers/input/keyboard/qt1050.c
+++ b/drivers/input/keyboard/qt1050.c
@@ -368,7 +368,7 @@ static int qt1050_parse_fw(struct qt1050_priv *ts)
dev_err(dev, "Button without pad number\n");
goto err;
}
- if (button.num < 0 || button.num > QT1050_MAX_KEYS - 1)
+ if (button.num > QT1050_MAX_KEYS - 1)
goto err;
ts->reg_keys |= BIT(button.num);
--
2.21.0
^ permalink raw reply related
* Feature request for `hid-magicmouse`
From: Tord Johan Espe @ 2019-05-27 18:50 UTC (permalink / raw)
To: jikos, benjamin.tissoires, rydberg, linux-input, linux-kernel
Hi,
This is the first time I have submitted any requests to the Linux
development community, so please excuse me if I'm doing it wrong, but
after reading the docs, it seemed like the correct way to do this was
to identify the people involved with the module and send an email.
My feature request is that I think the scrolling implementation in
`hid-magicmouse.c` should take the number of currently active touches
into consideration when deciding scroll speed.
When using the Apple Magic Mouse in MacOS, scrolling is automatically
slowed down when two fingers are touching the mouse simultaneously.
This is extremely helpful for preventing unwanted scrolling when e.g.
clicking and selecting text, which is, at least for me, really
important for the overall experience. I would love it if the same
behavior could be implemented in the Linux driver.
Best regards,
Tord Johan Espe
^ permalink raw reply
* Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
From: 'Dmitry Torokhov' @ 2019-05-28 1:21 UTC (permalink / raw)
To: 廖崇榮
Cc: 'Benjamin Tissoires', 'Rob Herring',
'Aaron Ma', 'Hans de Goede',
'open list:HID CORE LAYER', 'lkml', devicetree,
Harry Cutts
In-Reply-To: <00f901d5143f$f5ea8420$e1bf8c60$@emc.com.tw>
Hi Benjamin, KT,
On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> Hi
>
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Friday, May 24, 2019 5:37 PM
> To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
>
> On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> >
> > The width/height is actually in the same unit than X and Y. So we
> > should not tamper the data, but just set the proper resolution, so
> > that userspace can correctly detect which touch is a palm or a finger.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > --
> >
> > new in v2
> > ---
> > drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > 1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > b/drivers/input/mouse/elan_i2c_core.c
> > index 7ff044c6cd11..6f4feedb7765 100644
> > --- a/drivers/input/mouse/elan_i2c_core.c
> > +++ b/drivers/input/mouse/elan_i2c_core.c
> > @@ -45,7 +45,6 @@
> > #define DRIVER_NAME "elan_i2c"
> > #define ELAN_VENDOR_ID 0x04f3
> > #define ETP_MAX_PRESSURE 255
> > -#define ETP_FWIDTH_REDUCE 90
> > #define ETP_FINGER_WIDTH 15
> > #define ETP_RETRY_COUNT 3
> >
> > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > return;
> > }
> >
> > - /*
> > - * To avoid treating large finger as palm, let's reduce the
> > - * width x and y per trace.
> > - */
> > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > + area_x = mk_x * data->width_x;
> > + area_y = mk_y * data->width_y;
> >
> > major = max(area_x, area_y);
> > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@
> > static int elan_setup_input_device(struct elan_tp_data *data)
> > ETP_MAX_PRESSURE, 0, 0);
> > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > ETP_FINGER_WIDTH * max_width, 0, 0);
> > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > ETP_FINGER_WIDTH * min_width, 0, 0);
> > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
>
> I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
>
> TL;DR: I don't think we should export the resolution there :(
>
> KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
>
> The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> Chrome's finger/palm threshold may be different from other Linux distribution.
> We will discuss it with Google once the patch picked by chrome and cause something wrong.
Chrome has logic that contact with maximum major/minor is treated as a
palm, so here the driver (which originally came from Chrome OS)
artificially reduces the contact size to ensure that palm rejection
logic does not trigger.
I'm adding Harry to confirm whether we are still using this logic and to
see if we can adjust it to be something else.
Thanks.
--
Dmitry
^ permalink raw reply
* 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
From: Xiaoxiao Liu @ 2019-05-28 1:37 UTC (permalink / raw)
To: Pali Rohár, XiaoXiao Liu
Cc: dmitry.torokhov@gmail.com, peter.hutterer@who-t.net,
hui.wang@canonical.com, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Xiaojian Cao, zhangfp1@lenovo.com,
Naoki Saito
In-Reply-To: <20190527100913.sgxrjrmphsjfmcdb@pali>
Add Saito-san.
Hi Hui,
Does it mean that your device (reported to kernel) sends only trackstick packets and not touchpad?
-> Yes.
I guess that you want parenthesis around (param[1] & 0x20). And also describe what that 0x20 constant means.
It is not a warning.
-> Yes, it should be (param[1] & 0x20).
-> 0x20 is used for detect which type device is. I will correct it.
Hm... why your device does not match these constants?
->I am not clear what the alps_command_mode_read_reg(psmouse, 0xD7) used for.
-> But I know our device did not meet the condition if (reg_val == 0x0C || reg_val == 0x1D) from the running result.
Xiaoxiao Liu
xiaoxiao.liu-1@cn.alps.com
sliuuxiaonxiao@gmail.com
-----邮件原件-----
发件人: Pali Rohár <pali.rohar@gmail.com>
发送时间: Monday, May 27, 2019 6:09 PM
收件人: XiaoXiao Liu <sliuuxiaonxiao@gmail.com>
抄送: dmitry.torokhov@gmail.com; peter.hutterer@who-t.net; hui.wang@canonical.com; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; 曹 曉建 Xiaojian Cao <xiaojian.cao@cn.alps.com>; zhangfp1@lenovo.com; 劉 曉曉 Xiaoxiao Liu <xiaoxiao.liu-1@cn.alps.com>
主题: Re: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
Hi!
On Monday 27 May 2019 05:44:22 XiaoXiao Liu wrote:
> The alps devices which detected to use the ALPS_PROTO_V8 procotol
> contains ALPS touchpad and ALPS trackstick.The ALPS_PROTO_V8 procotol
> do not support the trackstick device process by default.
Normally PS/2 device handled by alps.c is touchpad and in some cases touchpad sends also trackstick data in that one PS/2 channel.
Does it mean that your device (reported to kernel) sends only trackstick packets and not touchpad?
> When the trackstick was detected to use ALPS_PROTO_V8 procotol, the v8
> process_packet method alps_process_packet_ss4_v2 will reject to report
> the data when the device using ALPS_PROTO_V8 procotol is not set the
> ALPS_DUALPOINT flag.
>
> The alps cs19 trackstick is detected to use the ALPS_PROTO_V8 procotol
> but without ALPS_DUALPOINT flag, the alps driver will not report the
> input data. so the trackstick will not work.
>
> solution: when the alps cs19 device detected, set the device
> ALPS_DUALPOINT flag,then the input data will be processed.
>
> Signed-off-by: XiaoXiao Liu <sliuuxiaonxiao@gmail.com>
> ---
> drivers/input/mouse/alps.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 0a6f7ca883e7..a54677cf7474 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -24,7 +24,7 @@
>
> #include "psmouse.h"
> #include "alps.h"
> -
> +#include "trackpoint.h"
> /*
> * Definitions for ALPS version 3 and 4 command mode protocol
> */
> @@ -220,6 +220,23 @@ static bool alps_is_valid_first_byte(struct alps_data *priv,
> return (data & priv->mask0) == priv->byte0; }
>
> +static int alps_check_cs19_trackstick(struct psmouse *psmouse) {
> + u8 param[2] = { 0 };
> + int error;
> +
> + error = ps2_command(&psmouse->ps2dev,
> + param, MAKE_PS2_CMD(0, 2, TP_READ_ID));
> + if (error)
> + return error;
> +
> + if (param[0] == TP_VARIANT_ALPS && param[1] & 0x20) {
I guess that you want parenthesis around (param[1] & 0x20). And also describe what that 0x20 constant means.
> + psmouse_warn(psmouse, "It is alps cs19 trackstick");
It is not a warning.
> + return 0;
> + }
> + return -1;
> +}
> +
> static void alps_report_buttons(struct input_dev *dev1, struct input_dev *dev2,
> int left, int right, int middle)
> {
> @@ -2568,8 +2585,12 @@ static int alps_update_dual_info_ss4_v2(unsigned char otp[][4],
> alps_exit_command_mode(psmouse);
> ps2_command(ps2dev, NULL, PSMOUSE_CMD_ENABLE);
>
> - if (reg_val == 0x0C || reg_val == 0x1D)
> + if (reg_val == 0x0C || reg_val == 0x1D) {
Hm... why your device does not match these constants?
> + is_dual = true;
> + } else if (alps_check_cs19_trackstick(psmouse) == 0) {
> + //For support Thinkpad CS19 TrackStick
> is_dual = true;
> + }
> }
> }
>
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* Re: [PATCH v3 0/8] Fix Elan I2C touchpads in latest generation from Lenovo
From: Dmitry Torokhov @ 2019-05-28 1:46 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: KT Liao, Rob Herring, Aaron Ma, Hans de Goede, linux-input,
linux-kernel, devicetree
In-Reply-To: <20190524135046.17710-1-benjamin.tissoires@redhat.com>
On Fri, May 24, 2019 at 03:50:38PM +0200, Benjamin Tissoires wrote:
> Here comes the v3.
>
> Very few changes from v2:
> - dropped the last 2 patches where I tried to be smart, and it turns out
> that it was not very a good idea
> - also removed the only other blacklisted model, as it has been tested with
> the v2 and it is also now working properly
Applied the lot, thank you.
>
> Cheers,
> Benjamin
>
> Benjamin Tissoires (8):
> Input: elantech - query the min/max information beforehand too
> Input: elantech - add helper function elantech_is_buttonpad()
> Input: elantech - detect middle button based on firmware version
> dt-bindings: add more optional properties for elan_i2c touchpads
> Input: elan_i2c - do not query the info if they are provided
> Input: elantech/SMBus - export all capabilities from the PS/2 node
> Input: elan_i2c - handle physical middle button
> Input: elantech: remove P52 and P72 from SMBus blacklist
>
> .../devicetree/bindings/input/elan_i2c.txt | 11 +
> drivers/input/mouse/elan_i2c_core.c | 72 +++-
> drivers/input/mouse/elantech.c | 320 ++++++++++--------
> drivers/input/mouse/elantech.h | 8 +
> 4 files changed, 246 insertions(+), 165 deletions(-)
>
> --
> 2.21.0
>
--
Dmitry
^ permalink raw reply
* Re: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
From: Pali Rohár @ 2019-05-28 7:18 UTC (permalink / raw)
To: Xiaoxiao Liu
Cc: XiaoXiao Liu, dmitry.torokhov@gmail.com, peter.hutterer@who-t.net,
hui.wang@canonical.com, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Xiaojian Cao, zhangfp1@lenovo.com,
Naoki Saito
In-Reply-To: <OSBPR01MB4855F61AE28B883CDD87F781DA1E0@OSBPR01MB4855.jpnprd01.prod.outlook.com>
On Tuesday 28 May 2019 01:37:14 Xiaoxiao Liu wrote:
> Add Saito-san.
>
> Hi Hui,
> Does it mean that your device (reported to kernel) sends only trackstick packets and not touchpad?
> -> Yes.
Ok, I think this answers all questions.
So your patch is not correct as it registers "fake" touchpad device even
there is no touchpad at all.
You should fix your patch to not register touchpad input device, in your
case it should register only trackstick device. I suggest to add some
flag which would indicate such device (e.g. ALPS_ONLY_TRACKSTICK).
Also currently kernel exports following names when device has both
trackstick and touchpad: "DualPoint Stick" and "DualPoint TouchPad".
And it exports name "GlidePoint" for touchpad-only device. So to be
consistent you need to also modify this code for trackstick-only device.
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* 答复: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
From: Xiaoxiao Liu @ 2019-05-28 7:55 UTC (permalink / raw)
To: Pali Rohár
Cc: XiaoXiao Liu, dmitry.torokhov@gmail.com, peter.hutterer@who-t.net,
hui.wang@canonical.com, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Xiaojian Cao, zhangfp1@lenovo.com,
Naoki Saito, Hideo Kawase
In-Reply-To: <20190528071824.jimhixhtsynzwixe@pali>
Add Kawase-san.
-----邮件原件-----
发件人: Pali Rohár <pali.rohar@gmail.com>
发送时间: Tuesday, May 28, 2019 3:18 PM
收件人: 劉 曉曉 Xiaoxiao Liu <xiaoxiao.liu-1@cn.alps.com>
抄送: XiaoXiao Liu <sliuuxiaonxiao@gmail.com>; dmitry.torokhov@gmail.com; peter.hutterer@who-t.net; hui.wang@canonical.com; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; 曹 曉建 Xiaojian Cao <xiaojian.cao@cn.alps.com>; zhangfp1@lenovo.com; 斉藤 直樹 Naoki Saito <naoki.saito@alpsalpine.com>
主题: Re: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.
On Tuesday 28 May 2019 01:37:14 Xiaoxiao Liu wrote:
> Add Saito-san.
>
> Hi Hui,
> Does it mean that your device (reported to kernel) sends only trackstick packets and not touchpad?
> -> Yes.
Ok, I think this answers all questions.
So your patch is not correct as it registers "fake" touchpad device even there is no touchpad at all.
You should fix your patch to not register touchpad input device, in your case it should register only trackstick device. I suggest to add some flag which would indicate such device (e.g. ALPS_ONLY_TRACKSTICK).
Also currently kernel exports following names when device has both trackstick and touchpad: "DualPoint Stick" and "DualPoint TouchPad".
And it exports name "GlidePoint" for touchpad-only device. So to be consistent you need to also modify this code for trackstick-only device.
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply
* [PATCH] HID: hid-logitech-hidpp: detect wireless lightspeed devices
From: Pedro Vanzella @ 2019-05-28 16:29 UTC (permalink / raw)
To: linux-input; +Cc: Pedro Vanzella, Jiri Kosina, Benjamin Tissoires, linux-kernel
Send a low device index when the device is connected via the lightspeed
receiver so that the receiver will pass the message along to the device
instead of responding. If we don't do that, we end up thinking it's a
hidpp10 device and miss out on all new features available to newer devices.
This will enable correct detection of the following models:
G603, GPro, G305, G613, G900 and G903, and possibly others.
Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
---
drivers/hid/hid-logitech-hidpp.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 72fc9c0566db..621fce141d9f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -62,6 +62,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_QUIRK_CLASS_K400 BIT(2)
#define HIDPP_QUIRK_CLASS_G920 BIT(3)
#define HIDPP_QUIRK_CLASS_K750 BIT(4)
+#define HIDPP_QUIRK_CLASS_LIGHTSPEED BIT(5)
/* bits 2..20 are reserved for classes */
/* #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */
@@ -236,7 +237,11 @@ static int __hidpp_send_report(struct hid_device *hdev,
* set the device_index as the receiver, it will be overwritten by
* hid_hw_request if needed
*/
- hidpp_report->device_index = 0xff;
+ if (hidpp->quirks & HIDPP_QUIRK_CLASS_LIGHTSPEED) {
+ hidpp_report->device_index = 0x01;
+ } else {
+ hidpp_report->device_index = 0xff;
+ }
if (hidpp->quirks & HIDPP_QUIRK_FORCE_OUTPUT_REPORTS) {
ret = hid_hw_output_report(hdev, (u8 *)hidpp_report, fields_count);
@@ -3753,6 +3758,9 @@ static const struct hid_device_id hidpp_devices[] = {
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC06B) },
{ /* Logitech G900 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081) },
+ { /* Logitech Gaming Mice over Lightspeed Receiver */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC539),
+ .driver_data = HIDPP_QUIRK_CLASS_LIGHTSPEED },
{ /* Logitech G920 Wheel over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL),
.driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
--
2.21.0
^ permalink raw reply related
* hid-related 5.2-rc1 boot hang
From: Dave Hansen @ 2019-05-28 16:43 UTC (permalink / raw)
To: Jiri Kosina, Hans de Goede, Benjamin Tissoires,
open list:HID CORE LAYER, LKML
I have a system that works fine on 5.1. When updating to 5.2-rc1, it
hangs at boot waiting on an instance of systemd-udevd. The kernel
backtrace (https://photos.app.goo.gl/EV8rf7FofWouvdeE8) looks like it's
doing an finit_module() that dives into the hid code and is waiting on a
request_module().
This hang only occurs if I have a particular USB device inserted:
> Bus 001 Device 007: ID 046d:c52b Logitech, Inc. Unifying Receiver
Bisecting the issue points at this (unlikely to be the culprit) commit:
> [161f62cd07fde123fd52bf6d5b6fd6513cca968e] HID: macally: Add support for Macally ikey keyboard
This bisect result is probably just a bisect artifact. The first real,
bad commit is a merge commit: 63b6f0b827d. This commit merges a bunch
of stuff, but includes changes to the hid request_module() code and to
the logitech-hidpp which is the driver for the above device.
I also have a picture of the hang which includes __request_module()
dumping out the string it is passed:
https://photos.app.goo.gl/tUETiCBZHJfKqWPu8
This is easy enough to work around, and the system works fine if I just
unplug the Logitech device and plug it in after boot. But, it would be
nice to figure out what's going wrong. I guess it could easily be some
interaction between systemd, the driver and the request_module() ordering.
^ permalink raw reply
* Re: [PATCH] HID: hid-logitech-hidpp: detect wireless lightspeed devices
From: Benjamin Tissoires @ 2019-05-28 17:07 UTC (permalink / raw)
To: Pedro Vanzella; +Cc: open list:HID CORE LAYER, Jiri Kosina, lkml
In-Reply-To: <20190528162924.32754-1-pedro@pedrovanzella.com>
On Tue, May 28, 2019 at 6:30 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote:
>
> Send a low device index when the device is connected via the lightspeed
> receiver so that the receiver will pass the message along to the device
> instead of responding. If we don't do that, we end up thinking it's a
> hidpp10 device and miss out on all new features available to newer devices.
>
> This will enable correct detection of the following models:
> G603, GPro, G305, G613, G900 and G903, and possibly others.
Thanks for the patch.
However, there is already support for this receiver in Linus' tree:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/hid/hid-logitech-dj.c?id=f5fb57a74e88bd1788f57bf77d587c91d4dc9d57
With kernel 5.2-rc1, the connected device should already be handled by
hid-logitech-hidpp :)
Cheers,
Benjamin
>
> Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com>
> ---
> drivers/hid/hid-logitech-hidpp.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 72fc9c0566db..621fce141d9f 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -62,6 +62,7 @@ MODULE_PARM_DESC(disable_tap_to_click,
> #define HIDPP_QUIRK_CLASS_K400 BIT(2)
> #define HIDPP_QUIRK_CLASS_G920 BIT(3)
> #define HIDPP_QUIRK_CLASS_K750 BIT(4)
> +#define HIDPP_QUIRK_CLASS_LIGHTSPEED BIT(5)
>
> /* bits 2..20 are reserved for classes */
> /* #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */
> @@ -236,7 +237,11 @@ static int __hidpp_send_report(struct hid_device *hdev,
> * set the device_index as the receiver, it will be overwritten by
> * hid_hw_request if needed
> */
> - hidpp_report->device_index = 0xff;
> + if (hidpp->quirks & HIDPP_QUIRK_CLASS_LIGHTSPEED) {
> + hidpp_report->device_index = 0x01;
> + } else {
> + hidpp_report->device_index = 0xff;
> + }
>
> if (hidpp->quirks & HIDPP_QUIRK_FORCE_OUTPUT_REPORTS) {
> ret = hid_hw_output_report(hdev, (u8 *)hidpp_report, fields_count);
> @@ -3753,6 +3758,9 @@ static const struct hid_device_id hidpp_devices[] = {
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC06B) },
> { /* Logitech G900 Gaming Mouse over USB */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081) },
> + { /* Logitech Gaming Mice over Lightspeed Receiver */
> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC539),
> + .driver_data = HIDPP_QUIRK_CLASS_LIGHTSPEED },
> { /* Logitech G920 Wheel over USB */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL),
> .driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
> --
> 2.21.0
>
^ permalink raw reply
* Re: hid-related 5.2-rc1 boot hang
From: Jiri Kosina @ 2019-05-28 17:14 UTC (permalink / raw)
To: Dave Hansen
Cc: Hans de Goede, Benjamin Tissoires, open list:HID CORE LAYER, LKML
In-Reply-To: <2c1684f6-9def-93dc-54ab-888142fd5e71@intel.com>
On Tue, 28 May 2019, Dave Hansen wrote:
> I have a system that works fine on 5.1. When updating to 5.2-rc1, it
> hangs at boot waiting on an instance of systemd-udevd. The kernel
> backtrace (https://photos.app.goo.gl/EV8rf7FofWouvdeE8) looks like it's
> doing an finit_module() that dives into the hid code and is waiting on a
> request_module().
Dave,
thanks for the report.
Just to confirm -- I guess reverting 4ceabaf79 and a025a18fe would work
this around, right?
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: hid-related 5.2-rc1 boot hang
From: Benjamin Tissoires @ 2019-05-28 17:45 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Dave Hansen, Hans de Goede, open list:HID CORE LAYER, LKML
In-Reply-To: <nycvar.YFH.7.76.1905281913140.1962@cbobk.fhfr.pm>
On Tue, May 28, 2019 at 7:15 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Tue, 28 May 2019, Dave Hansen wrote:
>
> > I have a system that works fine on 5.1. When updating to 5.2-rc1, it
> > hangs at boot waiting on an instance of systemd-udevd. The kernel
> > backtrace (https://photos.app.goo.gl/EV8rf7FofWouvdeE8) looks like it's
> > doing an finit_module() that dives into the hid code and is waiting on a
> > request_module().
>
> Dave,
>
> thanks for the report.
>
> Just to confirm -- I guess reverting 4ceabaf79 and a025a18fe would work
> this around, right?
>
It would be also interesting to know which distribution and which
systemd version you are running (if you are on systemd).
This rings a bell as I recall playing with
request_module/request_firmware a while ago, but Hans convinced me
that no such delay would be induced.
Cheers,
Benjamin
^ permalink raw reply
* Re: hid-related 5.2-rc1 boot hang
From: Dave Hansen @ 2019-05-28 18:11 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina
Cc: Hans de Goede, open list:HID CORE LAYER, LKML
In-Reply-To: <CAO-hwJJzNAuFbdMVFZ4+h7J=bh6QHr_MioyK2yTV=M5R6CTm=A@mail.gmail.com>
On 5/28/19 10:45 AM, Benjamin Tissoires wrote:
> On Tue, May 28, 2019 at 7:15 PM Jiri Kosina <jikos@kernel.org> wrote:
>> Just to confirm -- I guess reverting 4ceabaf79 and a025a18fe would work
>> this around, right?
Yes, reverting that pair on top of 5.2-rc1 works around the issue.
> It would be also interesting to know which distribution and which
> systemd version you are running (if you are on systemd).
$ systemd --version
systemd 229
+PAM +AUDIT +SELINUX +IMA +APPARMOR +SMACK +SYSVINIT +UTMP
+LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ -LZ4 +SECCOMP +BLKID +ELFUTILS
+KMOD -IDN
on "Ubuntu 16.04.6"
^ permalink raw reply
* Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
From: Harry Cutts @ 2019-05-28 18:13 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: 廖崇榮, Benjamin Tissoires, Rob Herring,
Aaron Ma, Hans de Goede, open list:HID CORE LAYER, lkml,
devicetree, seobrien
In-Reply-To: <20190528012101.GA193221@dtor-ws>
On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> Hi Benjamin, KT,
>
> On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > Hi
> >
> > -----Original Message-----
> > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > Sent: Friday, May 24, 2019 5:37 PM
> > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
> >
> > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > >
> > > The width/height is actually in the same unit than X and Y. So we
> > > should not tamper the data, but just set the proper resolution, so
> > > that userspace can correctly detect which touch is a palm or a finger.
> > >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >
> > > --
> > >
> > > new in v2
> > > ---
> > > drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > b/drivers/input/mouse/elan_i2c_core.c
> > > index 7ff044c6cd11..6f4feedb7765 100644
> > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > @@ -45,7 +45,6 @@
> > > #define DRIVER_NAME "elan_i2c"
> > > #define ELAN_VENDOR_ID 0x04f3
> > > #define ETP_MAX_PRESSURE 255
> > > -#define ETP_FWIDTH_REDUCE 90
> > > #define ETP_FINGER_WIDTH 15
> > > #define ETP_RETRY_COUNT 3
> > >
> > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > return;
> > > }
> > >
> > > - /*
> > > - * To avoid treating large finger as palm, let's reduce the
> > > - * width x and y per trace.
> > > - */
> > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > + area_x = mk_x * data->width_x;
> > > + area_y = mk_y * data->width_y;
> > >
> > > major = max(area_x, area_y);
> > > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@
> > > static int elan_setup_input_device(struct elan_tp_data *data)
> > > ETP_MAX_PRESSURE, 0, 0);
> > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > ETP_FINGER_WIDTH * max_width, 0, 0);
> > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > ETP_FINGER_WIDTH * min_width, 0, 0);
> > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
> >
> > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> >
> > TL;DR: I don't think we should export the resolution there :(
> >
> > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> >
> > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > Chrome's finger/palm threshold may be different from other Linux distribution.
> > We will discuss it with Google once the patch picked by chrome and cause something wrong.
>
> Chrome has logic that contact with maximum major/minor is treated as a
> palm, so here the driver (which originally came from Chrome OS)
> artificially reduces the contact size to ensure that palm rejection
> logic does not trigger.
>
> I'm adding Harry to confirm whether we are still using this logic and to
> see if we can adjust it to be something else.
I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.
^ permalink raw reply
* Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
From: Sean O'Brien @ 2019-05-29 0:12 UTC (permalink / raw)
To: Harry Cutts
Cc: Dmitry Torokhov, 廖崇榮, Benjamin Tissoires,
Rob Herring, Aaron Ma, Hans de Goede, open list:HID CORE LAYER,
lkml, devicetree
In-Reply-To: <CA+jURcsWe=fZ-catnCaH=A85vAhrv1w1E5nSwpJvBAwgCTNYfw@mail.gmail.com>
We do still use a maxed out major axis as a signal for a palm in the touchscreen
logic, but I'm not too concerned because if that axis is maxed out, the contact
should probably be treated as a palm anyway...
I'm more concerned with this affecting our gesture detection for
touchpad. It looks
like this change would cause all contacts to reported as some percentage bigger
than they are currently. Can you give me an idea of how big that percentage is?
On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
>
> On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Benjamin, KT,
> >
> > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > Hi
> > >
> > > -----Original Message-----
> > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > Sent: Friday, May 24, 2019 5:37 PM
> > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
> > >
> > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > >
> > > > The width/height is actually in the same unit than X and Y. So we
> > > > should not tamper the data, but just set the proper resolution, so
> > > > that userspace can correctly detect which touch is a palm or a finger.
> > > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >
> > > > --
> > > >
> > > > new in v2
> > > > ---
> > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > @@ -45,7 +45,6 @@
> > > > #define DRIVER_NAME "elan_i2c"
> > > > #define ELAN_VENDOR_ID 0x04f3
> > > > #define ETP_MAX_PRESSURE 255
> > > > -#define ETP_FWIDTH_REDUCE 90
> > > > #define ETP_FINGER_WIDTH 15
> > > > #define ETP_RETRY_COUNT 3
> > > >
> > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > > return;
> > > > }
> > > >
> > > > - /*
> > > > - * To avoid treating large finger as palm, let's reduce the
> > > > - * width x and y per trace.
> > > > - */
> > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > + area_x = mk_x * data->width_x;
> > > > + area_y = mk_y * data->width_y;
> > > >
> > > > major = max(area_x, area_y);
> > > > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@
> > > > static int elan_setup_input_device(struct elan_tp_data *data)
> > > > ETP_MAX_PRESSURE, 0, 0);
> > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > > ETP_FINGER_WIDTH * max_width, 0, 0);
> > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > > ETP_FINGER_WIDTH * min_width, 0, 0);
> > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
> > >
> > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > >
> > > TL;DR: I don't think we should export the resolution there :(
> > >
> > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > >
> > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> >
> > Chrome has logic that contact with maximum major/minor is treated as a
> > palm, so here the driver (which originally came from Chrome OS)
> > artificially reduces the contact size to ensure that palm rejection
> > logic does not trigger.
> >
> > I'm adding Harry to confirm whether we are still using this logic and to
> > see if we can adjust it to be something else.
>
> I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.
^ permalink raw reply
* Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
From: Benjamin Tissoires @ 2019-05-29 7:16 UTC (permalink / raw)
To: Sean O'Brien, Peter Hutterer
Cc: Harry Cutts, Dmitry Torokhov, 廖崇榮,
Rob Herring, Aaron Ma, Hans de Goede, open list:HID CORE LAYER,
lkml, devicetree
In-Reply-To: <CAOOzhkq+vD034Q2FKB2ryR7Q9nY=iQjdrREuihkZTaVcg+E_Xg@mail.gmail.com>
On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote:
>
> We do still use a maxed out major axis as a signal for a palm in the touchscreen
> logic, but I'm not too concerned because if that axis is maxed out, the contact
> should probably be treated as a palm anyway...
>
> I'm more concerned with this affecting our gesture detection for
> touchpad. It looks
> like this change would cause all contacts to reported as some percentage bigger
> than they are currently. Can you give me an idea of how big that percentage is?
On the P52, I currently have:
[ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
[ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429
-> with the computation done in the kernel:
width_ratio: 126
height_ratio: 123
For my average finger, the reported traces are between 4 and 6:
With the ETP_FWIDTH_REDUCE:
Major between 144 to 216
Minor between 132 to 198
Without:
Major between 504 to 756
Minor between 492 to 738
So a rough augmentation of 350%
For the Synaptics devices (over SMBus), they send the raw value of the
traces, so you will get a major/minor between 2 to 5. Max on these
axes is 15, so we should get the same percentage of value comparing to
the range.
Which is why libinput has a database of which device reports which
pressure/major/minor ranges as otherwise the values are just
impossible to understand.
Cheers,
Benjamin
>
> On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
> >
> > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Benjamin, KT,
> > >
> > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > > Hi
> > > >
> > > > -----Original Message-----
> > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > > Sent: Friday, May 24, 2019 5:37 PM
> > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
> > > >
> > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > > >
> > > > > The width/height is actually in the same unit than X and Y. So we
> > > > > should not tamper the data, but just set the proper resolution, so
> > > > > that userspace can correctly detect which touch is a palm or a finger.
> > > > >
> > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > >
> > > > > --
> > > > >
> > > > > new in v2
> > > > > ---
> > > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > > @@ -45,7 +45,6 @@
> > > > > #define DRIVER_NAME "elan_i2c"
> > > > > #define ELAN_VENDOR_ID 0x04f3
> > > > > #define ETP_MAX_PRESSURE 255
> > > > > -#define ETP_FWIDTH_REDUCE 90
> > > > > #define ETP_FINGER_WIDTH 15
> > > > > #define ETP_RETRY_COUNT 3
> > > > >
> > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > > > return;
> > > > > }
> > > > >
> > > > > - /*
> > > > > - * To avoid treating large finger as palm, let's reduce the
> > > > > - * width x and y per trace.
> > > > > - */
> > > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > > + area_x = mk_x * data->width_x;
> > > > > + area_y = mk_y * data->width_y;
> > > > >
> > > > > major = max(area_x, area_y);
> > > > > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@
> > > > > static int elan_setup_input_device(struct elan_tp_data *data)
> > > > > ETP_MAX_PRESSURE, 0, 0);
> > > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > > > ETP_FINGER_WIDTH * max_width, 0, 0);
> > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> > > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > > > ETP_FINGER_WIDTH * min_width, 0, 0);
> > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
> > > >
> > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > > >
> > > > TL;DR: I don't think we should export the resolution there :(
> > > >
> > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > > >
> > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> > >
> > > Chrome has logic that contact with maximum major/minor is treated as a
> > > palm, so here the driver (which originally came from Chrome OS)
> > > artificially reduces the contact size to ensure that palm rejection
> > > logic does not trigger.
> > >
> > > I'm adding Harry to confirm whether we are still using this logic and to
> > > see if we can adjust it to be something else.
> >
> > I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.
^ permalink raw reply
* Re: hid-related 5.2-rc1 boot hang
From: Hans de Goede @ 2019-05-29 9:17 UTC (permalink / raw)
To: Dave Hansen, Benjamin Tissoires, Jiri Kosina
Cc: open list:HID CORE LAYER, LKML
In-Reply-To: <8a17e6e2-b468-28fd-5b40-0c258ca7efa9@intel.com>
Hi,
On 5/28/19 8:11 PM, Dave Hansen wrote:
> On 5/28/19 10:45 AM, Benjamin Tissoires wrote:
>> On Tue, May 28, 2019 at 7:15 PM Jiri Kosina <jikos@kernel.org> wrote:
>>> Just to confirm -- I guess reverting 4ceabaf79 and a025a18fe would work
>>> this around, right?
>
> Yes, reverting that pair on top of 5.2-rc1 works around the issue.
Thank you for catching this and for testing the reverts. We've several bug
reports which I suspect are related to this.
/sbin/modprobe really should not hang when called by the kernel, as the
kernel does this in several other places too.
At the same time this clearly is a regression, so I'm afraid we will need
to revert the 2 commits.
Benjamin, Jiri, I really like the improvements these 2 commits bring
combined with Benjamin's changes removing the need for all the device specific
drivers to have HID_QUIRK_HAVE_SPECIAL_DRIVER quirk.
Maybe instead of reverting them, we wrap them in a Kconfig option, which
defaults to N, with a warning that this requires an userspace where
/sbin/modprobe does not hang ? It would be useful for the Kconfig
help text if we knew why it hangs. I guess this may have something to do
with it running from the initrd? Maybe this is not the real modprobe but
busybox's modprobe?
Dave, can you try building your initrd without the hid-logitech-dj module
included in the initrd?
Also can you check if your modprobe is provided by module-init-tools
or by kmod ?
I believe we really need more information before we can properly decide
how to deal with this. Luckily we still have same time.
Regards,
Hans
^ permalink raw reply
* RE: [PATCH v2 08/10] Input: elan_i2c - export true width/height
From: 廖崇榮 @ 2019-05-29 12:55 UTC (permalink / raw)
To: 'Benjamin Tissoires', 'Sean O'Brien',
'Peter Hutterer'
Cc: 'Harry Cutts', 'Dmitry Torokhov',
'Rob Herring', 'Aaron Ma',
'Hans de Goede', 'open list:HID CORE LAYER',
'lkml', devicetree
In-Reply-To: <CAO-hwJ+9tnmvD-K3_Ksesdvag1aNbLB7eJxb9ZKb7kM24unqQQ@mail.gmail.com>
-----Original Message-----
From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
Sent: Wednesday, May 29, 2019 3:17 PM
To: Sean O'Brien; Peter Hutterer
Cc: Harry Cutts; Dmitry Torokhov; 廖崇榮; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote:
>
> We do still use a maxed out major axis as a signal for a palm in the
> touchscreen logic, but I'm not too concerned because if that axis is
> maxed out, the contact should probably be treated as a palm anyway...
>
> I'm more concerned with this affecting our gesture detection for
> touchpad. It looks like this change would cause all contacts to
> reported as some percentage bigger than they are currently. Can you
> give me an idea of how big that percentage is?
On the P52, I currently have:
[ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
[ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429
-> with the computation done in the kernel:
width_ratio: 126
height_ratio: 123
For my average finger, the reported traces are between 4 and 6:
With the ETP_FWIDTH_REDUCE:
Major between 144 to 216
Minor between 132 to 198
Without:
Major between 504 to 756
Minor between 492 to 738
So a rough augmentation of 350%
For the Synaptics devices (over SMBus), they send the raw value of the traces, so you will get a major/minor between 2 to 5. Max on these axes is 15, so we should get the same percentage of value comparing to the range.
Elan's vendor report contains such information, which indicate how many trace are touched by finger/palm
mk_x = (finger_data[3] & 0x0f);
mk_y = (finger_data[3] >> 4);
Do we need to use mk_* for major/minor for keeping it consistent with other vendor?
But this modification will impact Chromebook's usability and Chrome test suite.
Which is why libinput has a database of which device reports which pressure/major/minor ranges as otherwise the values are just impossible to understand.
Cheers,
Benjamin
>
> On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
> >
> > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Benjamin, KT,
> > >
> > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > > Hi
> > > >
> > > > -----Original Message-----
> > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > > Sent: Friday, May 24, 2019 5:37 PM
> > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de
> > > > Goede
> > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true
> > > > width/height
> > > >
> > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > > >
> > > > > The width/height is actually in the same unit than X and Y. So
> > > > > we should not tamper the data, but just set the proper
> > > > > resolution, so that userspace can correctly detect which touch is a palm or a finger.
> > > > >
> > > > > Signed-off-by: Benjamin Tissoires
> > > > > <benjamin.tissoires@redhat.com>
> > > > >
> > > > > --
> > > > >
> > > > > new in v2
> > > > > ---
> > > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > > @@ -45,7 +45,6 @@
> > > > > #define DRIVER_NAME "elan_i2c"
> > > > > #define ELAN_VENDOR_ID 0x04f3
> > > > > #define ETP_MAX_PRESSURE 255
> > > > > -#define ETP_FWIDTH_REDUCE 90
> > > > > #define ETP_FINGER_WIDTH 15
> > > > > #define ETP_RETRY_COUNT 3
> > > > >
> > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > > > return;
> > > > > }
> > > > >
> > > > > - /*
> > > > > - * To avoid treating large finger as palm, let's reduce the
> > > > > - * width x and y per trace.
> > > > > - */
> > > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > > + area_x = mk_x * data->width_x;
> > > > > + area_y = mk_y * data->width_y;
> > > > >
> > > > > major = max(area_x, area_y);
> > > > > minor = min(area_x, area_y); @@ -1123,8
> > > > > +1118,10 @@ static int elan_setup_input_device(struct elan_tp_data *data)
> > > > > ETP_MAX_PRESSURE, 0, 0);
> > > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > > > ETP_FINGER_WIDTH * max_width, 0,
> > > > > 0);
> > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR,
> > > > > + data->x_res);
> > > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > > > ETP_FINGER_WIDTH * min_width, 0,
> > > > > 0);
> > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR,
> > > > > + data->y_res);
> > > >
> > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > > >
> > > > TL;DR: I don't think we should export the resolution there :(
> > > >
> > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > > >
> > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> > >
> > > Chrome has logic that contact with maximum major/minor is treated
> > > as a palm, so here the driver (which originally came from Chrome
> > > OS) artificially reduces the contact size to ensure that palm
> > > rejection logic does not trigger.
> > >
> > > I'm adding Harry to confirm whether we are still using this logic
> > > and to see if we can adjust it to be something else.
> >
> > I'm not very familiar with our touchpad code, so adding Sean
> > O'Brien, who is.
^ permalink raw reply
* Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
From: Benjamin Tissoires @ 2019-05-29 13:16 UTC (permalink / raw)
To: 廖崇榮
Cc: Sean O'Brien, Peter Hutterer, Harry Cutts, Dmitry Torokhov,
Rob Herring, Aaron Ma, Hans de Goede, open list:HID CORE LAYER,
lkml, devicetree
In-Reply-To: <010301d5161d$dd201e70$97605b50$@emc.com.tw>
On Wed, May 29, 2019 at 2:56 PM 廖崇榮 <kt.liao@emc.com.tw> wrote:
>
>
>
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Wednesday, May 29, 2019 3:17 PM
> To: Sean O'Brien; Peter Hutterer
> Cc: Harry Cutts; Dmitry Torokhov; 廖崇榮; Rob Herring; Aaron Ma; Hans de Goede; open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
>
> On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote:
> >
> > We do still use a maxed out major axis as a signal for a palm in the
> > touchscreen logic, but I'm not too concerned because if that axis is
> > maxed out, the contact should probably be treated as a palm anyway...
> >
> > I'm more concerned with this affecting our gesture detection for
> > touchpad. It looks like this change would cause all contacts to
> > reported as some percentage bigger than they are currently. Can you
> > give me an idea of how big that percentage is?
>
> On the P52, I currently have:
> [ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
> [ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429
>
> -> with the computation done in the kernel:
> width_ratio: 126
> height_ratio: 123
>
> For my average finger, the reported traces are between 4 and 6:
> With the ETP_FWIDTH_REDUCE:
> Major between 144 to 216
> Minor between 132 to 198
>
> Without:
> Major between 504 to 756
> Minor between 492 to 738
>
> So a rough augmentation of 350%
>
> For the Synaptics devices (over SMBus), they send the raw value of the traces, so you will get a major/minor between 2 to 5. Max on these axes is 15, so we should get the same percentage of value comparing to the range.
>
> Elan's vendor report contains such information, which indicate how many trace are touched by finger/palm
> mk_x = (finger_data[3] & 0x0f);
> mk_y = (finger_data[3] >> 4);
> Do we need to use mk_* for major/minor for keeping it consistent with other vendor?
IMO, no. It is better to send something closer to an actual unit
instead of 12,5th of mm.
However, the problem here is that major/minor can be swapped depending
on how the finger is placed (horizontally or vertically), so
unfortunately, if the axes and resolutions are not the same, then we
are screwed, this would just be a value without unit.
> But this modification will impact Chromebook's usability and Chrome test suite.
Yeah, there is no point breaking things just for the fun of it.
Cheers,
Benjamin
>
>
>
> Which is why libinput has a database of which device reports which pressure/major/minor ranges as otherwise the values are just impossible to understand.
>
> Cheers,
> Benjamin
>
>
>
> >
> > On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
> > >
> > > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > Hi Benjamin, KT,
> > > >
> > > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > > > Hi
> > > > >
> > > > > -----Original Message-----
> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > > > Sent: Friday, May 24, 2019 5:37 PM
> > > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de
> > > > > Goede
> > > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true
> > > > > width/height
> > > > >
> > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > > > >
> > > > > > The width/height is actually in the same unit than X and Y. So
> > > > > > we should not tamper the data, but just set the proper
> > > > > > resolution, so that userspace can correctly detect which touch is a palm or a finger.
> > > > > >
> > > > > > Signed-off-by: Benjamin Tissoires
> > > > > > <benjamin.tissoires@redhat.com>
> > > > > >
> > > > > > --
> > > > > >
> > > > > > new in v2
> > > > > > ---
> > > > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > > > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > > > @@ -45,7 +45,6 @@
> > > > > > #define DRIVER_NAME "elan_i2c"
> > > > > > #define ELAN_VENDOR_ID 0x04f3
> > > > > > #define ETP_MAX_PRESSURE 255
> > > > > > -#define ETP_FWIDTH_REDUCE 90
> > > > > > #define ETP_FINGER_WIDTH 15
> > > > > > #define ETP_RETRY_COUNT 3
> > > > > >
> > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > > > > return;
> > > > > > }
> > > > > >
> > > > > > - /*
> > > > > > - * To avoid treating large finger as palm, let's reduce the
> > > > > > - * width x and y per trace.
> > > > > > - */
> > > > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > > > + area_x = mk_x * data->width_x;
> > > > > > + area_y = mk_y * data->width_y;
> > > > > >
> > > > > > major = max(area_x, area_y);
> > > > > > minor = min(area_x, area_y); @@ -1123,8
> > > > > > +1118,10 @@ static int elan_setup_input_device(struct elan_tp_data *data)
> > > > > > ETP_MAX_PRESSURE, 0, 0);
> > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > > > > ETP_FINGER_WIDTH * max_width, 0,
> > > > > > 0);
> > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR,
> > > > > > + data->x_res);
> > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > > > > ETP_FINGER_WIDTH * min_width, 0,
> > > > > > 0);
> > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR,
> > > > > > + data->y_res);
> > > > >
> > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > > > >
> > > > > TL;DR: I don't think we should export the resolution there :(
> > > > >
> > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > > > >
> > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> > > >
> > > > Chrome has logic that contact with maximum major/minor is treated
> > > > as a palm, so here the driver (which originally came from Chrome
> > > > OS) artificially reduces the contact size to ensure that palm
> > > > rejection logic does not trigger.
> > > >
> > > > I'm adding Harry to confirm whether we are still using this logic
> > > > and to see if we can adjust it to be something else.
> > >
> > > I'm not very familiar with our touchpad code, so adding Sean
> > > O'Brien, who is.
>
^ permalink raw reply
* Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
From: Peter Hutterer @ 2019-05-30 0:22 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Sean O'Brien, Harry Cutts, Dmitry Torokhov,
廖崇榮, Rob Herring, Aaron Ma, Hans de Goede,
open list:HID CORE LAYER, lkml, devicetree
In-Reply-To: <CAO-hwJ+9tnmvD-K3_Ksesdvag1aNbLB7eJxb9ZKb7kM24unqQQ@mail.gmail.com>
On Wed, May 29, 2019 at 09:16:38AM +0200, Benjamin Tissoires wrote:
> On Wed, May 29, 2019 at 2:12 AM Sean O'Brien <seobrien@chromium.org> wrote:
> >
> > We do still use a maxed out major axis as a signal for a palm in the touchscreen
> > logic, but I'm not too concerned because if that axis is maxed out, the contact
> > should probably be treated as a palm anyway...
> >
> > I'm more concerned with this affecting our gesture detection for
> > touchpad. It looks
> > like this change would cause all contacts to reported as some percentage bigger
> > than they are currently. Can you give me an idea of how big that percentage is?
>
> On the P52, I currently have:
> [ +0.000009] max: (3045,1731) drivers/input/mouse/elan_i2c_core.c:428
> [ +0.000003] traces: (24,14) drivers/input/mouse/elan_i2c_core.c:429
>
> -> with the computation done in the kernel:
> width_ratio: 126
> height_ratio: 123
>
> For my average finger, the reported traces are between 4 and 6:
> With the ETP_FWIDTH_REDUCE:
> Major between 144 to 216
> Minor between 132 to 198
>
> Without:
> Major between 504 to 756
> Minor between 492 to 738
>
> So a rough augmentation of 350%
>
> For the Synaptics devices (over SMBus), they send the raw value of the
> traces, so you will get a major/minor between 2 to 5. Max on these
> axes is 15, so we should get the same percentage of value comparing to
> the range.
> Which is why libinput has a database of which device reports which
> pressure/major/minor ranges as otherwise the values are just
> impossible to understand.
yeah, I've given up on trying to guess finger/thumb/palm sizes.
git grep for these quirk names in libinput for the ranges:
AttrTouchSizeRange
AttrThumbSizeThreshold
AttrPalmSizeThreshold
There are also matching s/Size/Pressure/ entries for touchpads without
major/minor. Looking at the database now, the palm size thresholds range
entries are 5 (Wacom) and a set of 800-1600 for apple touchpads. So yeah,
all this is really a bit random :)
Feel free to steal those entries though and/or add to them where applicable.
Cheers,
Peter
>
> >
> > On Tue, May 28, 2019 at 11:13 AM Harry Cutts <hcutts@chromium.org> wrote:
> > >
> > > On Mon, 27 May 2019 at 18:21, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > Hi Benjamin, KT,
> > > >
> > > > On Mon, May 27, 2019 at 11:55:01AM +0800, 廖崇榮 wrote:
> > > > > Hi
> > > > >
> > > > > -----Original Message-----
> > > > > From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> > > > > Sent: Friday, May 24, 2019 5:37 PM
> > > > > To: Dmitry Torokhov; KT Liao; Rob Herring; Aaron Ma; Hans de Goede
> > > > > Cc: open list:HID CORE LAYER; lkml; devicetree@vger.kernel.org
> > > > > Subject: Re: [PATCH v2 08/10] Input: elan_i2c - export true width/height
> > > > >
> > > > > On Tue, May 21, 2019 at 3:28 PM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> > > > > >
> > > > > > The width/height is actually in the same unit than X and Y. So we
> > > > > > should not tamper the data, but just set the proper resolution, so
> > > > > > that userspace can correctly detect which touch is a palm or a finger.
> > > > > >
> > > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > > >
> > > > > > --
> > > > > >
> > > > > > new in v2
> > > > > > ---
> > > > > > drivers/input/mouse/elan_i2c_core.c | 11 ++++-------
> > > > > > 1 file changed, 4 insertions(+), 7 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/input/mouse/elan_i2c_core.c
> > > > > > b/drivers/input/mouse/elan_i2c_core.c
> > > > > > index 7ff044c6cd11..6f4feedb7765 100644
> > > > > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > > > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > > > > @@ -45,7 +45,6 @@
> > > > > > #define DRIVER_NAME "elan_i2c"
> > > > > > #define ELAN_VENDOR_ID 0x04f3
> > > > > > #define ETP_MAX_PRESSURE 255
> > > > > > -#define ETP_FWIDTH_REDUCE 90
> > > > > > #define ETP_FINGER_WIDTH 15
> > > > > > #define ETP_RETRY_COUNT 3
> > > > > >
> > > > > > @@ -915,12 +914,8 @@ static void elan_report_contact(struct elan_tp_data *data,
> > > > > > return;
> > > > > > }
> > > > > >
> > > > > > - /*
> > > > > > - * To avoid treating large finger as palm, let's reduce the
> > > > > > - * width x and y per trace.
> > > > > > - */
> > > > > > - area_x = mk_x * (data->width_x - ETP_FWIDTH_REDUCE);
> > > > > > - area_y = mk_y * (data->width_y - ETP_FWIDTH_REDUCE);
> > > > > > + area_x = mk_x * data->width_x;
> > > > > > + area_y = mk_y * data->width_y;
> > > > > >
> > > > > > major = max(area_x, area_y);
> > > > > > minor = min(area_x, area_y); @@ -1123,8 +1118,10 @@
> > > > > > static int elan_setup_input_device(struct elan_tp_data *data)
> > > > > > ETP_MAX_PRESSURE, 0, 0);
> > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0,
> > > > > > ETP_FINGER_WIDTH * max_width, 0, 0);
> > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MAJOR, data->x_res);
> > > > > > input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0,
> > > > > > ETP_FINGER_WIDTH * min_width, 0, 0);
> > > > > > + input_abs_set_res(input, ABS_MT_TOUCH_MINOR, data->y_res);
> > > > >
> > > > > I had a chat with Peter on Wednesday, and he mentioned that this is dangerous as Major/Minor are max/min of the width and height. And given that we might have 2 different resolutions, we would need to do some computation in the kernel to ensure the data is correct with respect to the resolution.
> > > > >
> > > > > TL;DR: I don't think we should export the resolution there :(
> > > > >
> > > > > KT, should I drop the patch entirely, or is there a strong argument for keeping the ETP_FWIDTH_REDUCE around?
> > > > > I suggest you apply the patch, I have no idea why ETP_FWIDTH_REDUCE existed.
> > > > > Our FW team know nothing about ETP_FWIDTH_REDUCE ether.
> > > > >
> > > > > The only side effect will happen on Chromebook because such computation have stayed in ChromeOS' kernel for four years.
> > > > > Chrome's finger/palm threshold may be different from other Linux distribution.
> > > > > We will discuss it with Google once the patch picked by chrome and cause something wrong.
> > > >
> > > > Chrome has logic that contact with maximum major/minor is treated as a
> > > > palm, so here the driver (which originally came from Chrome OS)
> > > > artificially reduces the contact size to ensure that palm rejection
> > > > logic does not trigger.
> > > >
> > > > I'm adding Harry to confirm whether we are still using this logic and to
> > > > see if we can adjust it to be something else.
> > >
> > > I'm not very familiar with our touchpad code, so adding Sean O'Brien, who is.
^ permalink raw reply
* Re: [PATCH] [trivial] HID: Typo s/to back 0/back to 0/
From: Andrej Shadura @ 2019-05-30 9:00 UTC (permalink / raw)
To: Geert Uytterhoeven, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel
In-Reply-To: <20190527122532.6084-1-geert+renesas@glider.be>
On 27/05/2019 14:25, Geert Uytterhoeven wrote:
> Fix a silly word ordering typo.
>
> Fixes: 42337b9d4d958daa ("HID: add driver for U2F Zero built-in LED and RNG")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> drivers/hid/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index c3c390ca369022f0..735223f90035b2bf 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1028,7 +1028,7 @@ config HID_U2FZERO
>
> U2F Zero only supports blinking its LED, so this driver doesn't
> allow setting the brightness to anything but 1, which will
> - trigger a single blink and immediately reset to back 0.
> + trigger a single blink and immediately reset back to 0.
>
> config HID_WACOM
> tristate "Wacom Intuos/Graphire tablet support (USB)"
That was a silly typo indeed :D
Not sure this is needed, but just in case:
Acked-by: Andrej Shadura <andrew.shadura@collabora.co.uk>
--
Cheers,
Andrej
^ permalink raw reply
* Re: hid-related 5.2-rc1 boot hang
From: Dave Hansen @ 2019-05-30 16:56 UTC (permalink / raw)
To: Hans de Goede, Benjamin Tissoires, Jiri Kosina
Cc: open list:HID CORE LAYER, LKML
In-Reply-To: <4689a737-6c40-b4ae-cc38-5df60318adce@redhat.com>
On 5/29/19 2:17 AM, Hans de Goede wrote:
...
> Dave, can you try building your initrd without the hid-logitech-dj module
> included in the initrd?
I did this on a vanilla 5.2-rc2 kernel (without the reverts) and still
experienced the boot hang while the device was inserted.
> Also can you check if your modprobe is provided by module-init-tools
> or by kmod ?
$ dpkg -S `which modprobe`
kmod: /sbin/modprobe
^ permalink raw reply
* [PATCH] drivers: hid: Add a module description line to the hid_hyperv driver
From: Sasha Levin @ 2019-05-30 17:37 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel
From: Joseph Salisbury <Joseph.Salisbury@microsoft.com>
This patch only adds a MODULE_DESCRIPTION statement to the driver.
This change is only cosmetic, so there should be no runtime impact.
Signed-off-by: Joseph Salisbury <joseph.salisbury@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-hyperv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
index 704049e62d58a..d3311d714d359 100644
--- a/drivers/hid/hid-hyperv.c
+++ b/drivers/hid/hid-hyperv.c
@@ -614,5 +614,7 @@ static void __exit mousevsc_exit(void)
}
MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Microsoft Hyper-V Synthetic HID Driver");
+
module_init(mousevsc_init);
module_exit(mousevsc_exit);
--
2.20.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox