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.
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.
>>
>> 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).
>>
>> * 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.
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.
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 :)
>>
>> * 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
