Bug#677164: [3.2.17-1 -> 3.2.18-1 regression] Wacom tablet in Thinkpad x220t not working
Jonathan Nieder <jrnieder@gmail.com> writes:
> Nils Kanning wrote:
>
>> patches 0020-0023: tablet works
>> patches 0020-0024: tablet works
>> patches 0020-0025: does not work
>> patches 0020-0026: does not work
>> patches 0020-0024,0026: does not work
>
> Thanks! How about 0026 alone?
>
> (0025 is more tricky to test because it depends on 0024. Quick
> two-patch series for that attached in case you are interested.)
If I may add a personal opinion here....
This commit is so obviously buggy that it should be reverted and redone
in any case:
>>From b4ede6ebf876fbe2c5242e71c7eb195c6f0de6cc Mon Sep 17 00:00:00 2001
> From: Ping Cheng <pinglinux@gmail.com>
> Date: Sun, 29 Apr 2012 21:09:18 -0700
> Subject: [PATCH 2/2] Input: wacom - add 0xE5 (MT device) support
>
> commit 1963518b9b1b8019d33b4b08deee6f873ffa2730 upstream.
>
> Main part of patch is adding support for a new Wacom MT touch
> packet and labels these devices using MTSCREEN type.
>
> Other items of interest:
What's that? There are supposed to be no "Other items of interest" in a
commit. One commit, one item.
> Delete some duplicate code in HID parsing for Y info since
> its already done in X path.
Deleting duclicate code while adding new features is a no-no.
> In wacom_query_tablet_data(), only invoke the set report
> that requests tablets to send Wacom Touch packets for
> Finger interfaces. Mostly, this is to make code intent clear.
I suspect this is the bug hitting Nils. Apart from the same comment as
above, the "make code intent clear" commit message just obfuscates the
fact that it changes code flow significantly for existing devices.
Such changes have absolutely no place in a new features patch.
These two hunks are suspicious because they remove the setting of
features->device_type:
> @@ -346,18 +350,15 @@ static int wacom_parse_hid(struct usb_interface *intf,
> case HID_USAGE_Y:
> if (usage == WCM_DESKTOP) {
> if (finger) {
> - features->device_type = BTN_TOOL_FINGER;
> - if (features->type == TABLETPC2FG) {
> - /* need to reset back */
> - features->pktlen = WACOM_PKGLEN_TPC2FG;
> + int type = features->type;
> +
> + if (type == TABLETPC2FG || type == MTSCREEN) {
> features->y_max =
> get_unaligned_le16(&report[i + 3]);
> features->y_phy =
> get_unaligned_le16(&report[i + 6]);
> i += 7;
> - } else if (features->type == BAMBOO_PT) {
> - /* need to reset back */
> - features->pktlen = WACOM_PKGLEN_BBTOUCH;
> + } else if (type == BAMBOO_PT) {
> features->y_phy =
> get_unaligned_le16(&report[i + 3]);
> features->y_max =
> @@ -371,10 +372,6 @@ static int wacom_parse_hid(struct usb_interface *intf,
> i += 4;
> }
> } else if (pen) {
> - /* penabled only accepts exact bytes of data */
> - if (features->type == TABLETPC2FG)
> - features->pktlen = WACOM_PKGLEN_GRAPHIRE;
> - features->device_type = BTN_TOOL_PEN;
> features->y_max =
> get_unaligned_le16(&report[i + 3]);
> i += 4;
This hunk is probably the one mucking up, as it adds a new test for
features->device_type == BTN_TOOL_FINGER, which given the above change
may never be true....:
> @@ -437,22 +434,29 @@ static int wacom_query_tablet_data(struct usb_interface *intf, struct wacom_feat
> if (!rep_data)
> return error;
>
> - /* ask to report tablet data if it is MT Tablet PC or
> - * not a Tablet PC */
> - if (features->type == TABLETPC2FG) {
> - do {
> - rep_data[0] = 3;
> - rep_data[1] = 4;
> - rep_data[2] = 0;
> - rep_data[3] = 0;
> - report_id = 3;
> - error = wacom_set_report(intf, WAC_HID_FEATURE_REPORT,
> - report_id, rep_data, 4, 1);
> - if (error >= 0)
> - error = wacom_get_report(intf,
> - WAC_HID_FEATURE_REPORT,
> - report_id, rep_data, 4, 1);
> - } while ((error < 0 || rep_data[1] != 4) && limit++ < WAC_MSG_RETRIES);
> + /* ask to report Wacom data */
> + if (features->device_type == BTN_TOOL_FINGER) {
> + /* if it is an MT Tablet PC touch */
> + if (features->type == TABLETPC2FG ||
> + features->type == MTSCREEN) {
> + do {
> + rep_data[0] = 3;
> + rep_data[1] = 4;
> + rep_data[2] = 0;
> + rep_data[3] = 0;
> + report_id = 3;
> + error = wacom_set_report(intf,
> + WAC_HID_FEATURE_REPORT,
> + report_id,
> + rep_data, 4, 1);
> + if (error >= 0)
> + error = wacom_get_report(intf,
> + WAC_HID_FEATURE_REPORT,
> + report_id,
> + rep_data, 4, 1);
> + } while ((error < 0 || rep_data[1] != 4) &&
> + limit++ < WAC_MSG_RETRIES);
> + }
> } else if (features->type != TABLETPC &&
> features->type != WIRELESS &&
> features->device_type == BTN_TOOL_PEN) {
Both those hunks should have been only changing the test
"type == TABLETPC2FG" to "type == TABLETPC2FG || type == MTSCREEN"
All the other changes unrelated to the new feature added and affecting
already supported devices have no place here. And the fact that they
seem completely unnecessary makes it worse.
IMHO you can ask upstream to just revert that commit without further
debugging. By creating a big commit like that, with multiple unrelated
changes mixed with new features, upstream has made sure that bisecting
the real bug is impossible. Patching that up is like painting over a
hole in the wall. You will never now what other devices this broke.
Bjørn
Reply to: