linux-kernel - Re: [PATCH] media: ttpci: remove autorepeat handling and use timer

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20171031182236.cxrasbayon7h52mm@dtor-ws>
Date:   Tue, 31 Oct 2017 11:22:36 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Sean Young <sean@...s.org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hans.verkuil@...co.com>,
        Kees Cook <keescook@...omium.org>, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: ttpci: remove autorepeat handling and use
 timer_setup

Hi Sean,

On Tue, Oct 31, 2017 at 05:45:58PM +0000, Sean Young wrote:
> Leave the autorepeat handling up to the input layer, and move
> to the new timer API.
> 
> Compile tested only.
> 
> Signed-off-by: Sean Young <sean@...s.org>
> ---
>  drivers/media/pci/ttpci/av7110.h    |  2 +-
>  drivers/media/pci/ttpci/av7110_ir.c | 54 ++++++++++++++-----------------------
>  2 files changed, 21 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
> index 347827925c14..bcb72ecbedc0 100644
> --- a/drivers/media/pci/ttpci/av7110.h
> +++ b/drivers/media/pci/ttpci/av7110.h
> @@ -93,7 +93,7 @@ struct infrared {
>  	u8			inversion;
>  	u16			last_key;
>  	u16			last_toggle;
> -	u8			delay_timer_finished;
> +	bool			keypressed;
>  };
>  
>  
> diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
> index ca05198de2c2..8207bead2224 100644
> --- a/drivers/media/pci/ttpci/av7110_ir.c
> +++ b/drivers/media/pci/ttpci/av7110_ir.c
> @@ -84,15 +84,16 @@ static u16 default_key_map [256] = {
>  
>  
>  /* key-up timer */
> -static void av7110_emit_keyup(unsigned long parm)
> +static void av7110_emit_keyup(struct timer_list *t)
>  {
> -	struct infrared *ir = (struct infrared *) parm;
> +	struct infrared *ir = from_timer(ir, t, keyup_timer);
>  
> -	if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> +	if (!ir || !ir->keypressed)
>  		return;
>  
>  	input_report_key(ir->input_dev, ir->last_key, 0);
>  	input_sync(ir->input_dev);
> +	ir->keypressed = false;
>  }
>  
>  
> @@ -105,6 +106,7 @@ static void av7110_emit_key(unsigned long parm)
>  	u8 addr;
>  	u16 toggle;
>  	u16 keycode;
> +	bool new_event;
>  
>  	/* extract device address and data */
>  	switch (ir->protocol) {
> @@ -152,29 +154,22 @@ static void av7110_emit_key(unsigned long parm)
>  		return;
>  	}
>  
> -	if (timer_pending(&ir->keyup_timer)) {
> -		del_timer(&ir->keyup_timer);
> -		if (ir->last_key != keycode || toggle != ir->last_toggle) {
> -			ir->delay_timer_finished = 0;
> -			input_event(ir->input_dev, EV_KEY, ir->last_key, 0);
> -			input_event(ir->input_dev, EV_KEY, keycode, 1);
> -			input_sync(ir->input_dev);
> -		} else if (ir->delay_timer_finished) {
> -			input_event(ir->input_dev, EV_KEY, keycode, 2);
> -			input_sync(ir->input_dev);
> -		}
> -	} else {
> -		ir->delay_timer_finished = 0;
> -		input_event(ir->input_dev, EV_KEY, keycode, 1);
> +	new_event = !ir->keypressed || ir->last_key != keycode ||
> +		   toggle != ir->last_toggle;
> +
> +	if (new_event && ir->keypressed)
> +		input_event(ir->input_dev, EV_KEY, ir->last_key, 1);
> +
> +	if (new_event) {
> +		input_event(ir->input_dev, EV_KEY, keycode, 0);

I do not think this is correct. You want to release the old button, and
press the new one, not the other way around.

Given that we are reworking the code, and input core actually filters
out duplicate events for you, you can probably simplify it all further:


	/* Release old key/button */
	if (ir->keypressed && ir->last_key != keycode)
		input_event(ir->input_dev, EV_KEY, ir->last_key, 0);

	input_event(ir->input_dev, EV_KEY, keycode, 1);
	input_sync(ir->input_dev);

and get rid of last_toggle member.

>  		input_sync(ir->input_dev);
>  	}
>  
> +	ir->keypressed = true;
>  	ir->last_key = keycode;
>  	ir->last_toggle = toggle;
>  
> -	ir->keyup_timer.expires = jiffies + UP_TIMEOUT;
> -	add_timer(&ir->keyup_timer);
> -
> +	mod_timer(&ir->keyup_timer, jiffies + UP_TIMEOUT);
>  }
>  
>  
> @@ -204,16 +199,6 @@ static void input_register_keys(struct infrared *ir)
>  	ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
>  }
>  
> -
> -/* called by the input driver after rep[REP_DELAY] ms */
> -static void input_repeat_key(unsigned long parm)
> -{
> -	struct infrared *ir = (struct infrared *) parm;
> -
> -	ir->delay_timer_finished = 1;
> -}
> -
> -
>  /* check for configuration changes */
>  int av7110_check_ir_config(struct av7110 *av7110, int force)
>  {
> @@ -333,8 +318,7 @@ int av7110_ir_init(struct av7110 *av7110)
>  	av_list[av_cnt++] = av7110;
>  	av7110_check_ir_config(av7110, true);
>  
> -	setup_timer(&av7110->ir.keyup_timer, av7110_emit_keyup,
> -		    (unsigned long)&av7110->ir);
> +	timer_setup(&av7110->ir.keyup_timer, av7110_emit_keyup, 0);
>  
>  	input_dev = input_allocate_device();
>  	if (!input_dev)
> @@ -365,8 +349,10 @@ int av7110_ir_init(struct av7110 *av7110)
>  		input_free_device(input_dev);
>  		return err;
>  	}
> -	input_dev->timer.function = input_repeat_key;
> -	input_dev->timer.data = (unsigned long) &av7110->ir;
> +
> +	/* Let the input layer handle autorepeat for us */
> +	input_dev->rep[REP_DELAY] = 250;
> +	input_dev->rep[REP_PERIOD] = 125;


I'd change this to:

	/*
	 * Input core's default autorepeat is 33 cps with 250 msec
	 * delay, let's adjust to numbers more suitable for remote
	 * control.
	 */
	input_enable_softrepeat(input_dev, 250, 125);

Thanks.

-- 
Dmitry