[SeaBIOS] Re: [PATCH] Support multiple USB HID keyboards/mice

[Kevin O'Connor]

On Sun, Oct 27, 2024 at 08:53:45AM +0000, Daniel Khodabakhsh wrote:
> From 354935deae7cb79ee9a400222ed950314694194a Mon Sep 17 00:00:00 2001
> From: Daniel Khodabakhsh 
> Date: Sat, 26 Oct 2024 04:18:49 -0700
> Subject: [PATCH] Support multiple USB HID devices by storing them in a linked
>  list.

Thanks.  In general it seems fine to me, but I have a few comments.
See below.
> src/hw/usb-hid.c | 108 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 77 insertions(+), 31 deletions(-)
>
> diff --git a/src/hw/usb-hid.c b/src/hw/usb-hid.c
> index dec198ac..b7d5533e 100644
> --- a/src/hw/usb-hid.c
> +++ b/src/hw/usb-hid.c
> @@ -1,20 +1,44 @@
>  // Code for handling USB Human Interface Devices (HID).
>  //
>  // Copyright (C) 2009  Kevin O'Connor 
> +// Copyright (C) 2024  Daniel Khodabakhsh 
>  //
>  // This file may be distributed under the terms of the GNU LGPLv3 license.
>
> #include "biosvar.h" // GET_GLOBAL
>  #include "config.h" // CONFIG_*
> -#include "output.h" // dprintf
> +#include "output.h" // dprintf, warn_noalloc
> +#include "malloc.h" // malloc_fseg
>  #include "ps2port.h" // ATKBD_CMD_GETID
> +#include "stacks.h" // mutex_lock, mutex_unlock
>  #include "usb.h" // usb_ctrlrequest
>  #include "usb-hid.h" // usb_keyboard_setup
>  #include "util.h" // process_key
>
> -struct usb_pipe *keyboard_pipe VARFSEG;
> -struct usb_pipe *mouse_pipe VARFSEG;
> +struct pipe_node {
> struct usb_pipe *pipe;
> struct pipe_node *next;
>
> +};
>
> +struct mutex_s usb_hid_lock;

What is this lock for?  The "thread" implementation in SeaBIOS is
cooperative, so I don't see how the code could be preempted between
lock/unlock anyway.  Maybe I'm missing something.
> +struct pipe_node *keyboard_pipe_node VARFSEG = NULL;
> +struct pipe_node *mouse_pipe_node VARFSEG = NULL;
>
> +static struct pipe_node*
> +add_pipe_node(struct pipe_node *old_root_node, struct usb_pipe *pipe)
> +{
> struct pipe_node *new_root_node = malloc_fseg(sizeof(struct pipe_node));
> if (!new_root_node) {
>    warn_noalloc();
>    return old_root_node;
> }
> new_root_node->pipe = pipe;
> new_root_node->next = old_root_node;
> return new_root_node;
>
> +}
>
> /****************************************************************
> Setup
>
> @@ -64,9 +88,6 @@ usb_kbd_setup(struct usbdevice_s *usbdev
>  {
>      if (! CONFIG_USB_KEYBOARD)
>          return -1;
>
> if (keyboard_pipe)
>    // XXX - this enables the first found keyboard (could be random)
>    return -1;
>
> if (epdesc->wMaxPacketSize < sizeof(struct keyevent)
>        || epdesc->wMaxPacketSize > MAX_KBD_EVENT) {
>
> @@ -87,10 +108,14 @@ usb_kbd_setup(struct usbdevice_s *usbdev
>      if (ret)
>          dprintf(3, "Warning: Failed to set key repeat rate\n");
> keyboard_pipe = usb_alloc_pipe(usbdev, epdesc);
>
> struct usb_pipe *keyboard_pipe = usb_alloc_pipe(usbdev, epdesc);
> if (!keyboard_pipe)
>     return -1;
>
> mutex_lock(&usb_hid_lock);
>
> keyboard_pipe_node = add_pipe_node(keyboard_pipe_node, keyboard_pipe);
>
> mutex_unlock(&usb_hid_lock);
>
> dprintf(1, "USB keyboard initialized\n");
>    return 0;
>
> }
> @@ -109,9 +134,6 @@ usb_mouse_setup(struct usbdevice_s *usbdev
>  {
>      if (! CONFIG_USB_MOUSE)
>          return -1;
>
> if (mouse_pipe)
>    // XXX - this enables the first found mouse (could be random)
>    return -1;
>
> if (epdesc->wMaxPacketSize < sizeof(struct mouseevent)
>        || epdesc->wMaxPacketSize > MAX_MOUSE_EVENT) {
>
> @@ -125,10 +147,14 @@ usb_mouse_setup(struct usbdevice_s *usbdev
>      if (ret)
>          return -1;
> mouse_pipe = usb_alloc_pipe(usbdev, epdesc);
>
> struct usb_pipe *mouse_pipe = usb_alloc_pipe(usbdev, epdesc);
> if (!mouse_pipe)
>     return -1;
>
> mutex_lock(&usb_hid_lock);
>
> mouse_pipe_node = add_pipe_node(mouse_pipe_node, mouse_pipe);
>
> mutex_unlock(&usb_hid_lock);
>
> dprintf(1, "USB mouse initialized\n");
>    return 0;
>
> }
> @@ -325,16 +351,20 @@ usb_check_key(void)
>  {
>      if (! CONFIG_USB_KEYBOARD)
>          return;
>
> struct usb_pipe *pipe = GET_GLOBAL(keyboard_pipe);
>
> if (!pipe)
>    return;
>
> for (;;) {
>    u8 data[MAX_KBD_EVENT];
>    int ret = usb_poll_intr(pipe, data);
>    if (ret)
>        break;
>    handle_key((void*)data);
> for (struct pipe_node *node = GET_GLOBAL(keyboard_pipe_node);
>
> node; node = GET_GLOBALFLAT(node->next)) {

Please wrap the lines to 80 characters - in keeping with the existing
code style for this file.
>    struct usb_pipe *pipe = GET_GLOBALFLAT(node->pipe);
>    if (!pipe)
>        continue;

How could pipe ever be null?
>    for (;;) {
>        u8 data[MAX_KBD_EVENT];
>        int ret = usb_poll_intr(pipe, data);
>        if (ret)
>            break;
>        handle_key((void*)data);
>    }
> }
>
> }
>
> @@ -344,7 +374,13 @@ usb_kbd_active(void)
>  {
>      if (! CONFIG_USB_KEYBOARD)
>          return 0;
> return GET_GLOBAL(keyboard_pipe) != NULL;
> for (struct pipe_node *node = GET_GLOBAL(keyboard_pipe_node);
>
> node; node = GET_GLOBALFLAT(node->next)) {
>    if (GET_GLOBALFLAT(node->pipe))
>        return 1;

Similar comment about line wrapping and pipe being null.
> }
> return 0;
>
> }
>
> // Handle a ps2 style keyboard command.
> @@ -390,16 +426,20 @@ usb_check_mouse(void)
>  {
>      if (! CONFIG_USB_MOUSE)
>          return;
>
> struct usb_pipe *pipe = GET_GLOBAL(mouse_pipe);
>
> if (!pipe)
>    return;
>
> for (;;) {
>    u8 data[MAX_MOUSE_EVENT];
>    int ret = usb_poll_intr(pipe, data);
>    if (ret)
>        break;
>    handle_mouse((void*)data);
> for (struct pipe_node *node = GET_GLOBAL(mouse_pipe_node); node;
>
> node = GET_GLOBALFLAT(node->next)) {
>    struct usb_pipe *pipe = GET_GLOBALFLAT(node->pipe);
>    if (!pipe)
>        continue;

Similar comment about line wrapping and pipe being null.
>    for (;;) {
>        u8 data[MAX_MOUSE_EVENT];
>        int ret = usb_poll_intr(pipe, data);
>        if (ret)
>            break;
>        handle_mouse((void*)data);
>    }
> }
>
> }
>
> @@ -409,7 +449,13 @@ usb_mouse_active(void)
>  {
>      if (! CONFIG_USB_MOUSE)
>          return 0;
> return GET_GLOBAL(mouse_pipe) != NULL;
> for (struct pipe_node *node = GET_GLOBAL(mouse_pipe_node); node;
>
> node = GET_GLOBALFLAT(node->next)) {
>    if (GET_GLOBALFLAT(node->pipe))
>        return 1;

Similar comment about line wrapping and pipe being null.
> }
> return 0;
>
> }
> // Handle a ps2 style mouse command.
>
> 2.46.0

Cheers,
-Kevin