Stefan Reinauer has uploaded this change for review. ( https://review.coreboot.org/c/em100/+/45538 )
Change subject: em100: Always install signal handler ......................................................................
em100: Always install signal handler
Always catch SIGINT, because we don't want the tool to be interrupted half way through operations. This might leave the EM100Pro in a bad state that requires physically reconnecting the device. This renders the tool virtually unusable for lab setups.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Change-Id: I649dc491f68f764fdb1352599ff17b36e3d39fd8 --- M em100.c 1 file changed, 13 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/38/45538/1
diff --git a/em100.c b/em100.c index 49d0d1e..580290a 100644 --- a/em100.c +++ b/em100.c @@ -840,6 +840,7 @@ unsigned long address_offset = 0; unsigned int spi_start_address = 0; const char *voltage = NULL; + struct sigaction signal_action;
while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhT", longopts, &idx)) != -1) { @@ -936,6 +937,18 @@ if (desiredchip && !chip) return 1;
+ /* Set up signal handler. This is used for two reasons: + * 1) to create a way to cleanly exit trace mode. + * 2) to make sure that the em100 is not left in an improper state + * when receiving SIGINT for other reasons during operation. In + * this second case, we just ignore SIGINT until em100 naturally + * terminates or receives a second signal. This is OK because the + * utility is short-running in nature. + */ + signal_action.sa_handler = exit_handler; + signal_action.sa_flags = 0; + sigemptyset(&signal_action.sa_mask); + sigaction(SIGINT, &signal_action, NULL);
if (em100.hwversion == HWVERSION_EM100PRO || em100.hwversion == HWVERSION_EM100PRO_EARLY) { printf("MCU version: %d.%02d\n", em100.mcu >> 8, em100.mcu & 0xff); @@ -1177,8 +1190,6 @@ }
if (trace || terminal) { - struct sigaction signal_action; - if ((holdpin == NULL) && (!set_hold_pin_state(&em100, 3))) { printf("Error: Failed to set EM100 to input\n"); return 1; @@ -1200,10 +1211,6 @@ }
printf(". Press CTL-C to exit.\n\n"); - signal_action.sa_handler = exit_handler; - signal_action.sa_flags = 0; - sigemptyset(&signal_action.sa_mask); - sigaction(SIGINT, &signal_action, NULL);
while (!do_exit_flag) { if (trace)
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/em100/+/45538 )
Change subject: em100: Always install signal handler ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/em100/+/45538/1/em100.c File em100.c:
https://review.coreboot.org/c/em100/+/45538/1/em100.c@948 PS1, Line 948: exit_handler It looks like the exit_handler only sets do_exit_flag=1 and that flag is only used in trace mode, making this have no effect for the second reason.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/45538 )
Change subject: em100: Always install signal handler ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/em100/+/45538/1/em100.c File em100.c:
https://review.coreboot.org/c/em100/+/45538/1/em100.c@948 PS1, Line 948: exit_handler
It looks like the exit_handler only sets do_exit_flag=1 and that flag is only used in trace mode, ma […]
It's true that it does nothing in that case, including not terminating the program. Which is what we want.
Ryan O'Leary has posted comments on this change. ( https://review.coreboot.org/c/em100/+/45538 )
Change subject: em100: Always install signal handler ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/em100/+/45538/1/em100.c File em100.c:
https://review.coreboot.org/c/em100/+/45538/1/em100.c@948 PS1, Line 948: exit_handler
It's true that it does nothing in that case, including not terminating the program. […]
Makes sense!
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/em100/+/45538 )
Change subject: em100: Always install signal handler ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/em100/+/45538/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/em100/+/45538/1//COMMIT_MSG@10 PS1, Line 10: stat nit: wrap at 72 characters
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/em100/+/45538 )
Change subject: em100: Always install signal handler ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/em100/+/45538/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/em100/+/45538/1//COMMIT_MSG@10 PS1, Line 10: stat
nit: wrap at 72 characters
done
Hello build bot (Jenkins), Ryan O'Leary, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/em100/+/45538
to look at the new patch set (#3).
Change subject: em100: Always install signal handler ......................................................................
em100: Always install signal handler
Always catch SIGINT, because we don't want the tool to be interrupted half way through operations. This might leave the EM100Pro in a bad state that requires physically reconnecting the device. This renders the tool virtually unusable for lab setups.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Change-Id: I649dc491f68f764fdb1352599ff17b36e3d39fd8 --- M em100.c 1 file changed, 13 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/em100 refs/changes/38/45538/3
Stefan Reinauer has submitted this change. ( https://review.coreboot.org/c/em100/+/45538 )
Change subject: em100: Always install signal handler ......................................................................
em100: Always install signal handler
Always catch SIGINT, because we don't want the tool to be interrupted half way through operations. This might leave the EM100Pro in a bad state that requires physically reconnecting the device. This renders the tool virtually unusable for lab setups.
Signed-off-by: Stefan Reinauer stefan.reinauer@coreboot.org Change-Id: I649dc491f68f764fdb1352599ff17b36e3d39fd8 Reviewed-on: https://review.coreboot.org/c/em100/+/45538 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Ryan O'Leary ryanoleary@google.com Reviewed-by: Angel Pons th3fanbus@gmail.com --- M em100.c 1 file changed, 13 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved Ryan O'Leary: Looks good to me, approved
diff --git a/em100.c b/em100.c index 49d0d1e..580290a 100644 --- a/em100.c +++ b/em100.c @@ -840,6 +840,7 @@ unsigned long address_offset = 0; unsigned int spi_start_address = 0; const char *voltage = NULL; + struct sigaction signal_action;
while ((opt = getopt_long(argc, argv, "c:d:a:m:u:rsvtO:F:f:g:S:V:p:DCx:lUhT", longopts, &idx)) != -1) { @@ -936,6 +937,18 @@ if (desiredchip && !chip) return 1;
+ /* Set up signal handler. This is used for two reasons: + * 1) to create a way to cleanly exit trace mode. + * 2) to make sure that the em100 is not left in an improper state + * when receiving SIGINT for other reasons during operation. In + * this second case, we just ignore SIGINT until em100 naturally + * terminates or receives a second signal. This is OK because the + * utility is short-running in nature. + */ + signal_action.sa_handler = exit_handler; + signal_action.sa_flags = 0; + sigemptyset(&signal_action.sa_mask); + sigaction(SIGINT, &signal_action, NULL);
if (em100.hwversion == HWVERSION_EM100PRO || em100.hwversion == HWVERSION_EM100PRO_EARLY) { printf("MCU version: %d.%02d\n", em100.mcu >> 8, em100.mcu & 0xff); @@ -1177,8 +1190,6 @@ }
if (trace || terminal) { - struct sigaction signal_action; - if ((holdpin == NULL) && (!set_hold_pin_state(&em100, 3))) { printf("Error: Failed to set EM100 to input\n"); return 1; @@ -1200,10 +1211,6 @@ }
printf(". Press CTL-C to exit.\n\n"); - signal_action.sa_handler = exit_handler; - signal_action.sa_flags = 0; - sigemptyset(&signal_action.sa_mask); - sigaction(SIGINT, &signal_action, NULL);
while (!do_exit_flag) { if (trace)
-
Angel Pons (Code Review) -
Ryan O'Leary (Code Review)
-
Stefan Reinauer (Code Review)