The branch main has been updated by wulf: URL: https://cgit.FreeBSD.org/src/commit/?id=2775d1d5d1efeed02f4a805a86f89ee96d68006b
commit 2775d1d5d1efeed02f4a805a86f89ee96d68006b Author: Vladimir Kondratyev <[email protected]> AuthorDate: 2020-11-09 19:25:14 +0000 Commit: Vladimir Kondratyev <[email protected]> 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); } _______________________________________________ [email protected] mailing list https://lists.freebsd.org/mailman/listinfo/dev-commits-src-main To unsubscribe, send any mail to "[email protected]"
