From owner-svn-src-all@FreeBSD.ORG Sat Mar 2 07:51:04 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 6E6C9AAB; Sat, 2 Mar 2013 07:51:04 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id F39DFF3D; Sat, 2 Mar 2013 07:51:03 +0000 (UTC) Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r227oxEJ023049 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 2 Mar 2013 18:51:01 +1100 Date: Sat, 2 Mar 2013 18:50:59 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Marius Strobl Subject: Re: svn commit: r247601 - head/sys/sparc64/sbus In-Reply-To: <201303020041.r220fq5N059799@svn.freebsd.org> Message-ID: <20130302182026.Y1572@besplex.bde.org> References: <201303020041.r220fq5N059799@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=D4sfsYtj c=1 sm=1 a=VnOwEx96OuMA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=av8Homa3Ie4A:10 a=AVZOoVRqcIQJYAXkCC4A:9 a=CjuIK1q_8ugA:10 a=53MiY31mBRR9oVUS:21 a=J_TLFwbwM9nZ-CZZ:21 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 02 Mar 2013 07:51:04 -0000 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