LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE on buttonpads
@ 2021-11-23 19:12 José Expósito
  2021-11-23 19:12 ` [PATCH 1/1] HID: multitouch: only map BTN_LEFT " José Expósito
  2021-11-24  9:39 ` [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE " Benjamin Tissoires
  0 siblings, 2 replies; 6+ messages in thread
From: José Expósito @ 2021-11-23 19:12 UTC (permalink / raw)
  To: jikos
  Cc: benjamin.tissoires, peter.hutterer, linux-input, linux-kernel,
	José Expósito

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/1] HID: multitouch: only map BTN_LEFT on buttonpads
  2021-11-23 19:12 [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE on buttonpads José Expósito
@ 2021-11-23 19:12 ` José Expósito
  2021-11-24  8:43   ` kernel test robot
  2021-11-24  9:39 ` [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE " Benjamin Tissoires
  1 sibling, 1 reply; 6+ messages in thread
From: José Expósito @ 2021-11-23 19:12 UTC (permalink / raw)
  To: jikos
  Cc: benjamin.tissoires, peter.hutterer, linux-input, linux-kernel,
	José Expósito

In addition to map the INPUT_PROP_BUTTONPAD property, make sure that
the BTN_RIGHT and BTN_MIDDLE key bits are not mapped.

Mapping more than one button on buttonpads is a bug plus avoids issues
with some touchpads on user space. For more information, check these
bug reports:

 - https://gitlab.freedesktop.org/libinput/libinput/-/issues/674
 - https://gitlab.freedesktop.org/libinput/libinput/-/issues/689
 - https://gitlab.freedesktop.org/libinput/libinput/-/issues/629

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/hid/hid-multitouch.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index e1afddb7b33d..37697ebe27f9 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1286,8 +1286,11 @@ static int mt_touch_input_configured(struct hid_device *hdev,
 	    (app->buttons_count == 1))
 		td->is_buttonpad = true;
 
-	if (td->is_buttonpad)
+	if (td->is_buttonpad) {
 		__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
+		__clear_bit(BTN_RIGHT, dev->keybit);
+		__clear_bit(BTN_MIDDLE, dev->keybit);
+	}
 
 	app->pending_palm_slots = devm_kcalloc(&hi->input->dev,
 					       BITS_TO_LONGS(td->maxcontacts),
-- 
2.25.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] HID: multitouch: only map BTN_LEFT on buttonpads
  2021-11-23 19:12 ` [PATCH 1/1] HID: multitouch: only map BTN_LEFT " José Expósito
@ 2021-11-24  8:43   ` kernel test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2021-11-24  8:43 UTC (permalink / raw)
  To: José Expósito, jikos
  Cc: kbuild-all, benjamin.tissoires, peter.hutterer, linux-input,
	linux-kernel, José Expósito

Hi "José,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hid/for-next]
[also build test ERROR on v5.16-rc2 next-20211124]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Jos-Exp-sito/Do-not-map-BTN_RIGHT-MIDDLE-on-buttonpads/20211124-031407
base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20211124/202111241646.G968t85X-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/67a7bd322df749f6ef9a142479668db93b93beca
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jos-Exp-sito/Do-not-map-BTN_RIGHT-MIDDLE-on-buttonpads/20211124-031407
        git checkout 67a7bd322df749f6ef9a142479668db93b93beca
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/hid/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/hid/hid-multitouch.c: In function 'mt_touch_input_configured':
>> drivers/hid/hid-multitouch.c:1291:26: error: 'dev' undeclared (first use in this function); did you mean 'hdev'?
    1291 |   __clear_bit(BTN_RIGHT, dev->keybit);
         |                          ^~~
         |                          hdev
   drivers/hid/hid-multitouch.c:1291:26: note: each undeclared identifier is reported only once for each function it appears in


vim +1291 drivers/hid/hid-multitouch.c

  1261	
  1262	static int mt_touch_input_configured(struct hid_device *hdev,
  1263					     struct hid_input *hi,
  1264					     struct mt_application *app)
  1265	{
  1266		struct mt_device *td = hid_get_drvdata(hdev);
  1267		struct mt_class *cls = &td->mtclass;
  1268		struct input_dev *input = hi->input;
  1269		int ret;
  1270	
  1271		if (!td->maxcontacts)
  1272			td->maxcontacts = MT_DEFAULT_MAXCONTACT;
  1273	
  1274		mt_post_parse(td, app);
  1275		if (td->serial_maybe)
  1276			mt_post_parse_default_settings(td, app);
  1277	
  1278		if (cls->is_indirect)
  1279			app->mt_flags |= INPUT_MT_POINTER;
  1280	
  1281		if (app->quirks & MT_QUIRK_NOT_SEEN_MEANS_UP)
  1282			app->mt_flags |= INPUT_MT_DROP_UNUSED;
  1283	
  1284		/* check for clickpads */
  1285		if ((app->mt_flags & INPUT_MT_POINTER) &&
  1286		    (app->buttons_count == 1))
  1287			td->is_buttonpad = true;
  1288	
  1289		if (td->is_buttonpad) {
  1290			__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
> 1291			__clear_bit(BTN_RIGHT, dev->keybit);
  1292			__clear_bit(BTN_MIDDLE, dev->keybit);
  1293		}
  1294	
  1295		app->pending_palm_slots = devm_kcalloc(&hi->input->dev,
  1296						       BITS_TO_LONGS(td->maxcontacts),
  1297						       sizeof(long),
  1298						       GFP_KERNEL);
  1299		if (!app->pending_palm_slots)
  1300			return -ENOMEM;
  1301	
  1302		ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags);
  1303		if (ret)
  1304			return ret;
  1305	
  1306		app->mt_flags = 0;
  1307		return 0;
  1308	}
  1309	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE on buttonpads
  2021-11-23 19:12 [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE on buttonpads José Expósito
  2021-11-23 19:12 ` [PATCH 1/1] HID: multitouch: only map BTN_LEFT " José Expósito
@ 2021-11-24  9:39 ` Benjamin Tissoires
  2021-11-24 19:53   ` José Expósito
  2021-12-01  5:56   ` Peter Hutterer
  1 sibling, 2 replies; 6+ messages in thread
From: Benjamin Tissoires @ 2021-11-24  9:39 UTC (permalink / raw)
  To: José Expósito
  Cc: Jiri Kosina, Peter Hutterer, open list:HID CORE LAYER, lkml

Hi José,

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE on buttonpads
  2021-11-24  9:39 ` [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE " Benjamin Tissoires
@ 2021-11-24 19:53   ` José Expósito
  2021-12-01  5:56   ` Peter Hutterer
  1 sibling, 0 replies; 6+ messages in thread
From: José Expósito @ 2021-11-24 19:53 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Jiri Kosina, Peter Hutterer, open list:HID CORE LAYER, lkml

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE on buttonpads
  2021-11-24  9:39 ` [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE " Benjamin Tissoires
  2021-11-24 19:53   ` José Expósito
@ 2021-12-01  5:56   ` Peter Hutterer
  1 sibling, 0 replies; 6+ messages in thread
From: Peter Hutterer @ 2021-12-01  5:56 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: José Expósito, Jiri Kosina, open list:HID CORE LAYER, lkml

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 <jose.exposito89@gmail.com> 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.

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.

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

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-12-01  5:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-23 19:12 [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE on buttonpads José Expósito
2021-11-23 19:12 ` [PATCH 1/1] HID: multitouch: only map BTN_LEFT " José Expósito
2021-11-24  8:43   ` kernel test robot
2021-11-24  9:39 ` [PATCH 0/1] Do not map BTN_RIGHT/MIDDLE " Benjamin Tissoires
2021-11-24 19:53   ` José Expósito
2021-12-01  5:56   ` Peter Hutterer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).