Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 15 Apr 2022 16:33:31 GMT
From:      Ed Maste <emaste@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: 3ec2816ad7a1 - stable/13 - kbd: replace vestigial spl calls with Giant assertions
Message-ID:  <202204151633.23FGXVkx038628@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by emaste:

URL: https://cgit.FreeBSD.org/src/commit/?id=3ec2816ad7a19dc2ae21ccd2d2d2c236027e4572

commit 3ec2816ad7a19dc2ae21ccd2d2d2c236027e4572
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2022-03-22 17:48:43 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2022-04-15 16:29:02 +0000

    kbd: replace vestigial spl calls with Giant assertions
    
    The keyboard driver was initially protected via spl* interrupt priority
    calls but (as part of a comprehensive effort) migrated to use the Giant
    lock (mutex).
    
    The spl calls left behind became NOPs but they can be confusing as they
    have no bearing on the actual mutual exclusion that is now present.
    
    Remove them from kbd and add assertions that Giant is held.  markj notes
    that there is conflation between the "bus topo" lock (which is Giant
    under the hood) and Giant.  The assertions could either be addressed as
    a small item along with bus topology locking work or they'll be removed
    if kbd is decoupled from Giant.
    
    PR:             206680
    Reviewed by:    markj
    MFC after:      3 weeks
    Sponsored by:   The FreeBSD Foundation
    Differential Revision:  https://reviews.freebsd.org/D34645
    
    (cherry picked from commit a0cd78bf2c148d7ab63454471771e3f4b572522f)
---
 sys/dev/kbd/kbd.c | 67 ++++++++++---------------------------------------------
 1 file changed, 12 insertions(+), 55 deletions(-)

diff --git a/sys/dev/kbd/kbd.c b/sys/dev/kbd/kbd.c
index 70c0ef15a56e..deb7672de7fa 100644
--- a/sys/dev/kbd/kbd.c
+++ b/sys/dev/kbd/kbd.c
@@ -35,7 +35,9 @@ __FBSDID("$FreeBSD$");
 #include <sys/param.h>
 #include <sys/systm.h>
 #include <sys/kernel.h>
+#include <sys/lock.h>
 #include <sys/malloc.h>
+#include <sys/mutex.h>
 #include <sys/conf.h>
 #include <sys/fcntl.h>
 #include <sys/poll.h>
