input/input06: Fix auto-repeat key test
PATCH v3 1/1 input/input06: Fix auto-repeat key test
The test currently fails for various reasons.
- The wait in the parent process is not long enough for kernel to send the auto-repeated keys.
- The test also fails when kernel sends auto-repeat configuration events as it doesn't recognize them.
Fix all that by refactoring the test and making sure we test for both the auto-repeat configuration events and the key press-repeat-release events.
---v2->v3
======
- Change the commit msg to reflect exactly what is being done.
- Run checkpatch.pl on the patch to ensure there are no errors.
- Make sure we test auto-repeat keys by increasing the sleep time in the parent process.
- Make sure we test both the auto-repeat configuration AND KEY_X press-repeat-release.
- Remove unnecessary check_size() function.
- Add checks for EV_SYN events that were broken in PATCH v2. The value in an EV_SYN event is undefined, and we were checking the value.
testcases/kernel/input/input06.c | 142 ++++++++++++++++++++++++++++++--------- 1 file changed, 112 insertions(+), 30 deletions(-)
testcases/kernel/input/input06.c +112 -30
@@ -37,6 +37,9 @@ static void cleanup(void);static int fd;static int fd2; +struct input_event events[64 ]; +static int num_events; +static int ev_iter;char *TCID ="input06" ;
@@ -95,8 +98,13 @@ static void send_events(void)send_event (fd,EV_KEY ,KEY_X ,1 );send_event (fd,EV_SYN ,0 ,0 ); -/* sleep to keep the key pressed for some time (auto-repeat) */ -usleep (1000 ); +/* +* Sleep long enough to keep the key pressed for some time +* (auto-repeat). Default kernel delay to start auto-repeat is 250ms +* and the period is 33ms. So, we wait for a generous 500ms to make +* sure we get the auto-repeated keys +*/ +usleep (500000 );send_event (fd,EV_KEY ,KEY_X ,0 );send_event (fd,EV_SYN ,0 ,0 );
@@ -107,13 +115,14 @@ static int check_event(struct input_event *iev, int event, int code, int value)return iev->type == event && iev->code == code && iev->value == value; } -static int check_bound (unsigned int i,unsigned int rd) +static int check_event_code (struct input_event *iev,int event,int code) { -return i <= rd /sizeof (struct input_event ); +return iev->type == event && iev->code == code; } -static void check_size (int rd) +static void read_events (void ) { +int rd =read (fd2, events,sizeof (events));if (rd <0 )tst_brkm (TBROK |TERRNO , cleanup,"read() failed" );
@@ -121,45 +130,118 @@ static void check_size(int rd)tst_brkm (TBROK , cleanup,"read size %i not multiple of %zu" , rd,sizeof (struct input_event )); } + + ev_iter =0 ; + num_events = rd /sizeof (struct input_event ); } -static int check_events (void ) +static int have_events (void ) { -struct input_event iev[64 ]; -unsigned int i; -int nb; -int rd; +return num_events && ev_iter < num_events; +} - i =0 ; - nb =0 ; +static struct input_event *next_event () +{ +struct input_event *ev =NULL ; - rd =read (fd2, iev,sizeof (iev)); +if (!have_events ()) +read_events (); -check_size (rd); + ev = &events[ev_iter++]; +return ev; +} -if (rd >0 &&check_event (&iev[i],EV_KEY ,KEY_X ,1 )) - i++; +static int check_sync_event (void ) +{ +return check_event_code (next_event (),EV_SYN ,SYN_REPORT ); +} -while (check_bound (i, rd) && !check_event (&iev[i],EV_KEY ,KEY_X ,0 )) { +static int parse_autorepeat_config (struct input_event *iev) +{ +if (!check_event_code (iev,EV_REP ,REP_DELAY )) { +tst_resm (TFAIL , +"Didn't get EV_REP configuration with code REP_DELAY" ); +return 0 ; + } + +if (!check_event_code (next_event (),EV_REP ,REP_PERIOD )) { +tst_resm (TFAIL , +"Didn't get EV_REP configuration with code REP_PERIOD" ); +return 0 ; + } + +return 1 ; +} + +static int parse_key (struct input_event *iev) +{ +int autorep_count =0 ; + +if (!check_event (iev,EV_KEY ,KEY_X ,1 ) || !check_sync_event ()) { +tst_resm (TFAIL ,"Didn't get expected key press for KEY_X" ); +return 0 ; + } -if (iev[i].type !=EV_SYN - && !check_event (&iev[i],EV_KEY ,KEY_X ,2 )) { -tst_resm (TINFO , -"Didn't receive EV_KEY KEY_X with value 2" ); + iev =next_event (); +while (check_event (iev,EV_KEY ,KEY_X ,2 ) &&check_sync_event ()) { + autorep_count++; + iev =next_event (); + } + +/* make sure we have atleast one auto-repeated key event */ +if (!autorep_count) { +tst_resm (TFAIL , +"Didn't get autorepeat events for the key - KEY_X" ); +return 0 ; + } + +if (!check_event (iev,EV_KEY ,KEY_X ,0 ) || !check_sync_event ()) { +tst_resm (TFAIL , +"Didn't get expected key release for KEY_X" ); +return 0 ; + } + +tst_resm (TINFO , +"Received %d repititions for KEY_X" , autorep_count); + +return 1 ; +} + +static int check_events (void ) +{ +struct input_event *iev; +int ret; +int rep_config_done =0 ; +int rep_keys_done =0 ; + +read_events (); + +while (have_events ()) { + iev =next_event (); +switch (iev->type) { +case EV_REP : + ret =parse_autorepeat_config (iev); +if (!rep_config_done) + rep_config_done =1 ; +break ; +case EV_KEY : + ret =parse_key (iev); +if (!rep_keys_done) + rep_keys_done =1 ; +break ; +default : +tst_resm (TFAIL , +"Unexpected event type '0x%04x' received" , + iev->type); + ret =0 ;break ; } - i++; - nb++; -if (i == rd /sizeof (struct input_event )) { - i =0 ; - rd =read (fd2, iev,sizeof (iev)); -check_size (rd); - } +if (!ret || (rep_config_done && rep_keys_done)) +break ; } -return (nb >0 &&check_bound (i, rd) - &&check_event (&iev[i],EV_KEY ,KEY_X ,0 )); +return ret; }static void cleanup (void )
Signature · 1 line
2.14.1.342.g6490525c54-goog
PATCH v3 1/1 input/input06: Fix auto-repeat key test
Hi!
Quoted 1 line
+static struct
input_event*next_event()
^
Old style definition, checkpatch.pl should warn about these
testcases/kernel/input/input06.c static void check_size(int rd) · line 145
Quoted 2 lines
testcases/kernel/input/input06.c +2
-0+{ +struct input_event *ev =NULL ;
^
Why do we initialize the value here if it's not used at all?
testcases/kernel/input/input06.c static void check_size(int rd) · line 151
Quoted 7 lines
testcases/kernel/input/input06.c +4
-2- rd =read (fd2, iev,sizeof (iev)); +if (!have_events ()) +read_events (); -check_size (rd); + ev = &events[ev_iter++]; +return ev;
Why not just return &events[ev_iter++]; ?
Quoted 70 lines
testcases/kernel/input/input06.c +60
-7+} -if (rd >0 &&check_event (&iev[i],EV_KEY ,KEY_X ,1 )) - i++; +static int check_sync_event (void ) +{ +return check_event_code (next_event (),EV_SYN ,SYN_REPORT ); +} -while (check_bound (i, rd) && !check_event (&iev[i],EV_KEY ,KEY_X ,0 )) { +static int parse_autorepeat_config (struct input_event *iev) +{ +if (!check_event_code (iev,EV_REP ,REP_DELAY )) { +tst_resm (TFAIL , +"Didn't get EV_REP configuration with code REP_DELAY" ); +return 0 ; + } + +if (!check_event_code (next_event (),EV_REP ,REP_PERIOD )) { +tst_resm (TFAIL , +"Didn't get EV_REP configuration with code REP_PERIOD" ); +return 0 ; + } + +return 1 ; +} + +static int parse_key (struct input_event *iev) +{ +int autorep_count =0 ; + +if (!check_event (iev,EV_KEY ,KEY_X ,1 ) || !check_sync_event ()) { +tst_resm (TFAIL ,"Didn't get expected key press for KEY_X" ); +return 0 ; + } -if (iev[i].type !=EV_SYN - && !check_event (&iev[i],EV_KEY ,KEY_X ,2 )) { -tst_resm (TINFO , -"Didn't receive EV_KEY KEY_X with value 2" ); + iev =next_event (); +while (check_event (iev,EV_KEY ,KEY_X ,2 ) &&check_sync_event ()) { + autorep_count++; + iev =next_event (); + } + +/* make sure we have atleast one auto-repeated key event */ +if (!autorep_count) { +tst_resm (TFAIL , +"Didn't get autorepeat events for the key - KEY_X" ); +return 0 ; + } + +if (!check_event (iev,EV_KEY ,KEY_X ,0 ) || !check_sync_event ()) { +tst_resm (TFAIL , +"Didn't get expected key release for KEY_X" ); +return 0 ; + } + +tst_resm (TINFO , +"Received %d repititions for KEY_X" , autorep_count); + +return 1 ; +} + +static int check_events (void ) +{ +struct input_event *iev; +int ret; +int rep_config_done =0 ;
testcases/kernel/input/input06.c static void check_size(int rd) · line 224
Quoted 10 lines
testcases/kernel/input/input06.c +10
-0+int rep_keys_done =0 ; + +read_events (); + +while (have_events ()) { + iev =next_event (); +switch (iev->type) { +case EV_REP : + ret =parse_autorepeat_config (iev); +if (!rep_config_done)
^
This if is useless here.
testcases/kernel/input/input06.c static void check_size(int rd) · line 229
Quoted 5 lines
testcases/kernel/input/input06.c +5
-0+ rep_config_done =1 ; +break ; +case EV_KEY : + ret =parse_key (iev); +if (!rep_keys_done)
^
Here as well.
Quoted 15 lines
testcases/kernel/input/input06.c +7
-5+ rep_keys_done =1 ; +break ; +default : +tst_resm (TFAIL , +"Unexpected event type '0x%04x' received" , + iev->type); + ret =0 ;break ; } - i++; - nb++; -if (i == rd /sizeof (struct input_event )) { - i =0 ; - rd =read (fd2, iev,sizeof (iev));
testcases/kernel/input/input06.c static void check_size(int rd) · line 245
Quoted 10 lines
testcases/kernel/input/input06.c +3
-4-check_size (rd); - } +if (!ret || (rep_config_done && rep_keys_done)) +break ; } -return (nb >0 &&check_bound (i, rd) - &&check_event (&iev[i],EV_KEY ,KEY_X ,0 )); +return ret; }
Also I would disagree that the value for sync event is undefined, it has been 0 since the beginning and if you look into the kernel input.h header it has:
static inline void input_sync(struct input_dev *dev)
{
input_event(dev, EV_SYN, SYN_REPORT, 0);
}Apart from that it looks OK.
Signature · 2 lines
Cyril Hrubis [email protected]
PATCH v3 1/1 input/input06: Fix auto-repeat key test
Quoted 6 lines · Cyril Hrubis
Hi!
Quoted 1 line
+static struct
input_event*next_event()^
Old style definition, checkpatch.pl should warn about these
It didn't but my checkpatch.pl may have been slightly older. Will fix it anyway.
Quoted 5 lines
testcases/kernel/input/input06.c static void check_size(int rd) · line 145
Quoted 2 lines
testcases/kernel/input/input06.c +2
-0+{ +struct input_event *ev =NULL ;^
Why do we initialize the value here if it's not used at all?
Agree, will fix.
Quoted 10 lines
testcases/kernel/input/input06.c static void check_size(int rd) · line 151
Quoted 8 lines
testcases/kernel/input/input06.c +4
-2- rd =read (fd2, iev,sizeof (iev)); +if (!have_events ()) +read_events (); -check_size (rd); + ev = &events[ev_iter++]; +return ev;Why not just return &events[ev_iter++]; ?
Wonder why I did that, but sure, will fix.
Quoted 83 lines
Quoted 71 lines
testcases/kernel/input/input06.c +60
-7+} -if (rd >0 &&check_event (&iev[i],EV_KEY ,KEY_X ,1 )) - i++; +static int check_sync_event (void ) +{ +return check_event_code (next_event (),EV_SYN ,SYN_REPORT ); +} -while (check_bound (i, rd) && !check_event (&iev[i],EV_KEY ,KEY_X ,0 )) { +static int parse_autorepeat_config (struct input_event *iev) +{ +if (!check_event_code (iev,EV_REP ,REP_DELAY )) { +tst_resm (TFAIL , +"Didn't get EV_REP configuration with code REP_DELAY" ); +return 0 ; + } + +if (!check_event_code (next_event (),EV_REP ,REP_PERIOD )) { +tst_resm (TFAIL , +"Didn't get EV_REP configuration with code REP_PERIOD" ); +return 0 ; + } + +return 1 ; +} + +static int parse_key (struct input_event *iev) +{ +int autorep_count =0 ; + +if (!check_event (iev,EV_KEY ,KEY_X ,1 ) || !check_sync_event ()) { +tst_resm (TFAIL ,"Didn't get expected key press for KEY_X" ); +return 0 ; + } -if (iev[i].type !=EV_SYN - && !check_event (&iev[i],EV_KEY ,KEY_X ,2 )) { -tst_resm (TINFO , -"Didn't receive EV_KEY KEY_X with value 2" ); + iev =next_event (); +while (check_event (iev,EV_KEY ,KEY_X ,2 ) &&check_sync_event ()) { + autorep_count++; + iev =next_event (); + } + +/* make sure we have atleast one auto-repeated key event */ +if (!autorep_count) { +tst_resm (TFAIL , +"Didn't get autorepeat events for the key - KEY_X" ); +return 0 ; + } + +if (!check_event (iev,EV_KEY ,KEY_X ,0 ) || !check_sync_event ()) { +tst_resm (TFAIL , +"Didn't get expected key release for KEY_X" ); +return 0 ; + } + +tst_resm (TINFO , +"Received %d repititions for KEY_X" , autorep_count); + +return 1 ; +} + +static int check_events (void ) +{ +struct input_event *iev; +int ret; +int rep_config_done =0 ;testcases/kernel/input/input06.c static void check_size(int rd) · line 224
Quoted 10 lines
testcases/kernel/input/input06.c +10
-0+int rep_keys_done =0 ; + +read_events (); + +while (have_events ()) { + iev =next_event (); +switch (iev->type) { +case EV_REP : + ret =parse_autorepeat_config (iev); +if (!rep_config_done)^
This if is useless here.
Quoted 7 lines
testcases/kernel/input/input06.c static void check_size(int rd) · line 229
Quoted 5 lines
testcases/kernel/input/input06.c +5
-0+ rep_config_done =1 ; +break ; +case EV_KEY : + ret =parse_key (iev); +if (!rep_keys_done)^
Here as well.
ack.
Quoted 21 lines
Quoted 21 lines
testcases/kernel/input/input06.c +9
-7+ rep_keys_done =1 ; +break ; +default : +tst_resm (TFAIL , +"Unexpected event type '0x%04x' received" , + iev->type); + ret =0 ;break ; } - i++; - nb++; -if (i == rd /sizeof (struct input_event )) { - i =0 ; - rd =read (fd2, iev,sizeof (iev)); -check_size (rd); - } +if (!ret || (rep_config_done && rep_keys_done)) +break ; }
Quoted 8 lines
testcases/kernel/input/input06.c static void check_size(int rd) · line 245
Quoted 4 lines
testcases/kernel/input/input06.c +1
-2-return (nb >0 &&check_bound (i, rd) - &&check_event (&iev[i],EV_KEY ,KEY_X ,0 )); +return ret; }Also I would disagree that the value for sync event is undefined, it has been 0 since the beginning and if you look into the kernel
input.hheader it has:
I thought that too but then I saw a test incorrectly failing due to value check for '0'. So, I went by the documentation ...
From: https://www.kernel.org/doc/Documentation/input/event-codes.txt
"
Event codes:
===========
Event codes define the precise type of event.
EV_SYN:
----------EV_SYN event values are undefined. Their usage is defined only by when they are sent in the evdev event stream.
* SYN_REPORT:
- Used to synchronize and separate events into packets of input data changes occurring at the same moment in time. For example, motion of a mouse may set the REL_X and REL_Y values for one motion, then emit a SYN_REPORT. The next motion will emit more REL_X and REL_Y values and send anot
....
"
Quoted 9 lines
static inline void
input_sync(struct input_dev *dev){ input_event(dev, EV_SYN, SYN_REPORT, 0); }Apart from that it looks OK.
ack, will re-spin a v4 with your comments addressed.
- ssp