Date: Wed, 31 Aug 2016 11:10:40 +0000 (UTC) From: Bruce Evans <bde@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r305121 - head/sys/dev/syscons Message-ID: <201608311110.u7VBAesL079612@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: bde Date: Wed Aug 31 11:10:39 2016 New Revision: 305121 URL: https://svnweb.freebsd.org/changeset/base/305121 Log: Add some locking to sc_cngetc(). Keyboard input needs Giant locking, and that is not possible to do correctly here. Use mtx_trylock() and proceed unlocked as before if we can't acquire Giant (non-recursively), except in kdb mode don't even try to acquire Giant. Everything here is a hack, but it often works. Even if mtx_trylock() succeeds, this might be a LOR. Keyboard input also needs screen locking, to handle screen updates and switches. Add this, using the same simplistic screen locking as for sc_cnputc(). Giant must be acquired before the screen lock, and the screen lock must be dropped when calling the keyboard driver (else it would get a harmless LOR if it tries to acquire Giant). It was intended that sc cn open/close hide the locking calls, and they do for i/o functions functions except for this complication. Non-console keyboard input is still only Giant-locked, with screen locking in some called functions. This is correct for the keyboard parts only. When Giant cannot be acquired properly, atkbd and kbdmux tend to race and work (they assume that the caller acquired Giant properly and don't try to acquire it again or check that it has been acquired, and the races rarely matter), while ukbd tends to deadlock or panic (since it does the opposite, and has other usb threads to deadlock with). The keyboard (Giant) locking here does very little, but the screen locking completes screen locking for console mode except for not detecting or handling deadlock. Modified: head/sys/dev/syscons/syscons.c head/sys/dev/syscons/syscons.h Modified: head/sys/dev/syscons/syscons.c ============================================================================== --- head/sys/dev/syscons/syscons.c Wed Aug 31 10:45:53 2016 (r305120) +++ head/sys/dev/syscons/syscons.c Wed Aug 31 11:10:39 2016 (r305121) @@ -1649,11 +1649,33 @@ sc_cnterm(struct consdev *cp) static void sccnclose(sc_softc_t *sc, struct sc_cnstate *sp); static int sc_cngetc_locked(struct sc_cnstate *sp); +static void sccnkbdlock(sc_softc_t *sc, struct sc_cnstate *sp); +static void sccnkbdunlock(sc_softc_t *sc, struct sc_cnstate *sp); static void sccnopen(sc_softc_t *sc, struct sc_cnstate *sp, int flags); static void sccnscrlock(sc_softc_t *sc, struct sc_cnstate *sp); static void sccnscrunlock(sc_softc_t *sc, struct sc_cnstate *sp); static void +sccnkbdlock(sc_softc_t *sc, struct sc_cnstate *sp) +{ + /* + * Locking method: hope for the best. + * The keyboard is supposed to be Giant locked. We can't handle that + * in general. The kdb_active case here is not safe, and we will + * proceed without the lock in all cases. + */ + sp->kbd_locked = !kdb_active && mtx_trylock(&Giant); +} + +static void +sccnkbdunlock(sc_softc_t *sc, struct sc_cnstate *sp) +{ + if (sp->kbd_locked) + mtx_unlock(&Giant); + sp->kbd_locked = FALSE; +} + +static void sccnscrlock(sc_softc_t *sc, struct sc_cnstate *sp) { SC_VIDEO_LOCK(sc); @@ -1674,11 +1696,14 @@ sccnopen(sc_softc_t *sc, struct sc_cnsta sp->kbd_opened = FALSE; sp->scr_opened = FALSE; + sp->kbd_locked = FALSE; /* Opening the keyboard is optional. */ if (!(flags & 1) || sc->kbd == NULL) goto over_keyboard; + sccnkbdlock(sc, sp); + /* * Make sure the keyboard is accessible even when the kbd device * driver is disabled. @@ -1726,6 +1751,7 @@ sccnclose(sc_softc_t *sc, struct sc_cnst kbdd_disable(sc->kbd); sp->kbd_opened = FALSE; + sccnkbdunlock(sc, sp); } /* @@ -1751,6 +1777,7 @@ sc_cngrab(struct consdev *cp) if (lev >= 0 && lev < 2) { sccnopen(sc, &sc->grab_state[lev], 1 | 2); sccnscrunlock(sc, &sc->grab_state[lev]); + sccnkbdunlock(sc, &sc->grab_state[lev]); } } @@ -1763,6 +1790,7 @@ sc_cnungrab(struct consdev *cp) sc = sc_console->sc; lev = atomic_load_acq_int(&sc->grab_level) - 1; if (lev >= 0 && lev < 2) { + sccnkbdlock(sc, &sc->grab_state[lev]); sccnscrlock(sc, &sc->grab_state[lev]); sccnclose(sc, &sc->grab_state[lev]); } @@ -1825,16 +1853,20 @@ sc_cnputc(struct consdev *cd, int c) static int sc_cngetc(struct consdev *cd) { + struct sc_cnstate st; int c, s; /* assert(sc_console != NULL) */ + sccnopen(sc_console->sc, &st, 1); s = spltty(); /* block sckbdevent and scrn_timer while we poll */ - if (sc_console->sc->kbd == NULL) { + if (!st.kbd_opened) { splx(s); - return -1; + sccnclose(sc_console->sc, &st); + return -1; /* means no keyboard since we fudged the locking */ } - c = sc_cngetc_locked(NULL); + c = sc_cngetc_locked(&st); splx(s); + sccnclose(sc_console->sc, &st); return c; } @@ -1858,7 +1890,7 @@ sc_cngetc_locked(struct sc_cnstate *sp) if (fkeycp < fkey.len) return fkey.str[fkeycp++]; - c = scgetc(scp->sc, SCGETC_CN | SCGETC_NONBLOCK, NULL); + c = scgetc(scp->sc, SCGETC_CN | SCGETC_NONBLOCK, sp); switch (KEYFLAGS(c)) { case 0: /* normal char */ @@ -3464,7 +3496,11 @@ next_code: scp = sc->cur_scp; /* first see if there is something in the keyboard port */ for (;;) { + if (flags & SCGETC_CN) + sccnscrunlock(sc, sp); c = kbdd_read_char(sc->kbd, !(flags & SCGETC_NONBLOCK)); + if (flags & SCGETC_CN) + sccnscrlock(sc, sp); if (c == ERRKEY) { if (!(flags & SCGETC_CN)) sc_bell(scp, bios_value.bell_pitch, BELL_DURATION); Modified: head/sys/dev/syscons/syscons.h ============================================================================== --- head/sys/dev/syscons/syscons.h Wed Aug 31 10:45:53 2016 (r305120) +++ head/sys/dev/syscons/syscons.h Wed Aug 31 11:10:39 2016 (r305121) @@ -190,6 +190,7 @@ struct scr_stat; struct tty; struct sc_cnstate { + u_char kbd_locked; u_char kbd_opened; u_char scr_opened; };
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201608311110.u7VBAesL079612>