[PATCH 1/3] HID: wacom: Call 'wacom_query_tablet_data' only after 'hid_hw_start'

[PATCH 1/3] HID: wacom: Call 'wacom_query_tablet_data' only after 'hid_hw_start'

[Jason Gerecke]

When connecting the Cintiq Companion 2 as an external tablet (i.e., using
it in "hybrid" mode) it has been seen to cause the kernel of the machine
it is connected to to Oops. The cause has been traced to us attempting to
switch the tablet's mode prior to actually starting HID device (resulting
in the eventual dereference of the uninitialized control URB).

This has not been an issue in the past since we've historically used
'hid_hw_raw_request' to perform the mode switch. HID_GENERIC devices,
however use 'hid_hw_request' which assumes that 'hid_hw_start' has already
been called. That we've not encountered problems beforehand seems to
be entirely due to luck.

Since the timing of the mode switch is not critical, we can simply
move it to occur after the call to 'hid_hw_start'. Alternately, we
could have potentially modified 'wacom_hid_set_device_mode' to use
'hid_hw_raw_reqest'.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
Jiri,

Please mark this for stable since it possible other HID_GENERIC 
devices may trigger this Oops as well.

Thanks,
Jason

 drivers/hid/wacom_sys.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index b4de325..45656e8 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1778,9 +1778,6 @@ static int wacom_probe(struct hid_device *hdev,
 		features->device_type |= WACOM_DEVICETYPE_PEN;
 	}
 
