Re: kern/56438: panic when trying to use gpioiic in -current 9.99.90

[Brad Spencer <brad%anduin.eldar.org@localhost>]

mlelstv%serpens.de@localhost (Michael van Elst) writes:

> The following reply was made to PR kern/56438; it has been noted by GNATS.
>
> From: mlelstv%serpens.de@localhost (Michael van Elst)
> To: gnats-bugs%netbsd.org@localhost
> Cc: 
> Subject: Re: kern/56438: panic when trying to use gpioiic in -current 9.99.90
> Date: Mon, 4 Oct 2021 17:20:02 -0000 (UTC)
>
>  brad%anduin.eldar.org@localhost (Brad Spencer) writes:
>  
>  >mlelstv%serpens.de@localhost (Michael van Elst) writes:
>  
>  >So, something like the following..  I made this change and tested it and
>  >the panic appears to be gone and devices attach as expected again,
>  
>  Yes, but there are a few more cases. And the compat50 code misses any kind
>  of locking.
>  

After a bit of back and forth email with Michael van Elst who provided
more explanation... included is a more complete patch.  I tested
attaching and detaching on real hardware with real devices.  I can't
test the 5.0 path as I don't have anything that old and I am not sure
rescan at the gpio device level will do much as the busses lower down,
in particular, gpioiic and gpioow do not support the concept of rescan
and I think that they would have to.  In any case, the rescan case
doesn't panic either.  This probably should be commited as it improves
the situation with -current.

--- src/sys/dev/gpio/gpio.c.ORIG	2021-09-26 13:41:58.259633657 -0400
+++ src/sys/dev/gpio/gpio.c	2021-10-05 07:01:37.664529748 -0400
@@ -172,12 +172,14 @@
 	if (error)
 		return;
 
+	KERNEL_LOCK(1, NULL);
 	LIST_FOREACH(gdev, &sc->sc_devs, sc_next)
 		if (gdev->sc_dev == child) {
 			LIST_REMOVE(gdev, sc_next);
 			kmem_free(gdev, sizeof(struct gpio_dev));
 			break;
 		}
+	KERNEL_UNLOCK_ONE(NULL);
 
 	mutex_enter(&sc->sc_mtx);
 	sc->sc_attach_busy = 0;
@@ -190,8 +192,10 @@
 gpio_rescan(device_t self, const char *ifattr, const int *locators)
 {
 
+	KERNEL_LOCK(1, NULL);
 	config_search(self, NULL,
 	    CFARGS(.search = gpio_search));
+	KERNEL_UNLOCK_ONE(NULL);
 
 	return 0;
 }
@@ -849,6 +853,7 @@
 		locs[GPIOCF_MASK] = ga.ga_mask;
 		locs[GPIOCF_FLAG] = ga.ga_flags;
 
+		KERNEL_LOCK(1, NULL);
 		cf = config_search(sc->sc_dev, &ga,
 		    CFARGS(.locators = locs));
 		if (cf != NULL) {
@@ -869,6 +874,8 @@
 #endif
 		} else
 			error = EINVAL;
+		KERNEL_UNLOCK_ONE(NULL);
+
 		mutex_enter(&sc->sc_mtx);
 		sc->sc_attach_busy = 0;
 		cv_signal(&sc->sc_attach);
@@ -1106,6 +1113,7 @@
 		if (error)
 			return EBUSY;
 
+		KERNEL_LOCK(1, NULL);
 		attach = data;
 		LIST_FOREACH(gdev, &sc->sc_devs, sc_next) {
 			if (strcmp(device_xname(gdev->sc_dev),
@@ -1115,11 +1123,15 @@
 				cv_signal(&sc->sc_attach);
 				mutex_exit(&sc->sc_mtx);
 
-				if (config_detach(gdev->sc_dev, 0) == 0)
+				if (config_detach(gdev->sc_dev, 0) == 0) {
+					KERNEL_UNLOCK_ONE(NULL);
 					return 0;
+				}
 				break;
 			}
 		}
+		KERNEL_UNLOCK_ONE(NULL);
+
 		if (gdev == NULL) {
 			mutex_enter(&sc->sc_mtx);
 			sc->sc_attach_busy = 0;



-- 
Brad Spencer - brad%anduin.eldar.org@localhost - KC8VKS - http://anduin.eldar.org