From owner-freebsd-bugs@FreeBSD.ORG Sat Jan 5 15:50:04 2008 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8B59A16A4A7 for ; Sat, 5 Jan 2008 15:50:04 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 95E4713C458 for ; Sat, 5 Jan 2008 15:50:04 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.14.2/8.14.2) with ESMTP id m05Fo4ld024091 for ; Sat, 5 Jan 2008 15:50:04 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.2/8.14.1/Submit) id m05Fo4ls024090; Sat, 5 Jan 2008 15:50:04 GMT (envelope-from gnats) Date: Sat, 5 Jan 2008 15:50:04 GMT Message-Id: <200801051550.m05Fo4ls024090@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Takeharu KATO Cc: Subject: Re: kern/119350: cpufreq(est) reports invalid frequency. X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Takeharu KATO List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Jan 2008 15:50:04 -0000 The following reply was made to PR kern/119350; it has been noted by GNATS. From: Takeharu KATO To: bug-followup@FreeBSD.org, takeharu1219@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); }