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.
From 1822908294904302ed5c5e103ef502ae13db09ab Mon Sep 17 00:00:00 2001
From: Daniel Khodabakhsh <d.khodabak...@gmail.com>
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 | 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
