On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> 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 <jason.gerecke@xxxxxxxxx>
>> >> ---
>> >> 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
>> >>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
