PERFORCE change 119366 for review

[Attilio Rao]

Rui Paulo wrote:
> http://perforce.freebsd.org/chv.cgi?CH=119366
> 
> Change 119366 by rpaulo at 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