Date: Sun, 13 May 2012 17:15:30 +0000 (UTC) From: Andriy Gapon <avg@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-9@freebsd.org Subject: svn commit: r235411 - in stable/9/sys: dev/usb/input i386/conf kern Message-ID: <201205131715.q4DHFUME092865@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: avg Date: Sun May 13 17:15:30 2012 New Revision: 235411 URL: http://svn.freebsd.org/changeset/base/235411 Log: MFC r228765: ukbd: adjust for SCHEDULER_STOPPED() and overhaul locking code Modified: stable/9/sys/dev/usb/input/ukbd.c Directory Properties: stable/9/sys/ (props changed) stable/9/sys/amd64/include/xen/ (props changed) stable/9/sys/boot/ (props changed) stable/9/sys/boot/i386/efi/ (props changed) stable/9/sys/boot/ia64/efi/ (props changed) stable/9/sys/boot/ia64/ski/ (props changed) stable/9/sys/boot/powerpc/boot1.chrp/ (props changed) stable/9/sys/boot/powerpc/ofw/ (props changed) stable/9/sys/cddl/contrib/opensolaris/ (props changed) stable/9/sys/conf/ (props changed) stable/9/sys/contrib/dev/acpica/ (props changed) stable/9/sys/contrib/octeon-sdk/ (props changed) stable/9/sys/contrib/pf/ (props changed) stable/9/sys/contrib/x86emu/ (props changed) stable/9/sys/fs/ (props changed) stable/9/sys/fs/ntfs/ (props changed) stable/9/sys/i386/conf/XENHVM (props changed) stable/9/sys/kern/subr_witness.c (props changed) Modified: stable/9/sys/dev/usb/input/ukbd.c ============================================================================== --- stable/9/sys/dev/usb/input/ukbd.c Sun May 13 17:14:26 2012 (r235410) +++ stable/9/sys/dev/usb/input/ukbd.c Sun May 13 17:15:30 2012 (r235411) @@ -198,7 +198,6 @@ struct ukbd_softc { int sc_mode; /* input mode (K_XLATE,K_RAW,K_CODE) */ int sc_state; /* shift/lock key state */ int sc_accents; /* accent key index (> 0) */ - int sc_poll_tick_last; int sc_led_size; int sc_kbd_size; @@ -227,7 +226,6 @@ struct ukbd_softc { uint8_t sc_id_events; uint8_t sc_kbd_id; - uint8_t sc_poll_detected; uint8_t sc_buffer[UKBD_BUFFER_SIZE]; }; @@ -247,6 +245,33 @@ struct ukbd_softc { SCAN_PREFIX_CTL | SCAN_PREFIX_SHIFT) #define SCAN_CHAR(c) ((c) & 0x7f) +#define UKBD_LOCK() mtx_lock(&Giant) +#define UKBD_UNLOCK() mtx_unlock(&Giant) + +#ifdef INVARIANTS + +/* + * Assert that the lock is held in all contexts + * where the code can be executed. + */ +#define UKBD_LOCK_ASSERT() mtx_assert(&Giant, MA_OWNED) + +/* + * Assert that the lock is held in the contexts + * where it really has to be so. + */ +#define UKBD_CTX_LOCK_ASSERT() \ + do { \ + if (!kdb_active && panicstr == NULL) \ + mtx_assert(&Giant, MA_OWNED); \ + } while (0) +#else + +#define UKBD_LOCK_ASSERT() (void)0 +#define UKBD_CTX_LOCK_ASSERT() (void)0 + +#endif + struct ukbd_mods { uint32_t mask, key; }; @@ -339,8 +364,6 @@ static int ukbd_ioctl(keyboard_t *, u_lo static int ukbd_enable(keyboard_t *); static int ukbd_disable(keyboard_t *); static void ukbd_interrupt(struct ukbd_softc *); -static int ukbd_is_polling(struct ukbd_softc *); -static int ukbd_polls_other_thread(struct ukbd_softc *); static void ukbd_event_keyinput(struct ukbd_softc *); static device_probe_t ukbd_probe; @@ -370,7 +393,8 @@ ukbd_start_timer(struct ukbd_softc *sc) static void ukbd_put_key(struct ukbd_softc *sc, uint32_t key) { - mtx_assert(&Giant, MA_OWNED); + + UKBD_CTX_LOCK_ASSERT(); DPRINTF("0x%02x (%d) %s\n", key, key, (key & KEY_RELEASE) ? "released" : "pressed"); @@ -388,52 +412,32 @@ ukbd_put_key(struct ukbd_softc *sc, uint } static void -ukbd_yield(void) -{ - struct thread *td = curthread; - uint32_t old_prio; - - DROP_GIANT(); - - thread_lock(td); - - /* get current priority */ - old_prio = td->td_base_pri; - - /* set new priority */ - sched_prio(td, td->td_user_pri); - - /* cause a task switch */ - mi_switch(SW_INVOL | SWT_RELINQUISH, NULL); - - /* restore priority */ - sched_prio(td, old_prio); - - thread_unlock(td); - - PICKUP_GIANT(); -} - -static void ukbd_do_poll(struct ukbd_softc *sc, uint8_t wait) { - DPRINTFN(2, "polling\n"); - /* update stats about last polling event */ - sc->sc_poll_tick_last = ticks; - sc->sc_poll_detected = 1; + UKBD_CTX_LOCK_ASSERT(); + KASSERT((sc->sc_flags & UKBD_FLAG_POLLING) != 0, + ("ukbd_do_poll called when not polling\n")); + DPRINTFN(2, "polling\n"); - if (kdb_active == 0) { + if (!kdb_active && !SCHEDULER_STOPPED()) { + /* + * In this context the kernel is polling for input, + * but the USB subsystem works in normal interrupt-driven + * mode, so we just wait on the USB threads to do the job. + * 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) { - - /* give USB threads a chance to run */ - ukbd_yield(); - - /* check if we should wait */ + /* + * Give USB threads a chance to run. Note that + * kern_yield performs DROP_GIANT + PICKUP_GIANT. + */ + kern_yield(PRI_UNCHANGED); if (!wait) break; } - return; /* Only poll if KDB is active */ + return; } while (sc->sc_inputs == 0) { @@ -441,7 +445,6 @@ ukbd_do_poll(struct ukbd_softc *sc, uint usbd_transfer_poll(sc->sc_xfer, UKBD_N_TRANSFER); /* Delay-optimised support for repetition of keys */ - if (ukbd_any_key_pressed(sc)) { /* a key is pressed - need timekeeping */ DELAY(1000); @@ -462,16 +465,16 @@ ukbd_get_key(struct ukbd_softc *sc, uint { int32_t c; - mtx_assert(&Giant, MA_OWNED); + UKBD_CTX_LOCK_ASSERT(); + KASSERT((!kdb_active && !SCHEDULER_STOPPED()) + || (sc->sc_flags & UKBD_FLAG_POLLING) != 0, + ("not polling in kdb or panic\n")); if (sc->sc_inputs == 0) { /* start transfer, if not already started */ usbd_transfer_start(sc->sc_xfer[UKBD_INTR_DT]); } - if (ukbd_polls_other_thread(sc)) - return (-1); - if (sc->sc_flags & UKBD_FLAG_POLLING) ukbd_do_poll(sc, wait); @@ -499,6 +502,8 @@ ukbd_interrupt(struct ukbd_softc *sc) uint8_t i; uint8_t j; + UKBD_CTX_LOCK_ASSERT(); + if (sc->sc_ndata.keycode[0] == KEY_ERROR) return; @@ -584,7 +589,9 @@ ukbd_event_keyinput(struct ukbd_softc *s { int c; - if (ukbd_is_polling(sc)) + UKBD_CTX_LOCK_ASSERT(); + + if ((sc->sc_flags & UKBD_FLAG_POLLING) != 0) return; if (sc->sc_inputs == 0) @@ -608,7 +615,7 @@ ukbd_timeout(void *arg) { struct ukbd_softc *sc = arg; - mtx_assert(&Giant, MA_OWNED); + UKBD_LOCK_ASSERT(); sc->sc_time_ms += 25; /* milliseconds */ @@ -656,6 +663,8 @@ ukbd_intr_callback(struct usb_xfer *xfer uint8_t id; int len; + UKBD_LOCK_ASSERT(); + usbd_xfer_status(xfer, &len, NULL, NULL, NULL); pc = usbd_xfer_get_frame(xfer, 0); @@ -842,6 +851,8 @@ ukbd_set_leds_callback(struct usb_xfer * uint8_t any; int len; + UKBD_LOCK_ASSERT(); + #ifdef USB_DEBUG if (ukbd_no_leds) return; @@ -972,6 +983,7 @@ ukbd_probe(device_t dev) int error; uint16_t d_len; + UKBD_LOCK_ASSERT(); DPRINTFN(11, "\n"); if (sw == NULL) { @@ -998,7 +1010,7 @@ ukbd_probe(device_t dev) if (error) return (ENXIO); - /* + /* * NOTE: we currently don't support USB mouse and USB keyboard * on the same USB endpoint. */ @@ -1165,6 +1177,8 @@ ukbd_attach(device_t dev) uint16_t n; uint16_t hid_len; + UKBD_LOCK_ASSERT(); + kbd_init_struct(kbd, UKBD_DRIVER_NAME, KB_OTHER, unit, 0, 0, 0); kbd->kb_data = (void *)sc; @@ -1241,14 +1255,10 @@ ukbd_attach(device_t dev) /* ignore if SETIDLE fails, hence it is not crucial */ usbd_req_set_idle(sc->sc_udev, NULL, sc->sc_iface_index, 0, 0); - mtx_lock(&Giant); - ukbd_ioctl(kbd, KDSETLED, (caddr_t)&sc->sc_state); KBD_INIT_DONE(kbd); - mtx_unlock(&Giant); - if (kbd_register(kbd) < 0) { goto detach; } @@ -1266,15 +1276,10 @@ ukbd_attach(device_t dev) if (bootverbose) { genkbd_diag(kbd, bootverbose); } - /* lock keyboard mutex */ - - mtx_lock(&Giant); /* start the keyboard */ - usbd_transfer_start(sc->sc_xfer[UKBD_INTR_DT]); - mtx_unlock(&Giant); return (0); /* success */ detach: @@ -1288,9 +1293,9 @@ ukbd_detach(device_t dev) struct ukbd_softc *sc = device_get_softc(dev); int error; - DPRINTF("\n"); + UKBD_LOCK_ASSERT(); - mtx_lock(&Giant); + DPRINTF("\n"); sc->sc_flags |= UKBD_FLAG_GONE; @@ -1318,8 +1323,6 @@ ukbd_detach(device_t dev) } sc->sc_kbd.kb_flags = 0; - mtx_unlock(&Giant); - usbd_transfer_unsetup(sc->sc_xfer, UKBD_N_TRANSFER); usb_callout_drain(&sc->sc_callout); @@ -1335,12 +1338,10 @@ ukbd_resume(device_t dev) { struct ukbd_softc *sc = device_get_softc(dev); - mtx_lock(&Giant); + UKBD_LOCK_ASSERT(); ukbd_clear_state(&sc->sc_kbd); - mtx_unlock(&Giant); - return (0); } @@ -1400,15 +1401,11 @@ ukbd_lock(keyboard_t *kbd, int lock) static int ukbd_enable(keyboard_t *kbd) { - if (!mtx_owned(&Giant)) { - /* XXX cludge */ - int retval; - mtx_lock(&Giant); - retval = ukbd_enable(kbd); - mtx_unlock(&Giant); - return (retval); - } + + UKBD_LOCK(); KBD_ACTIVATE(kbd); + UKBD_UNLOCK(); + return (0); } @@ -1416,44 +1413,24 @@ ukbd_enable(keyboard_t *kbd) static int ukbd_disable(keyboard_t *kbd) { - if (!mtx_owned(&Giant)) { - /* XXX cludge */ - int retval; - mtx_lock(&Giant); - retval = ukbd_disable(kbd); - mtx_unlock(&Giant); - return (retval); - } + + UKBD_LOCK(); KBD_DEACTIVATE(kbd); + UKBD_UNLOCK(); + return (0); } /* check if data is waiting */ +/* Currently unused. */ static int ukbd_check(keyboard_t *kbd) { struct ukbd_softc *sc = kbd->kb_data; - if (!KBD_IS_ACTIVE(kbd)) - return (0); + UKBD_CTX_LOCK_ASSERT(); - if (sc->sc_flags & UKBD_FLAG_POLLING) { - if (!mtx_owned(&Giant)) { - /* XXX cludge */ - int retval; - mtx_lock(&Giant); - retval = ukbd_check(kbd); - mtx_unlock(&Giant); - return (retval); - } - } else { - /* XXX the keyboard layer requires Giant */ - if (!mtx_owned(&Giant)) - return (0); - } - - /* check if key belongs to this thread */ - if (ukbd_polls_other_thread(sc)) + if (!KBD_IS_ACTIVE(kbd)) return (0); if (sc->sc_flags & UKBD_FLAG_POLLING) @@ -1472,30 +1449,13 @@ ukbd_check(keyboard_t *kbd) /* check if char is waiting */ static int -ukbd_check_char(keyboard_t *kbd) +ukbd_check_char_locked(keyboard_t *kbd) { struct ukbd_softc *sc = kbd->kb_data; - if (!KBD_IS_ACTIVE(kbd)) - return (0); + UKBD_CTX_LOCK_ASSERT(); - if (sc->sc_flags & UKBD_FLAG_POLLING) { - if (!mtx_owned(&Giant)) { - /* XXX cludge */ - int retval; - mtx_lock(&Giant); - retval = ukbd_check_char(kbd); - mtx_unlock(&Giant); - return (retval); - } - } else { - /* XXX the keyboard layer requires Giant */ - if (!mtx_owned(&Giant)) - return (0); - } - - /* check if key belongs to this thread */ - if (ukbd_polls_other_thread(sc)) + if (!KBD_IS_ACTIVE(kbd)) return (0); if ((sc->sc_composed_char > 0) && @@ -1505,39 +1465,34 @@ ukbd_check_char(keyboard_t *kbd) return (ukbd_check(kbd)); } +static int +ukbd_check_char(keyboard_t *kbd) +{ + int result; + + UKBD_LOCK(); + result = ukbd_check_char_locked(kbd); + UKBD_UNLOCK(); + + return (result); +} /* read one byte from the keyboard if it's allowed */ +/* Currently unused. */ static int ukbd_read(keyboard_t *kbd, int wait) { struct ukbd_softc *sc = kbd->kb_data; int32_t usbcode; - #ifdef UKBD_EMULATE_ATSCANCODE uint32_t keycode; uint32_t scancode; #endif - if (!KBD_IS_ACTIVE(kbd)) - return (-1); - if (sc->sc_flags & UKBD_FLAG_POLLING) { - if (!mtx_owned(&Giant)) { - /* XXX cludge */ - int retval; - mtx_lock(&Giant); - retval = ukbd_read(kbd, wait); - mtx_unlock(&Giant); - return (retval); - } - } else { - /* XXX the keyboard layer requires Giant */ - if (!mtx_owned(&Giant)) - return (-1); - } + UKBD_CTX_LOCK_ASSERT(); - /* check if key belongs to this thread */ - if (ukbd_polls_other_thread(sc)) + if (!KBD_IS_ACTIVE(kbd)) return (-1); #ifdef UKBD_EMULATE_ATSCANCODE @@ -1574,38 +1529,19 @@ ukbd_read(keyboard_t *kbd, int wait) /* read char from the keyboard */ static uint32_t -ukbd_read_char(keyboard_t *kbd, int wait) +ukbd_read_char_locked(keyboard_t *kbd, int wait) { struct ukbd_softc *sc = kbd->kb_data; uint32_t action; uint32_t keycode; int32_t usbcode; - #ifdef UKBD_EMULATE_ATSCANCODE uint32_t scancode; - #endif - if (!KBD_IS_ACTIVE(kbd)) - return (NOKEY); - - if (sc->sc_flags & UKBD_FLAG_POLLING) { - if (!mtx_owned(&Giant)) { - /* XXX cludge */ - int retval; - mtx_lock(&Giant); - retval = ukbd_read_char(kbd, wait); - mtx_unlock(&Giant); - return (retval); - } - } else { - /* XXX the keyboard layer requires Giant */ - if (!mtx_owned(&Giant)) - return (NOKEY); - } + UKBD_CTX_LOCK_ASSERT(); - /* check if key belongs to this thread */ - if (ukbd_polls_other_thread(sc)) + if (!KBD_IS_ACTIVE(kbd)) return (NOKEY); next_code: @@ -1782,39 +1718,32 @@ errkey: return (ERRKEY); } +/* Currently wait is always false. */ +static uint32_t +ukbd_read_char(keyboard_t *kbd, int wait) +{ + uint32_t keycode; + + UKBD_LOCK(); + keycode = ukbd_read_char_locked(kbd, wait); + UKBD_UNLOCK(); + + return (keycode); +} + /* some useful control functions */ static int -ukbd_ioctl(keyboard_t *kbd, u_long cmd, caddr_t arg) +ukbd_ioctl_locked(keyboard_t *kbd, u_long cmd, caddr_t arg) { struct ukbd_softc *sc = kbd->kb_data; int i; - #if defined(COMPAT_FREEBSD6) || defined(COMPAT_FREEBSD5) || \ defined(COMPAT_FREEBSD4) || defined(COMPAT_43) int ival; #endif - if (!mtx_owned(&Giant)) { - /* - * XXX big problem: If scroll lock is pressed and "printf()" - * is called, the CPU will get here, to un-scroll lock the - * keyboard. But if "printf()" acquires the "Giant" lock, - * there will be a locking order reversal problem, so the - * keyboard system must get out of "Giant" first, before the - * CPU can proceed here ... - */ - switch (cmd) { - case KDGKBMODE: - case KDSKBMODE: - /* workaround for Geli */ - mtx_lock(&Giant); - i = ukbd_ioctl(kbd, cmd, arg); - mtx_unlock(&Giant); - return (i); - default: - return (EINVAL); - } - } + + UKBD_LOCK_ASSERT(); switch (cmd) { case KDGKBMODE: /* get keyboard mode */ @@ -1839,7 +1768,7 @@ ukbd_ioctl(keyboard_t *kbd, u_long cmd, case K_RAW: case K_CODE: if (sc->sc_mode != *(int *)arg) { - if (ukbd_is_polling(sc) == 0) + if ((sc->sc_flags & UKBD_FLAG_POLLING) == 0) ukbd_clear_state(kbd); sc->sc_mode = *(int *)arg; } @@ -1943,19 +1872,44 @@ ukbd_ioctl(keyboard_t *kbd, u_long cmd, return (0); } +static int +ukbd_ioctl(keyboard_t *kbd, u_long cmd, caddr_t arg) +{ + int result; + + /* + * XXX KDGKBSTATE, KDSKBSTATE and KDSETLED can be called from any + * context where printf(9) can be called, which among other things + * includes interrupt filters and threads with any kinds of locks + * already held. For this reason it would be dangerous to acquire + * the Giant here unconditionally. On the other hand we have to + * have it to handle the ioctl. + * So we make our best effort to auto-detect whether we can grab + * the Giant or not. Blame syscons(4) for this. + */ + switch (cmd) { + case KDGKBSTATE: + case KDSKBSTATE: + case KDSETLED: + if (!mtx_owned(&Giant) && !SCHEDULER_STOPPED()) + return (EDEADLK); /* best I could come up with */ + /* FALLTHROUGH */ + default: + UKBD_LOCK(); + result = ukbd_ioctl_locked(kbd, cmd, arg); + UKBD_UNLOCK(); + return (result); + } +} + + /* clear the internal state of the keyboard */ static void ukbd_clear_state(keyboard_t *kbd) { struct ukbd_softc *sc = kbd->kb_data; - if (!mtx_owned(&Giant)) { - /* XXX cludge */ - mtx_lock(&Giant); - ukbd_clear_state(kbd); - mtx_unlock(&Giant); - return; - } + UKBD_CTX_LOCK_ASSERT(); sc->sc_flags &= ~(UKBD_FLAG_COMPOSE | UKBD_FLAG_POLLING); sc->sc_state &= LOCK_MASK; /* preserve locking key state */ @@ -1986,43 +1940,11 @@ ukbd_set_state(keyboard_t *kbd, void *bu } static int -ukbd_is_polling(struct ukbd_softc *sc) -{ - int delta; - - if (sc->sc_flags & UKBD_FLAG_POLLING) - return (1); /* polling */ - - delta = ticks - sc->sc_poll_tick_last; - if ((delta < 0) || (delta >= hz)) { - sc->sc_poll_detected = 0; - return (0); /* not polling */ - } - - return (sc->sc_poll_detected); -} - -static int -ukbd_polls_other_thread(struct ukbd_softc *sc) -{ - return (ukbd_is_polling(sc) && - (sc->sc_poll_thread != curthread)); -} - -static int ukbd_poll(keyboard_t *kbd, int on) { struct ukbd_softc *sc = kbd->kb_data; - if (!mtx_owned(&Giant)) { - /* XXX cludge */ - int retval; - mtx_lock(&Giant); - retval = ukbd_poll(kbd, on); - mtx_unlock(&Giant); - return (retval); - } - + UKBD_LOCK(); if (on) { sc->sc_flags |= UKBD_FLAG_POLLING; sc->sc_poll_thread = curthread; @@ -2030,6 +1952,8 @@ ukbd_poll(keyboard_t *kbd, int on) sc->sc_flags &= ~UKBD_FLAG_POLLING; ukbd_start_timer(sc); /* start timer */ } + UKBD_UNLOCK(); + return (0); } @@ -2038,6 +1962,8 @@ ukbd_poll(keyboard_t *kbd, int on) static void ukbd_set_leds(struct ukbd_softc *sc, uint8_t leds) { + + UKBD_LOCK_ASSERT(); DPRINTF("leds=0x%02x\n", leds); sc->sc_leds = leds;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201205131715.q4DHFUME092865>