kern/119350: cpufreq(est) reports invalid frequency.

[Takeharu KATO]

The following reply was made to PR kern/119350; it has been noted by GNATS.

From: Takeharu KATO <takeharu1219 at ybb.ne.jp>
To: bug-followup at FreeBSD.org,  takeharu1219 at ybb.ne.jp
Cc:  
Subject: Re: kern/119350: cpufreq(est) reports invalid frequency.
Date: Sun, 06 Jan 2008 00:14:37 +0900

 Hi,
 
 I sent wrong patch by mistake, so I re-post correct patch.
 
 
 --- sys.orig2/i386/cpufreq/est.c	2006-05-11 17:35:44.000000000 +0000
 +++ sys/i386/cpufreq/est.c	2008-01-05 18:39:22.022325881 +0000
 @@ -902,6 +902,8 @@
  static int	est_set(device_t dev, const struct cf_setting *set);
  static int	est_get(device_t dev, struct cf_setting *set);
  static int	est_type(device_t dev, int *type);
 +static int	est_set_id16(uint16_t id16, int need_check);
 +static void	est_get_id16(uint16_t *id16_p);
  
  static device_method_t est_methods[] = {
  	/* Device interface */
 @@ -1068,7 +1070,6 @@
  
  	return (0);
  }
 -
  static int
  est_acpi_info(device_t dev, freq_info **freqs)
  {
 @@ -1076,7 +1077,8 @@
  	struct cf_setting *sets;
  	freq_info *table;
  	device_t perf_dev;
 -	int count, error, i;
 +	int count, error, i, idx;
 +	uint16_t saved_id16;
  
  	perf_dev = device_find_child(device_get_parent(dev), "acpi_perf", -1);
  	if (perf_dev == NULL || !device_is_attached(perf_dev))
 @@ -1098,19 +1100,39 @@
  		error = ENOMEM;
  		goto out;
  	}
 -	for (i = 0; i < count; i++) {
 +	for (i = 0, idx = 0; i < count; i++) {
  		/*
 -		 * TODO: Figure out validity checks for id16.  Linux checks
 -		 * that the control and status values match.
 +		 * Confirm id16 value is correct.
  		 */
 -		table[i].freq = sets[i].freq;
 -		table[i].volts = sets[i].volts;
 -		table[i].id16 = sets[i].spec[0];
 -		table[i].power = sets[i].power;
 +		if (sets[i].freq > 0) {
 +
 +			/* save current value  */
 +			est_get_id16(&saved_id16);
 +
 +			/* 
 +			 * Try to set specified value 
 +			 */
 +			error = est_set_id16(sets[i].spec[0], 1);
 +			if (error != 0) {
 +				if (bootverbose) 
 +					device_printf(dev, 
 +					    "Invalid freq %u, ignored.\n", 
 +					    sets[i].freq);
 +			} else {
 +				table[idx].freq = sets[i].freq;
 +				table[idx].volts = sets[i].volts;
 +				table[idx].id16 = sets[i].spec[0];
 +				table[idx].power = sets[i].power;
 +				++idx;
 +			}
 +
 +			/* restore saved setting */
 +			est_set_id16(sets[i].spec[0], 0); 
 +		}
  	}
  
  	/* Mark end of table with a terminator. */
 -	bzero(&table[i], sizeof(freq_info));
 +	bzero(&table[idx], sizeof(freq_info));
  
  	sc->acpi_settings = TRUE;
  	*freqs = table;
 @@ -1148,6 +1170,37 @@
  	*freqs = p->freqtab;
  	return (0);
  }
 +static void
 +est_get_id16(uint16_t *id16_p) {
 +	*id16_p = rdmsr(MSR_PERF_STATUS) & 0xffff;
 +}
 +
 +static int
 +est_set_id16(uint16_t id16, int need_check) {
 +	uint64_t msr;
 +	uint16_t new_id16;
 +	int rc = 0;
 +
 +	/*
 +	 * Try to set freq.
 +	 */
 +
 +	/* Read the current register, mask out the old, set the new id. */
 +	msr = rdmsr(MSR_PERF_CTL);
 +	msr = (msr & ~0xffff) | id16;
 +	wrmsr(MSR_PERF_CTL, msr);
 +
 +	/* Wait a short while for the new setting.  XXX Is this necessary? */
 +	DELAY(EST_TRANS_LAT);
 +
 +	if  (need_check) {
 +		est_get_id16(&new_id16);		
 +		if (new_id16 != id16) 
 +			rc = ENXIO; /* Can not set this value */
 +	}
 +
 +	return (rc);
 +}
  
  static freq_info *
  est_get_current(freq_info *freq_list)
 @@ -1162,7 +1215,7 @@
  	 * we get a temporary invalid result.
  	 */
  	for (i = 0; i < 5; i++) {
 -		id16 = rdmsr(MSR_PERF_STATUS) & 0xffff;
 +		est_get_id16(&id16);
  		for (f = freq_list; f->id16 != 0; f++) {
  			if (f->id16 == id16)
  				return (f);
 @@ -1201,7 +1254,6 @@
  {
  	struct est_softc *sc;
  	freq_info *f;
 -	uint64_t msr;
  
  	/* Find the setting matching the requested one. */
  	sc = device_get_softc(dev);
 @@ -1213,12 +1265,7 @@
  		return (EINVAL);
  
  	/* Read the current register, mask out the old, set the new id. */
 -	msr = rdmsr(MSR_PERF_CTL);
 -	msr = (msr & ~0xffff) | f->id16;
 -	wrmsr(MSR_PERF_CTL, msr);
 -
 -	/* Wait a short while for the new setting.  XXX Is this necessary? */
 -	DELAY(EST_TRANS_LAT);
 +	est_set_id16(f->id16, 0);
  
  	return (0);
  }