Re: [PATCH 03/11] aiptek: Fix probable mask then right shift defects
From: Joe Perches <joe@perches.com>
Date: 2014-10-27 17:56:59
Also in:
lkml
Subsystem:
input (keyboard, mouse, joystick, touchscreen) drivers, the rest · Maintainers:
Dmitry Torokhov, Linus Torvalds
On Mon, 2014-10-27 at 07:44 -0700, Dmitry Torokhov wrote:
Hi Joe,
Hello Dmitry.
On Sun, Oct 26, 2014 at 10:24:59PM -0700, Joe Perches wrote:quoted
Precedence of & and >> is not the same and is not left to right. shift has higher precedence and should be done after the mask.Looking at the protocol description the current code is exactly right. We want to "move" button bits first as in packet type 1 they are in a different place than in other packets. I'll take a patch that adds parenthesis around shifts to make clear it is intended.
I think it's more sensible to do the shift first to a temporary then direct comparisons. Perhaps something like this cleanup that also uses a temporary for aiptek->curSetting and !!(foo & mask) instead of ((foo & mask) != 0) ? 1 : 0; --- drivers/input/tablet/aiptek.c | 149 ++++++++++++++++++++---------------------- 1 file changed, 72 insertions(+), 77 deletions(-)
diff --git a/drivers/input/tablet/aiptek.c b/drivers/input/tablet/aiptek.c
index e7f966d..9c46618 100644
--- a/drivers/input/tablet/aiptek.c
+++ b/drivers/input/tablet/aiptek.c@@ -433,6 +433,7 @@ static const char *map_val_to_str(const struct aiptek_map *map, int val) static void aiptek_irq(struct urb *urb) { struct aiptek *aiptek = urb->context; + struct aiptek_settings *settings = &aiptek->curSetting; unsigned char *data = aiptek->data; struct input_dev *inputdev = aiptek->inputdev; struct usb_interface *intf = aiptek->intf;
@@ -472,26 +473,31 @@ static void aiptek_irq(struct urb *urb) * tool generated the event. */ if (data[0] == 1) { - if (aiptek->curSetting.coordinateMode == + if (settings->coordinateMode == AIPTEK_COORDINATE_ABSOLUTE_MODE) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_RELATIVE_IN_ABSOLUTE; } else { - x = (signed char) data[2]; - y = (signed char) data[3]; - - /* jitterable keeps track of whether any button has been pressed. - * We're also using it to remap the physical mouse button mask - * to pseudo-settings. (We don't specifically care about it's - * value after moving/transposing mouse button bitmasks, except + /* + * Shift buttons pressed to the curSettings location. + * jitterable keeps track of whether any button has + * been pressed. We're also using it to remap the + * physical mouse button mask to pseudo-settings. + * (We don't specifically care about it's value after + * moving/transposing mouse button bitmasks, except * that a non-zero value indicates that one or more * mouse button was pressed.) */ + unsigned char buttons = data[1] << 2; + + x = (signed char)data[2]; + y = (signed char)data[3]; + jitterable = data[1] & 0x07; - left = (data[1] & aiptek->curSetting.mouseButtonLeft >> 2) != 0 ? 1 : 0; - right = (data[1] & aiptek->curSetting.mouseButtonRight >> 2) != 0 ? 1 : 0; - middle = (data[1] & aiptek->curSetting.mouseButtonMiddle >> 2) != 0 ? 1 : 0; + left = !!(buttons & settings->mouseButtonLeft); + right = !!(buttons & settings->mouseButtonRight); + middle = !!(buttons & settings->mouseButtonMiddle); input_report_key(inputdev, BTN_LEFT, left); input_report_key(inputdev, BTN_MIDDLE, middle);
@@ -505,10 +511,10 @@ static void aiptek_irq(struct urb *urb) /* Wheel support is in the form of a single-event * firing. */ - if (aiptek->curSetting.wheel != AIPTEK_WHEEL_DISABLE) { + if (settings->wheel != AIPTEK_WHEEL_DISABLE) { input_report_rel(inputdev, REL_WHEEL, - aiptek->curSetting.wheel); - aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE; + settings->wheel); + settings->wheel = AIPTEK_WHEEL_DISABLE; } if (aiptek->lastMacro != -1) { input_report_key(inputdev,
@@ -522,26 +528,26 @@ static void aiptek_irq(struct urb *urb) * absolute coordinates. */ else if (data[0] == 2) { - if (aiptek->curSetting.coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) { + if (settings->coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_ABSOLUTE_IN_RELATIVE; } else if (!AIPTEK_POINTER_ALLOW_STYLUS_MODE - (aiptek->curSetting.pointerMode)) { + (settings->pointerMode)) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_TOOL_DISALLOWED; } else { x = get_unaligned_le16(data + 1); y = get_unaligned_le16(data + 3); z = get_unaligned_le16(data + 6); - dv = (data[5] & 0x01) != 0 ? 1 : 0; - p = (data[5] & 0x02) != 0 ? 1 : 0; - tip = (data[5] & 0x04) != 0 ? 1 : 0; + dv = !!(data[5] & 0x01); + p = !!(data[5] & 0x02); + tip = !!(data[5] & 0x04); /* Use jitterable to re-arrange button masks */ jitterable = data[5] & 0x18; - bs = (data[5] & aiptek->curSetting.stylusButtonLower) != 0 ? 1 : 0; - pck = (data[5] & aiptek->curSetting.stylusButtonUpper) != 0 ? 1 : 0; + bs = !!(data[5] & settings->stylusButtonLower); + pck = !!(data[5] & settings->stylusButtonUpper); /* dv indicates 'data valid' (e.g., the tablet is in sync * and has delivered a "correct" report) We will ignore
@@ -552,14 +558,14 @@ static void aiptek_irq(struct urb *urb) * tool key, and set the new one. */ if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { + settings->toolMode) { input_report_key(inputdev, aiptek->previousToolMode, 0); input_report_key(inputdev, - aiptek->curSetting.toolMode, + settings->toolMode, 1); aiptek->previousToolMode = - aiptek->curSetting.toolMode; + settings->toolMode; } if (p != 0) {
@@ -571,27 +577,25 @@ static void aiptek_irq(struct urb *urb) input_report_key(inputdev, BTN_STYLUS, bs); input_report_key(inputdev, BTN_STYLUS2, pck); - if (aiptek->curSetting.xTilt != - AIPTEK_TILT_DISABLE) { + if (settings->xTilt != AIPTEK_TILT_DISABLE) { input_report_abs(inputdev, ABS_TILT_X, - aiptek->curSetting.xTilt); + settings->xTilt); } - if (aiptek->curSetting.yTilt != AIPTEK_TILT_DISABLE) { + if (settings->yTilt != AIPTEK_TILT_DISABLE) { input_report_abs(inputdev, ABS_TILT_Y, - aiptek->curSetting.yTilt); + settings->yTilt); } /* Wheel support is in the form of a single-event * firing. */ - if (aiptek->curSetting.wheel != - AIPTEK_WHEEL_DISABLE) { + if (settings->wheel != AIPTEK_WHEEL_DISABLE) { input_report_abs(inputdev, ABS_WHEEL, - aiptek->curSetting.wheel); - aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE; + settings->wheel); + settings->wheel = AIPTEK_WHEEL_DISABLE; } } input_report_abs(inputdev, ABS_MISC, p | AIPTEK_REPORT_TOOL_STYLUS);
@@ -607,10 +611,10 @@ static void aiptek_irq(struct urb *urb) /* Report 3's come from the mouse in absolute mode. */ else if (data[0] == 3) { - if (aiptek->curSetting.coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) { + if (settings->coordinateMode == AIPTEK_COORDINATE_RELATIVE_MODE) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_SENDING_ABSOLUTE_IN_RELATIVE; } else if (!AIPTEK_POINTER_ALLOW_MOUSE_MODE - (aiptek->curSetting.pointerMode)) { + (settings->pointerMode)) { aiptek->diagnostic = AIPTEK_DIAGNOSTIC_TOOL_DISALLOWED; } else { x = get_unaligned_le16(data + 1);
@@ -618,25 +622,25 @@ static void aiptek_irq(struct urb *urb) jitterable = data[5] & 0x1c; - dv = (data[5] & 0x01) != 0 ? 1 : 0; - p = (data[5] & 0x02) != 0 ? 1 : 0; - left = (data[5] & aiptek->curSetting.mouseButtonLeft) != 0 ? 1 : 0; - right = (data[5] & aiptek->curSetting.mouseButtonRight) != 0 ? 1 : 0; - middle = (data[5] & aiptek->curSetting.mouseButtonMiddle) != 0 ? 1 : 0; + dv = !!(data[5] & 0x01); + p = !!(data[5] & 0x02); + left = !!(data[5] & settings->mouseButtonLeft); + right = !!(data[5] & settings->mouseButtonRight); + middle = !!(data[5] & settings->mouseButtonMiddle); if (dv != 0) { /* If the selected tool changed, reset the old * tool key, and set the new one. */ if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { + settings->toolMode) { input_report_key(inputdev, aiptek->previousToolMode, 0); input_report_key(inputdev, - aiptek->curSetting.toolMode, + settings->toolMode, 1); aiptek->previousToolMode = - aiptek->curSetting.toolMode; + settings->toolMode; } if (p != 0) {
@@ -650,11 +654,11 @@ static void aiptek_irq(struct urb *urb) /* Wheel support is in the form of a single-event * firing. */ - if (aiptek->curSetting.wheel != AIPTEK_WHEEL_DISABLE) { + if (settings->wheel != AIPTEK_WHEEL_DISABLE) { input_report_abs(inputdev, ABS_WHEEL, - aiptek->curSetting.wheel); - aiptek->curSetting.wheel = AIPTEK_WHEEL_DISABLE; + settings->wheel); + settings->wheel = AIPTEK_WHEEL_DISABLE; } } input_report_abs(inputdev, ABS_MISC, p | AIPTEK_REPORT_TOOL_MOUSE);
@@ -672,11 +676,11 @@ static void aiptek_irq(struct urb *urb) else if (data[0] == 4) { jitterable = data[1] & 0x18; - dv = (data[1] & 0x01) != 0 ? 1 : 0; - p = (data[1] & 0x02) != 0 ? 1 : 0; - tip = (data[1] & 0x04) != 0 ? 1 : 0; - bs = (data[1] & aiptek->curSetting.stylusButtonLower) != 0 ? 1 : 0; - pck = (data[1] & aiptek->curSetting.stylusButtonUpper) != 0 ? 1 : 0; + dv = !!(data[1] & 0x01); + p = !!(data[1] & 0x02); + tip = !!(data[1] & 0x04); + bs = !!(data[1] & settings->stylusButtonLower); + pck = !!(data[1] & settings->stylusButtonUpper); macro = dv && p && tip && !(data[3] & 1) ? (data[3] >> 1) : -1; z = get_unaligned_le16(data + 4);
@@ -685,15 +689,12 @@ static void aiptek_irq(struct urb *urb) /* If the selected tool changed, reset the old * tool key, and set the new one. */ - if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { + if (aiptek->previousToolMode != settings->toolMode) { input_report_key(inputdev, aiptek->previousToolMode, 0); input_report_key(inputdev, - aiptek->curSetting.toolMode, - 1); - aiptek->previousToolMode = - aiptek->curSetting.toolMode; + settings->toolMode, 1); + aiptek->previousToolMode = settings->toolMode; } }
@@ -715,24 +716,23 @@ static void aiptek_irq(struct urb *urb) else if (data[0] == 5) { jitterable = data[1] & 0x1c; - dv = (data[1] & 0x01) != 0 ? 1 : 0; - p = (data[1] & 0x02) != 0 ? 1 : 0; - left = (data[1]& aiptek->curSetting.mouseButtonLeft) != 0 ? 1 : 0; - right = (data[1] & aiptek->curSetting.mouseButtonRight) != 0 ? 1 : 0; - middle = (data[1] & aiptek->curSetting.mouseButtonMiddle) != 0 ? 1 : 0; + dv = !!(data[1] & 0x01); + p = !!(data[1] & 0x02); + left = !!(data[1]& settings->mouseButtonLeft); + right = !!(data[1] & settings->mouseButtonRight); + middle = !!(data[1] & settings->mouseButtonMiddle); macro = dv && p && left && !(data[3] & 1) ? (data[3] >> 1) : 0; if (dv) { /* If the selected tool changed, reset the old * tool key, and set the new one. */ - if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { + if (aiptek->previousToolMode != settings->toolMode) { input_report_key(inputdev, aiptek->previousToolMode, 0); input_report_key(inputdev, - aiptek->curSetting.toolMode, 1); - aiptek->previousToolMode = aiptek->curSetting.toolMode; + settings->toolMode, 1); + aiptek->previousToolMode = settings->toolMode; } }
@@ -770,15 +770,10 @@ static void aiptek_irq(struct urb *urb) /* If the selected tool changed, reset the old tool key, and set the new one. */ - if (aiptek->previousToolMode != - aiptek->curSetting.toolMode) { - input_report_key(inputdev, - aiptek->previousToolMode, 0); - input_report_key(inputdev, - aiptek->curSetting.toolMode, - 1); - aiptek->previousToolMode = - aiptek->curSetting.toolMode; + if (aiptek->previousToolMode != settings->toolMode) { + input_report_key(inputdev, aiptek->previousToolMode, 0); + input_report_key(inputdev, settings->toolMode, 1); + aiptek->previousToolMode = settings->toolMode; } input_report_key(inputdev, macroKeyEvents[macro], 1);
@@ -802,9 +797,9 @@ static void aiptek_irq(struct urb *urb) */ if (aiptek->previousJitterable != jitterable && - aiptek->curSetting.jitterDelay != 0 && aiptek->inDelay != 1) { + settings->jitterDelay != 0 && aiptek->inDelay != 1) { aiptek->endDelay = jiffies + - ((aiptek->curSetting.jitterDelay * HZ) / 1000); + ((settings->jitterDelay * HZ) / 1000); aiptek->inDelay = 1; } aiptek->previousJitterable = jitterable;