-	/* Note that if query fails it is not a hard failure */
-	wacom_query_tablet_data(hdev, features);
-
 	/* touch only Bamboo doesn't support pen */
 	if ((features->type == BAMBOO_TOUCH) &&
 	    (features->device_type & WACOM_DEVICETYPE_PEN)) {
@@ -1833,6 +1830,9 @@ static int wacom_probe(struct hid_device *hdev,
 		goto fail_hw_start;
 	}
 
+	/* Note that if query fails it is not a hard failure */
+	wacom_query_tablet_data(hdev, features);
+
 	if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
 		error = hid_hw_open(hdev);
 
-- 
2.6.2

[PATCH 2/3] HID: wacom: Remove useless conditions from 'wacom_query_tablet_data'

[Jason Gerecke]

When support for the Cintiq Companion Hybrid and Cintiq Companion 2 was
added (36d3c51 and f7acb55), the 'wacom_query_tablet_data' function was
updated to include references to CINTIQ_HYBRID and CINTIQ_COMPANION_2
with the thought that they were necessary to switch the touch interface
into the proper mode. This is unnecessary, however, since those types
are only ever associated with the pen interface -- the touch interfaces
are either CINTIQ_24HDT or HID_GENERIC. To avoid confusion in the future,
we remove the unnecessary CINTIQ_HYBRID and CINTIQ_COMPANION_2 conditions.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_sys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 45656e8..69ff5b5 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -422,7 +422,7 @@ static int wacom_query_tablet_data(struct hid_device *hdev,
 			/* MT Tablet PC touch */
 			return wacom_set_device_mode(hdev, 3, 4, 4);
 		}
-		else if (features->type == WACOM_24HDT || features->type == CINTIQ_HYBRID || features->type == CINTIQ_COMPANION_2) {
+		else if (features->type == WACOM_24HDT) {
 			return wacom_set_device_mode(hdev, 18, 3, 2);
 		}
 		else if (features->type == WACOM_27QHDT) {
-- 
2.6.2

Re: [PATCH 2/3] HID: wacom: Remove useless conditions from 'wacom_query_tablet_data'

[Ping Cheng]

On Mon, Nov 2, 2015 at 3:41 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> When support for the Cintiq Companion Hybrid and Cintiq Companion 2 was
> added (36d3c51 and f7acb55), the 'wacom_query_tablet_data' function was
> updated to include references to CINTIQ_HYBRID and CINTIQ_COMPANION_2
> with the thought that they were necessary to switch the touch interface
> into the proper mode. This is unnecessary, however, since those types
> are only ever associated with the pen interface -- the touch interfaces
> are either CINTIQ_24HDT or HID_GENERIC. To avoid confusion in the future,
> we remove the unnecessary CINTIQ_HYBRID and CINTIQ_COMPANION_2 conditions.
>
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>

Reviewed-by: Ping Cheng <pingc@wacom.com>

for this patch and the 3/3 of this set.

Ping

> ---
>  drivers/hid/wacom_sys.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 45656e8..69ff5b5 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -422,7 +422,7 @@ static int wacom_query_tablet_data(struct hid_device *hdev,
>                         /* MT Tablet PC touch */
>                         return wacom_set_device_mode(hdev, 3, 4, 4);
>                 }
> -               else if (features->type == WACOM_24HDT || features->type == CINTIQ_HYBRID || features->type == CINTIQ_COMPANION_2) {
> +               else if (features->type == WACOM_24HDT) {
>                         return wacom_set_device_mode(hdev, 18, 3, 2);
>                 }
>                 else if (features->type == WACOM_27QHDT) {
> --
> 2.6.2
>

Re: [PATCH 2/3] HID: wacom: Remove useless conditions from 'wacom_query_tablet_data'

[Jiri Kosina]

On Mon, 2 Nov 2015, Ping Cheng wrote:

> On Mon, Nov 2, 2015 at 3:41 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> > When support for the Cintiq Companion Hybrid and Cintiq Companion 2 was
> > added (36d3c51 and f7acb55), the 'wacom_query_tablet_data' function was
> > updated to include references to CINTIQ_HYBRID and CINTIQ_COMPANION_2
> > with the thought that they were necessary to switch the touch interface
> > into the proper mode. This is unnecessary, however, since those types
> > are only ever associated with the pen interface -- the touch interfaces
> > are either CINTIQ_24HDT or HID_GENERIC. To avoid confusion in the future,
> > we remove the unnecessary CINTIQ_HYBRID and CINTIQ_COMPANION_2 conditions.
> >
> > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> 
> Reviewed-by: Ping Cheng <pingc@wacom.com>
> 
> for this patch and the 3/3 of this set.

This is really late in the game (the merge window is already open), but 
given the nature of the patches (we'll be sending them as -rc bugfixes 
later anyway), I've just picked those up to for-4.4/wacom.

1/1 I am sure you two will have some more discussion about :) so I am 
ignoring it for now.

-- 
Jiri Kosina
SUSE Labs

[PATCH 3/3] HID: wacom: Fix ABS_MISC reporting for Cintiq Companion 2

[Jason Gerecke]

The pad handling code introduced for the Cintiq Companion 2 (f7acb55)
looks at the wrong bytes in the report when deciding whether ABS_MISC
should be sent. This does not cause any issues with the X driver now
that the pen and pad have been split to separate devices, but is
incorrect and has caused issues when backporting to distros with pre-
3.17 kernels.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/wacom_wac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 453da6d..9db6a8b 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -966,7 +966,7 @@ static int wacom_intuos_irq(struct wacom_wac *wacom)
 			input_report_key(input, BTN_A, (data[2] & 0x80));  /* Down   */
 			input_report_key(input, BTN_0, (data[1] & 0x01));  /* Center */
 
-			if (data[4] | (data[3] & 0x01)) {
+			if (data[2] | (data[1] & 0x07)) {
 				input_report_abs(input, ABS_MISC, PAD_DEVICE_ID);
 			} else {
 				input_report_abs(input, ABS_MISC, 0);
-- 
2.6.2

Re: [PATCH 1/3] HID: wacom: Call 'wacom_query_tablet_data' only after 'hid_hw_start'

[Ping Cheng]

On Mon, Nov 2, 2015 at 3:41 PM, Jason Gerecke <killertofu@gmail.com> wrote:
>
> When connecting the Cintiq Companion 2 as an external tablet (i.e., using
> it in "hybrid" mode) it has been seen to cause the kernel of the machine
> it is connected to to Oops. The cause has been traced to us attempting to
> switch the tablet's mode prior to actually starting HID device (resulting
> in the eventual dereference of the uninitialized control URB).
>
> This has not been an issue in the past since we've historically used
> 'hid_hw_raw_request' to perform the mode switch. HID_GENERIC devices,
> however use 'hid_hw_request' which assumes that 'hid_hw_start' has already
> been called. That we've not encountered problems beforehand seems to
> be entirely due to luck.
>
> Since the timing of the mode switch is not critical, we can simply

The timing of the mode switch makes a difference for Bamboo touch only
devices. Without switching mode for pen interface before it is
ignored, we will not receive touch data later.

All other devices should be fine with your moved mode switch code.

> move it to occur after the call to 'hid_hw_start'. Alternately, we
> could have potentially modified 'wacom_hid_set_device_mode' to use
> 'hid_hw_raw_reqest'.
>
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>

Nacked-by: Ping Cheng <pingc@wacom.com>

Ping


> ---
> Jiri,
>
> Please mark this for stable since it possible other HID_GENERIC
> devices may trigger this Oops as well.
>
> Thanks,
> Jason
>
>  drivers/hid/wacom_sys.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index b4de325..45656e8 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -1778,9 +1778,6 @@ static int wacom_probe(struct hid_device *hdev,
>                 features->device_type |= WACOM_DEVICETYPE_PEN;
>         }
>
> -       /* Note that if query fails it is not a hard failure */
> -       wacom_query_tablet_data(hdev, features);
> -
>         /* touch only Bamboo doesn't support pen */
>         if ((features->type == BAMBOO_TOUCH) &&
>             (features->device_type & WACOM_DEVICETYPE_PEN)) {
> @@ -1833,6 +1830,9 @@ static int wacom_probe(struct hid_device *hdev,
>                 goto fail_hw_start;
>         }
>
> +       /* Note that if query fails it is not a hard failure */
> +       wacom_query_tablet_data(hdev, features);
> +
>         if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
>                 error = hid_hw_open(hdev);
>
> --
> 2.6.2
>