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

Message ID	20160711180711.17537-2-killertofu@gmail.com (mailing list archive)
State	New, archived
Delegated to:	Jiri Kosina
Headers	show Return-Path: <linux-input-owner@kernel.org> From: Jason Gerecke <killertofu@gmail.com> To: linux-input@vger.kernel.org Cc: Ping Cheng <pinglinux@gmail.com>, Aaron Skomra <skomra@gmail.com>, Benjamin Tissoires <benjamin.tissoires@redhat.com>, Jason Gerecke <killertofu@gmail.com>, Jason Gerecke <jason.gerecke@wacom.com> Subject: [PATCH 2/2] HID: wacom: Replace 'oVid' and 'oPid' with heuristics Date: Mon, 11 Jul 2016 11:07:11 -0700 Message-Id: <20160711180711.17537-2-killertofu@gmail.com> In-Reply-To: <20160711180711.17537-1-killertofu@gmail.com> References: <20160711180711.17537-1-killertofu@gmail.com> Sender: linux-input-owner@vger.kernel.org Precedence: bulk

Gerecke, Jason July 11, 2016, 6:07 p.m. UTC

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.

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.

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

  * Two devices which have different "directness" are not siblings.

  * Two devices which do not serve complementary roles (i.e. pen/touch)
    are not siblings.

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

Benjamin Tissoires July 12, 2016, 9:05 a.m. UTC | #1

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.

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

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

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

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

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

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

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

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

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

