Re: [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE on buttonpads

[Benjamin Tissoires]

Hi José,

On Tue, Nov 23, 2021 at 8:12 PM José Expósito <jose.exposito89@xxxxxxxxx> 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.

As long as udev intrinsic is happy with it (and it correctly tags the
touchpad as ID_INPUT_something), I'm fine with it.

Also, you might want to point at the specification regarding button
pads: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection#device-capabilities-feature-report

The way I read it: if the device exports the Button type value
feature, and it is 0 or 1 (click-pad or pressure-pad), there should
not be discrete buttons.

>
> 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
>