From 46a310b80bc2c9ccc019649c9da91194cbc10944 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Thu, 16 Jun 2011 15:36:38 -0400 Subject: [CPUFREQ] Don't set stat->last_index to -1 if the pol->cur has incorrect value. If the driver submitted an non-existing pol>cur value (say it used the default initialized value of zero), when the cpufreq stats tries to setup its initial values it incorrectly sets stat->last_index to -1 (or 0xfffff...). And cpufreq_stats_update tries to update at that index location and fails. This can be caused by: stat->last_index = freq_table_get_index(stat, policy->cur); not finding the appropiate frequency in the table (b/c the policy->cur is wrong) and we end up crashing. The fix however is concentrated in the 'cpufreq_stats_update' as the last_index (and old_index) are updated there. Which means it can reset the last_index to -1 again and on the next iteration cause a crash. Without this patch, the following crash is observed: powernow-k8: Found 1 AMD Athlon(tm) 64 Processor 3700+ (1 cpu cores) (version 2.20.00) powernow-k8: fid 0x2 (1000 MHz), vid 0x12 powernow-k8: fid 0xa (1800 MHz), vid 0xa powernow-k8: fid 0xc (2000 MHz), vid 0x8 powernow-k8: fid 0xe (2200 MHz), vid 0x8 Marking TSC unstable due to cpufreq changes powernow-k8: fid trans failed, fid 0x2, curr 0x0 BUG: unable to handle kernel paging request at ffff880807e07b78 IP: [] cpufreq_stats_update+0x46/0x5b .. snip.. Pid: 1, comm: swapper Not tainted 3.0.0-rc2 #45 MICRO-STAR INTERNATIONAL CO., LTD MS-7094/MS-7094 ..snip.. Call Trace: [] cpufreq_stat_notifier_trans+0x48/0x7c [] notifier_call_chain+0x32/0x5e [] __srcu_notifier_call_chain+0x47/0x63 [] srcu_notifier_call_chain+0xf/0x11 [] cpufreq_notify_transition+0x111/0x134 [] powernowk8_target+0x53b/0x617 [] __cpufreq_driver_target+0x2e/0x30 [] cpufreq_governor_dbs+0x339/0x356 [] __cpufreq_governor+0xa8/0xe9 [] __cpufreq_set_policy+0x132/0x13e [] cpufreq_add_dev_interface+0x272/0x28c Reported-by: Tobias Diedrich Tested-by: Tobias Diedrich Signed-off-by: Konrad Rzeszutek Wilk Signed-off-by: Dave Jones --- drivers/cpufreq/cpufreq_stats.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'drivers') diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index 853f92d23dd..faf7c521784 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -298,11 +298,13 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb, old_index = stat->last_index; new_index = freq_table_get_index(stat, freq->new); - cpufreq_stats_update(freq->cpu); - if (old_index == new_index) + /* We can't do stat->time_in_state[-1]= .. */ + if (old_index == -1 || new_index == -1) return 0; - if (old_index == -1 || new_index == -1) + cpufreq_stats_update(freq->cpu); + + if (old_index == new_index) return 0; spin_lock(&cpufreq_stats_lock); -- cgit v1.2.3 From a9d3d2068064b7a6395871a49616d3784f802d50 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Thu, 16 Jun 2011 15:36:39 -0400 Subject: [CPUFREQ] powernow-k8: Don't notify of successful transition if we failed (vid case). Before this patch if we failed the vid transition would still try to submit the "new" frequencies to cpufreq. That is incorrect - also we could submit a non-existing frequency value which would cause cpufreq to crash. The ultimate fix is in cpufreq to deal with incorrect values, but this patch improves the error recovery in the AMD powernowk8 driver. The failure that was reported was as follows: powernow-k8: Found 1 AMD Athlon(tm) 64 Processor 3700+ (1 cpu cores) (version 2.20.00) powernow-k8: fid 0x2 (1000 MHz), vid 0x12 powernow-k8: fid 0xa (1800 MHz), vid 0xa powernow-k8: fid 0xc (2000 MHz), vid 0x8 powernow-k8: fid 0xe (2200 MHz), vid 0x8 Marking TSC unstable due to cpufreq changes powernow-k8: fid trans failed, fid 0x2, curr 0x0 BUG: unable to handle kernel paging request at ffff880807e07b78 IP: [] cpufreq_stats_update+0x46/0x5b ... And transition fails and data->currfid ends up with 0. Since the machine does not support 800Mhz value when the calculation is done ('find_khz_freq_from_fid(data->currfid);') it reports the new frequency as 800000 which is bogus. This patch fixes the issue during target setting. The patch however does not fix the issue in 'powernowk8_cpu_init' where the pol->cur can also be set with the 800000 value: pol->cur = find_khz_freq_from_fid(data->currfid); dprintk("policy current frequency %d kHz\n", pol->cur); /* min/max the cpu is capable of */ if (cpufreq_frequency_table_cpuinfo(pol, data->powernow_table)) { The fix for that looks to update cpufreq_frequency_table_cpuinfo to check pol->cur.... but that would cause an regression in how the acpi-cpufreq driver works (it sets cpu->cur after calling cpufreq_frequency_table_cpuinfo). Instead the fix will be to let cpufreq gracefully handle bogus data (another patch). Acked-by: Borislav Petkov CC: andre.przywara@amd.com CC: Mark.Langsdorf@amd.com Reported-by: Tobias Diedrich Tested-by: Tobias Diedrich [v1: Rebased on v3.0-rc2, reduced patch to deal with vid case] Signed-off-by: Konrad Rzeszutek Wilk Signed-off-by: Dave Jones --- drivers/cpufreq/powernow-k8.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers') diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c index 83479b6fb9a..287c56f6749 100644 --- a/drivers/cpufreq/powernow-k8.c +++ b/drivers/cpufreq/powernow-k8.c @@ -1079,6 +1079,9 @@ static int transition_frequency_fidvid(struct powernow_k8_data *data, } res = transition_fid_vid(data, fid, vid); + if (res) + return res; + freqs.new = find_khz_freq_from_fid(data->currfid); for_each_cpu(i, data->available_cores) { -- cgit v1.2.3 From fbb5b89eabea5ae7d621b7861863159560d8faa4 Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk Date: Thu, 16 Jun 2011 15:36:40 -0400 Subject: [CPUFREQ] powernow-k8: Don't try to transition if the pstate is incorrect This patch augments the pstate transition code to error out (instead of returning 0) when an incorrect pstate is provided. Suggested-by: Borislav Petkov CC: andre.przywara@amd.com CC: Mark.Langsdorf@amd.com Signed-off-by: Konrad Rzeszutek Wilk Signed-off-by: Dave Jones --- drivers/cpufreq/powernow-k8.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers') diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c index 287c56f6749..bce576d7478 100644 --- a/drivers/cpufreq/powernow-k8.c +++ b/drivers/cpufreq/powernow-k8.c @@ -1104,7 +1104,8 @@ static int transition_frequency_pstate(struct powernow_k8_data *data, /* get MSR index for hardware pstate transition */ pstate = index & HW_PSTATE_MASK; if (pstate > data->max_hw_pstate) - return 0; + return -EINVAL; + freqs.old = find_khz_freq_from_pstate(data->powernow_table, data->currpstate); freqs.new = find_khz_freq_from_pstate(data->powernow_table, pstate); -- cgit v1.2.3