Re: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics

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:

quoted

On Jul 11 2016 or thereabouts, Jason Gerecke wrote:

quoted

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.

quoted

quoted

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

quoted

quoted

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

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

quoted

quoted

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

quoted

quoted

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 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 :)

quoted

quoted

+	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.

quoted

quoted

+
+	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;

quoted

quoted

 
-	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.

quoted

quoted

+
+	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.

quoted

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....

quoted

quoted

+
+	/* 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html