From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2E269C2D0DB for ; Fri, 31 Jan 2020 15:46:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E621420CC7 for ; Fri, 31 Jan 2020 15:45:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="ZzEZmXoK" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729030AbgAaPp7 (ORCPT ); Fri, 31 Jan 2020 10:45:59 -0500 Received: from us-smtp-2.mimecast.com ([207.211.31.81]:20610 "EHLO us-smtp-delivery-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728846AbgAaPp7 (ORCPT ); Fri, 31 Jan 2020 10:45:59 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580485558; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ojdqvwAj2g8/T0aLiCiKPcuvDlaGomkubMbQbtIHtOo=; b=ZzEZmXoKLll+mHmmZpmOBPGHVwFhicKx0D/1XbOjSwnnSlzZcSqvy7hCJQdCZAxU/4nxR5 lxI90UQN51iX6dY1QMHXs/Fc2QBCF1WbRIvrD/bH7XRVmYTNL7wRoDVW4DCVq5zluyzXVQ vi9qkykmdrMVDa7ApCcbEFKELnRXmGA= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-195-E131Qht3NiypkTyJKqH_9g-1; Fri, 31 Jan 2020 10:45:42 -0500 X-MC-Unique: E131Qht3NiypkTyJKqH_9g-1 Received: by mail-wr1-f69.google.com with SMTP id v17so3505276wrm.17 for ; Fri, 31 Jan 2020 07:45:42 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=ojdqvwAj2g8/T0aLiCiKPcuvDlaGomkubMbQbtIHtOo=; b=a6gn0jbX0SCdwJwQW411m9Xw4PIHxOhByLOf2hsKzLRf54YbPBczss/geT5Dk2eYuz pf6YWj3gTDTOyuJ+vJUi+h2XdVlESJcNgKooKPrG1uzVinEKXM+/LamTz4B4t9/sO6l2 4RnqTC0SaH5BcrIsgXxAEFDLTuABCaqNtrGBlmVLKGlhfiY1wLrl0hcu3/bUn3KCdujt OrjgZ9D6qYkb9nlhe4xyqbzhMWCOcx2xmDoxZWg+xZoS5j6Hr9tkX3IZOVwgMnxWSKSW wgPatzXB+xI+g/6nZ1ebHeHECjjZexNbaTcJGMCsurxbNlmQSMdEuEudDPYIorrv65iN 9g6g== X-Gm-Message-State: APjAAAW11t0WqEJKsfkUrSXsS53kPTAS6MmOYNkHEyIn/MVUy+iR1KxM pfjHvBXa8AuCx2dRjxOLkVFP/zfaBZWFxNB+BwbhGK2LEWJhI7xt8YvcnrfLaaRaoTtZ4+7Qf99 /tEmNRXPcTZItIv2qwMnNcm8= X-Received: by 2002:a7b:cf01:: with SMTP id l1mr12624966wmg.86.1580485540963; Fri, 31 Jan 2020 07:45:40 -0800 (PST) X-Google-Smtp-Source: APXvYqxab9HnbNSQW2BwxHCZktavfkKg3vJVJ7HVzz8qxyt3ch26FGDLZ/+PanYkyqbf4AUz3Z7zwQ== X-Received: by 2002:a7b:cf01:: with SMTP id l1mr12624930wmg.86.1580485540531; Fri, 31 Jan 2020 07:45:40 -0800 (PST) Received: from localhost.localdomain ([62.72.193.75]) by smtp.gmail.com with ESMTPSA id b137sm12146107wme.26.2020.01.31.07.45.39 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 31 Jan 2020 07:45:40 -0800 (PST) Subject: Re: [PATCH] HID: ite: Only bind to keyboard USB interface on Acer SW5-012 keyboard dock To: Z R , Benjamin Tissoires Cc: Jiri Kosina , "open list:HID CORE LAYER" , "3.8+" References: <20200131124553.27796-1-hdegoede@redhat.com> <6d94cf74-c28f-08b0-a136-044c231b8bc5@redhat.com> From: Hans de Goede Message-ID: Date: Fri, 31 Jan 2020 16:45:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org Hi, On 1/31/20 4:38 PM, Z R wrote: > Hi Benjamin, > hid-record for keyboard and touchpad. With Commit 8f18eca9ebc5 reverted and from unmodified kernel. > > I hope it is what you asked for :-) > > Currently waiting for reworked patch from Hans. I just send you the reworked patch. Does the recordning include pressing of the wlan on/off key (Fn + F3 I believe) ? That is the whole reason why the special hid-ite driver is necessary. Benjamin about the wlan on/off key. AFAICR on a press + release of the key a single hid input report for the generic-desktop Wireless Radio Controls group is send. This input-report only has the one button with usage code HID_GD_RFKILL_BTN in there and it is always 0. It is as if the input-report is only send on release and not on press. So the hid-ite code emulates a press + release whenever the input-report is send. IOW the receiving of the input report is (ab)used as indication of the button having been pressed. Regards, Hans > > Bye for now > Zdeněk > > pá 31. 1. 2020 v 15:12 odesílatel Benjamin Tissoires > napsal: > > On Fri, Jan 31, 2020 at 3:04 PM Hans de Goede > wrote: > > > > Hi, > > > > On 1/31/20 2:54 PM, Benjamin Tissoires wrote: > > > On Fri, Jan 31, 2020 at 2:41 PM Hans de Goede > 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 > 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@vger.kernel.org > > >>>> Fixes: 8f18eca9ebc5 ("HID: ite: Add USB id match for Acer SW5-012 keyboard dock") > > >>>> Reported-by: Zdeněk Rampas > > > >>>> Signed-off-by: Hans de Goede > > > >>>> --- > > >>>>    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 > > >>>>    #include > > >>>>    #include > > >>>> +#include > > >>>> > > >>>>    #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 :) > > > > Actually if we want to check the group and there are only 2 interfaces we do > > not need to use the match callback at all, w e can simply match on the > > group of the interface which we do want: > > > > diff --git a/drivers/hid/hid-ite.c b/drivers/hid/hid-ite.c > > index db0f35be5a8b..21bd48f16033 100644 > > --- a/drivers/hid/hid-ite.c > > +++ b/drivers/hid/hid-ite.c > > @@ -56,8 +56,9 @@ static const struct hid_device_id ite_devices[] = { > >         { HID_USB_DEVICE(USB_VENDOR_ID_ITE, USB_DEVICE_ID_ITE8595) }, > >         { HID_USB_DEVICE(USB_VENDOR_ID_258A, USB_DEVICE_ID_258A_6A88) }, > >         /* ITE8595 USB kbd ctlr, with Synaptics touchpad connected to it. */ > > -       { HID_USB_DEVICE(USB_VENDOR_ID_SYNAPTICS, > > -                        USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5_012) }, > > +       { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC, > > +                    USB_VENDOR_ID_SYNAPTICS, > > +                    USB_DEVICE_ID_SYNAPTICS_ACER_SWITCH5_012) }, > >         { } > >   }; > >   MODULE_DEVICE_TABLE(hid, ite_devices); > > > > Much cleaner > > yep > > > (and now I don't need to write a test, which is always > > a good motivation to come up with a cleaner solution :) > > Hehe, too bad, you already picked up my curiosity on this one, and I > really would like to see the report descriptors and some events of the > keys that are fixed by hid-ite.c. > This will be a hard requirement to accept this patch . > > More seriously, Zdeněk, can you run hid-recorder from > https://gitlab.freedesktop.org/libevdev/hid-tools/ and provide me the > report descriptor for all of your ITE HID devices? I'll add the > matching tests in hid-tools and be sure we do not regress in the > future. > > > > > Let me turn this into a proper patch and then I will send that to > > Zdeněk (off-list) for him to test (note don't worry if you do > > not have time to test this weekend, then I'll do it on Monday). > > > > Regards, > > > > Hans > > > > p.s. > > > > 1. My train is approaching Brussels (Fosdem) so my email response > > time will soon become irregular. > > How dare you? :) > > > > > 2. Benjamin will you be at Fosdem too ? > > > > Unfortunately no. Already got my quota of meeting people for this year > between Kernel Recipes in September, XDC in October and LCA last week. > So I need to keep in a quiet environment for a little bit :) > > Cheers, > Benjamin >