@@ -97,13 +99,11 @@ kbd_realloc_array(void)
 {
 	keyboard_t **new_kbd;
 	int newsize;
-	int s;
 
-	s = spltty();
+	GIANT_REQUIRED;
 	newsize = rounddown(keyboards + ARRAY_DELTA, ARRAY_DELTA);
 	new_kbd = malloc(sizeof(*new_kbd)*newsize, M_DEVBUF, M_NOWAIT|M_ZERO);
 	if (new_kbd == NULL) {
-		splx(s);
 		return (ENOMEM);
 	}
 	bcopy(keyboard, new_kbd, sizeof(*keyboard)*keyboards);
@@ -111,7 +111,6 @@ kbd_realloc_array(void)
 		free(keyboard, M_DEVBUF);
 	keyboard = new_kbd;
 	keyboards = newsize;
-	splx(s);
 
 	if (bootverbose)
 		printf("kbd: new array size %d\n", keyboards);
@@ -245,30 +244,26 @@ int
 kbd_unregister(keyboard_t *kbd)
 {
 	int error;
-	int s;
 
+	GIANT_REQUIRED;
 	if ((kbd->kb_index < 0) || (kbd->kb_index >= keyboards))
 		return (ENOENT);
 	if (keyboard[kbd->kb_index] != kbd)
 		return (ENOENT);
 
-	s = spltty();
 	if (KBD_IS_BUSY(kbd)) {
 		error = (*kbd->kb_callback.kc_func)(kbd, KBDIO_UNLOADING,
 		    kbd->kb_callback.kc_arg);
 		if (error) {
-			splx(s);
 			return (error);
 		}
 		if (KBD_IS_BUSY(kbd)) {
-			splx(s);
 			return (EBUSY);
 		}
 	}
 	KBD_INVALID(kbd);
 	keyboard[kbd->kb_index] = NULL;
 
-	splx(s);
 	return (0);
 }
 
@@ -333,16 +328,14 @@ kbd_allocate(char *driver, int unit, void *id, kbd_callback_func_t *func,
 	     void *arg)
 {
 	int index;
-	int s;
 
+	GIANT_REQUIRED;
 	if (func == NULL)
 		return (-1);
 
-	s = spltty();
 	index = kbd_find_keyboard(driver, unit);
 	if (index >= 0) {
 		if (KBD_IS_BUSY(keyboard[index])) {
-			splx(s);
 			return (-1);
 		}
 		keyboard[index]->kb_token = id;
@@ -351,7 +344,6 @@ kbd_allocate(char *driver, int unit, void *id, kbd_callback_func_t *func,
 		keyboard[index]->kb_callback.kc_arg = arg;
 		kbdd_clear_state(keyboard[index]);
 	}
-	splx(s);
 	return (index);
 }
 
@@ -359,9 +351,8 @@ int
 kbd_release(keyboard_t *kbd, void *id)
 {
 	int error;
-	int s;
 
-	s = spltty();
+	GIANT_REQUIRED;
 	if (!KBD_IS_VALID(kbd) || !KBD_IS_BUSY(kbd)) {
 		error = EINVAL;
 	} else if (kbd->kb_token != id) {
@@ -374,7 +365,6 @@ kbd_release(keyboard_t *kbd, void *id)
 		kbdd_clear_state(kbd);
 		error = 0;
 	}
-	splx(s);
 	return (error);
 }
 
@@ -383,9 +373,8 @@ kbd_change_callback(keyboard_t *kbd, void *id, kbd_callback_func_t *func,
 		    void *arg)
 {
 	int error;
-	int s;
 
-	s = spltty();
+	GIANT_REQUIRED;
 	if (!KBD_IS_VALID(kbd) || !KBD_IS_BUSY(kbd)) {
 		error = EINVAL;
 	} else if (kbd->kb_token != id) {
@@ -397,7 +386,6 @@ kbd_change_callback(keyboard_t *kbd, void *id, kbd_callback_func_t *func,
 		kbd->kb_callback.kc_arg = arg;
 		error = 0;
 	}
-	splx(s);
 	return (error);
 }
 
@@ -542,20 +530,17 @@ genkbdopen(struct cdev *dev, int mode, int flag, struct thread *td)
 {
 	keyboard_t *kbd;
 	genkbd_softc_t *sc;
-	int s;
 	int i;
 
-	s = spltty();
+	GIANT_REQUIRED;
 	sc = dev->si_drv1;
 	kbd = kbd_get_keyboard(KBD_INDEX(dev));
 	if ((sc == NULL) || (kbd == NULL) || !KBD_IS_VALID(kbd)) {
-		splx(s);
 		return (ENXIO);
 	}
 	i = kbd_allocate(kbd->kb_name, kbd->kb_unit, sc,
 	    genkbd_event, (void *)sc);
 	if (i < 0) {
-		splx(s);
 		return (EBUSY);
 	}
 	/* assert(i == kbd->kb_index) */
@@ -567,7 +552,6 @@ genkbdopen(struct cdev *dev, int mode, int flag, struct thread *td)
 	 */
 
 	sc->gkb_q_length = 0;
-	splx(s);
 
 	return (0);
 }
@@ -577,13 +561,12 @@ genkbdclose(struct cdev *dev, int mode, int flag, struct thread *td)
 {
 	keyboard_t *kbd;
 	genkbd_softc_t *sc;
-	int s;
 
+	GIANT_REQUIRED;
 	/*
 	 * NOTE: the device may have already become invalid.
 	 * kbd == NULL || !KBD_IS_VALID(kbd)
 	 */
-	s = spltty();
 	sc = dev->si_drv1;
 	kbd = kbd_get_keyboard(KBD_INDEX(dev));
 	if ((sc == NULL) || (kbd == NULL) || !KBD_IS_VALID(kbd)) {
@@ -591,7 +574,6 @@ genkbdclose(struct cdev *dev, int mode, int flag, struct thread *td)
 	} else {
 		kbd_release(kbd, (void *)sc);
 	}
-	splx(s);
 	return (0);
 }
 
@@ -603,35 +585,29 @@ genkbdread(struct cdev *dev, struct uio *uio, int flag)
 	u_char buffer[KB_BUFSIZE];
 	int len;
 	int error;
-	int s;
 
+	GIANT_REQUIRED;
 	/* wait for input */
-	s = spltty();
 	sc = dev->si_drv1;
 	kbd = kbd_get_keyboard(KBD_INDEX(dev));
 	if ((sc == NULL) || (kbd == NULL) || !KBD_IS_VALID(kbd)) {
-		splx(s);
 		return (ENXIO);
 	}
 	while (sc->gkb_q_length == 0) {
 		if (flag & O_NONBLOCK) {
-			splx(s);
 			return (EWOULDBLOCK);
 		}
 		sc->gkb_flags |= KB_ASLEEP;
 		error = tsleep(sc, PZERO | PCATCH, "kbdrea", 0);
 		kbd = kbd_get_keyboard(KBD_INDEX(dev));
 		if ((kbd == NULL) || !KBD_IS_VALID(kbd)) {
-			splx(s);
 			return (ENXIO);	/* our keyboard has gone... */
 		}
 		if (error) {
 			sc->gkb_flags &= ~KB_ASLEEP;
-			splx(s);
 			return (error);
 		}
 	}
-	splx(s);
 
 	/* copy as much input as possible */
 	error = 0;
@@ -680,10 +656,9 @@ genkbdpoll(struct cdev *dev, int events, struct thread *td)
 	keyboard_t *kbd;
 	genkbd_softc_t *sc;
 	int revents;
-	int s;
 
+	GIANT_REQUIRED;
 	revents = 0;
-	s = spltty();
 	sc = dev->si_drv1;
 	kbd = kbd_get_keyboard(KBD_INDEX(dev));
 	if ((sc == NULL) || (kbd == NULL) || !KBD_IS_VALID(kbd)) {
@@ -694,7 +669,6 @@ genkbdpoll(struct cdev *dev, int events, struct thread *td)
 		else
 			selrecord(td, &sc->gkb_rsel);
 	}
-	splx(s);
 	return (revents);
 }
 
@@ -818,11 +792,10 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 	okeymap_t *omapp;
 	keyarg_t *keyp;
 	fkeyarg_t *fkeyp;
-	int s;
 	int i, j;
 	int error;
 
-	s = spltty();
+	GIANT_REQUIRED;
 	switch (cmd) {
 
 	case KDGKBINFO:		/* get keyboard information */
@@ -848,7 +821,6 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 	case GIO_KEYMAP:	/* get keyboard translation table */
 		error = copyout(kbd->kb_keymap, *(void **)arg,
 		    sizeof(keymap_t));
-		splx(s);
 		return (error);
 	case OGIO_KEYMAP:	/* get keyboard translation table (compat) */
 		mapp = kbd->kb_keymap;
@@ -879,7 +851,6 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 		} else {
 			error = copyin(*(void **)arg, mapp, sizeof *mapp);
 			if (error != 0) {
-				splx(s);
 				free(mapp, M_TEMP);
 				return (error);
 			}
@@ -887,7 +858,6 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 
 		error = keymap_change_ok(kbd->kb_keymap, mapp, curthread);
 		if (error != 0) {
-			splx(s);
 			free(mapp, M_TEMP);
 			return (error);
 		}
@@ -896,7 +866,6 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 		free(mapp, M_TEMP);
 		break;
 #else
-		splx(s);
 		return (ENODEV);
 #endif
 
@@ -904,7 +873,6 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 		keyp = (keyarg_t *)arg;
 		if (keyp->keynum >= sizeof(kbd->kb_keymap->key) /
 		    sizeof(kbd->kb_keymap->key[0])) {
-			splx(s);
 			return (EINVAL);
 		}
 		bcopy(&kbd->kb_keymap->key[keyp->keynum], &keyp->key,
@@ -915,20 +883,17 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 		keyp = (keyarg_t *)arg;
 		if (keyp->keynum >= sizeof(kbd->kb_keymap->key) /
 		    sizeof(kbd->kb_keymap->key[0])) {
-			splx(s);
 			return (EINVAL);
 		}
 		error = key_change_ok(&kbd->kb_keymap->key[keyp->keynum],
 		    &keyp->key, curthread);
 		if (error != 0) {
-			splx(s);
 			return (error);
 		}
 		bcopy(&keyp->key, &kbd->kb_keymap->key[keyp->keynum],
 		    sizeof(keyp->key));
 		break;
 #else
-		splx(s);
 		return (ENODEV);
 #endif
 
@@ -940,20 +905,17 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 		error = accent_change_ok(kbd->kb_accentmap,
 		    (accentmap_t *)arg, curthread);
 		if (error != 0) {
-			splx(s);
 			return (error);
 		}
 		bcopy(arg, kbd->kb_accentmap, sizeof(*kbd->kb_accentmap));
 		break;
 #else
-		splx(s);
 		return (ENODEV);
 #endif
 
 	case GETFKEY:		/* get functionkey string */
 		fkeyp = (fkeyarg_t *)arg;
 		if (fkeyp->keynum >= kbd->kb_fkeytab_size) {
-			splx(s);
 			return (EINVAL);
 		}
 		bcopy(kbd->kb_fkeytab[fkeyp->keynum].str, fkeyp->keydef,
@@ -964,13 +926,11 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 #ifndef KBD_DISABLE_KEYMAP_LOAD
 		fkeyp = (fkeyarg_t *)arg;
 		if (fkeyp->keynum >= kbd->kb_fkeytab_size) {
-			splx(s);
 			return (EINVAL);
 		}
 		error = fkey_change_ok(&kbd->kb_fkeytab[fkeyp->keynum],
 		    fkeyp, curthread);
 		if (error != 0) {
-			splx(s);
 			return (error);
 		}
 		kbd->kb_fkeytab[fkeyp->keynum].len = min(fkeyp->flen, MAXFK);
@@ -978,16 +938,13 @@ genkbd_commonioctl(keyboard_t *kbd, u_long cmd, caddr_t arg)
 		    kbd->kb_fkeytab[fkeyp->keynum].len);
 		break;
 #else
-		splx(s);
 		return (ENODEV);
 #endif
 
 	default:
-		splx(s);
 		return (ENOIOCTL);
 	}
 
-	splx(s);
 	return (0);
 }
 



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202204151633.23FGXVkx038628>