Hi,

On 1/31/20 2:10 PM, Benjamin Tissoires wrote:

Hi Hans,

On Fri, Jan 31, 2020 at 1:46 PM Hans de Goede [off-list ref] wrote:

quoted

Commit 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard
dock") added the USB id for the Acer SW5-012's keyboard dock to the
hid-ite driver to fix the rfkill driver not working.

Most keyboard docks with an ITE 8595 keyboard/touchpad controller have the
"Wireless Radio Control" bits which need the special hid-ite driver on the
second USB interface (the mouse interface) and their touchpad only supports
mouse emulation, so using generic hid-input handling for anything but
the "Wireless Radio Control" bits is fine. On these devices we simply bind
to all USB interfaces.

But unlike other ITE8595 using keyboard docks, the Acer Aspire Switch 10
(SW5-012)'s touchpad not only does mouse emulation it also supports
HID-multitouch and all the keys including the "Wireless Radio Control"
bits have been moved to the first USB interface (the keyboard intf).

So we need hid-ite to handle the first (keyboard) USB interface and have
it NOT bind to the second (mouse) USB interface so that that can be
handled by hid-multitouch.c and we get proper multi-touch support.

This commit adds a match callback to hid-ite which makes it only
match the first USB interface when running on the Acer SW5-012,
fixing the regression to mouse-emulation mode introduced by adding the
keyboard dock USB id.

Note the match function only does the special only bind to the first
USB interface on the Acer SW5-012, on other devices the hid-ite driver
actually must bind to the second interface as that is where the
"Wireless Radio Control" bits are.

This is not a full review, but a couple of things that popped out
while scrolling through the patch.

quoted

Cc: stable@vger.kernel.org
Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
Reported-by: Zdeněk Rampas <redacted>
Signed-off-by: Hans de Goede <redacted>
---
  drivers/hid/hid-ite.c | 34 ++++++++++++++++++++++++++++++++++
  1 file changed, 34 insertions(+)

diff --git a/drivers/hid/hid-ite.c b/drivers/hid/hid-ite.c
index c436e12feb23..69a4ddfd033d 100644
--- a/drivers/hid/hid-ite.c
+++ b/drivers/hid/hid-ite.c

@@ -8,9 +8,12 @@
  #include <linux/input.h>
  #include <linux/hid.h>
  #include <linux/module.h>
+#include <linux/usb.h>

  #include "hid-ids.h"

+#define ITE8595_KBD_USB_INTF           0
+
  static int ite_event(struct hid_device *hdev, struct hid_field *field,
                      struct hid_usage *usage, __s32 value)
  {

@@ -37,6 +40,36 @@ static int ite_event(struct hid_device *hdev, struct hid_field *field,
         return 0;
  }

+static bool ite_match(struct hid_device *hdev, bool ignore_special_driver)
+{
+       struct usb_interface *intf;
+
+       if (ignore_special_driver)
+               return false;
+
+       /*
+        * Most keyboard docks with an ITE 8595 keyboard/touchpad controller
+        * have the "Wireless Radio Control" bits which need this special
+        * driver on the second USB interface (the mouse interface). On
+        * these devices we simply bind to all USB interfaces.
+        *
+        * The Acer Aspire Switch 10 (SW5-012) is special, its touchpad
+        * not only does mouse emulation it also supports HID-multitouch
+        * and all the keys including the "Wireless Radio Control" bits
+        * have been moved to the first USB interface (the keyboard intf).
+        *
+        * We want the hid-multitouch driver to bind to the touchpad, so on
+        * the Acer SW5-012 we should only bind to the keyboard USB intf.
+        */
+       if (hdev->bus != BUS_USB || hdev->vendor != USB_VENDOR_ID_SYNAPTICS ||
+                    hdev->product != USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5_012)

Isn't there an existing matching function we can use here, instead of
checking each individual field?

There is hid_match_one_id() but that is not exported (can be fixed) and it
requires a struct hid_device_id, which either requires declaring an extra
standalone struct hid_device_id for the SW5-012 kbd-dock, or hardcoding an
index into the existing hid_device_id array for the driver (with the hardcoding
being error prone, so not a good idea).

Given the problems with using hid_match_one_id() I decided to just go with
the above.

But see below.

quoted

+               return true;
+
+       intf = to_usb_interface(hdev->dev.parent);

And this is oops-prone. You need:
- ensure hid_is_using_ll_driver(hdev, &usb_hid_driver) returns true.
- add a dependency on USBHID in the KConfig now that you are checking
on the USB transport layer.

That being said, I would love instead:
- to have a non USB version of this match, where you decide which
component needs to be handled based on the report descriptor

Actually your idea to use the desciptors is not bad, but since what
we really want is to not bind to the interface which is marked for the
hid-multitouch driver I just realized we can just check that.

So how about:

static bool ite_match(struct hid_device *hdev, bool ignore_special_driver)
{
         if (ignore_special_driver)
                 return false;

         /*
          * Some keyboard docks with an ITE 8595 keyboard/touchpad controller
          * support the HID multitouch protocol for the touchpad, in that
          * case the "Wireless Radio Control" bits which we care about are
          * on the other interface; and we should not bind to the multitouch
          * capable interface as that breaks multitouch support.
          */
         return hdev->group != HID_GROUP_MULTITOUCH_WIN_8;
}

? (note untested)

Zdeněk  I have attached a new version of the patch which uses this
improved version of the match function, if you have a chance to test it
this weekend that would be great, otherwise I will test it on my own
sw5-012 on Monday.

- have a regression test in
https://gitlab.freedesktop.org/libevdev/hid-tools for this particular
device, because I never intended the .match callback to be used by
anybody else than hid-generic, and opening this can of worms is prone
to introduce regressions in the future.

Ugh, I can understand your desire for a test for this, but writing
tests is not really my thing. Anyways do you have an example test I
could use as a start ?

Regards,

Hans