Date: Mon, 07 May 2007 06:36:39 +0200 From: Attilio Rao <attilio@FreeBSD.org> To: Rui Paulo <rpaulo@FreeBSD.org> Cc: Perforce Change Reviews <perforce@FreeBSD.org> Subject: Re: PERFORCE change 119366 for review Message-ID: <463EACD7.8000905@FreeBSD.org> In-Reply-To: <200705062022.l46KMIud093057@repoman.freebsd.org> References: <200705062022.l46KMIud093057@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/conf.h> > #include <sys/kernel.h> > #include <sys/sysctl.h> > +#include <sys/proc.h> /* for curthread */ > +#include <sys/sched.h> > > #include <machine/specialreg.h> > #include <machine/cpufunc.h> > @@ -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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?463EACD7.8000905>