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


On Thu, Nov 21, 2024 at 9:19 PM Daniel Khodabakhsh
<d.khodabak...@gmail.com> 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 <d.khodabak...@gmail.com>
> ---
>  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 <ke...@koconnor.net>
> +// Copyright (C) 2024  Daniel Khodabakhsh <d.khodabak...@gmail.com>
>  //
>  // 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
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

Reply via email to