| Message ID | 20191008132043.7966-3-daniel.thompson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kdb: Cleanup code to read user input and handle escape sequences | expand |
Hi, On Tue, Oct 8, 2019 at 6:21 AM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > Currently kdb_read_get_key() contains complex control flow that, on > close inspection, turns out to be unnecessary. In particular: > > 1. It is impossible to enter the branch conditioned on (escape_delay == 1) > except when the loop enters with (escape_delay == 2) allowing us to > combine the branches. > > 2. Most of the code conditioned on (escape_delay == 2) simply modifies > local data and then breaks out of the loop causing the function to > return escape_data[0]. > > 3. Based on #2 there is not actually any need to ever explicitly set > escape_delay to 2 because we it is much simpler to directly return > escape_data[0] instead. > > 4. escape_data[0] is, for all but one exit path, known to be '\e'. > > Simplify the code based on these observations. > > There is a subtle (and harmless) change of behaviour resulting from this > simplification: instead of letting the escape timeout after ~1998 > milliseconds we now timeout after ~2000 milliseconds > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> > --- > kernel/debug/kdb/kdb_io.c | 38 ++++++++++++++------------------------ > 1 file changed, 14 insertions(+), 24 deletions(-) Looks great. My only comment is that since this was the 2nd patch in the series I spent a whole bunch of time deducing all these same things when reviewing the first patch. :-P Reviewed-by: Douglas Anderson <dianders@chromium.org>
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index 68e2c29f14f5..78cb6e339408 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -122,25 +122,18 @@ static int kdb_read_get_key(char *buffer, size_t bufsize) touch_nmi_watchdog(); f = &kdb_poll_funcs[0]; } - if (escape_delay == 2) { - *ped = '\0'; - ped = escape_data; - --escape_delay; - } - if (escape_delay == 1) { - key = *ped++; - if (!*ped) - --escape_delay; - break; - } + key = (*f)(); + if (key == -1) { if (escape_delay) { udelay(ESCAPE_UDELAY); - --escape_delay; + if (--escape_delay == 0) + return '\e'; } continue; } + if (bufsize <= 2) { if (key == '\r') key = '\n'; @@ -148,28 +141,25 @@ static int kdb_read_get_key(char *buffer, size_t bufsize) *buffer = '\0'; return -1; } + if (escape_delay == 0 && key == '\e') { escape_delay = ESCAPE_DELAY; ped = escape_data; f_escape = f; } if (escape_delay) { - *ped++ = key; - if (f_escape != f) { - escape_delay = 2; - continue; - } + if (f_escape != f) + return '\e'; + *ped++ = key; key = kdb_read_handle_escape(escape_data, ped - escape_data); - if (key > 0) { - escape_data[0] = key; - escape_data[1] = '\0'; - } - if (key) - escape_delay = 2; - continue; + if (key < 0) + return '\e'; + if (key == 0) + continue; } + break; /* A key to process */ } return key;
Currently kdb_read_get_key() contains complex control flow that, on close inspection, turns out to be unnecessary. In particular: 1. It is impossible to enter the branch conditioned on (escape_delay == 1) except when the loop enters with (escape_delay == 2) allowing us to combine the branches. 2. Most of the code conditioned on (escape_delay == 2) simply modifies local data and then breaks out of the loop causing the function to return escape_data[0]. 3. Based on #2 there is not actually any need to ever explicitly set escape_delay to 2 because we it is much simpler to directly return escape_data[0] instead. 4. escape_data[0] is, for all but one exit path, known to be '\e'. Simplify the code based on these observations. There is a subtle (and harmless) change of behaviour resulting from this simplification: instead of letting the escape timeout after ~1998 milliseconds we now timeout after ~2000 milliseconds Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> --- kernel/debug/kdb/kdb_io.c | 38 ++++++++++++++------------------------ 1 file changed, 14 insertions(+), 24 deletions(-) -- 2.21.0