Re: [PATCH v2] kdb: fix ctrl+e/a/f/b/d/p/n broken in keyboard mode

[Doug Anderson]

Hi,

On Mon, Nov 11, 2024 at 1:22 PM Nir Lichtman <nir@xxxxxxxxxxxx> wrote:
>
> Problem: When using kdb via keyboard it does not react to control
> characters which are supported in serial mode.
>
> Example: Chords such as ctrl+a/e/d/p do not work in keyboard mode
>
> Solution: Before disregarding non-printable key characters, check if they
> are one of the supported control characters, I have took the control
> characters from the switch case upwards in this function that translates
> scan codes of arrow keys/backspace/home/.. to the control characters.
>
> Suggested-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Signed-off-by: Nir Lichtman <nir@xxxxxxxxxxxx>
> ---
>
> v2: Add CTRL macro following Douglas's suggestion in the CR of v1
>
>  kernel/debug/kdb/kdb_keyboard.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/debug/kdb/kdb_keyboard.c b/kernel/debug/kdb/kdb_keyboard.c
> index 3c2987f46f6e..9b8b172f48c3 100644
> --- a/kernel/debug/kdb/kdb_keyboard.c
> +++ b/kernel/debug/kdb/kdb_keyboard.c
> @@ -25,6 +25,8 @@
>  #define KBD_STAT_OBF           0x01    /* Keyboard output buffer full */
>  #define KBD_STAT_MOUSE_OBF     0x20    /* Mouse output buffer full */
>
> +#define CTRL(c) (c - 64)

My fault, but just to have good macro hygiene the above should have
extra parens around the "c" to make sure some hidden
order-of-operations problem doesn't come up. It obviously won't with
what we're using the macro for right now, but I could imagine some
automated test might balk about the missing parens... AKA:

#define CTRL(c) ((c) - 64)


> @@ -123,24 +125,24 @@ int kdb_get_kbd_char(void)
>                 return 8;
>         }
>
> -       /* Special Key */
> +       /* Translate special keys to equivalent CTRL control characters */
>         switch (scancode) {
>         case 0xF: /* Tab */
> -               return 9;
> +               return CTRL('I');
>         case 0x53: /* Del */
> -               return 4;
> +               return CTRL('D');
>         case 0x47: /* Home */
> -               return 1;
> +               return CTRL('A');
>         case 0x4F: /* End */
> -               return 5;
> +               return CTRL('E');
>         case 0x4B: /* Left */
> -               return 2;
> +               return CTRL('B');
>         case 0x48: /* Up */
> -               return 16;
> +               return CTRL('P');
>         case 0x50: /* Down */
> -               return 14;
> +               return CTRL('N');
>         case 0x4D: /* Right */
> -               return 6;
> +               return CTRL('F');
>         }
>
>         if (scancode == 0xe0)
> @@ -172,6 +174,19 @@ int kdb_get_kbd_char(void)
>         switch (KTYP(keychar)) {
>         case KT_LETTER:
>         case KT_LATIN:
> +               switch (keychar) {
> +                       /* non-printable supported control characters */
> +                       case CTRL('A'): /* Home */

nit: I believe that Linux conventions is that the "case" lines up
directly under the "switch".

-Doug