linux-kernel - Re: [RFC/PATCH] Input: release MT fingers on device reset/disconnect

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120924165940.GA451@polaris.bitmath.org>
Date:	Mon, 24 Sep 2012 18:59:40 +0200
From:	"Henrik Rydberg" <rydberg@...omail.se>
To:	Yufeng Shen <miletus@...omium.org>
Cc:	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Daniel Kurtz <djkurtz@...omium.org>,
	Benson Leung <bleung@...omium.org>
Subject: Re: [RFC/PATCH] Input: release MT fingers on device reset/disconnect

Hi Yufeng,

> We are already releasing all pressed keys on device reset and
> disconnect. This patch adds the finger release for multi-touch
> device.
> 
> Different than the key release logic that only the pressed keys
> are released, here all the possible fingers are released even
> it is not recorded as touched down in the input_dev's internal
> state. The motivation is that while releasing an alreay released
> finger won't do harm to downstream, it is helpful in the case
> when the downstream is out of sync with input_dev's internal
> state, we are bringing them back to in sync by cleaning up both
> input_dev and downstream's finger states.
> 
> Signed-off-by: Yufeng Shen <miletus@...omium.org>
> ---

To reset the input state centrally upon disconnect makes some sense;
upon suspend/resume, less so. It is really up to the driver to
determine what part of the input state survives a suspend/resume
cycle

>  drivers/input/input.c |   34 ++++++++++++++++++++++++++++++++--
>  1 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 768e46b..ebdc450 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -584,6 +584,34 @@ void input_close_device(struct input_handle *handle)
>  EXPORT_SYMBOL(input_close_device);
>  
>  /*
> + * Simulate finger release events for all possible fingers. It has no harm
> + * to release an alreay released finger, but has the benefit that if the
> + * downstream finger states are out of sync with input_dev's finger states,
> + * we are helping to clean them up.
> + * The function must be called with dev->event_lock held.
> + */
> +static void input_dev_release_mt(struct input_dev *dev)

This should be moved to input-mt.c instead.

> +{
> +	int i;
> +
> +	if (!dev->mt)
> +		return;
> +
> +	if (!is_event_supported(EV_ABS, dev->evbit, EV_MAX) ||
> +	    !is_event_supported(ABS_MT_SLOT, dev->absbit, ABS_MAX) ||
> +	    !is_event_supported(ABS_MT_TRACKING_ID, dev->absbit, ABS_MAX))
> +		return;

No need to check those; implied by mtsize != 0.

> +
> +	for (i = 0; i < dev->mtsize; i++) {
> +		input_mt_set_value(&dev->mt[i], ABS_MT_TRACKING_ID, -1);
> +		input_pass_event(dev, EV_ABS, ABS_MT_SLOT, i);
> +		input_pass_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
> +	}

Please use the proper api here (see any mt driver for hints).

> +
> +	input_pass_event(dev, EV_SYN, SYN_REPORT, 1);

input_sync()

> +}
> +
> +/*
>   * Simulate keyup events for all keys that are marked as pressed.
>   * The function must be called with dev->event_lock held.
>   */
> @@ -627,6 +655,7 @@ static void input_disconnect_device(struct input_dev *dev)
>  	 * reach any handlers.
>  	 */
>  	input_dev_release_keys(dev);
> +	input_dev_release_mt(dev);
>  
>  	list_for_each_entry(handle, &dev->h_list, d_node)
>  		handle->open = 0;
> @@ -1569,7 +1598,7 @@ static void input_dev_toggle(struct input_dev *dev, bool activate)
>   * @dev: input device whose state needs to be reset
>   *
>   * This function tries to reset the state of an opened input device and
> - * bring internal state and state if the hardware in sync with each other.
> + * bring internal state and state of the hardware in sync with each other.
>   * We mark all keys as released, restore LED state, repeat rate, etc.
>   */
>  void input_reset_device(struct input_dev *dev)
> @@ -1581,10 +1610,11 @@ void input_reset_device(struct input_dev *dev)
>  
>  		/*
>  		 * Keys that have been pressed at suspend time are unlikely
> -		 * to be still pressed when we resume.
> +		 * to be still pressed when we resume. Same logic for MT.
>  		 */
>  		spin_lock_irq(&dev->event_lock);
>  		input_dev_release_keys(dev);
> +		input_dev_release_mt(dev);

Since this is called from input_dev_resume(), I am not so sure this is
a good idea. Rather, we should be able to rely on the driver's
suspend/resume implementation here.

>  		spin_unlock_irq(&dev->event_lock);
>  	}
>  
> -- 
> 1.7.7.3
> 

Thanks,
Henrik
--
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/