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

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

The following reply was made to PR kern/56438; it has been noted by GNATS.

From: Brad Spencer <brad%anduin.eldar.org@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: kern-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: Re: kern/56438: panic when trying to use gpioiic in -current 9.99.90
Date: Fri, 08 Oct 2021 05:51:48 -0400

 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