| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20141031165136.GB32331@dtor-ws>
Date: Fri, 31 Oct 2014 09:51:36 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc: Henrik Rydberg <rydberg@...omail.se>,
Hans de Goede <hdegoede@...hat.com>,
Peter Hutterer <peter.hutterer@...-t.net>,
linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
Daniel Kurtz <djkurtz@...omium.org>,
Chung-yih Wang <cywang@...omium.org>
Subject: Re: [PATCH 1/2] Input: synaptics: Use in-kernel tracking for
reporting mt data
On Thu, Oct 30, 2014 at 02:33:06PM -0400, Benjamin Tissoires wrote:
> The current code tries to consider all states and transitions to properly
> detect which finger is attached to which slot. The code is quite huge
> and difficult to read.
> If the sensor manages to group the touch points but is not reliable in
> giving tracking ids, we can simply use the kernel tracking method. Note
> that it is already used by Cr-48 Chromebooks.
>
> Incidentaly, this fixes a bug reported by Peter Hutterer:
> """
> on the Lenovo T440, run:
> evemu-record /dev/input/event4 | grep BTN_
>
> then put one, two, three, two fingers down
> when you go from 3 to 2 fingers the driver sends a spurious BTN_TOUCH 0
> event:
>
> E: 0.000000 0001 014a 0001 # EV_KEY / BTN_TOUCH 1
> E: 0.000000 0001 0145 0001 # EV_KEY / BTN_TOOL_FINGER 1
> E: 0.770008 0001 0145 0000 # EV_KEY / BTN_TOOL_FINGER 0
> E: 0.770008 0001 014d 0001 # EV_KEY / BTN_TOOL_DOUBLETAP 1
> E: 1.924716 0001 014d 0000 # EV_KEY / BTN_TOOL_DOUBLETAP 0
> E: 1.924716 0001 014e 0001 # EV_KEY / BTN_TOOL_TRIPLETAP 1
>
> .. changing from 3 to 2 fingers now
>
> E: 3.152641 0001 014a 0000 # EV_KEY / BTN_TOUCH 0
> E: 3.152641 0001 014d 0001 # EV_KEY / BTN_TOOL_DOUBLETAP 1
> E: 3.152641 0001 014e 0000 # EV_KEY / BTN_TOOL_TRIPLETAP 0
> E: 3.176948 0001 014a 0001 # EV_KEY / BTN_TOUCH 1
>
> quick look in the kernel shows it's caused by hw.z going to 0 for a packet,
> so probably a firmware bug. either way, it makes it hard to track BTN_TOUCH
> as signal that at least one finger is down.
> """
>
> The in-kernel tracking is enough to remove this spurious BTN_TOUCH 0.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> ---
>
> Hi Dmitry,
>
> I started working on that for 2 other bug reports
> https://bugs.freedesktop.org/show_bug.cgi?id=81278
> and
> https://bugs.freedesktop.org/show_bug.cgi?id=76722
>
> I thought the cursor jumps could be fixed by the in-kernel tracking, but the
> tracking needs a little bit more work to filter them out (patches to follow soon).
>
> From a user perspective, this patch does not change anything to what the user
> previously had. It also fixes Peter's bug that's why I decide to send this out
> by itself (removing ~350 lines of code and fixing bugs is always nice).
>
> I think the cursor jump fixes will need more bikeshedding in input-mt.c (I am
> *really* bad at designing APIs), so I'll send it later as an RFC.
Daniel and Chung-yih were working on the driver so let's see if they
have a moment...
>
> Cheers,
> Benjamin
>
> drivers/input/mouse/synaptics.c | 397 ++++------------------------------------
> drivers/input/mouse/synaptics.h | 18 +-
> 2 files changed, 34 insertions(+), 381 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 9031a0a..fd89249 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -569,14 +569,6 @@ static void synaptics_pt_create(struct psmouse *psmouse)
> * Functions to interpret the absolute mode packets
> ****************************************************************************/
>
> -static void synaptics_mt_state_set(struct synaptics_mt_state *state, int count,
> - int sgm, int agm)
> -{
> - state->count = count;
> - state->sgm = sgm;
> - state->agm = agm;
> -}
> -
> static void synaptics_parse_agm(const unsigned char buf[],
> struct synaptics_data *priv,
> struct synaptics_hw_state *hw)
> @@ -595,16 +587,13 @@ static void synaptics_parse_agm(const unsigned char buf[],
> break;
>
> case 2:
> - /* AGM-CONTACT packet: (count, sgm, agm) */
> - synaptics_mt_state_set(&agm->mt_state, buf[1], buf[2], buf[4]);
> + /* AGM-CONTACT packet: we are only interested in the count */
> + priv->agm_count = buf[1];
> break;
>
> default:
> break;
> }
> -
> - /* Record that at least one AGM has been received since last SGM */
> - priv->agm_pending = true;
> }
>
> static bool is_forcepad;
> @@ -798,388 +787,68 @@ static void synaptics_report_buttons(struct psmouse *psmouse,
> input_report_key(dev, BTN_0 + i, hw->ext_buttons & (1 << i));
> }
>
> -static void synaptics_report_slot(struct input_dev *dev, int slot,
> - const struct synaptics_hw_state *hw)
> -{
> - input_mt_slot(dev, slot);
> - input_mt_report_slot_state(dev, MT_TOOL_FINGER, (hw != NULL));
> - if (!hw)
> - return;
> -
> - input_report_abs(dev, ABS_MT_POSITION_X, hw->x);
> - input_report_abs(dev, ABS_MT_POSITION_Y, synaptics_invert_y(hw->y));
> - input_report_abs(dev, ABS_MT_PRESSURE, hw->z);
> -}
> -
> static void synaptics_report_mt_data(struct psmouse *psmouse,
> - struct synaptics_mt_state *mt_state,
> - const struct synaptics_hw_state *sgm)
> + const struct synaptics_hw_state *sgm,
> + int num_fingers)
> {
> struct input_dev *dev = psmouse->dev;
> struct synaptics_data *priv = psmouse->private;
> - struct synaptics_hw_state *agm = &priv->agm;
> - struct synaptics_mt_state *old = &priv->mt_state;
> + const struct synaptics_hw_state *hw[2] = { sgm, &priv->agm };
> + struct input_mt_pos pos[2];
> + int slot[2], nsemi, i;
>
> - switch (mt_state->count) {
> - case 0:
> - synaptics_report_slot(dev, 0, NULL);
> - synaptics_report_slot(dev, 1, NULL);
> - break;
> - case 1:
> - if (mt_state->sgm == -1) {
> - synaptics_report_slot(dev, 0, NULL);
> - synaptics_report_slot(dev, 1, NULL);
> - } else if (mt_state->sgm == 0) {
> - synaptics_report_slot(dev, 0, sgm);
> - synaptics_report_slot(dev, 1, NULL);
> - } else {
> - synaptics_report_slot(dev, 0, NULL);
> - synaptics_report_slot(dev, 1, sgm);
> - }
> - break;
> - default:
> - /*
> - * If the finger slot contained in SGM is valid, and either
> - * hasn't changed, or is new, or the old SGM has now moved to
> - * AGM, then report SGM in MTB slot 0.
> - * Otherwise, empty MTB slot 0.
> - */
> - if (mt_state->sgm != -1 &&
> - (mt_state->sgm == old->sgm ||
> - old->sgm == -1 || mt_state->agm == old->sgm))
> - synaptics_report_slot(dev, 0, sgm);
> - else
> - synaptics_report_slot(dev, 0, NULL);
> + nsemi = clamp_val(num_fingers, 0, 2);
>
> - /*
> - * If the finger slot contained in AGM is valid, and either
> - * hasn't changed, or is new, then report AGM in MTB slot 1.
> - * Otherwise, empty MTB slot 1.
> - *
> - * However, in the case where the AGM is new, make sure that
> - * that it is either the same as the old SGM, or there was no
> - * SGM.
> - *
> - * Otherwise, if the SGM was just 1, and the new AGM is 2, then
> - * the new AGM will keep the old SGM's tracking ID, which can
> - * cause apparent drumroll. This happens if in the following
> - * valid finger sequence:
> - *
> - * Action SGM AGM (MTB slot:Contact)
> - * 1. Touch contact 0 (0:0)
> - * 2. Touch contact 1 (0:0, 1:1)
> - * 3. Lift contact 0 (1:1)
> - * 4. Touch contacts 2,3 (0:2, 1:3)
> - *
> - * In step 4, contact 3, in AGM must not be given the same
> - * tracking ID as contact 1 had in step 3. To avoid this,
> - * the first agm with contact 3 is dropped and slot 1 is
> - * invalidated (tracking ID = -1).
> - */
> - if (mt_state->agm != -1 &&
> - (mt_state->agm == old->agm ||
> - (old->agm == -1 &&
> - (old->sgm == -1 || mt_state->agm == old->sgm))))
> - synaptics_report_slot(dev, 1, agm);
> - else
> - synaptics_report_slot(dev, 1, NULL);
> - break;
> + for (i = 0; i < nsemi; i++) {
> + pos[i].x = hw[i]->x;
> + pos[i].y = synaptics_invert_y(hw[i]->y);
> }
>
> + input_mt_assign_slots(dev, slot, pos, nsemi);
> +
> + for (i = 0; i < nsemi; i++) {
> + input_mt_slot(dev, slot[i]);
> + input_mt_report_slot_state(dev, MT_TOOL_FINGER, true);
> + input_report_abs(dev, ABS_MT_POSITION_X, pos[i].x);
> + input_report_abs(dev, ABS_MT_POSITION_Y, pos[i].y);
> + input_report_abs(dev, ABS_MT_PRESSURE, hw[i]->z);
> + }
> +
> + input_mt_drop_unused(dev);
> +
> /* Don't use active slot count to generate BTN_TOOL events. */
> input_mt_report_pointer_emulation(dev, false);
>
> /* Send the number of fingers reported by touchpad itself. */
> - input_mt_report_finger_count(dev, mt_state->count);
> + input_mt_report_finger_count(dev, num_fingers);
>
> synaptics_report_buttons(psmouse, sgm);
>
> input_sync(dev);
> }
>
> -/* Handle case where mt_state->count = 0 */
> -static void synaptics_image_sensor_0f(struct synaptics_data *priv,
> - struct synaptics_mt_state *mt_state)
> -{
> - synaptics_mt_state_set(mt_state, 0, -1, -1);
> - priv->mt_state_lost = false;
> -}
> -
> -/* Handle case where mt_state->count = 1 */
> -static void synaptics_image_sensor_1f(struct synaptics_data *priv,
> - struct synaptics_mt_state *mt_state)
> -{
> - struct synaptics_hw_state *agm = &priv->agm;
> - struct synaptics_mt_state *old = &priv->mt_state;
> -
> - /*
> - * If the last AGM was (0,0,0), and there is only one finger left,
> - * then we absolutely know that SGM contains slot 0, and all other
> - * fingers have been removed.
> - */
> - if (priv->agm_pending && agm->z == 0) {
> - synaptics_mt_state_set(mt_state, 1, 0, -1);
> - priv->mt_state_lost = false;
> - return;
> - }
> -
> - switch (old->count) {
> - case 0:
> - synaptics_mt_state_set(mt_state, 1, 0, -1);
> - break;
> - case 1:
> - /*
> - * If mt_state_lost, then the previous transition was 3->1,
> - * and SGM now contains either slot 0 or 1, but we don't know
> - * which. So, we just assume that the SGM now contains slot 1.
> - *
> - * If pending AGM and either:
> - * (a) the previous SGM slot contains slot 0, or
> - * (b) there was no SGM slot
> - * then, the SGM now contains slot 1
> - *
> - * Case (a) happens with very rapid "drum roll" gestures, where
> - * slot 0 finger is lifted and a new slot 1 finger touches
> - * within one reporting interval.
> - *
> - * Case (b) happens if initially two or more fingers tap
> - * briefly, and all but one lift before the end of the first
> - * reporting interval.
> - *
> - * (In both these cases, slot 0 will becomes empty, so SGM
> - * contains slot 1 with the new finger)
> - *
> - * Else, if there was no previous SGM, it now contains slot 0.
> - *
> - * Otherwise, SGM still contains the same slot.
> - */
> - if (priv->mt_state_lost ||
> - (priv->agm_pending && old->sgm <= 0))
> - synaptics_mt_state_set(mt_state, 1, 1, -1);
> - else if (old->sgm == -1)
> - synaptics_mt_state_set(mt_state, 1, 0, -1);
> - break;
> - case 2:
> - /*
> - * If mt_state_lost, we don't know which finger SGM contains.
> - *
> - * So, report 1 finger, but with both slots empty.
> - * We will use slot 1 on subsequent 1->1
> - */
> - if (priv->mt_state_lost) {
> - synaptics_mt_state_set(mt_state, 1, -1, -1);
> - break;
> - }
> - /*
> - * Since the last AGM was NOT (0,0,0), it was the finger in
> - * slot 0 that has been removed.
> - * So, SGM now contains previous AGM's slot, and AGM is now
> - * empty.
> - */
> - synaptics_mt_state_set(mt_state, 1, old->agm, -1);
> - break;
> - case 3:
> - /*
> - * Since last AGM was not (0,0,0), we don't know which finger
> - * is left.
> - *
> - * So, report 1 finger, but with both slots empty.
> - * We will use slot 1 on subsequent 1->1
> - */
> - synaptics_mt_state_set(mt_state, 1, -1, -1);
> - priv->mt_state_lost = true;
> - break;
> - case 4:
> - case 5:
> - /* mt_state was updated by AGM-CONTACT packet */
> - break;
> - }
> -}
> -
> -/* Handle case where mt_state->count = 2 */
> -static void synaptics_image_sensor_2f(struct synaptics_data *priv,
> - struct synaptics_mt_state *mt_state)
> -{
> - struct synaptics_mt_state *old = &priv->mt_state;
> -
> - switch (old->count) {
> - case 0:
> - synaptics_mt_state_set(mt_state, 2, 0, 1);
> - break;
> - case 1:
> - /*
> - * If previous SGM contained slot 1 or higher, SGM now contains
> - * slot 0 (the newly touching finger) and AGM contains SGM's
> - * previous slot.
> - *
> - * Otherwise, SGM still contains slot 0 and AGM now contains
> - * slot 1.
> - */
> - if (old->sgm >= 1)
> - synaptics_mt_state_set(mt_state, 2, 0, old->sgm);
> - else
> - synaptics_mt_state_set(mt_state, 2, 0, 1);
> - break;
> - case 2:
> - /*
> - * If mt_state_lost, SGM now contains either finger 1 or 2, but
> - * we don't know which.
> - * So, we just assume that the SGM contains slot 0 and AGM 1.
> - */
> - if (priv->mt_state_lost)
> - synaptics_mt_state_set(mt_state, 2, 0, 1);
> - /*
> - * Otherwise, use the same mt_state, since it either hasn't
> - * changed, or was updated by a recently received AGM-CONTACT
> - * packet.
> - */
> - break;
> - case 3:
> - /*
> - * 3->2 transitions have two unsolvable problems:
> - * 1) no indication is given which finger was removed
> - * 2) no way to tell if agm packet was for finger 3
> - * before 3->2, or finger 2 after 3->2.
> - *
> - * So, report 2 fingers, but empty all slots.
> - * We will guess slots [0,1] on subsequent 2->2.
> - */
> - synaptics_mt_state_set(mt_state, 2, -1, -1);
> - priv->mt_state_lost = true;
> - break;
> - case 4:
> - case 5:
> - /* mt_state was updated by AGM-CONTACT packet */
> - break;
> - }
> -}
> -
> -/* Handle case where mt_state->count = 3 */
> -static void synaptics_image_sensor_3f(struct synaptics_data *priv,
> - struct synaptics_mt_state *mt_state)
> -{
> - struct synaptics_mt_state *old = &priv->mt_state;
> -
> - switch (old->count) {
> - case 0:
> - synaptics_mt_state_set(mt_state, 3, 0, 2);
> - break;
> - case 1:
> - /*
> - * If previous SGM contained slot 2 or higher, SGM now contains
> - * slot 0 (one of the newly touching fingers) and AGM contains
> - * SGM's previous slot.
> - *
> - * Otherwise, SGM now contains slot 0 and AGM contains slot 2.
> - */
> - if (old->sgm >= 2)
> - synaptics_mt_state_set(mt_state, 3, 0, old->sgm);
> - else
> - synaptics_mt_state_set(mt_state, 3, 0, 2);
> - break;
> - case 2:
> - /*
> - * If the AGM previously contained slot 3 or higher, then the
> - * newly touching finger is in the lowest available slot.
> - *
> - * If SGM was previously 1 or higher, then the new SGM is
> - * now slot 0 (with a new finger), otherwise, the new finger
> - * is now in a hidden slot between 0 and AGM's slot.
> - *
> - * In all such cases, the SGM now contains slot 0, and the AGM
> - * continues to contain the same slot as before.
> - */
> - if (old->agm >= 3) {
> - synaptics_mt_state_set(mt_state, 3, 0, old->agm);
> - break;
> - }
> -
> - /*
> - * After some 3->1 and all 3->2 transitions, we lose track
> - * of which slot is reported by SGM and AGM.
> - *
> - * For 2->3 in this state, report 3 fingers, but empty all
> - * slots, and we will guess (0,2) on a subsequent 0->3.
> - *
> - * To userspace, the resulting transition will look like:
> - * 2:[0,1] -> 3:[-1,-1] -> 3:[0,2]
> - */
> - if (priv->mt_state_lost) {
> - synaptics_mt_state_set(mt_state, 3, -1, -1);
> - break;
> - }
> -
> - /*
> - * If the (SGM,AGM) really previously contained slots (0, 1),
> - * then we cannot know what slot was just reported by the AGM,
> - * because the 2->3 transition can occur either before or after
> - * the AGM packet. Thus, this most recent AGM could contain
> - * either the same old slot 1 or the new slot 2.
> - * Subsequent AGMs will be reporting slot 2.
> - *
> - * To userspace, the resulting transition will look like:
> - * 2:[0,1] -> 3:[0,-1] -> 3:[0,2]
> - */
> - synaptics_mt_state_set(mt_state, 3, 0, -1);
> - break;
> - case 3:
> - /*
> - * If, for whatever reason, the previous agm was invalid,
> - * Assume SGM now contains slot 0, AGM now contains slot 2.
> - */
> - if (old->agm <= 2)
> - synaptics_mt_state_set(mt_state, 3, 0, 2);
> - /*
> - * mt_state either hasn't changed, or was updated by a recently
> - * received AGM-CONTACT packet.
> - */
> - break;
> -
> - case 4:
> - case 5:
> - /* mt_state was updated by AGM-CONTACT packet */
> - break;
> - }
> -}
> -
> -/* Handle case where mt_state->count = 4, or = 5 */
> -static void synaptics_image_sensor_45f(struct synaptics_data *priv,
> - struct synaptics_mt_state *mt_state)
> -{
> - /* mt_state was updated correctly by AGM-CONTACT packet */
> - priv->mt_state_lost = false;
> -}
> -
> static void synaptics_image_sensor_process(struct psmouse *psmouse,
> struct synaptics_hw_state *sgm)
> {
> struct synaptics_data *priv = psmouse->private;
> - struct synaptics_hw_state *agm = &priv->agm;
> - struct synaptics_mt_state mt_state;
> -
> - /* Initialize using current mt_state (as updated by last agm) */
> - mt_state = agm->mt_state;
> + int num_fingers;
>
> /*
> * Update mt_state using the new finger count and current mt_state.
> */
> if (sgm->z == 0)
> - synaptics_image_sensor_0f(priv, &mt_state);
> + num_fingers = 0;
> else if (sgm->w >= 4)
> - synaptics_image_sensor_1f(priv, &mt_state);
> + num_fingers = 1;
> else if (sgm->w == 0)
> - synaptics_image_sensor_2f(priv, &mt_state);
> - else if (sgm->w == 1 && mt_state.count <= 3)
> - synaptics_image_sensor_3f(priv, &mt_state);
> + num_fingers = 2;
> + else if (sgm->w == 1)
> + num_fingers = priv->agm_count ? priv->agm_count : 3;
> else
> - synaptics_image_sensor_45f(priv, &mt_state);
> + num_fingers = 4;
>
> /* Send resulting input events to user space */
> - synaptics_report_mt_data(psmouse, &mt_state, sgm);
> -
> - /* Store updated mt_state */
> - priv->mt_state = agm->mt_state = mt_state;
> - priv->agm_pending = false;
> + synaptics_report_mt_data(psmouse, sgm, num_fingers);
> }
>
> static void synaptics_profile_sensor_process(struct psmouse *psmouse,
> @@ -1439,7 +1108,7 @@ static void set_input_params(struct psmouse *psmouse,
> ABS_MT_POSITION_Y);
> /* Image sensors can report per-contact pressure */
> input_set_abs_params(dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> - input_mt_init_slots(dev, 2, INPUT_MT_POINTER);
> + input_mt_init_slots(dev, 2, INPUT_MT_POINTER | INPUT_MT_TRACK);
>
> /* Image sensors can signal 4 and 5 finger clicks */
> __set_bit(BTN_TOOL_QUADTAP, dev->keybit);
> diff --git a/drivers/input/mouse/synaptics.h b/drivers/input/mouse/synaptics.h
> index 1bd01f2..6faf9bb 100644
> --- a/drivers/input/mouse/synaptics.h
> +++ b/drivers/input/mouse/synaptics.h
> @@ -119,16 +119,6 @@
> #define SYN_REDUCED_FILTER_FUZZ 8
>
> /*
> - * A structure to describe which internal touchpad finger slots are being
> - * reported in raw packets.
> - */
> -struct synaptics_mt_state {
> - int count; /* num fingers being tracked */
> - int sgm; /* which slot is reported by sgm pkt */
> - int agm; /* which slot is reported by agm pkt*/
> -};
> -
> -/*
> * A structure to describe the state of the touchpad hardware (buttons and pad)
> */
> struct synaptics_hw_state {
> @@ -143,9 +133,6 @@ struct synaptics_hw_state {
> unsigned int down:1;
> unsigned char ext_buttons;
> signed char scroll;
> -
> - /* As reported in last AGM-CONTACT packets */
> - struct synaptics_mt_state mt_state;
> };
>
> struct synaptics_data {
> @@ -170,15 +157,12 @@ struct synaptics_data {
>
> struct serio *pt_port; /* Pass-through serio port */
>
> - struct synaptics_mt_state mt_state; /* Current mt finger state */
> - bool mt_state_lost; /* mt_state may be incorrect */
> -
> /*
> * Last received Advanced Gesture Mode (AGM) packet. An AGM packet
> * contains position data for a second contact, at half resolution.
> */
> struct synaptics_hw_state agm;
> - bool agm_pending; /* new AGM packet received */
> + unsigned int agm_count; /* finger count reported by agm */
>
> /* ForcePad handling */
> unsigned long press_start;
> --
> 2.1.0
>
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists