From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Gerecke Subject: Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics Date: Wed, 3 Aug 2016 10:13:48 -0700 Message-ID: References: <20160711180711.17537-1-killertofu@gmail.com> <20160711180711.17537-2-killertofu@gmail.com> <20160712090510.GG4663@mail.corp.redhat.com> <20160725090218.GC19383@mail.corp.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Return-path: Received: from mail-io0-f194.google.com ([209.85.223.194]:33187 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932740AbcHCRNu convert rfc822-to-8bit (ORCPT ); Wed, 3 Aug 2016 13:13:50 -0400 Received: by mail-io0-f194.google.com with SMTP id y195so18585727iod.0 for ; Wed, 03 Aug 2016 10:13:49 -0700 (PDT) In-Reply-To: <20160725090218.GC19383@mail.corp.redhat.com> Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Benjamin Tissoires Cc: Linux Input , Ping Cheng , Aaron Skomra , Jason Gerecke On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires wrote: > Hi Jason, > > [I've seen v2 and v3 but the discussion is mainly going there, so > pulling this one out of the archives] > > On Jul 20 2016 or thereabouts, Jason Gerecke wrote: >> On 07/12/2016 02:05 AM, Benjamin Tissoires wrote: >> > On Jul 11 2016 or thereabouts, Jason Gerecke wrote: >> >> The 'oVid' and 'oPid' variables used by wacom_are_sibling are a hacky >> >> solution to the problem of the driver having few good heuristics to use >> >> to determine if two devices should be considered siblings or not. This >> >> commit replaces them with heuristics that leverage the information we >> >> have available to determine if two devices are likely siblings or not. >> > >> > I agree it's nicer to have a better heuristic for sibling matching, >> > but I wonder if this heuristic is reliable enough when talking about >> > future devices. It might be, but I know from experience that the >> > firmware team can be very original and find a way that screws up us all >> > :/ >> > >> > Keeping the safety net of reducing the checks with ovid/opid might come >> > handy in the future. >> > >> >> The biggest problem with oVid/oPid is that they're kinda antithetical to >> the HID_GENERIC codepath. If a device is split between two PIDs then >> arbitration is broken for any kernel that doesn't include specific >> support for the device. > > Well, we currently already have different paths for HID_GENERIC and > other various protocols. Why is that a problem to have heuristics for > HID_GENERIC and ovid/opid for those we need to have special handling? > >> >> I agree that heuristics aren't as confidence-inspiring as explicit >> notation, but if they can be made to work well enough then I'd prefer >> using them. Either way, I suppose as userspace starts taking over >> arbitration duties it will become a more and more moot issue. > > Yep, but currently the patch might link 2 devices that were not bound > together before, so I still think ovid/opid is the best way for the non > generic devices. For generic devices (future I think) we can always ask > people to use userspace touch arbitration. > >> >> >> >> >> Written out, the new heuristics are basically: >> >> >> >> * If a device with the same VID/PID as that being probed already exists, >> >> it should be preferentially checked for siblingship. >> > >> > That's assuming you don't have 2 27QHD connected as 2 monitors (if you >> > really have a lot of money to spend) :) >> > >> > I think the code is OK, just not the comment above (mostly the >> > "preferentially" word itches me) >> > >> >> I'll try to come up with better / more clear wording (see my later >> comments on the "Try to find an already-probed interface from the same >> device" hunk for more detail). > > Thanks for the changes in v2/3 > >> >> >> >> >> * Two HID devices which have the same VID/PID are not siblings if they >> >> are not part of the same logical hardware device. >> >> >> >> * Two HID devices which have different VID/PIDs are not siblings if >> >> they have different parent (e.g. USB hub) devices. >> >> >> >> * Devices without the WACOM_DEVICETYPE_DIRECT flag set may only be >> >> siblings of devies with the same VID/PID (Wacom often splits their >> >> direct input tablets into two devices to make it easier to meet >> >> Microsoft's touchscreen requirements). >> > >> > That's a strong assumption on the future. If you are forced to use the >> > Precision Touchpad protocol for Intuos, this will just blow up. >> > >> >> I originally didn't include this condition in my checks since I was >> similarly wary. Leaving it out does open two small corner cases though. >> >> 1) A pen-only tablet connected to the same hub as a touch-only tablet. >> Touch-only Wacom tablets aren't particularly common and it would >> be a little strange to have one paired with a pen-only tablet, but it >> could conceivably happen. Although pairing the devices shouldn't >> normally be an issue since a user isn't likely to use both >> simultaneously, it might cause problems for multi-seat setups. > > Yes, multi-seats is one big issue here. > >> >> 2) A pen-only tablet connected to the same hub as a pen-and-touch tablet >> which has the *touch* interface probed first. As far as I'm aware, there >> aren't any Wacom devices that have the USB interfaces ordered >> touch-then-pen, but as you point out, who knows what those tricksy >> firmware engineers will do in the future to ruin your day. > > And the winner is... the Cintiq 13HDT -> touch interface and then Pen :) > > Which means with the new patch, connecting a Cintiq 21UX (1 not 2) which > is a direct device which I assume doesn't have an internal hub, you end > up binding the 21UX pen with the 13HD touch, and things gets nasty for > the 13HD pen interface. > Argh. Good to know. >> >> I'm open to leaving the check in or out depending on your feelings. If >> you've got thoughts on how to close these corner cases as well, I'm all >> ears :) > > I really think the ovid/opid approach solves most of the issues with the > current hardware. You are concerned about future hardware that will be > handled by HID_GENERIC. Why not just adding a special case for > HID_GENERIC that uses your heuristics? > In userspace I think we will still have the ovid/opid approach in > libwacom because it restricts the amount of combinations by a very good > factor. > > If the kernel with the new heuristics (HID_GENERIC only) fails, we will > be able to tell users to switch to userspace touch arbitration. > > /me turns in circle a little, but how does that sounds? > > Cheers, > Benjamin > That sounds reasonable. I'll return the old oVid/oPid checks, and integrate them with heuristics for HID_GENERIC devices. Patch to come soon (TM). Jason --- Now instead of four in the eights place / you’ve got three, ‘Cause you added one / (That is to say, eight) to the two, / But you can’t take seven from three, / So you look at the sixty-fours.... >> >> >> >> >> * Two devices which have different "directness" are not siblings. >> >> >> >> * Two devices which do not serve complementary roles (i.e. pen/touch) >> >> are not siblings. >> > >> > I think it would be useful to have these write outs as comments in the >> > code. It's quite tricky to understand the actual code without these >> > explanations. >> > >> >> Ack. >> >> >> >> >> Signed-off-by: Jason Gerecke >> >> --- >> >> drivers/hid/wacom_sys.c | 58 +++++++++++++++++++++++++++++++++++++++---------- >> >> drivers/hid/wacom_wac.c | 41 ++++++++++++++-------------------- >> >> drivers/hid/wacom_wac.h | 2 -- >> >> 3 files changed, 62 insertions(+), 39 deletions(-) >> >> >> >> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c >> >> index 4a0bb6f..a5bc038 100644 >> >> --- a/drivers/hid/wacom_sys.c >> >> +++ b/drivers/hid/wacom_sys.c >> >> @@ -532,31 +532,65 @@ static bool wacom_are_sibling(struct hid_device *hdev, >> >> { >> >> struct wacom *wacom = hid_get_drvdata(hdev); >> >> struct wacom_features *features = &wacom->wacom_wac.features; >> >> - int vid = features->oVid; >> >> - int pid = features->oPid; >> >> - int n1,n2; >> >> + struct wacom *sibling_wacom = hid_get_drvdata(sibling); >> >> + struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features; >> >> + int n1, n2; >> >> >> >> - if (vid == 0 && pid == 0) { >> >> - vid = hdev->vendor; >> >> - pid = hdev->product; >> >> + /* Compare the physical path. Require devices with the same >> >> + * PID to share the same device, and devices with different >> >> + * PIDs to share parent devices. >> >> + */ >> > >> > I stumbled this morning upon: >> > http://www.theregister.co.uk/2016/07/11/linus_torvalds_in_sweary_rant_about_punctuation_in_kernel_comments/ >> > >> > Please make sure to follow the comments style guidelines :) >> > >> >> Whatever makes the style gods happy :) >> >> >> + if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) { >> >> + n1 = strrchr(hdev->phys, '/') - hdev->phys; >> >> + n2 = strrchr(sibling->phys, '/') - sibling->phys; >> >> + } >> >> + else { >> >> + n1 = strrchr(hdev->phys, '.') - hdev->phys; >> >> + n2 = strrchr(sibling->phys, '.') - sibling->phys; >> >> } >> >> >> >> - if (vid != sibling->vendor || pid != sibling->product) >> >> + if (n1 != n2 || n1 <= 0 || n2 <= 0) >> >> return false; >> >> >> >> - /* Compare the physical path. */ >> >> - n1 = strrchr(hdev->phys, '.') - hdev->phys; >> >> - n2 = strrchr(sibling->phys, '.') - sibling->phys; >> >> - if (n1 != n2 || n1 <= 0 || n2 <= 0) >> >> + if (strncmp(hdev->phys, sibling->phys, n1)) >> >> + return false; >> >> + >> >> + if (hdev->vendor != sibling->vendor || hdev->product != sibling->product) { >> >> + if(!(features->device_type & WACOM_DEVICETYPE_DIRECT)) >> >> + return false; >> >> + } >> > >> > As mentioned in the commit log, I am not sure it's such a good idea to >> > have this check. Does it really remove a false positive or can this just >> > be considered as an extra check that can be safely removed? >> > >> >> See comment above. >> >> >> + >> >> + if ((features->device_type & WACOM_DEVICETYPE_DIRECT) != >> >> + (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT)) >> >> return false; >> > >> > unless I am mistaken, you might as well need {if indirect and sibling >> > is not indirect, than false }. >> > >> >> That case should be covered since we're comparing the actual values of >> each device's "direct" flag for equality (direct/indirect and >> indirect/direct will return false). >> >> If its not immediately clear though, I could decompose it into >> independent checks for each case. E.g.: >> >> if ((features->device_type & WACOM_DEVICETYPE_DIRECT) && >> !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT)) >> return false; >> >> if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) && >> (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT)) >> return false; >> >> >> >> >> - return !strncmp(hdev->phys, sibling->phys, n1); >> >> + if ((features->device_type & WACOM_DEVICETYPE_PEN) && >> >> + !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH)) >> >> + return false; >> >> + >> >> + if ((features->device_type & WACOM_DEVICETYPE_TOUCH) && >> >> + !(sibling_features->device_type & WACOM_DEVICETYPE_PEN)) >> >> + return false; >> > >> > I think it would be better to have those last 3 (plus mine) tests at the >> > beginning, before doing string lookouts. They seem to be the most >> > reliable ones and able to exclude a lot of false positive. >> > >> >> Reasonable enough. >> >> >> + >> >> + return true; >> >> } >> >> >> >> static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev) >> >> { >> >> struct wacom_hdev_data *data; >> >> >> >> + /* Try to find an already-probed interface from the same device */ >> >> + list_for_each_entry(data, &wacom_udev_list, list) { >> >> + int n1, n2; >> >> + n1 = strrchr(hdev->phys, '/') - hdev->phys; >> >> + n2 = strrchr(data->dev->phys, '/') - data->dev->phys; >> >> + if (n1 != n2 || n1 <= 0 || n2 <= 0) >> >> + continue; >> >> + if (!strncmp(hdev->phys, data->dev->phys, n1)) >> >> + return data; >> >> + } >> > >> > I can't see the benefit of having this here, while it seems to be >> > already tested in wacom_are_sibling(). >> > >> >> There's a subtle issue with not performing this search before proceeding >> to trying to pair arbitrary devices in wacom_are_sibling. Imagine that >> you've already got a pen-only device connected to your system. Now, >> connect a (non-split) pen+touch device to the same hub. When the touch >> interface is probed, the first device checked its checked against in >> wacom_are_sibling is that pen-only device, and provided it passes all >> the checks then it will be incorrectly paired with it. >> >> This is actually probably a leftover from before I added the requirement >> that only direct devices can have different PIDs and might not be >> strictly necessary in the patch's current state, but I'd feel better >> keeping it in either way. >> >> > Also, if there is a need for it, there is a common pattern used 3 times >> > here: >> > int n1, n2; >> > n1 = strrchr(hdev->phys, separator) - hdev->phys; >> > n2 = strrchr(other->dev->phys, separator) - other->dev->phys; >> > if (n1 != n2 || n1 <= 0 || n2 <= 0) >> > continue; >> > return !strncmp(hdev->phys, other->dev->phys, n1); >> > >> > It would make sense to put a name on it and have a separate function. >> > >> > Cheers, >> > Benjamin >> > >> >> Good call, if the block remains necessary. >> >> Jason >> --- >> Now instead of four in the eights place / >> you’ve got three, ‘Cause you added one / >> (That is to say, eight) to the two, / >> But you can’t take seven from three, / >> So you look at the sixty-fours.... >> >> >> + >> >> + /* Fallback to finding devices that appear to be "siblings" */ >> >> list_for_each_entry(data, &wacom_udev_list, list) { >> >> if (wacom_are_sibling(hdev, data->dev)) { >> >> kref_get(&data->kref); >> >> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c >> >> index 2523a29..cb6fc63 100644 >> >> --- a/drivers/hid/wacom_wac.c >> >> +++ b/drivers/hid/wacom_wac.c >> >> @@ -3229,11 +3229,10 @@ static const struct wacom_features wacom_features_0xF4 = >> >> static const struct wacom_features wacom_features_0xF8 = >> >> { "Wacom Cintiq 24HD touch", 104080, 65200, 2047, 63, /* Pen */ >> >> WACOM_24HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 16, >> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf6 }; >> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET }; >> >> static const struct wacom_features wacom_features_0xF6 = >> >> { "Wacom Cintiq 24HD touch", .type = WACOM_24HDT, /* Touch */ >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0xf8, .touch_max = 10, >> >> + .touch_max = 10, >> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE }; >> >> static const struct wacom_features wacom_features_0x32A = >> >> { "Wacom Cintiq 27QHD", 119740, 67520, 2047, 63, >> >> @@ -3242,11 +3241,10 @@ static const struct wacom_features wacom_features_0x32A = >> >> static const struct wacom_features wacom_features_0x32B = >> >> { "Wacom Cintiq 27QHD touch", 119740, 67520, 2047, 63, >> >> WACOM_27QHD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 0, >> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32C }; >> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET }; >> >> static const struct wacom_features wacom_features_0x32C = >> >> { "Wacom Cintiq 27QHD touch", .type = WACOM_27QHDT, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x32B, .touch_max = 10 }; >> >> + .touch_max = 10 }; >> >> static const struct wacom_features wacom_features_0x3F = >> >> { "Wacom Cintiq 21UX", 87200, 65600, 1023, 63, >> >> CINTIQ, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 8 }; >> >> @@ -3263,11 +3261,10 @@ static const struct wacom_features wacom_features_0x304 = >> >> static const struct wacom_features wacom_features_0x333 = >> >> { "Wacom Cintiq 13HD touch", 59152, 33448, 2047, 63, >> >> WACOM_13HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9, >> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x335 }; >> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET }; >> >> static const struct wacom_features wacom_features_0x335 = >> >> { "Wacom Cintiq 13HD touch", .type = WACOM_24HDT, /* Touch */ >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x333, .touch_max = 10, >> >> + .touch_max = 10, >> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE }; >> >> static const struct wacom_features wacom_features_0xC7 = >> >> { "Wacom DTU1931", 37832, 30305, 511, 0, >> >> @@ -3298,11 +3295,10 @@ static const struct wacom_features wacom_features_0x57 = >> >> static const struct wacom_features wacom_features_0x59 = /* Pen */ >> >> { "Wacom DTH2242", 95640, 54060, 2047, 63, >> >> DTK, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 6, >> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5D }; >> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET }; >> >> static const struct wacom_features wacom_features_0x5D = /* Touch */ >> >> { "Wacom DTH2242", .type = WACOM_24HDT, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x59, .touch_max = 10, >> >> + .touch_max = 10, >> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE }; >> >> static const struct wacom_features wacom_features_0xCC = >> >> { "Wacom Cintiq 21UX2", 86800, 65200, 2047, 63, >> >> @@ -3315,11 +3311,10 @@ static const struct wacom_features wacom_features_0xFA = >> >> static const struct wacom_features wacom_features_0x5B = >> >> { "Wacom Cintiq 22HDT", 95440, 53860, 2047, 63, >> >> WACOM_22HD, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 18, >> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5e }; >> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET }; >> >> static const struct wacom_features wacom_features_0x5E = >> >> { "Wacom Cintiq 22HDT", .type = WACOM_24HDT, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x5b, .touch_max = 10, >> >> + .touch_max = 10, >> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE }; >> >> static const struct wacom_features wacom_features_0x90 = >> >> { "Wacom ISDv4 90", 26202, 16325, 255, 0, >> >> @@ -3461,20 +3456,18 @@ static const struct wacom_features wacom_features_0x6004 = >> >> static const struct wacom_features wacom_features_0x307 = >> >> { "Wacom ISDv5 307", 59152, 33448, 2047, 63, >> >> CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9, >> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x309 }; >> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET }; >> >> static const struct wacom_features wacom_features_0x309 = >> >> { "Wacom ISDv5 309", .type = WACOM_24HDT, /* Touch */ >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x0307, .touch_max = 10, >> >> + .touch_max = 10, >> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE }; >> >> static const struct wacom_features wacom_features_0x30A = >> >> { "Wacom ISDv5 30A", 59152, 33448, 2047, 63, >> >> CINTIQ_HYBRID, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 9, >> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30C }; >> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET }; >> >> static const struct wacom_features wacom_features_0x30C = >> >> { "Wacom ISDv5 30C", .type = WACOM_24HDT, /* Touch */ >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x30A, .touch_max = 10, >> >> + .touch_max = 10, >> >> .check_for_hid_type = true, .hid_type = HID_TYPE_USBNONE }; >> >> static const struct wacom_features wacom_features_0x318 = >> >> { "Wacom USB Bamboo PAD", 4095, 4095, /* Touch */ >> >> @@ -3485,11 +3478,9 @@ static const struct wacom_features wacom_features_0x319 = >> >> static const struct wacom_features wacom_features_0x325 = >> >> { "Wacom ISDv5 325", 59552, 33848, 2047, 63, >> >> CINTIQ_COMPANION_2, WACOM_INTUOS3_RES, WACOM_INTUOS3_RES, 11, >> >> - WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET, >> >> - .oVid = USB_VENDOR_ID_WACOM, .oPid = 0x326 }; >> >> + WACOM_CINTIQ_OFFSET, WACOM_CINTIQ_OFFSET }; >> >> static const struct wacom_features wacom_features_0x326 = /* Touch */ >> >> - { "Wacom ISDv5 326", .type = HID_GENERIC, .oVid = USB_VENDOR_ID_WACOM, >> >> - .oPid = 0x325 }; >> >> + { "Wacom ISDv5 326", .type = HID_GENERIC }; >> >> static const struct wacom_features wacom_features_0x323 = >> >> { "Wacom Intuos P M", 21600, 13500, 1023, 31, >> >> INTUOSHT, WACOM_INTUOS_RES, WACOM_INTUOS_RES, >> >> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h >> >> index 7ad6273..a5bd05a 100644 >> >> --- a/drivers/hid/wacom_wac.h >> >> +++ b/drivers/hid/wacom_wac.h >> >> @@ -181,8 +181,6 @@ struct wacom_features { >> >> int tilt_fuzz; >> >> unsigned quirks; >> >> unsigned touch_max; >> >> - int oVid; >> >> - int oPid; >> >> int pktlen; >> >> bool check_for_hid_type; >> >> int hid_type; >> >> -- >> >> 2.9.0 >> >> >>