From owner-p4-projects@FreeBSD.ORG Sun May 6 20:33:11 2007 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 86E9816A40A; Sun, 6 May 2007 20:33:11 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 3F92D16A404 for ; Sun, 6 May 2007 20:33:11 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from ug-out-1314.google.com (ug-out-1314.google.com [66.249.92.168]) by mx1.freebsd.org (Postfix) with ESMTP id 952A913C459 for ; Sun, 6 May 2007 20:33:10 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: by ug-out-1314.google.com with SMTP id 71so813761ugh for ; Sun, 06 May 2007 13:33:09 -0700 (PDT) DKIM-Signature: a=rsa-sha1; c=relaxed/relaxed; d=gmail.com; s=beta; h=domainkey-signature:received:received:message-id:date:from:reply-to:user-agent:mime-version:to:cc:subject:references:in-reply-to:content-type:content-transfer-encoding:sender; b=obNtkKNVlionbDK62YiJdsVNmvXj3rfVjqPS4yYOpgTC8J61meK87VwyC2StvgiB+TJjQP1dQyUKOGrbEEqn04ldLjHYaZ1vdUEC1hJpMqJ0LnEXJ6b/kZH6lgd+LEPmBayJWLhdBOCxNigH/ex6hHoARQrElEluNttLl/tz4dM= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=beta; h=received:message-id:date:from:reply-to:user-agent:mime-version:to:cc:subject:references:in-reply-to:content-type:content-transfer-encoding:sender; b=lVvJs3kdbjlI6+ECIlCgN0Z0Gvr0cwjsP/jwP5CPJvSuQDK8AryLONaH3bjTQObid0ZmLJuEzGESBHToICh/BPc9y0n1eVVYCNw4JmJJVPlEYaAfrN+vbVNQsLkTe28myt1fw2kq3Dm4UBXE4QCy1s3Yj+k1mV4Nzs105orSo1g= Received: by 10.66.236.13 with SMTP id j13mr4503496ugh.1178483589700; Sun, 06 May 2007 13:33:09 -0700 (PDT) Received: from ?151.75.242.78? ( [151.75.242.78]) by mx.google.com with ESMTP id b30sm3325ika.2007.05.06.13.33.05; Sun, 06 May 2007 13:33:06 -0700 (PDT) Message-ID: <463EACD7.8000905@FreeBSD.org> Date: Mon, 07 May 2007 06:36:39 +0200 From: Attilio Rao User-Agent: Thunderbird 1.5 (X11/20060526) MIME-Version: 1.0 To: Rui Paulo References: <200705062022.l46KMIud093057@repoman.freebsd.org> In-Reply-To: <200705062022.l46KMIud093057@repoman.freebsd.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: Attilio Rao Cc: Perforce Change Reviews Subject: Re: PERFORCE change 119366 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: attilio@FreeBSD.org List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 06 May 2007 20:33:12 -0000 Rui Paulo wrote: > http://perforce.freebsd.org/chv.cgi?CH=119366 > > Change 119366 by rpaulo@rpaulo_epsilon on 2007/05/06 20:21:17 > > Final changes for this driver. > * Use dev.cpu.N.temperature instead of creating new OIDs in hw. > * Use sched_bind/unbind when reading MSRs or when doing a > cpuid. > * The sysctl should be read only, not read-write. > > Affected files ... > > .. //depot/projects/soc2007/rpaulo-macbook/dev/msrtemp/msrtemp.c#3 edit > > Differences ... > > ==== //depot/projects/soc2007/rpaulo-macbook/dev/msrtemp/msrtemp.c#3 (text+ko) ==== > > @@ -23,7 +23,7 @@ > * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > * POSSIBILITY OF SUCH DAMAGE. > * > - * $P4: //depot/projects/soc2007/rpaulo-macbook/dev/msrtemp/msrtemp.c#2 $ > + * $P4: //depot/projects/soc2007/rpaulo-macbook/dev/msrtemp/msrtemp.c#3 $ > * > */ > > @@ -38,6 +38,8 @@ > #include > #include > #include > +#include /* for curthread */ > +#include > > #include > #include > @@ -46,8 +48,7 @@ > struct msrtemp_softc { > device_t sc_dev; > > - struct sysctl_ctx_list sc_sysctl_ctx; > - struct sysctl_oid *sc_sysctl_tree; > + struct sysctl_oid *sc_oid; > }; > > /* > @@ -58,7 +59,7 @@ > static int msrtemp_attach(device_t); > static int msrtemp_detach(device_t); > > -static int msrtemp_get_temp(void); > +static int msrtemp_get_temp(int); > static int msrtemp_get_temp_sysctl(SYSCTL_HANDLER_ARGS); > > static device_method_t msrtemp_methods[] = { > @@ -124,44 +125,42 @@ > static int > msrtemp_attach(device_t dev) > { > + int regs[4]; > struct msrtemp_softc *sc = device_get_softc(dev); > - struct sysctl_oid *top; > - char name[2]; > + device_t pdev; > + > + pdev = device_get_parent(dev); > > if (bootverbose) { > + mtx_lock_spin(&sched_lock); > + sched_bind(curthread, device_get_unit(dev)); > + > /* > * CPUID 0x06 returns 1 if the processor has on-die thermal > * sensors. We already checked that in the identify routine. > * EBX[0:3] contains the number of sensors. > */ > - int regs[4]; > do_cpuid(0x06, regs); > - > + > + sched_unbind(curthread); > + mtx_unlock_spin(&sched_lock); > + > device_printf(dev, "%d digital thermal sensor(s)\n", > regs[2] & 0x03); > } > device_printf(dev, "current temperature: %d degC\n", > - msrtemp_get_temp()); > + msrtemp_get_temp(device_get_unit(dev))); > > - sysctl_ctx_init(&sc->sc_sysctl_ctx); > - sc->sc_sysctl_tree = SYSCTL_ADD_NODE(&sc->sc_sysctl_ctx, > - SYSCTL_STATIC_CHILDREN(_hw), > - OID_AUTO, > - device_get_name(dev), > - CTLFLAG_RD, 0, ""); > - > - name[0] = '0' + device_get_unit(dev); > - name[1] = 0; > - > - top = SYSCTL_ADD_NODE(&sc->sc_sysctl_ctx, > - SYSCTL_CHILDREN(sc->sc_sysctl_tree), > - OID_AUTO, name, CTLFLAG_RD, 0, ""); > - > - SYSCTL_ADD_PROC(&sc->sc_sysctl_ctx, > - SYSCTL_CHILDREN(top), > - OID_AUTO, "temperature", CTLTYPE_INT | CTLFLAG_RW, > - NULL, 0, msrtemp_get_temp_sysctl, "I", > - "Current temperature in degC"); > + /* > + * Add the "temperature" MIB to dev.cpu.N. > + */ > + sc->sc_oid = SYSCTL_ADD_PROC(device_get_sysctl_ctx(pdev), > + SYSCTL_CHILDREN( > + device_get_sysctl_tree(pdev)), > + OID_AUTO, "temperature", > + CTLTYPE_INT | CTLFLAG_RD, > + dev, 0, msrtemp_get_temp_sysctl, "I", > + "Current temperature in degC"); > return 0; > } > > @@ -170,17 +169,19 @@ > { > struct msrtemp_softc *sc = device_get_softc(dev); > > - sysctl_ctx_free(&sc->sc_sysctl_ctx); > + sysctl_remove_oid(sc->sc_oid, 1, 0); > > return 0; > } > > > static int > -msrtemp_get_temp(void) > +msrtemp_get_temp(int cpu) > { > uint64_t temp; > > + mtx_lock_spin(&sched_lock); > + sched_bind(curthread, cpu); > /* > * The digital temperature reading is located at bit 16 > * of MSR_THERM_STATUS. > @@ -188,7 +189,7 @@ > * There is a bit on that MSR that indicates whether the > * temperature is valid or not. > * > - * The temperature is located by subtracting the temperature > + * The temperature is computed by subtracting the temperature > * reading by Tj(max). > * > * 100 is Tj(max). On some CPUs it should be 85, but Intel > @@ -196,6 +197,9 @@ > * 100 degC for everyone. > */ > temp = rdmsr(MSR_THERM_STATUS); > + > + sched_unbind(curthread); > + mtx_unlock_spin(&sched_lock); > > /* > * Bit 31 contains "Reading valid" > @@ -216,8 +220,9 @@ > msrtemp_get_temp_sysctl(SYSCTL_HANDLER_ARGS) > { > int temp; > + device_t dev = (device_t) arg1; > > - temp = msrtemp_get_temp(); > + temp = msrtemp_get_temp(device_get_unit(dev)); > > return sysctl_handle_int(oidp, &temp, 0, req); > } Being more specific, you don't need lock at all there. wrmsr/cpuid are atomic. So you don't need to hang at all there. Attilio