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

[Kevin O'Connor]

On Sat, Jan 04, 2025 at 06:13:36AM +0000, Daniel Khodabakhsh wrote:
> Hi Kevin!
>
> Just wanted to give this one a bump. Inspecting logs it looks like
> multiple threads come in to initialise USB devices. Should I still
> remove the lock in this case and considering the initialisation loop
> that happens for usb hubs that i mentioned on November 15th? I created
> a diagram of the program flow for usb initialisation attached as an
> image

It is possible for there to be multiple simultaneous SeaBIOS "threads"
during initialization, but those "threads" are actually coroutines and
it's never possible for two coroutines to be running at the same time
and it's never possible for a coroutine to preempt another coroutine.
That is, coroutines only switch to other coroutines during explicit
SeaBIOS yield() calls.

So, the snippet:
> mutex_lock(&usb_hid_lock);
> new_node->next = *list;
> *list = new_node;
> mutex_unlock(&usb_hid_lock);

looks odd to me as the lock doesn't add any additional protection
because nothing can interrupt the those two lines of code anyway.

Cheers,
-Kevin
> On Thu, Nov 21, 2024 at 9:19 PM Daniel Khodabakhsh
>  wrote:
> > As per your comments in my other patch Kevin I've updated this one
> > with the signed-off by line as well:
> >
> > Support multiple USB HID devices by storing them in a linked list.
> > Signed-off-by: Daniel Khodabakhsh
> >
> > src/hw/usb-hid.c | 109 ++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 70 insertions(+), 39 deletions(-)
> >
> > diff --git a/src/hw/usb-hid.c b/src/hw/usb-hid.c
> > index dec198ac..34e6802f 100644
> > --- a/src/hw/usb-hid.c
> > +++ b/src/hw/usb-hid.c
> > @@ -1,20 +1,54 @@
> >  // 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;
> >
> > +struct pipe_node *keyboards VARFSEG = NULL;
> > +struct pipe_node *mice VARFSEG = NULL;
> >
> > +static int
> > +add_pipe_node(struct pipe_node **list
> >          , struct usbdevice_s *usbdev
> >          , struct usb_endpoint_descriptor *epdesc)
> >
> > +{
> >
> > struct usb_pipe *pipe = usb_alloc_pipe(usbdev, epdesc);
> >
> > if (!pipe)
> >    return -1;
> >
> > struct pipe_node *new_node = malloc_fseg(sizeof(struct pipe_node));
> >
> > if (!new_node) {
> >    warn_noalloc();
> >    return -1;
> >
> > }
> >
> > new_node->pipe = pipe;
> >
> > mutex_lock(&usb_hid_lock);
> >
> > new_node->next = *list;
> >
> > *list = new_node;
> >
> > mutex_unlock(&usb_hid_lock);
> >
> > return 0;
> >
> > +}
> >
> > /****************************************************************
> > Setup
> >
> > @@ -64,9 +98,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) {
> >
> > @@ -76,19 +107,16 @@ usb_kbd_setup(struct usbdevice_s *usbdev
> >      }
> >  // Enable "boot" protocol.
> > int ret = set_protocol(usbdev->defpipe, 0,
> >
> > usbdev->iface->bInterfaceNumber);
> > if (ret) {
> >
> > if (set_protocol(usbdev->defpipe, 0, usbdev->iface->bInterfaceNumber)) {
> >     dprintf(3, "Failed to set boot protocol\n");
> >     return -1;
> > }
> >
> > // Periodically send reports to enable key repeat.
> > ret = set_idle(usbdev->defpipe, KEYREPEATMS);
> > if (ret)
> > if (set_idle(usbdev->defpipe, KEYREPEATMS))
> >     dprintf(3, "Warning: Failed to set key repeat rate\n");
> > keyboard_pipe = usb_alloc_pipe(usbdev, epdesc);
> > if (!keyboard_pipe)
> >
> > if (add_pipe_node(&keyboards, usbdev, epdesc))
> >     return -1;
> >
> > dprintf(1, "USB keyboard initialized\n");
> >
> > @@ -109,9 +137,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) {
> >
> > @@ -121,12 +146,10 @@ usb_mouse_setup(struct usbdevice_s *usbdev
> >      }
> >  // Enable "boot" protocol.
> > int ret = set_protocol(usbdev->defpipe, 0,
> >
> > usbdev->iface->bInterfaceNumber);
> > if (ret)
> > if (set_protocol(usbdev->defpipe, 0, usbdev->iface->bInterfaceNumber))
> >     return -1;
> > mouse_pipe = usb_alloc_pipe(usbdev, epdesc);
> > if (!mouse_pipe)
> >
> > if (add_pipe_node(&mice, usbdev, epdesc))
> >     return -1;
> >
> > dprintf(1, "USB mouse initialized\n");
> >
> > @@ -325,16 +348,19 @@ 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(keyboards)
> >     node;
> >     node = GET_GLOBALFLAT(node->next)) {
> >    struct usb_pipe *pipe = GET_GLOBALFLAT(node->pipe);
> >    for (;;) {
> >        u8 data[MAX_KBD_EVENT];
> >        int ret = usb_poll_intr(pipe, data);
> >        if (ret)
> >            break;
> >        handle_key((void*)data);
> >    }
> > }
> >
> > }
> >
> > @@ -344,7 +370,8 @@ usb_kbd_active(void)
> >  {
> >      if (! CONFIG_USB_KEYBOARD)
> >          return 0;
> > return GET_GLOBAL(keyboard_pipe) != NULL;
> > return GET_GLOBAL(keyboards) != NULL;
> >
> > }
> >
> > // Handle a ps2 style keyboard command.
> > @@ -390,16 +417,19 @@ 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(mice);
> >     node;
> >     node = GET_GLOBALFLAT(node->next)) {
> >    struct usb_pipe *pipe = GET_GLOBALFLAT(node->pipe);
> >    for (;;) {
> >        u8 data[MAX_MOUSE_EVENT];
> >        int ret = usb_poll_intr(pipe, data);
> >        if (ret)
> >            break;
> >        handle_mouse((void*)data);
> >    }
> > }
> >
> > }
> >
> > @@ -409,7 +439,8 @@ usb_mouse_active(void)
> >  {
> >      if (! CONFIG_USB_MOUSE)
> >          return 0;
> > return GET_GLOBAL(mouse_pipe) != NULL;
> > return GET_GLOBAL(mice) != NULL;
> >
> > }
> > // Handle a ps2 style mouse command.
> >
> > 2.46.0