[PATCH] Support multiple USB HID keyboards/mice

[Daniel Khodabakhsh]

This change allows the use of multiple USB HID keyboards and mice.
It's similar to the change by Stef van Os from this thread:

https://mail.coreboot.org/hyperkitty/list/[e-mail]/thread/O6Y4LUK...

however this new change uses the linked list approach instead of a fixed array.

Tested on QEMU and an Asus KGPE-D16 with 2 keyboards and one mice

[Daniel Khodabakhsh]

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.

---
 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;
+
+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)) {
+        struct usb_pipe *pipe = GET_GLOBALFLAT(node->pipe);
+
+        if (!pipe)
+            continue;
+
+        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;
+    }
+
+    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;
+
+        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;
+    }
+
+    return 0;
 }

// Handle a ps2 style mouse command.

[Daniel Khodabakhsh]

Reworked the change a bit to escalate memory errors allowing
usb.c:configure_usb_device() to free memory during device
initialisation.
Also removed some code duplication.

From f6a653b0fcac0359d598c342ed0a51b05d6ebe16 Mon Sep 17 00:00:00 2001
From: Daniel Khodabakhsh 
Date: Thu, 7 Nov 2024 10:34:47 -0800
Subject: [PATCH] Support multiple USB HID devices by storing them in a linked
 list.

---
 src/hw/usb-hid.c | 121 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 82 insertions(+), 39 deletions(-)

diff --git a/src/hw/usb-hid.c b/src/hw/usb-hid.c
index dec198ac..2350e121 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 *keyboard_pipe_node VARFSEG = NULL;
+struct pipe_node *mouse_pipe_node VARFSEG = NULL;

+static int
+add_pipe_node(struct pipe_node **root_node
+              , 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_root_node = malloc_fseg(sizeof(struct pipe_node));
+    if (!new_root_node) {
+        warn_noalloc();
+        return -1;
+    }
+
+    new_root_node->pipe = pipe;
+
+    mutex_lock(&usb_hid_lock);
+    new_root_node->next = *root_node;
+    *root_node = new_root_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(&keyboard_pipe_node, 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(&mouse_pipe_node, usbdev, epdesc))
         return -1;

dprintf(1, "USB mouse initialized\n");
@@ -325,16 +348,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)) {
+        struct usb_pipe *pipe = GET_GLOBALFLAT(node->pipe);
+
+        if (!pipe)
+            continue;
+
+        for (;;) {
+            u8 data[MAX_KBD_EVENT];
+            int ret = usb_poll_intr(pipe, data);
+            if (ret)
+                break;
+            handle_key((void*)data);
+        }
     }
 }

@@ -344,7 +371,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;
+    }
+
+    return 0;
 }

// Handle a ps2 style keyboard command.
@@ -390,16 +423,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;
+
+        for (;;) {
+            u8 data[MAX_MOUSE_EVENT];
+            int ret = usb_poll_intr(pipe, data);
+            if (ret)
+                break;
+            handle_mouse((void*)data);
+        }
     }
 }

@@ -409,7 +446,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;
+    }
+
+    return 0;
 }

// Handle a ps2 style mouse command.

[Gerd Hoffmann]

Hi,
> +struct pipe_node {
> struct usb_pipe *pipe;
> struct pipe_node *next;
>
> +};

seabios has a linked list implementation (see src/list.h).
Please use that instead of rolling your own.

Otherwise the patch looks sane on a quick glance.

take care,
  Gerd

[Daniel Khodabakhsh]

Hi Gerd,

Thanks for the reviews!
> seabios has a linked list implementation (see src/list.h).
> Please use that instead of rolling your own.
>
> Otherwise the patch looks sane on a quick glance.

