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.4 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,USER_AGENT_SANE_1 autolearn=unavailable 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 38AE9C2D0DB for ; Fri, 31 Jan 2020 14:04:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id ECE542067C for ; Fri, 31 Jan 2020 14:04:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="UR4jKORw" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728735AbgAaOEn (ORCPT ); Fri, 31 Jan 2020 09:04:43 -0500 Received: from us-smtp-delivery-1.mimecast.com ([205.139.110.120]:44370 "EHLO us-smtp-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728500AbgAaOEn (ORCPT ); Fri, 31 Jan 2020 09:04:43 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580479482; 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=VdOW7PNJeuMmmDY9kZwQNkZeEVlpGmsTxVCL30ndvIg=; b=UR4jKORwiyyaYhqdDmn4LrTpvyIOe21CQqwpTRZdjfcTPYyoRsQe6SyvraEkdWoW20xd0z jJmqWOyUKEqUXNhTgBZEVnvz1VArxQkwcQfQcArVYTX25mURXRI9iI+SUTSsnpPD38W7bj cXHxZvVWlcMfoqYuU1PZD8iKdSbr9zg= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-194-x7Suu6xLMl6SXGIEmIZ2tA-1; Fri, 31 Jan 2020 09:04:39 -0500 X-MC-Unique: x7Suu6xLMl6SXGIEmIZ2tA-1 Received: by mail-wr1-f72.google.com with SMTP id t3so3368947wrm.23 for ; Fri, 31 Jan 2020 06:04:39 -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=VdOW7PNJeuMmmDY9kZwQNkZeEVlpGmsTxVCL30ndvIg=; b=owTHfxOX7cMTOKtgx86d0lV29ULKsX7EzOUl4w95Jmj3SqzPr6pkYoLdIcKy5EQWpY DFmwTuUQYu6acCqEcn/ioh37g/RnPt7++fjjVvTX9coLotPJaliGUyDeeKOi0wIP22Jv nMvGmOIJ9PcJfd1Vb1UEME1qVWNAOK05BPW0+ZlEOdmdK52EYDInv92sVt1qzBW/0F+1 CMYF6bNf8Uo0yzmQ79e/onGaoUZjOipqyJN7WQyD8gwoAVVKFcxOriRMiLGthMQXae+j D6FMtQ7T8468hJr/5NYvdpeXxcwqx+63ohLrQacC0ux4ZjvZZrl9bRSB2kBIfUWKlc0l 8eTw== X-Gm-Message-State: APjAAAWufh3bZShV/wDcwWOkN+WF11jfQRilILwpuea6/eLwFLXQpnwg iozwu4EvyVW7Od/RmeuI0yADXGC+Bo79co8y3C9eW34BioK8/6DovK8FO8jke6/veScq4Rb/793 rabitVlAS67vDqkfXEHjzBi8= X-Received: by 2002:adf:cd04:: with SMTP id w4mr13108321wrm.219.1580479467277; Fri, 31 Jan 2020 06:04:27 -0800 (PST) X-Google-Smtp-Source: APXvYqwGp9bgSUTCDiHCJ4QeoJtRo7//i35bQ2dIxy7owX0NHN+MtvuteERwIDccDqwixzEi1IoVPg== X-Received: by 2002:adf:cd04:: with SMTP id w4mr13108286wrm.219.1580479466826; Fri, 31 Jan 2020 06:04:26 -0800 (PST) Received: from localhost.localdomain ([62.72.193.75]) by smtp.gmail.com with ESMTPSA id w8sm11030625wmm.0.2020.01.31.06.04.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 31 Jan 2020 06:04:26 -0800 (PST) Subject: Re: [PATCH] HID: ite: Only bind to keyboard USB interface on Acer SW5-012 keyboard dock To: Benjamin Tissoires Cc: Jiri Kosina , "open list:HID CORE LAYER" , "3.8+" , =?UTF-8?Q?Zden=c4=9bk_Rampas?= References: <20200131124553.27796-1-hdegoede@redhat.com> From: Hans de Goede Message-ID: <6d94cf74-c28f-08b0-a136-044c231b8bc5@redhat.com> Date: Fri, 31 Jan 2020 15:04:23 +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 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 (and now I don't need to write a test, which is always a good motivation to come up with a cleaner solution :) 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. 2. Benjamin will you be at Fosdem too ?