> +
> +	/* 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

Gerecke, Jason July 20, 2016, 4:36 p.m. UTC | #2

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

Benjamin Tissoires July 25, 2016, 9:02 a.m. UTC | #3

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

Gerecke, Jason Aug. 3, 2016, 5:13 p.m. UTC | #4

On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> 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@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

Gerecke, Jason Aug. 5, 2016, 10:53 p.m. UTC | #5

On 08/03/2016 10:13 AM, Jason Gerecke wrote:
> On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> 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....
> 

One question before I post the updated v4 patch: did you want me to
remove the "direct-input devices may not be siblings of indirect-input
devices" check? It opens up the holes mentioned above, but would
properly arbitrate hypothetical future split-indirect devices.

Since the new arbitration rules only apply to HID_GENERIC devices and
userspace will eventually take over the task anyway, I'm okay with
either option personally.

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

Benjamin Tissoires Aug. 8, 2016, 4:36 p.m. UTC | #6

On Aug 05 2016 or thereabouts, Jason Gerecke wrote:
> On 08/03/2016 10:13 AM, Jason Gerecke wrote:
> > On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> 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....
> > 
> 
> One question before I post the updated v4 patch: did you want me to
> remove the "direct-input devices may not be siblings of indirect-input
> devices" check? It opens up the holes mentioned above, but would
> properly arbitrate hypothetical future split-indirect devices.

I thought this check would be without ambiguity (if the directness is
not the same, that means they are on distinct physical devices).
So I'd say this check is necessary.

> 
> Since the new arbitration rules only apply to HID_GENERIC devices and
> userspace will eventually take over the task anyway, I'm okay with
> either option personally.

Also, there is one thing that might have sense since you are now having
the heuristic only for hid-generic.
We might want to be sure to have the proper sibling matching on some
rare cases. So I think it should be interesting to have:
- .ovid/.opid == 0/0 meaning "match against the current device vid/pid"
  (like what the current code does)
- .ovid/.opid == 0xffff/0xffff (HID_ANY_ID) meaning "use the heuristic,
  you can have any other vid/pid"
- .ovid/.opid != 0/0 and not 0xffff/0xffff meaning "match only the
  specified vid/pid"

This would allow to register a new device using HID_GENERIC but with a
specific ovid/opid.

One extra check could also be that we are sure that the sibling device
also is registered as HID_GENERIC + .ovid/.opid == 0xffff/0xffff to
avoid matching against something we already fixed the ovid/opid...

This might be a little over-processed however :)

Cheers,
Benjamin

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

Gerecke, Jason Aug. 8, 2016, 5:41 p.m. UTC | #7

On 08/08/2016 09:36 AM, Benjamin Tissoires wrote:
> On Aug 05 2016 or thereabouts, Jason Gerecke wrote:
>> On 08/03/2016 10:13 AM, Jason Gerecke wrote:
>>> On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
>>> <benjamin.tissoires@redhat.com> 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....
>>>
>>
>> One question before I post the updated v4 patch: did you want me to
>> remove the "direct-input devices may not be siblings of indirect-input
>> devices" check? It opens up the holes mentioned above, but would
>> properly arbitrate hypothetical future split-indirect devices.
> 
> I thought this check would be without ambiguity (if the directness is
> not the same, that means they are on distinct physical devices).
> So I'd say this check is necessary.
> 

Oops -- my brain must have shut off early for the weekend. Copy/pasted
the wrong heuristic description. I actually meant to ask if you wanted
me to keep/nuke the "Devices with different VID/PIDs may not be siblings
unless they are direct input devices" check since its presence may cause
problems for future precision touchpads but its absence will cause
problems for pen-only/touch-only tablets behind the same hub.

>>
>> Since the new arbitration rules only apply to HID_GENERIC devices and
>> userspace will eventually take over the task anyway, I'm okay with
>> either option personally.
> 
> Also, there is one thing that might have sense since you are now having
> the heuristic only for hid-generic.
> We might want to be sure to have the proper sibling matching on some
> rare cases. So I think it should be interesting to have:
> - .ovid/.opid == 0/0 meaning "match against the current device vid/pid"
>   (like what the current code does)
> - .ovid/.opid == 0xffff/0xffff (HID_ANY_ID) meaning "use the heuristic,
>   you can have any other vid/pid"
> - .ovid/.opid != 0/0 and not 0xffff/0xffff meaning "match only the
>   specified vid/pid"
> 
> This would allow to register a new device using HID_GENERIC but with a
> specific ovid/opid.
> 
> One extra check could also be that we are sure that the sibling device
> also is registered as HID_GENERIC + .ovid/.opid == 0xffff/0xffff to
> avoid matching against something we already fixed the ovid/opid...
> 
> This might be a little over-processed however :)
> 
> Cheers,
> Benjamin
> 

I kinda like the sound of it since it would make the logic a little more
straightforward. The special ovid/opid meanings would apply equally to
all devices, meaning I wouldn't have to anymore ignore the 0/0 case for
HID_GENERIC.

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

Benjamin Tissoires Aug. 8, 2016, 5:56 p.m. UTC | #8

On Aug 08 2016 or thereabouts, Jason Gerecke wrote:
> On 08/08/2016 09:36 AM, Benjamin Tissoires wrote:
> > On Aug 05 2016 or thereabouts, Jason Gerecke wrote:
> >> On 08/03/2016 10:13 AM, Jason Gerecke wrote:
> >>> On Mon, Jul 25, 2016 at 2:02 AM, Benjamin Tissoires
> >>> <benjamin.tissoires@redhat.com> 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....
> >>>
> >>
> >> One question before I post the updated v4 patch: did you want me to
> >> remove the "direct-input devices may not be siblings of indirect-input
> >> devices" check? It opens up the holes mentioned above, but would
> >> properly arbitrate hypothetical future split-indirect devices.
> > 
> > I thought this check would be without ambiguity (if the directness is
> > not the same, that means they are on distinct physical devices).
> > So I'd say this check is necessary.
> > 
> 
> Oops -- my brain must have shut off early for the weekend. Copy/pasted
> the wrong heuristic description. I actually meant to ask if you wanted
> me to keep/nuke the "Devices with different VID/PIDs may not be siblings
> unless they are direct input devices" check since its presence may cause
> problems for future precision touchpads but its absence will cause
> problems for pen-only/touch-only tablets behind the same hub.

In that case, I'd prefer this heuristic to be out. Besides the
multi-seats use case, I am not sure a user would bother having both a
pen-only tablet and a touch-only tablet, using both at the same time.

These are the cases where we can teach users to switch to no touch
arbitration or do a per-case entry in the list of devices saying that
they don't have any sibling (or can be matched with themself only).

> 
> >>
> >> Since the new arbitration rules only apply to HID_GENERIC devices and
> >> userspace will eventually take over the task anyway, I'm okay with
> >> either option personally.
> > 
> > Also, there is one thing that might have sense since you are now having
> > the heuristic only for hid-generic.
> > We might want to be sure to have the proper sibling matching on some
> > rare cases. So I think it should be interesting to have:
> > - .ovid/.opid == 0/0 meaning "match against the current device vid/pid"
> >   (like what the current code does)
> > - .ovid/.opid == 0xffff/0xffff (HID_ANY_ID) meaning "use the heuristic,
> >   you can have any other vid/pid"
> > - .ovid/.opid != 0/0 and not 0xffff/0xffff meaning "match only the
> >   specified vid/pid"
> > 
> > This would allow to register a new device using HID_GENERIC but with a
> > specific ovid/opid.
> > 
> > One extra check could also be that we are sure that the sibling device
> > also is registered as HID_GENERIC + .ovid/.opid == 0xffff/0xffff to
> > avoid matching against something we already fixed the ovid/opid...
> > 
> > This might be a little over-processed however :)
> > 
> > Cheers,
> > Benjamin
> > 
> 
> I kinda like the sound of it since it would make the logic a little more
> straightforward. The special ovid/opid meanings would apply equally to
> all devices, meaning I wouldn't have to anymore ignore the 0/0 case for
> HID_GENERIC.

Oh, then if it makes the logic simpler, go for it :)

Cheers,
Benjamin

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

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

Commit Message

Comments

Patch