svn commit: r247601 - head/sys/sparc64/sbus

[Bruce Evans]

On Sat, 2 Mar 2013, Marius Strobl wrote:

> Log:
>  - Apparently, it's no longer a problem to call shutdown_nice(9) from within
>    an interrupt filter (some other drivers in the tree do the same). So
>    change the overtemperature and power fail interrupts from handlers in order
>    to code and get rid of a !INTR_MPSAFE handlers.

Sigh.

Maybe the only thing that "works" better with a filter is that LOR detection
for it is broken since filters don't use normal mutexes?

> Modified: head/sys/sparc64/sbus/sbus.c
> ==============================================================================
> --- head/sys/sparc64/sbus/sbus.c	Sat Mar  2 00:37:31 2013	(r247600)
> +++ head/sys/sparc64/sbus/sbus.c	Sat Mar  2 00:41:51 2013	(r247601)
> @@ -199,7 +199,7 @@ static driver_t sbus_driver = {
>
> static devclass_t sbus_devclass;
>
> -EARLY_DRIVER_MODULE(sbus, nexus, sbus_driver, sbus_devclass, 0, 0,
> +EARLY_DRIVER_MODULE(sbus, nexus, sbus_driver, sbus_devclass, NULL, NULL,
>     BUS_PASS_BUS);
> MODULE_DEPEND(sbus, nexus, 1, 1, 1);
> MODULE_VERSION(sbus, 1);
> @@ -410,7 +410,7 @@ sbus_attach(device_t dev)
> 	    INTVEC(SYSIO_READ8(sc, SBR_THERM_INT_MAP)) != vec ||
> 	    intr_vectors[vec].iv_ic != &sbus_ic ||
> 	    bus_setup_intr(dev, sc->sc_ot_ires, INTR_TYPE_MISC | INTR_BRIDGE,
> -	    NULL, sbus_overtemp, sc, &sc->sc_ot_ihand) != 0)
> +	    sbus_overtemp, NULL, sc, &sc->sc_ot_ihand) != 0)
> 		panic("%s: failed to set up temperature interrupt", __func__);
> 	i = 3;
> 	sc->sc_pf_ires = bus_alloc_resource_any(dev, SYS_RES_IRQ, &i,

It still doesn't claim INTR_MPSAFE.  Maybe that is meaningless for filters.
But some filters claim it.

> @@ -897,31 +897,33 @@ sbus_get_devinfo(device_t bus, device_t
>  * This handles the interrupt and powers off the machine.
>  * The same needs to be done to PCI controller drivers.
>  */
> -static void
> -sbus_overtemp(void *arg)
> +static int
> +sbus_overtemp(void *arg __unused)
> {
> 	static int shutdown;
>
> 	/* As the interrupt is cleared we may be called multiple times. */
> 	if (shutdown != 0)
> -		return;
> +		return (FILTER_HANDLED);
> 	shutdown++;
> 	printf("DANGER: OVER TEMPERATURE detected\nShutting down NOW.\n");

Calling the any() function printf() is also invalid in fast interrupt
handlers.  It is especially unsafe in practice in versions that serialize
the output -- this printf() may be long delayed.

> 	shutdown_nice(RB_POWEROFF);
> +	return (FILTER_HANDLED);
> }
>
> /* Try to shut down in time in case of power failure. */
> -static void
> -sbus_pwrfail(void *arg)
> +static int
> +sbus_pwrfail(void *arg __unused)
> {
> 	static int shutdown;
>
> 	/* As the interrupt is cleared we may be called multiple times. */
> 	if (shutdown != 0)
> -		return;
> +		return (FILTER_HANDLED);
> 	shutdown++;
> 	printf("Power failure detected\nShutting down NOW.\n");
> -	shutdown_nice(0);
> +	shutdown_nice(FILTER_HANDLED);

FILTER_HANDLED is a garbage arg for shutdown_nice().  It happens to be
2, which happens to be RB_SINGLE, which is a boot flag and not a reboot
flag, and is also not really used, so this bug has little effect.

> +	return (FILTER_HANDLED);
> }
>
> static int
>

I don't see anything to make these more MPSAFE than before.  They seem
to be basically correct and MPSAFE as ordinary interrupt handlers, but
basically incorrect and MPSAFE as filters.  The shutdown_nice() call
is MPSAFE if calling it is safe at all.  You just need to ensure that
the handlers are not connected to multiple interrupt sources (including
ones for other devies), so that rest of the global state accessed by
the handlers doesn't need locking.  This state is just the `shutdown'
variable in each, so it could easily be protected by an atomic op.
The locking is just as missing for the filter version as for the normal
version if there can be multiple interrupt sources.  However, the worst
that can happen if the counter gets messed up seems to be multiple
printf()s and multiple SIGINTs sent to init.  Both are probably
harmless.

Bruce