[LTP] [PATCH] input/input06: Ignore auto-repeat config events

[LTP] [PATCH] input/input06: Ignore auto-repeat config events

[Sandeep Patil]

The test currently fails on some android devices because the kernel
sends the EV_REP event with the codes REP_DELAY and REP_PERIOD and
their corresponding values for the uinput device the test creates.

This results in incorrect test failure as follows:

 input06     0  TINFO  :  Didn't receive EV_KEY KEY_X with value 2
 input06     1  TFAIL  :  external/ltp/testcases/kernel/input/input06.c:64: Wrong data received in
                         eventX

Signed-off-by: Sandeep Patil <sspatil@google.com>
---
 testcases/kernel/input/input06.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/testcases/kernel/input/input06.c b/testcases/kernel/input/input06.c
index c4fc1ef57..8d04db8c6 100644
--- a/testcases/kernel/input/input06.c
+++ b/testcases/kernel/input/input06.c
@@ -137,12 +137,26 @@ static int check_events(void)
 
 	check_size(rd);
 
+	/*
+	 * Ignore auto-repeat configuration codes
+	 * (EV_REP, {REP_PERIOD, REP_TYPE), value)
+	 */
+	while (iev[i].type == EV_REP) {
+		i++;
+		if (i == rd / sizeof(struct input_event)) {
+			i = 0;
+			rd = read(fd2, iev, sizeof(iev));
+			check_size(rd);
+		}
+	}
+
 	if (rd > 0 && check_event(&iev[i], EV_KEY, KEY_X, 1))
 		i++;
 
 	while (check_bound(i, rd) && !check_event(&iev[i], EV_KEY, KEY_X, 0)) {
 
 		if (iev[i].type != EV_SYN
+			&& iev[i].type != EV_REP
 			&& !check_event(&iev[i], EV_KEY, KEY_X, 2)) {
 			tst_resm(TINFO,
 				"Didn't receive EV_KEY KEY_X with value 2");
-- 
2.14.0.434.g98096fd7a8-goog

[LTP] [PATCH] input/input06: Ignore auto-repeat config events

[Sandeep Patil]

(bumping up the thread)

On Mon, Aug 14, 2017 at 04:32:05PM -0700, Sandeep Patil wrote:
> The test currently fails on some android devices because the kernel
> sends the EV_REP event with the codes REP_DELAY and REP_PERIOD and
> their corresponding values for the uinput device the test creates.
> 
> This results in incorrect test failure as follows:
> 
>  input06     0  TINFO  :  Didn't receive EV_KEY KEY_X with value 2
>  input06     1  TFAIL  :  external/ltp/testcases/kernel/input/input06.c:64: Wrong data received in
>                          eventX
> 
> Signed-off-by: Sandeep Patil <sspatil@google.com>
> ---
>  testcases/kernel/input/input06.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/testcases/kernel/input/input06.c b/testcases/kernel/input/input06.c
> index c4fc1ef57..8d04db8c6 100644
> --- a/testcases/kernel/input/input06.c
> +++ b/testcases/kernel/input/input06.c
> @@ -137,12 +137,26 @@ static int check_events(void)
>  
>  	check_size(rd);
>  
> +	/*
> +	 * Ignore auto-repeat configuration codes
> +	 * (EV_REP, {REP_PERIOD, REP_TYPE), value)
> +	 */
> +	while (iev[i].type == EV_REP) {
> +		i++;
> +		if (i == rd / sizeof(struct input_event)) {
> +			i = 0;
> +			rd = read(fd2, iev, sizeof(iev));
> +			check_size(rd);
> +		}
> +	}
> +
>  	if (rd > 0 && check_event(&iev[i], EV_KEY, KEY_X, 1))
>  		i++;
>  
>  	while (check_bound(i, rd) && !check_event(&iev[i], EV_KEY, KEY_X, 0)) {
>  
>  		if (iev[i].type != EV_SYN
> +			&& iev[i].type != EV_REP
>  			&& !check_event(&iev[i], EV_KEY, KEY_X, 2)) {
>  			tst_resm(TINFO,
>  				"Didn't receive EV_KEY KEY_X with value 2");
> -- 
> 2.14.0.434.g98096fd7a8-goog
> 

[LTP] [PATCH] input/input06: Ignore auto-repeat config events

[Cyril Hrubis]

Hi!
> diff --git a/testcases/kernel/input/input06.c b/testcases/kernel/input/input06.c
> index c4fc1ef57..8d04db8c6 100644
> --- a/testcases/kernel/input/input06.c
> +++ b/testcases/kernel/input/input06.c
> @@ -137,12 +137,26 @@ static int check_events(void)
>  
>  	check_size(rd);
>  
> +	/*
> +	 * Ignore auto-repeat configuration codes
> +	 * (EV_REP, {REP_PERIOD, REP_TYPE), value)
> +	 */
> +	while (iev[i].type == EV_REP) {
> +		i++;
> +		if (i == rd / sizeof(struct input_event)) {
> +			i = 0;
> +			rd = read(fd2, iev, sizeof(iev));
> +			check_size(rd);
> +		}
> +	}

I do not like much that we added a just another loop that fills in
the event array here. Maybe we should rewrite this as a recursive
descent parser now.

I.e.

	while (have_events()) {
		ev = next_event();

		switch (ev.type)
		case EV_REP:
			parse_rep(ev);
		break;
		case EV_KEY:
			parse_key(ev);
		break;
	}


The parse_rep() would check that we got correct sequence of
configuration events and the parse_key() would check that we have a
correct sequence of key press events, i.e. key down (1) a few key repeat
(2) and key up (0), which would be something as:

parse_key()
{
	if (!check_event(ev, EV_KEY, KEY_X, 1))
		tst_res(TFAIL, ...);

	ev = next_event();

	if (!check_event(ev, EV_KEY, KEY_X, 2))
		tst_res(TFAIL, ...);

	do {
		ev = next_event();
	} while (!check_event(ev, EV_KEY, KEY_X, 2);

	if (!chek_event(ev, EV_KEY, KEY_X, 0))
		tst_res(TFAIL, ...);
}

>  	if (rd > 0 && check_event(&iev[i], EV_KEY, KEY_X, 1))
>  		i++;
>  
>  	while (check_bound(i, rd) && !check_event(&iev[i], EV_KEY, KEY_X, 0)) {
>  
>  		if (iev[i].type != EV_SYN
> +			&& iev[i].type != EV_REP

What is this here for? Shouldn't all the configuration events be read at
this point?

>  			&& !check_event(&iev[i], EV_KEY, KEY_X, 2)) {
>  			tst_resm(TINFO,
>  				"Didn't receive EV_KEY KEY_X with value 2");
> -- 
> 2.14.0.434.g98096fd7a8-goog
> 

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] input/input06: Ignore auto-repeat config events

[Sandeep Patil]

On Tue, Aug 22, 2017 at 10:53:56AM +0200, Cyril Hrubis wrote:
> Hi!
> > diff --git a/testcases/kernel/input/input06.c b/testcases/kernel/input/input06.c
> > index c4fc1ef57..8d04db8c6 100644
> > --- a/testcases/kernel/input/input06.c
> > +++ b/testcases/kernel/input/input06.c
> > @@ -137,12 +137,26 @@ static int check_events(void)
> >  
> >  	check_size(rd);
> >  
> > +	/*
> > +	 * Ignore auto-repeat configuration codes
> > +	 * (EV_REP, {REP_PERIOD, REP_TYPE), value)
> > +	 */
> > +	while (iev[i].type == EV_REP) {
> > +		i++;
> > +		if (i == rd / sizeof(struct input_event)) {
> > +			i = 0;
> > +			rd = read(fd2, iev, sizeof(iev));
> > +			check_size(rd);
> > +		}
> > +	}
> 
> I do not like much that we added a just another loop that fills in
> the event array here. Maybe we should rewrite this as a recursive
> descent parser now.
> 
> I.e.
> 
> 	while (have_events()) {
> 		ev = next_event();
> 
> 		switch (ev.type)
> 		case EV_REP:
> 			parse_rep(ev);
> 		break;
> 		case EV_KEY:
> 			parse_key(ev);
> 		break;
> 	}
> 
> 
> The parse_rep() would check that we got correct sequence of
> configuration events and the parse_key() would check that we have a
> correct sequence of key press events, i.e. key down (1) a few key repeat
> (2) and key up (0), which would be something as:
> 
> parse_key()
> {
> 	if (!check_event(ev, EV_KEY, KEY_X, 1))
> 		tst_res(TFAIL, ...);
> 
> 	ev = next_event();
> 
> 	if (!check_event(ev, EV_KEY, KEY_X, 2))
> 		tst_res(TFAIL, ...);
> 
> 	do {
> 		ev = next_event();
> 	} while (!check_event(ev, EV_KEY, KEY_X, 2);
> 
> 	if (!chek_event(ev, EV_KEY, KEY_X, 0))
> 		tst_res(TFAIL, ...);
> }

Agree, I planned to do that too but with changing all input tests to use the
new library (like it is being done everywhere else). There is no reason for
this not happen before that though ... so will do.


> 
> >  	if (rd > 0 && check_event(&iev[i], EV_KEY, KEY_X, 1))
> >  		i++;
> >  
> >  	while (check_bound(i, rd) && !check_event(&iev[i], EV_KEY, KEY_X, 0)) {
> >  
> >  		if (iev[i].type != EV_SYN
> > +			&& iev[i].type != EV_REP
> 
> What is this here for? Shouldn't all the configuration events be read at
> this point?

It should be, however I also saw the kernel sending these up during
->resume() and so wanted to make sure that is covered too. Either way, the
suggested implementation above will take care of everything...

Thanks for the feedback
- ssp

> 
> >  			&& !check_event(&iev[i], EV_KEY, KEY_X, 2)) {
> >  			tst_resm(TINFO,
> >  				"Didn't receive EV_KEY KEY_X with value 2");
> > -- 
> > 2.14.0.434.g98096fd7a8-goog
> > 
> 
> -- 
> Cyril Hrubis
> chrubis@suse.cz