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

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

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.

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

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