git: 2775d1d5d1ef - main - hkbd(4): Split driver lock on interrupt and syscons locks
Vladimir Kondratyev
wulf at FreeBSD.org
Thu Jan 7 23:20:52 UTC 2021
The branch main has been updated by wulf:
URL: https://cgit.FreeBSD.org/src/commit/?id=2775d1d5d1efeed02f4a805a86f89ee96d68006b
commit 2775d1d5d1efeed02f4a805a86f89ee96d68006b
Author: Vladimir Kondratyev <wulf at FreeBSD.org>
AuthorDate: 2020-11-09 19:25:14 +0000
Commit: Vladimir Kondratyev <wulf at FreeBSD.org>
CommitDate: 2021-01-07 23:18:43 +0000
hkbd(4): Split driver lock on interrupt and syscons locks
This allows to mark HID-device interrupt handlers as MP-SAFE.
Atomics-based lockless key event queue with swi_giant taskqueue is used
to pass key-press events into Giant-protected system console.
Reviewed by: hselasky (as part of D27991)
---
sys/dev/hid/hkbd.c | 164 +++++++++++++++++++++++++++++++----------------------
1 file changed, 95 insertions(+), 69 deletions(-)
diff --git a/sys/dev/hid/hkbd.c b/sys/dev/hid/hkbd.c
index 321a51734664..63dd626cdef9 100644
--- a/sys/dev/hid/hkbd.c
+++ b/sys/dev/hid/hkbd.c
@@ -63,6 +63,9 @@ __FBSDID("$FreeBSD$");
#include <sys/proc.h>
#include <sys/kdb.h>
#include <sys/epoch.h>
+#include <sys/taskqueue.h>
+
+#include <machine/atomic.h>
#define HID_DEBUG_VAR hkbd_debug
#include <dev/hid/hid.h>
@@ -139,6 +142,8 @@ struct hkbd_softc {
struct hid_location sc_loc_numlock;
struct hid_location sc_loc_capslock;
struct hid_location sc_loc_scrolllock;
+ struct mtx sc_mtx;
+ struct task sc_task;
struct callout sc_callout;
struct hkbd_data sc_ndata;
struct hkbd_data sc_odata;
@@ -178,9 +183,8 @@ struct hkbd_softc {
int sc_led_size;
int sc_kbd_size;
- uint16_t sc_inputs;
- uint16_t sc_inputhead;
- uint16_t sc_inputtail;
+ uint32_t sc_inputhead;
+ uint32_t sc_inputtail;
uint8_t sc_iface_index;
uint8_t sc_iface_no;
@@ -211,15 +215,27 @@ struct hkbd_softc {
SCAN_PREFIX_CTL | SCAN_PREFIX_SHIFT)
#define SCAN_CHAR(c) ((c) & 0x7f)
-#define HKBD_LOCK() do { \
+#define HKBD_LOCK(sc) do { \
+ if (!HID_IN_POLLING_MODE()) \
+ mtx_lock(&(sc)->sc_mtx); \
+} while (0)
+#define HKBD_UNLOCK(sc) do { \
+ if (!HID_IN_POLLING_MODE()) \
+ mtx_unlock(&(sc)->sc_mtx); \
+} while (0)
+#define HKBD_LOCK_ASSERT(sc) do { \
+ if (!HID_IN_POLLING_MODE()) \
+ mtx_assert(&(sc)->sc_mtx, MA_OWNED); \
+} while (0)
+#define SYSCONS_LOCK() do { \
if (!HID_IN_POLLING_MODE()) \
mtx_lock(&Giant); \
} while (0)
-#define HKBD_UNLOCK() do { \
+#define SYSCONS_UNLOCK() do { \
if (!HID_IN_POLLING_MODE()) \
mtx_unlock(&Giant); \
} while (0)
-#define HKBD_LOCK_ASSERT() do { \
+#define SYSCONS_LOCK_ASSERT() do { \
if (!HID_IN_POLLING_MODE()) \
mtx_assert(&Giant, MA_OWNED); \
} while (0)
@@ -292,15 +308,16 @@ static int hkbd_ioctl(keyboard_t *, u_long, caddr_t);
static int hkbd_enable(keyboard_t *);
static int hkbd_disable(keyboard_t *);
static void hkbd_interrupt(struct hkbd_softc *);
-static void hkbd_event_keyinput(struct hkbd_softc *);
-static device_probe_t hkbd_probe;
-static device_attach_t hkbd_attach;
-static device_detach_t hkbd_detach;
-static device_resume_t hkbd_resume;
+static task_fn_t hkbd_event_keyinput;
+
+static device_probe_t hkbd_probe;
+static device_attach_t hkbd_attach;
+static device_detach_t hkbd_detach;
+static device_resume_t hkbd_resume;
#ifdef EVDEV_SUPPORT
-static evdev_event_t hkbd_ev_event;
+static evdev_event_t hkbd_ev_event;
static const struct evdev_methods hkbd_evdev_methods = {
.ev_event = hkbd_ev_event,
@@ -365,8 +382,9 @@ hkbd_start_timer(struct hkbd_softc *sc)
static void
hkbd_put_key(struct hkbd_softc *sc, uint32_t key)
{
+ uint32_t tail;
- HKBD_LOCK_ASSERT();
+ HKBD_LOCK_ASSERT(sc);
DPRINTF("0x%02x (%d) %s\n", key, key,
(key & KEY_RELEASE) ? "released" : "pressed");
@@ -377,13 +395,10 @@ hkbd_put_key(struct hkbd_softc *sc, uint32_t key)
evdev_hid2key(KEY_INDEX(key)), !(key & KEY_RELEASE));
#endif
- if (sc->sc_inputs < HKBD_IN_BUF_SIZE) {
+ tail = (sc->sc_inputtail + 1) % HKBD_IN_BUF_SIZE;
+ if (tail != atomic_load_acq_32(&sc->sc_inputhead)) {
sc->sc_input[sc->sc_inputtail] = key;
- ++(sc->sc_inputs);
- ++(sc->sc_inputtail);
- if (sc->sc_inputtail >= HKBD_IN_BUF_SIZE) {
- sc->sc_inputtail = 0;
- }
+ atomic_store_rel_32(&sc->sc_inputtail, tail);
} else {
DPRINTF("input buffer is full\n");
}
@@ -393,7 +408,7 @@ static void
hkbd_do_poll(struct hkbd_softc *sc, uint8_t wait)
{
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
KASSERT((sc->sc_flags & HKBD_FLAG_POLLING) != 0,
("hkbd_do_poll called when not polling\n"));
DPRINTFN(2, "polling\n");
@@ -406,7 +421,8 @@ hkbd_do_poll(struct hkbd_softc *sc, uint8_t wait)
* Note that we currently hold the Giant, but it's also used
* as the transfer mtx, so we must release it while waiting.
*/
- while (sc->sc_inputs == 0) {
+ while (sc->sc_inputhead ==
+ atomic_load_acq_32(&sc->sc_inputtail)) {
/*
* Give USB threads a chance to run. Note that
* kern_yield performs DROP_GIANT + PICKUP_GIANT.
@@ -418,7 +434,7 @@ hkbd_do_poll(struct hkbd_softc *sc, uint8_t wait)
return;
}
- while (sc->sc_inputs == 0) {
+ while (sc->sc_inputhead == sc->sc_inputtail) {
hidbus_intr_poll(sc->sc_dev);
/* Delay-optimised support for repetition of keys */
@@ -440,9 +456,10 @@ hkbd_do_poll(struct hkbd_softc *sc, uint8_t wait)
static int32_t
hkbd_get_key(struct hkbd_softc *sc, uint8_t wait)
{
+ uint32_t head;
int32_t c;
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
KASSERT(!HID_IN_POLLING_MODE() ||
(sc->sc_flags & HKBD_FLAG_POLLING) != 0,
("not polling in kdb or panic\n"));
@@ -450,15 +467,13 @@ hkbd_get_key(struct hkbd_softc *sc, uint8_t wait)
if (sc->sc_flags & HKBD_FLAG_POLLING)
hkbd_do_poll(sc, wait);
- if (sc->sc_inputs == 0) {
+ head = sc->sc_inputhead;
+ if (head == atomic_load_acq_32(&sc->sc_inputtail)) {
c = -1;
} else {
- c = sc->sc_input[sc->sc_inputhead];
- --(sc->sc_inputs);
- ++(sc->sc_inputhead);
- if (sc->sc_inputhead >= HKBD_IN_BUF_SIZE) {
- sc->sc_inputhead = 0;
- }
+ c = sc->sc_input[head];
+ head = (head + 1) % HKBD_IN_BUF_SIZE;
+ atomic_store_rel_32(&sc->sc_inputhead, head);
}
return (c);
}
@@ -469,7 +484,7 @@ hkbd_interrupt(struct hkbd_softc *sc)
const uint32_t now = sc->sc_time_ms;
unsigned key;
- HKBD_LOCK_ASSERT();
+ HKBD_LOCK_ASSERT(sc);
/* Check for key changes, the order is:
* 1. Modifier keys down
@@ -548,20 +563,22 @@ hkbd_interrupt(struct hkbd_softc *sc)
#endif
/* wakeup keyboard system */
- hkbd_event_keyinput(sc);
+ if (!HID_IN_POLLING_MODE())
+ taskqueue_enqueue(taskqueue_swi_giant, &sc->sc_task);
}
static void
-hkbd_event_keyinput(struct hkbd_softc *sc)
+hkbd_event_keyinput(void *context, int pending)
{
+ struct hkbd_softc *sc = context;
int c;
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
if ((sc->sc_flags & HKBD_FLAG_POLLING) != 0)
return;
- if (sc->sc_inputs == 0)
+ if (sc->sc_inputhead == atomic_load_acq_32(&sc->sc_inputtail))
return;
if (KBD_IS_ACTIVE(&sc->sc_kbd) &&
@@ -583,7 +600,7 @@ hkbd_timeout(void *arg)
struct hkbd_softc *sc = arg;
struct epoch_tracker et;
- HKBD_LOCK_ASSERT();
+ HKBD_LOCK_ASSERT(sc);
sc->sc_time_ms += sc->sc_delay;
sc->sc_delay = 0;
@@ -593,9 +610,10 @@ hkbd_timeout(void *arg)
epoch_exit_preempt(INPUT_EPOCH, &et);
/* Make sure any leftover key events gets read out */
- hkbd_event_keyinput(sc);
+ taskqueue_enqueue(taskqueue_swi_giant, &sc->sc_task);
- if (hkbd_any_key_pressed(sc) || (sc->sc_inputs != 0)) {
+ if (hkbd_any_key_pressed(sc) ||
+ atomic_load_acq_32(&sc->sc_inputhead) != sc->sc_inputtail) {
hkbd_start_timer(sc);
}
}
@@ -634,7 +652,7 @@ hkbd_intr_callback(void *context, void *data, hid_size_t len)
uint8_t modifiers;
int offset;
- HKBD_LOCK_ASSERT();
+ HKBD_LOCK_ASSERT(sc);
DPRINTF("actlen=%d bytes\n", len);
@@ -747,6 +765,8 @@ hkbd_probe(device_t dev)
if (error != 0)
return (error);
+ hidbus_set_desc(dev, "Keyboard");
+
return (BUS_PROBE_DEFAULT);
}
@@ -869,21 +889,21 @@ hkbd_attach(device_t dev)
#endif
sc->sc_dev = dev;
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
kbd_init_struct(kbd, HKBD_DRIVER_NAME, KB_OTHER, unit, 0, 0, 0);
kbd->kb_data = (void *)sc;
- hidbus_set_desc(dev, "Keyboard");
-
sc->sc_mode = K_XLATE;
- callout_init_mtx(&sc->sc_callout, &Giant, 0);
+ mtx_init(&sc->sc_mtx, "hkbd lock", NULL, MTX_DEF);
+ TASK_INIT(&sc->sc_task, 0, hkbd_event_keyinput, sc);
+ callout_init_mtx(&sc->sc_callout, &sc->sc_mtx, 0);
hidbus_set_intr(dev, hkbd_intr_callback, sc);
- /* interrupt handler will be called with Giant taken */
- hidbus_set_lock(dev, &Giant);
+ /* interrupt handler will be called with hkbd mutex taken */
+ hidbus_set_lock(dev, &sc->sc_mtx);
/* interrupt handler can be called during panic */
hidbus_set_flags(dev, hidbus_get_flags(dev) & HIDBUS_FLAG_CAN_POLL);
@@ -1005,13 +1025,15 @@ hkbd_detach(device_t dev)
struct hkbd_softc *sc = device_get_softc(dev);
int error;
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
DPRINTF("\n");
sc->sc_flags |= HKBD_FLAG_GONE;
+ HKBD_LOCK(sc);
callout_stop(&sc->sc_callout);
+ HKBD_UNLOCK(sc);
/* kill any stuck keys */
if (sc->sc_flags & HKBD_FLAG_ATTACHED) {
@@ -1022,9 +1044,13 @@ hkbd_detach(device_t dev)
memset(&sc->sc_ndata, 0, sizeof(sc->sc_ndata));
/* process releasing of all keys */
+ HKBD_LOCK(sc);
hkbd_interrupt(sc);
+ HKBD_UNLOCK(sc);
+ taskqueue_drain(taskqueue_swi_giant, &sc->sc_task);
}
+ mtx_destroy(&sc->sc_mtx);
hkbd_disable(&sc->sc_kbd);
#ifdef KBD_INSTALL_CDEV
@@ -1063,7 +1089,7 @@ hkbd_resume(device_t dev)
{
struct hkbd_softc *sc = device_get_softc(dev);
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
hkbd_clear_state(&sc->sc_kbd);
@@ -1143,9 +1169,9 @@ static int
hkbd_enable(keyboard_t *kbd)
{
- HKBD_LOCK();
+ SYSCONS_LOCK();
KBD_ACTIVATE(kbd);
- HKBD_UNLOCK();
+ SYSCONS_UNLOCK();
return (0);
}
@@ -1155,9 +1181,9 @@ static int
hkbd_disable(keyboard_t *kbd)
{
- HKBD_LOCK();
+ SYSCONS_LOCK();
KBD_DEACTIVATE(kbd);
- HKBD_UNLOCK();
+ SYSCONS_UNLOCK();
return (0);
}
@@ -1169,7 +1195,7 @@ hkbd_check(keyboard_t *kbd)
{
struct hkbd_softc *sc = kbd->kb_data;
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
if (!KBD_IS_ACTIVE(kbd))
return (0);
@@ -1182,7 +1208,7 @@ hkbd_check(keyboard_t *kbd)
return (1);
}
#endif
- if (sc->sc_inputs > 0) {
+ if (sc->sc_inputhead != atomic_load_acq_32(&sc->sc_inputtail)) {
return (1);
}
return (0);
@@ -1194,7 +1220,7 @@ hkbd_check_char_locked(keyboard_t *kbd)
{
struct hkbd_softc *sc = kbd->kb_data;
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
if (!KBD_IS_ACTIVE(kbd))
return (0);
@@ -1211,9 +1237,9 @@ hkbd_check_char(keyboard_t *kbd)
{
int result;
- HKBD_LOCK();
+ SYSCONS_LOCK();
result = hkbd_check_char_locked(kbd);
- HKBD_UNLOCK();
+ SYSCONS_UNLOCK();
return (result);
}
@@ -1231,7 +1257,7 @@ hkbd_read(keyboard_t *kbd, int wait)
#endif
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
if (!KBD_IS_ACTIVE(kbd))
return (-1);
@@ -1280,7 +1306,7 @@ hkbd_read_char_locked(keyboard_t *kbd, int wait)
uint32_t scancode;
#endif
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
if (!KBD_IS_ACTIVE(kbd))
return (NOKEY);
@@ -1453,9 +1479,9 @@ hkbd_read_char(keyboard_t *kbd, int wait)
{
uint32_t keycode;
- HKBD_LOCK();
+ SYSCONS_LOCK();
keycode = hkbd_read_char_locked(kbd, wait);
- HKBD_UNLOCK();
+ SYSCONS_UNLOCK();
return (keycode);
}
@@ -1476,7 +1502,7 @@ hkbd_ioctl_locked(keyboard_t *kbd, u_long cmd, caddr_t arg)
#endif
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
switch (cmd) {
case KDGKBMODE: /* get keyboard mode */
@@ -1649,9 +1675,9 @@ hkbd_ioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
return (EDEADLK); /* best I could come up with */
/* FALLTHROUGH */
default:
- HKBD_LOCK();
+ SYSCONS_LOCK();
result = hkbd_ioctl_locked(kbd, cmd, arg);
- HKBD_UNLOCK();
+ SYSCONS_UNLOCK();
return (result);
}
}
@@ -1662,7 +1688,7 @@ hkbd_clear_state(keyboard_t *kbd)
{
struct hkbd_softc *sc = kbd->kb_data;
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
sc->sc_flags &= ~(HKBD_FLAG_COMPOSE | HKBD_FLAG_POLLING);
sc->sc_state &= LOCK_MASK; /* preserve locking key state */
@@ -1697,7 +1723,7 @@ hkbd_poll(keyboard_t *kbd, int on)
{
struct hkbd_softc *sc = kbd->kb_data;
- HKBD_LOCK();
+ SYSCONS_LOCK();
/*
* Keep a reference count on polling to allow recursive
* cngrab() during a panic for example.
@@ -1714,7 +1740,7 @@ hkbd_poll(keyboard_t *kbd, int on)
sc->sc_flags &= ~HKBD_FLAG_POLLING;
sc->sc_delay = 0;
}
- HKBD_UNLOCK();
+ SYSCONS_UNLOCK();
return (0);
}
@@ -1730,7 +1756,7 @@ hkbd_set_leds(struct hkbd_softc *sc, uint8_t leds)
int len;
int error;
- HKBD_LOCK_ASSERT();
+ SYSCONS_LOCK_ASSERT();
DPRINTF("leds=0x%02x\n", leds);
#ifdef HID_DEBUG
@@ -1784,9 +1810,9 @@ hkbd_set_leds(struct hkbd_softc *sc, uint8_t leds)
DPRINTF("len=%d, id=%d\n", len, id);
/* start data transfer */
- HKBD_UNLOCK();
+ SYSCONS_UNLOCK();
error = hid_write(sc->sc_dev, buf, len);
- HKBD_LOCK();
+ SYSCONS_LOCK();
return (error);
}
More information about the dev-commits-src-all
mailing list