Re: [PATCH v2] Input - mt: Fix input_mt_get_slot_by_key

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Mon, Mar 30, 2015 at 02:54:15PM -0400, Benjamin Tissoires wrote:
> The case occurred recently with a touchscreen using twice a slot during a
> single EV_SYN event:
> 
> E: 0.288415 0000 0000 0000      # ------------ SYN_REPORT (0) ----------
> E: 0.296207 0003 002f 0000      # EV_ABS / ABS_MT_SLOT          0
> E: 0.296207 0003 0039 -001      # EV_ABS / ABS_MT_TRACKING_ID   -1
> E: 0.296207 0003 002f 0001      # EV_ABS / ABS_MT_SLOT          1
> E: 0.296207 0003 0035 0908      # EV_ABS / ABS_MT_POSITION_X    908
> E: 0.296207 0003 0036 1062      # EV_ABS / ABS_MT_POSITION_Y    1062
> E: 0.296207 0003 002f 0000      # EV_ABS / ABS_MT_SLOT          0
> E: 0.296207 0003 0039 8787      # EV_ABS / ABS_MT_TRACKING_ID   8787
> E: 0.296207 0003 0035 1566      # EV_ABS / ABS_MT_POSITION_X    1566
> E: 0.296207 0003 0036 0861      # EV_ABS / ABS_MT_POSITION_Y    861
> E: 0.296207 0003 0000 0908      # EV_ABS / ABS_X                908
> E: 0.296207 0003 0001 1062      # EV_ABS / ABS_Y                1062
> E: 0.296207 0000 0000 0000      # ------------ SYN_REPORT (0) ----------
> 
> This occurred because while having already slots 0 and 1 assigned, the
> touchscreen sent:
> 
> 0.293377 Tip Switch: 0 | Contact Id: 0 | X:  539 | Y: 1960 | Contact Count: 3
> 0.294783 Tip Switch: 1 | Contact Id: 1 | X:  908 | Y: 1062 | Contact Count: 0
> 0.296187 Tip Switch: 1 | Contact Id: 2 | X: 1566 | Y:  861 | Contact Count: 0
> 
> Slot 0 is released correclty, but when we look for Contact ID 2, the slot
> 0 is then picked up again because it is marked as inactive (trackingID < 0).
> 
> This is wrong, and we should not reuse a slot in the same frame.
> The test should also check for input_mt_is_used().

I wonder if we should call it "input_mt_is_used_in_frame" or similar.

> 
> In addition, we need to initialize mt->frame to an other value than 0.
> With mt->frame being 0, all slots are tags as currently used, and so
> input_mt_get_slot_by_key() would return -1 for all requests.
> 
> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=88903
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> 
> Changes in v2:
> - add note in input_mt_get_slot_by_key that input_mt_sync_frame() is required
> 
> Hi Dmitry, Henrik, Jiri,
> 
> With https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/commit/?h=for-4.1/wacom&id=9a1c001298fd567c0f0776ab54ab9965eeb9019f
> in Jiri's tree, scheduled for 4.1, this patch should not break any existing
> driver. I'd like us to stage it somewhere so that it doesn't get forgotten.
> 
> Henrik's previous concerns were that input_mt_sync_frame() may not be called
> by a driver using input_mt_get_slot_by_key(), and now, no driver should be in
> this case.

I'm OK with it going through Juri's tree.

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>


> 
> Cheers,
> Benjamin
> 
>  drivers/input/input-mt.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index cb150a1..34feb3e 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -88,10 +88,13 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
>  			goto err_mem;
>  	}
>  
> -	/* Mark slots as 'unused' */
> +	/* Mark slots as 'inactive' */
>  	for (i = 0; i < num_slots; i++)
>  		input_mt_set_value(&mt->slots[i], ABS_MT_TRACKING_ID, -1);
>  
> +	/* Mark slots as 'unused' */
> +	mt->frame = 1;
> +
>  	dev->mt = mt;
>  	return 0;
>  err_mem:
> @@ -439,6 +442,8 @@ EXPORT_SYMBOL(input_mt_assign_slots);
>   * set the key on the first unused slot and return.
>   *
>   * If no available slot can be found, -1 is returned.
> + * Note that for this function to work properly, input_mt_sync_frame() has
> + * to be called at each frame.
>   */
>  int input_mt_get_slot_by_key(struct input_dev *dev, int key)
>  {
> @@ -453,7 +458,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int key)
>  			return s - mt->slots;
>  
>  	for (s = mt->slots; s != mt->slots + mt->num_slots; s++)
> -		if (!input_mt_is_active(s)) {
> +		if (!input_mt_is_active(s) && !input_mt_is_used(mt, s)) {
>  			s->key = key;
>  			return s - mt->slots;
>  		}
> -- 
> 2.3.3
> 

-- 
Dmitry