On Fri, Jan 31, 2020 at 2:41 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> 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 <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> 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.
> >
> >>
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock")
> >> Reported-by: Zdeněk Rampas <zdenda.rampas@xxxxxxxxx>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> 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.
right. An other solution would be to have a local macro/function that
does that. Because as soon as you start adding a quirk, an other comes
right after.
>
> But see below.
>
> >
> >> + 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;
> }
Yep, I like that very much :)
>
> ? (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 ?
So, if I am not wrong:
- keyboard docks with an ITE 8595 keyboard/touchpad controller are
registered in hid-ite.c, but should be really bound to hid-multitouch.
- there are at least 2 HID interfaces (sharing the same VID/PID), one
for the keyboard (hid-ite.c), and one for the touchpad
(hid-multitouch.c)
- what matters in the end, is that the multitouch interface gets
correctly mapped and handled.
So if that is somehow accurate, I think a test would be a matter of:
- duplicating https://gitlab.freedesktop.org/libevdev/hid-tools/blob/master/tests/test_multitouch.py#L1568-1570,
- add the report descriptors of the touchpad interface,
- and check that the unpatched kernel picks up hid-ite.c, which leads
to errors in the multitouch tests
- ideally, we should also add a test for the expected hid-ite.c part,
but I can do that later with the report descriptors.
So initially, it shouldn't be more than a 3 lines patch (hopefully).
Actually 4, because you also need to force the VID/PID with
`info=(0x3, 0xVID, 0xPID)` (see TestZytronic_14c8_0005 for an example,
in the same file)
Cheers,
Benjamin
>
> Regards,
>
> Hans
