Messages in this thread | | | Date | Fri, 4 Sep 2020 10:36:04 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH] cpufreq,cppc: fix issue when hotplugging out policy->cpu |
| |
On 03-09-20, 12:19, Ionela Voinescu wrote: > An issue is observed in the cpufreq CPPC driver when having dependency > domains (PSD) and the policy->cpu is hotplugged out. > > Considering a platform with 4 CPUs and 2 PSD domains (CPUs 0 and 1 in > PSD-1, CPUs 2 and 3 in PSD-2), cppc_cpufreq_cpu_init() will be called > for the two cpufreq policies that are created and it will set > all_cpu_data[policy->cpu]->cpu = policy->cpu. > > Therefore all_cpu_data[0]->cpu=0, and all_cpu_data[2]->cpu=2. But for > CPUs 1 and 3, all_cpu_data[{1,3}]->cpu will remain 0 from the structure > allocation. > > If CPU 2 is hotplugged out, CPU 3 will become policy->cpu. But its > all_cpu_data[3]->cpu will remain 0. Later, when the .target() function > is called for policy2, the cpu argument to cppc_set_perf() will be 0 and > therefore it will use the performance controls of CPU 0, which will > result in a performance level change for the wrong domain. > > While the possibility of setting a correct CPU value in the per-cpu > cppc_cpudata structure is available, it can be noticed that this cpu value > is not used at all outside the .target() function, where it's not actually > necessary. Therefore, remove the cpu variable from the cppc_cpudata > structure and use policy->cpu in the .target() function as done for the > other CPPC cpufreq functions. > > Fixes: 5477fb3bd1e8 ("ACPI / CPPC: Add a CPUFreq driver for use with CPPC") > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > > Testing was done on a Juno R2 platform (with the proper ACPI/CPPC setup): > CPUs 0, 1, 2, 3 are in PSD-0 (policy0), CPUs 4 and 5 are in PSD-4 > (policy4). > > Before the fix: > > root@sqwt-ubuntu:~# dmesg | grep address > [ 2.165177] ACPI CPPC: ACPI desired perf address 0: - ffff80001004d200 > [ 2.174226] ACPI CPPC: ACPI desired perf address 1: - ffff800010055200 > [ 2.183231] ACPI CPPC: ACPI desired perf address 2: - ffff80001005d200 > [ 2.192234] ACPI CPPC: ACPI desired perf address 3: - ffff800010065200 > [ 2.201245] ACPI CPPC: ACPI desired perf address 4: - ffff80001006d218 > [ 2.210256] ACPI CPPC: ACPI desired perf address 5: - ffff800011ff1218 > [..] > [ 2.801940] ACPI CPPC: Writing to address for CPU 0:ffff80001004d200: 38300 > [ 2.835286] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 102400 > [..] > root@sqwt-ubuntu:~# cd /sys/devices/system/cpu/cpufreq/ > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 600000 > policy4/scaling_setspeed > [ 72.098758] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 51200 > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 1200000 > policy4/scaling_setspeed > [ 85.430645] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 102400 > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 0 > ../cpu4/online > [ 102.606380] CPPC Cpufreq:CPPC: Calculate: (6285/261)*4266=102727. > [ 102.612491] CPPC Cpufreq:CPPC: Core rate = 1203832, arch timer rate: 50000000 > [ 102.619659] ACPI CPPC: Writing to address for CPU 0:ffff80001004d200: 102400 > [ 102.626898] CPU4: shutdown > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 600000 > policy4/scaling_setspeed > [ 141.116882] ACPI CPPC: Writing to address for CPU 0:ffff80001004d200: 51200 > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 1200000 > policy4/scaling_setspeed > [ 159.288273] ACPI CPPC: Writing to address for CPU 0:ffff80001004d200: 102400 > > > After the fix: > > root@sqwt-ubuntu:~# cd /sys/devices/system/cpu/cpufreq/ > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 600000 > policy4/scaling_setspeed > [ 139.903322] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 51200 > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 1200000 > policy4/scaling_setspeed > [ 147.279040] ACPI CPPC: Writing to address for CPU 4:ffff80001006d218: 102400 > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 0 > ../cpu4/online > [ 153.598686] CPPC Cpufreq:CPPC: Calculate: (6171/253)*4266=104053. > [ 153.604797] CPPC Cpufreq:CPPC: Core rate = 1219371, arch timer rate: 50000000 > [ 153.611960] ACPI CPPC: Writing to address for CPU 5:ffff800011ff1218: 102400 > [ 153.619190] CPU4: shutdown > [ 153.621911] psci: CPU4 killed (polled 0 ms) > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 600000 > policy4/scaling_setspeed > [ 170.122495] ACPI CPPC: Writing to address for CPU 5:ffff800011ff1218: 51200 > root@sqwt-ubuntu:/sys/devices/system/cpu/cpufreq# echo 1200000 > policy4/scaling_setspeed > [ 177.206342] ACPI CPPC: Writing to address for CPU 5:ffff800011ff1218: 102400 > > Thanks, > Ionela. > > drivers/cpufreq/cppc_cpufreq.c | 8 ++++---- > include/acpi/cppc_acpi.h | 1 - > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index f29e8d0553a8..54457f5fe49e 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -149,8 +149,9 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy, > unsigned int target_freq, > unsigned int relation) > { > - struct cppc_cpudata *cpu; > struct cpufreq_freqs freqs; > + int cpu_num = policy->cpu; > + struct cppc_cpudata *cpu; > u32 desired_perf; > int ret = 0; > > @@ -166,12 +167,12 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy, > freqs.new = target_freq; > > cpufreq_freq_transition_begin(policy, &freqs); > - ret = cppc_set_perf(cpu->cpu, &cpu->perf_ctrls); > + ret = cppc_set_perf(cpu_num, &cpu->perf_ctrls); > cpufreq_freq_transition_end(policy, &freqs, ret != 0); > > if (ret) > pr_debug("Failed to set target on CPU:%d. ret:%d\n", > - cpu->cpu, ret); > + cpu_num, ret); > > return ret; > } > @@ -247,7 +248,6 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) > > cpu = all_cpu_data[policy->cpu]; > > - cpu->cpu = cpu_num; > ret = cppc_get_perf_caps(policy->cpu, &cpu->perf_caps); > > if (ret) { > diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h > index a6a9373ab863..451132ec83c9 100644 > --- a/include/acpi/cppc_acpi.h > +++ b/include/acpi/cppc_acpi.h > @@ -124,7 +124,6 @@ struct cppc_perf_fb_ctrs { > > /* Per CPU container for runtime CPPC management. */ > struct cppc_cpudata { > - int cpu; > struct cppc_perf_caps perf_caps; > struct cppc_perf_ctrls perf_ctrls; > struct cppc_perf_fb_ctrs perf_fb_ctrs;
With the way things are designed, I believe this is one of the bugs out of many.
The structure cppc_cpudata must be shared across all CPUs of the same policy, so they all end up using the same set of values for different variables. i.e. it shouldn't be a per-cpu thing at all. Just allocate it from cpufreq_driver->init and store in policy->driver_data for use elsewhere.
That would be a proper fix IMO, we just avoided one of the bugs here otherwise.
-- viresh
| |