Thanks for this suggestion as well. I tried it out but I'm having
trouble getting it to work. My SeaBIOS hangs after the splash screen.
I believe it's due to how the hlist_for_each_entry macro retrieves the
node container via the inner macro call to container_of. Since the
containers are in FSEG (so they can be used during runtime) I believe
they need to be retrieved with GET_GLOBAL (like I do in the previous
patches' for loops). I attached the hlist version as
0001-Use-hlists-to-track-USB-HID-devices.patch

Is it necessary to use hlist? Linked lists are simple enough and I'm
not using the double-linked list nature of hlist. I made a few more
tweaks to the original linked list approach as
0001-Support-multiple-USB-HID-devices-3.patch

Cheers,
- Daniel K.

[Gerd Hoffmann]

On Wed, Nov 13, 2024 at 11:14:28AM +0000, Daniel Khodabakhsh wrote:
> Hi Gerd,
>
> Thanks for the reviews!
> > seabios has a linked list implementation (see src/list.h).
> > Please use that instead of rolling your own.
> >
> > Otherwise the patch looks sane on a quick glance.
>
> Thanks for this suggestion as well. I tried it out but I'm having
> trouble getting it to work. My SeaBIOS hangs after the splash screen.
> I believe it's due to how the hlist_for_each_entry macro retrieves the
> node container via the inner macro call to container_of. Since the
> containers are in FSEG (so they can be used during runtime) I believe
> they need to be retrieved with GET_GLOBAL (like I do in the previous
> patches' for loops). I attached the hlist version as
> 0001-Use-hlists-to-track-USB-HID-devices.patch
>
> Is it necessary to use hlist? Linked lists are simple enough and I'm
> not using the double-linked list nature of hlist.

Given that there isn't much list management to do here (the patch never
removes anything from the lists) I'd say it's ok to an exception in
this case.

Kevin?

take care,
  Gerd

[Kevin O'Connor]

On Thu, Nov 14, 2024 at 02:54:38PM +0100, Gerd Hoffmann wrote:
> On Wed, Nov 13, 2024 at 11:14:28AM +0000, Daniel Khodabakhsh wrote:
> > Hi Gerd,
> >
> > Thanks for the reviews!
> > > seabios has a linked list implementation (see src/list.h).
> > > Please use that instead of rolling your own.
> > >
> > > Otherwise the patch looks sane on a quick glance.
> >
> > Thanks for this suggestion as well. I tried it out but I'm having
> > trouble getting it to work. My SeaBIOS hangs after the splash screen.
> > I believe it's due to how the hlist_for_each_entry macro retrieves the
> > node container via the inner macro call to container_of. Since the
> > containers are in FSEG (so they can be used during runtime) I believe
> > they need to be retrieved with GET_GLOBAL (like I do in the previous
> > patches' for loops). I attached the hlist version as
> > 0001-Use-hlists-to-track-USB-HID-devices.patch
> >
> > Is it necessary to use hlist? Linked lists are simple enough and I'm
> > not using the double-linked list nature of hlist.
>
> Given that there isn't much list management to do here (the patch never
> removes anything from the lists) I'd say it's ok to an exception in
> this case.
>
> Kevin?

Yeah - probably best to "open code" linked lists if they are needed in
16bit mode.

Cheers,
-Kevin

[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

[Daniel Khodabakhsh]

Thanks for the review Kevin!
> > +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.

Good question. I'm not the most versed in system programming but I
noticed that if a hub is found in usb.c:configure_usb_device ->
usb-hub.c:usb_hub_setup -> usb.c:enumerate is called which calls
run_thread(usb_hub_port_setup) -> usb.c:usb_hub_port_setup ->
usb.c:configure_usb_device. I assumed this meant that multiple threads
may find keyboards or mice which is why I put this lock in.

With this information, is it still the case that I don't need a lock here?

I'll keep it in the version I'm attaching to this email pending your reply
> > 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.

Will do! Updated in the latest version attached to this email
> >    struct usb_pipe *pipe = GET_GLOBALFLAT(node->pipe);
> >    if (!pipe)
> >        continue;
>
> How could pipe ever be null?

Based on your question I'm guessing this won't be the case. I noticed
that usb.c can make usb_free_pipe calls so I thought I should check
just to be sure.
I've just removed these checks now

What's the best way to post the latest changes, is just an attachment
preferred or in the email body as well?

Cheers,
- Daniel K.

[Daniel Khodabakhsh]

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.

[Daniel Khodabakhsh]

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
 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

[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

[Daniel Khodabakhsh]

Thanks for the explanation Kevin!

I made the modification and tried it locally and it does work.

I've updated the patch and attached it to this email.

Cheers,
- Daniel K.

[Kevin O'Connor]

On Sun, Jan 05, 2025 at 08:28:24AM +0000, Daniel Khodabakhsh wrote:
> Thanks for the explanation Kevin!
>
> I made the modification and tried it locally and it does work.
>
> I've updated the patch and attached it to this email.

Thanks.  I committed this change.

-Kevin