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

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
>

[PATCH v2] 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).

Commit 3b164a0 moved the mode switch from occuring post-start to occurring
pre-start. The change was not seen to cause issues largely due to the fact
that most devices mode switch with 'hid_hw_raw_request' (which is safe to
call prior to start) rather than 'hid_hw_request'.

Moving the call back to its original location resolves the issue, but
causes some touch-only Bamboo tablets (e.g. 056a:00d0) to stop working.
The affected tablets require us to perform a mode switch on their
vestigial pen interface prior ignoring with -ENODEV, meaning that the
code which is responsible for doing the ignoring has to move as well.

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

Ping clued me in to the fact that 'wacom_query_tablet_data' hasn't always 
been in this problematic location. The offending commit mentioned in the 
revised commit summary is queued for 4.4, so this only needs to get into 
the next RC rather than be targeted for stable.

Jason

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

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index abb7fdf..e06af5b 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -1778,24 +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)) {
-		error = -ENODEV;
-		goto fail_shared_data;
-	}
-
-	/* pen only Bamboo neither support touch nor pad */
-	if ((features->type == BAMBOO_PEN) &&
-	    ((features->device_type & WACOM_DEVICETYPE_TOUCH) ||
-	    (features->device_type & WACOM_DEVICETYPE_PAD))) {
-		error = -ENODEV;
-		goto fail_shared_data;
-	}
-
 	wacom_calculate_res(features);
 
 	wacom_update_name(wacom);
@@ -1833,6 +1815,24 @@ 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);
+
+	/* touch only Bamboo doesn't support pen */
+	if ((features->type == BAMBOO_TOUCH) &&
+	    (features->device_type & WACOM_DEVICETYPE_PEN)) {
+		error = -ENODEV;
+		goto fail_hw_start;
+	}
+
+	/* pen only Bamboo neither support touch nor pad */
+	if ((features->type == BAMBOO_PEN) &&
+	    ((features->device_type & WACOM_DEVICETYPE_TOUCH) ||
+	    (features->device_type & WACOM_DEVICETYPE_PAD))) {
+		error = -ENODEV;
+		goto fail_hw_start;
+	}
+
 	if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
 		error = hid_hw_open(hdev);
 
-- 
2.6.2

Re: [PATCH v2] HID: wacom: Call 'wacom_query_tablet_data' only after 'hid_hw_start'

[Ping Cheng]

On Tue, Nov 3, 2015 at 4:57 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).
>
> Commit 3b164a0 moved the mode switch from occuring post-start to occurring
> pre-start. The change was not seen to cause issues largely due to the fact
> that most devices mode switch with 'hid_hw_raw_request' (which is safe to
> call prior to start) rather than 'hid_hw_request'.
>
> Moving the call back to its original location resolves the issue, but
> causes some touch-only Bamboo tablets (e.g. 056a:00d0) to stop working.
> The affected tablets require us to perform a mode switch on their
> vestigial pen interface prior ignoring with -ENODEV, meaning that the
> code which is responsible for doing the ignoring has to move as well.

We'll need to clean up the code a bit more. But, that's for future.
This patch is good to go.

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

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

Ping

> ---
> Jiri,
>
> Ping clued me in to the fact that 'wacom_query_tablet_data' hasn't always
> been in this problematic location. The offending commit mentioned in the
> revised commit summary is queued for 4.4, so this only needs to get into
> the next RC rather than be targeted for stable.
>
> Jason
>
>  drivers/hid/wacom_sys.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index abb7fdf..e06af5b 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -1778,24 +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)) {
> -               error = -ENODEV;
> -               goto fail_shared_data;
> -       }
> -
> -       /* pen only Bamboo neither support touch nor pad */
> -       if ((features->type == BAMBOO_PEN) &&
> -           ((features->device_type & WACOM_DEVICETYPE_TOUCH) ||
> -           (features->device_type & WACOM_DEVICETYPE_PAD))) {
> -               error = -ENODEV;
> -               goto fail_shared_data;
> -       }
> -
>         wacom_calculate_res(features);
>
>         wacom_update_name(wacom);
> @@ -1833,6 +1815,24 @@ 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);
> +
> +       /* touch only Bamboo doesn't support pen */
> +       if ((features->type == BAMBOO_TOUCH) &&
> +           (features->device_type & WACOM_DEVICETYPE_PEN)) {
> +               error = -ENODEV;
> +               goto fail_hw_start;
> +       }
> +
> +       /* pen only Bamboo neither support touch nor pad */
> +       if ((features->type == BAMBOO_PEN) &&
> +           ((features->device_type & WACOM_DEVICETYPE_TOUCH) ||
> +           (features->device_type & WACOM_DEVICETYPE_PAD))) {
> +               error = -ENODEV;
> +               goto fail_hw_start;
> +       }
> +
>         if (features->device_type & WACOM_DEVICETYPE_WL_MONITOR)
>                 error = hid_hw_open(hdev);
>
> --
> 2.6.2
>

Re: [PATCH v2] HID: wacom: Call 'wacom_query_tablet_data' only after 'hid_hw_start'

[Jiri Kosina]

On Tue, 3 Nov 2015, Jason Gerecke 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).
> 
> Commit 3b164a0 moved the mode switch from occuring post-start to occurring
> pre-start. The change was not seen to cause issues largely due to the fact
> that most devices mode switch with 'hid_hw_raw_request' (which is safe to
> call prior to start) rather than 'hid_hw_request'.
> 
> Moving the call back to its original location resolves the issue, but
> causes some touch-only Bamboo tablets (e.g. 056a:00d0) to stop working.
> The affected tablets require us to perform a mode switch on their
> vestigial pen interface prior ignoring with -ENODEV, meaning that the
> code which is responsible for doing the ignoring has to move as well.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
> Jiri,
> 
> Ping clued me in to the fact that 'wacom_query_tablet_data' hasn't always 
> been in this problematic location. The offending commit mentioned in the 
> revised commit summary is queued for 4.4, so this only needs to get into 
> the next RC rather than be targeted for stable.

Queued as super-late patch into for-4.4/wacom.

-- 
Jiri Kosina
SUSE Labs