From: José Expósito <hidden> Date: 2021-11-23 19:12:56
Hi all,
Historically, libinput has relayed on the INPUT_PROP_BUTTONPAD property
to detect buttonpads.
Since buttonpads are expected to have only one button (BTN_LEFT),
recently we added a new rule to detect buttonpads: Where a touchpad
maps the BTN_RIGHT bit, libinput assumes it is NOT a buttonpad.
However, this change leaded to several false possitives, so we ended up
reverting it. For more context:
https://gitlab.freedesktop.org/libinput/libinput/-/issues/704
And for a full list of affected hardware, HID reports and bug reports
please see:
https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/726
My understanding is that buttonpads should not map BTN_RIGHT and/or
BTN_MIDDLE and to avoid it I would like to fix the required drivers.
One option to fix it (this patch) is to clear the bits that might have
been added because of the HID descriptor on every driver.
However, since this code will be common to all drivers, I would like to
ask if you consider it worth it to add a function to handle adding
properties.
A function similar to input_set_capability but for props could be added
in input.h/c:
/**
* input_set_property - add a property to the device
* @dev: device to add the property to
* @property: type of the property (INPUT_PROP_POINTER, INPUT_PROP_DIRECT...)
*
* In addition to setting up corresponding bit in dev->propbit the function
* might add or remove related capabilities.
*/
void input_set_property(struct input_dev *dev, unsigned int property)
{
switch (property) {
case INPUT_PROP_POINTER:
case INPUT_PROP_DIRECT:
case INPUT_PROP_SEMI_MT:
case INPUT_PROP_TOPBUTTONPAD:
case INPUT_PROP_POINTING_STICK:
case INPUT_PROP_ACCELEROMETER:
break;
case INPUT_PROP_BUTTONPAD:
input_set_capability(dev, EV_KEY, BTN_LEFT);
__clear_bit(BTN_RIGHT, dev->keybit);
__clear_bit(BTN_MIDDLE, dev->keybit);
break;
default:
pr_err("%s: unknown property %u\n", __func__, property);
dump_stack();
return;
}
__set_bit(property, dev->propbit);
}
EXPORT_SYMBOL(input_set_property);
Which approach do you think is the best?
Thank you very much in advance,
Jose
José Expósito (1):
HID: multitouch: only map BTN_LEFT on buttonpads
drivers/hid/hid-multitouch.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
--
2.25.1
From: Benjamin Tissoires <hidden> Date: 2021-11-24 09:39:22
Hi José,
On Tue, Nov 23, 2021 at 8:12 PM José Expósito [off-list ref] wrote:
Hi all,
Historically, libinput has relayed on the INPUT_PROP_BUTTONPAD property
to detect buttonpads.
Since buttonpads are expected to have only one button (BTN_LEFT),
recently we added a new rule to detect buttonpads: Where a touchpad
maps the BTN_RIGHT bit, libinput assumes it is NOT a buttonpad.
However, this change leaded to several false possitives, so we ended up
reverting it. For more context:
https://gitlab.freedesktop.org/libinput/libinput/-/issues/704
And for a full list of affected hardware, HID reports and bug reports
please see:
https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/726
My understanding is that buttonpads should not map BTN_RIGHT and/or
BTN_MIDDLE and to avoid it I would like to fix the required drivers.
One option to fix it (this patch) is to clear the bits that might have
been added because of the HID descriptor on every driver.
However, since this code will be common to all drivers, I would like to
ask if you consider it worth it to add a function to handle adding
properties.
A function similar to input_set_capability but for props could be added
in input.h/c:
/**
* input_set_property - add a property to the device
* @dev: device to add the property to
* @property: type of the property (INPUT_PROP_POINTER, INPUT_PROP_DIRECT...)
*
* In addition to setting up corresponding bit in dev->propbit the function
* might add or remove related capabilities.
*/
void input_set_property(struct input_dev *dev, unsigned int property)
{
switch (property) {
case INPUT_PROP_POINTER:
case INPUT_PROP_DIRECT:
case INPUT_PROP_SEMI_MT:
case INPUT_PROP_TOPBUTTONPAD:
case INPUT_PROP_POINTING_STICK:
case INPUT_PROP_ACCELEROMETER:
break;
case INPUT_PROP_BUTTONPAD:
input_set_capability(dev, EV_KEY, BTN_LEFT);
__clear_bit(BTN_RIGHT, dev->keybit);
__clear_bit(BTN_MIDDLE, dev->keybit);
break;
default:
pr_err("%s: unknown property %u\n", __func__, property);
dump_stack();
return;
}
__set_bit(property, dev->propbit);
}
EXPORT_SYMBOL(input_set_property);
Which approach do you think is the best?
I think it depends if you plan on fixing just hid-multitouch or the others.
If you have more than one driver, then yes, adding a new symbol in
hid-input.c makes sense. If not, then you are just exposing a new
function we won't know if there are users and we won't be able to
change without care.
Cheers,
Benjamin
Thank you very much in advance,
Jose
José Expósito (1):
HID: multitouch: only map BTN_LEFT on buttonpads
drivers/hid/hid-multitouch.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
--
2.25.1
From: José Expósito <hidden> Date: 2021-11-24 19:53:28
Hi Benjamin,
Thank you very much for your quick answer.
On Wed, Nov 24, 2021 at 10:39:02AM +0100, Benjamin Tissoires wrote:
As long as udev intrinsic is happy with it (and it correctly tags the
touchpad as ID_INPUT_something), I'm fine with it.
Yes, the device is still tagged correctly. For example, this is the original
output for "libinput record" (libinput issue 674):
Supported Events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 272 (BTN_LEFT)
Event code 273 (BTN_RIGHT)
Event code 325 (BTN_TOOL_FINGER)
[...]
udev:
properties:
- ID_INPUT=1
- ID_INPUT_HEIGHT_MM=61
- ID_INPUT_TOUCHPAD=1
- ID_INPUT_WIDTH_MM=93
And the same output after applying the patch:
Supported Events:
Event type 0 (EV_SYN)
Event type 1 (EV_KEY)
Event code 272 (BTN_LEFT)
Event code 325 (BTN_TOOL_FINGER)
[...]
udev:
properties:
- ID_INPUT=1
- ID_INPUT_HEIGHT_MM=61
- ID_INPUT_TOUCHPAD=1
- ID_INPUT_WIDTH_MM=93
Notice that BTN_RIGHT is not present but the udev tags are the same.
I don't have access to that specific touchpad, but I own a Magic
Trackpad 1 and 2 -whose driver clears the BTN_RIGHT bit- and they
are properly tagged as well.
I think it depends if you plan on fixing just hid-multitouch or the others.
If you have more than one driver, then yes, adding a new symbol in
hid-input.c makes sense. If not, then you are just exposing a new
function we won't know if there are users and we won't be able to
change without care.
I'd like to fix the issue on every driver. It is not a big amount of
duplicated code, just a couple of lines on drivers that don't already
clear the BTN_RIGHT/MIDDLE bit, but I agree with you, moving into a
common function is cleaner.
Also, the "input_set_property" function would allow us to add more
conditions associated with other properties in case we wanted to.
Thanks again for your input, I'll send the patchset for review as soon as
possible.
Jose
From: Peter Hutterer <hidden> Date: 2021-12-01 05:56:51
On Wed, Nov 24, 2021 at 10:39:02AM +0100, Benjamin Tissoires wrote:
Hi José,
On Tue, Nov 23, 2021 at 8:12 PM José Expósito [off-list ref] wrote:
quoted
Hi all,
Historically, libinput has relayed on the INPUT_PROP_BUTTONPAD property
to detect buttonpads.
Since buttonpads are expected to have only one button (BTN_LEFT),
recently we added a new rule to detect buttonpads: Where a touchpad
maps the BTN_RIGHT bit, libinput assumes it is NOT a buttonpad.
However, this change leaded to several false possitives, so we ended up
reverting it. For more context:
https://gitlab.freedesktop.org/libinput/libinput/-/issues/704
And for a full list of affected hardware, HID reports and bug reports
please see:
https://gitlab.freedesktop.org/libinput/libinput/-/merge_requests/726
My understanding is that buttonpads should not map BTN_RIGHT and/or
BTN_MIDDLE and to avoid it I would like to fix the required drivers.
As long as udev intrinsic is happy with it (and it correctly tags the
touchpad as ID_INPUT_something), I'm fine with it.
fwiw, udev's builtin input-id touchpad check is
ABS_X && ABS_Y && BTN_TOOL_FINGER && !BTN_TOOL_PEN && !INPUT_PROP_DIRECT
it doesn't care about the actual buttons so this patch wouldn't affect it.
Yeah, it sounds like there *should* not be any buttons but
There is nothing to explicitly forbid extra buttons for click/pressurepads
which is probably how those devices get past the windows driver
implementation.
Cheers,
Peter