| Message ID | 20201105125524.4409-5-ionela.voinescu@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/8] cppc_cpufreq: fix misspelling, code style and readability issues | expand |
Hi, On 11/5/20 6:55 AM, Ionela Voinescu wrote: > The cppc_cpudata per-cpu storage was inefficient (1) additional to causing > functional issues (2) when CPUs are hotplugged out, due to per-cpu data > being improperly initialised. > > (1) The amount of information needed for CPPC performance control in its > cpufreq driver depends on the domain (PSD) coordination type: > > ANY: One set of CPPC control and capability data (e.g desired > performance, highest/lowest performance, etc) applies to all > CPUs in the domain. > > ALL: Same as ANY. To be noted that this type is not currently > supported. When supported, information about which CPUs > belong to a domain is needed in order for frequency change > requests to be sent to each of them. > > HW: It's necessary to store CPPC control and capability > information for all the CPUs. HW will then coordinate the > performance state based on their limitations and requests. > > NONE: Same as HW. No HW coordination is expected. > > Despite this, the previous initialisation code would indiscriminately > allocate memory for all CPUs (all_cpu_data) and unnecessarily > duplicate performance capabilities and the domain sharing mask and type > for each possible CPU. I should have mentioned this on the last set. If the common case on arm/acpi machines is a single core per _PSD (which I believe it is), then you are actually increasing the overhead doing this. > > (2) With the current per-cpu structure, when having ANY coordination, > the cppc_cpudata cpu information is not initialised (will remain 0) > for all CPUs in a policy, other than policy->cpu. When policy->cpu is > hotplugged out, the driver will incorrectly use the uninitialised (0) > value of the other CPUs when making frequency changes. Additionally, > the previous values stored in the perf_ctrls.desired_perf will be > lost when policy->cpu changes. > > Due to the current storage of driver data not being fit for purpose, > replace the per-cpu CPPC/PSD data with a list of domains which in turn > will point to a list of CPUs with their controls and capabilities, > belonging to the domain. > > This new structure has the following benefits: > - Clearly separates PSD (domain) data from CPPC (performance monitoring > and control) data and stores one mask of CPUs belonging to a domain > and its type in a single structure, for each domain. > > - For ANY domain coordination, a single cppc_cpudata set of capabilities > and controls are stored, for policy->cpu, and used for describing > performance controls and capabilities even when policy->cpu changes as > a result of hotplug. All the CPUs in the domain will belong to the > same policy. > > - For HW or lack of coordination, a full map of domains and CPUs is > obtained. Each CPU will have its own policy, but for HW, as a result > of PSD information, a shared_cpu_map mask could be used to identify > the domain CPU content. > > Additional changes: > > - A pointer to the policy's cppc_cpudata is stored in policy->driver_data > > - All data allocation is done from the driver's init function. At that > point the domain is either created or retrieved. For this purpose > acpi_get_psd_map() was changed to create a map of the domain of a > single CPU, rather than the full system domain map. > > - When CPU's are hotplugged out and in, old information can be retrieved. > > Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/acpi/cppc_acpi.c | 126 +++++++------------ > drivers/cpufreq/cppc_cpufreq.c | 217 ++++++++++++++++++++------------- > include/acpi/cppc_acpi.h | 14 ++- > 3 files changed, 190 insertions(+), 167 deletions(-) > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > index 7a99b19bb893..75e36b909ae6 100644 > --- a/drivers/acpi/cppc_acpi.c > +++ b/drivers/acpi/cppc_acpi.c > @@ -414,108 +414,72 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) > } > > /** > - * acpi_get_psd_map - Map the CPUs in a common freq domain. > - * @all_cpu_data: Ptrs to CPU specific CPPC data including PSD info. > + * acpi_get_psd_map - Map the CPUs in the freq domain of a given cpu > + * @cpu: Find all CPUs that share a domain with cpu. > + * @domain: Pointer to given domain data storage > * > * Return: 0 for success or negative value for err. > */ > -int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data) > +int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain) > { > - int count_target; > - int retval = 0; > - unsigned int i, j; > - cpumask_var_t covered_cpus; > - struct cppc_cpudata *pr, *match_pr; > - struct acpi_psd_package *pdomain; > - struct acpi_psd_package *match_pdomain; > struct cpc_desc *cpc_ptr, *match_cpc_ptr; > - > - if (!zalloc_cpumask_var(&covered_cpus, GFP_KERNEL)) > - return -ENOMEM; > + struct acpi_psd_package *match_pdomain; > + struct acpi_psd_package *pdomain; > + int count_target, i; > > /* > * Now that we have _PSD data from all CPUs, let's setup P-state > * domain info. > */ > - for_each_possible_cpu(i) { > - if (cpumask_test_cpu(i, covered_cpus)) > - continue; > + cpc_ptr = per_cpu(cpc_desc_ptr, cpu); > + if (!cpc_ptr) > + return -EFAULT; > > - pr = all_cpu_data[i]; > - cpc_ptr = per_cpu(cpc_desc_ptr, i); > - if (!cpc_ptr) { > - retval = -EFAULT; > - goto err_ret; > - } > + pdomain = &(cpc_ptr->domain_info); > + cpumask_set_cpu(cpu, domain->shared_cpu_map); > + if (pdomain->num_processors <= 1) > + return 0; > > - pdomain = &(cpc_ptr->domain_info); > - cpumask_set_cpu(i, pr->shared_cpu_map); > - cpumask_set_cpu(i, covered_cpus); > - if (pdomain->num_processors <= 1) > - continue; > + /* Validate the Domain info */ > + count_target = pdomain->num_processors; > + if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL) > + domain->shared_type = CPUFREQ_SHARED_TYPE_ALL; > + else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL) > + domain->shared_type = CPUFREQ_SHARED_TYPE_HW; > + else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY) > + domain->shared_type = CPUFREQ_SHARED_TYPE_ANY; > > - /* Validate the Domain info */ > - count_target = pdomain->num_processors; > - if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL) > - pr->shared_type = CPUFREQ_SHARED_TYPE_ALL; > - else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL) > - pr->shared_type = CPUFREQ_SHARED_TYPE_HW; > - else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY) > - pr->shared_type = CPUFREQ_SHARED_TYPE_ANY; > - > - for_each_possible_cpu(j) { > - if (i == j) > - continue; > - > - match_cpc_ptr = per_cpu(cpc_desc_ptr, j); > - if (!match_cpc_ptr) { > - retval = -EFAULT; > - goto err_ret; > - } > - > - match_pdomain = &(match_cpc_ptr->domain_info); > - if (match_pdomain->domain != pdomain->domain) > - continue; > + for_each_possible_cpu(i) { > + if (i == cpu) > + continue; > > - /* Here i and j are in the same domain */ > - if (match_pdomain->num_processors != count_target) { > - retval = -EFAULT; > - goto err_ret; > - } > + match_cpc_ptr = per_cpu(cpc_desc_ptr, i); > + if (!match_cpc_ptr) > + goto err_fault; > > - if (pdomain->coord_type != match_pdomain->coord_type) { > - retval = -EFAULT; > - goto err_ret; > - } > + match_pdomain = &(match_cpc_ptr->domain_info); > + if (match_pdomain->domain != pdomain->domain) > + continue; > > - cpumask_set_cpu(j, covered_cpus); > - cpumask_set_cpu(j, pr->shared_cpu_map); > - } > + /* Here i and cpu are in the same domain */ > + if (match_pdomain->num_processors != count_target) > + goto err_fault; > > - for_each_cpu(j, pr->shared_cpu_map) { > - if (i == j) > - continue; > + if (pdomain->coord_type != match_pdomain->coord_type) > + goto err_fault; > > - match_pr = all_cpu_data[j]; > - match_pr->shared_type = pr->shared_type; > - cpumask_copy(match_pr->shared_cpu_map, > - pr->shared_cpu_map); > - } > + cpumask_set_cpu(i, domain->shared_cpu_map); > } > - goto out; > > -err_ret: > - for_each_possible_cpu(i) { > - pr = all_cpu_data[i]; > + return 0; > > - /* Assume no coordination on any error parsing domain info */ > - cpumask_clear(pr->shared_cpu_map); > - cpumask_set_cpu(i, pr->shared_cpu_map); > - pr->shared_type = CPUFREQ_SHARED_TYPE_ALL; > - } > -out: > - free_cpumask_var(covered_cpus); > - return retval; > +err_fault: > + /* Assume no coordination on any error parsing domain info */ > + cpumask_clear(domain->shared_cpu_map); > + cpumask_set_cpu(cpu, domain->shared_cpu_map); > + domain->shared_type = CPUFREQ_SHARED_TYPE_ALL; > + > + return -EFAULT; > } > EXPORT_SYMBOL_GPL(acpi_get_psd_map); > > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c > index 7cc9bd8568de..ac95b4424a2e 100644 > --- a/drivers/cpufreq/cppc_cpufreq.c > +++ b/drivers/cpufreq/cppc_cpufreq.c > @@ -30,13 +30,12 @@ > #define DMI_PROCESSOR_MAX_SPEED 0x14 > > /* > - * These structs contain information parsed from per CPU > - * ACPI _CPC structures. > - * e.g. For each CPU the highest, lowest supported > - * performance capabilities, desired performance level > - * requested etc. > + * This list contains information parsed from per CPU ACPI _CPC and _PSD > + * structures: this is a list of domain data which in turn contains a list > + * of cpus with their controls and capabilities, belonging to the domain. > */ > -static struct cppc_cpudata **all_cpu_data; > +static LIST_HEAD(domain_list); > + > static bool boost_supported; > > struct cppc_workaround_oem_info { > @@ -148,8 +147,9 @@ static unsigned int cppc_cpufreq_khz_to_perf(struct cppc_cpudata *cpu_data, > static int cppc_cpufreq_set_target(struct cpufreq_policy *policy, > unsigned int target_freq, > unsigned int relation) > + > { > - struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu]; > + struct cppc_cpudata *cpu_data = policy->driver_data; > struct cpufreq_freqs freqs; > u32 desired_perf; > int ret = 0; > @@ -182,7 +182,7 @@ static int cppc_verify_policy(struct cpufreq_policy_data *policy) > > static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy) > { > - struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu]; > + struct cppc_cpudata *cpu_data = policy->driver_data; > struct cppc_perf_caps *caps = &cpu_data->perf_caps; > unsigned int cpu = policy->cpu; > int ret; > @@ -238,25 +238,107 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu) > } > #endif > > -static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) > +static struct psd_data *cppc_cpufreq_get_domain(unsigned int cpu) > { > - struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu]; > - struct cppc_perf_caps *caps = &cpu_data->perf_caps; > - unsigned int cpu = policy->cpu; > - int ret = 0; > + struct psd_data *domain; > + int ret; > > - cpu_data->cpu = cpu; > - ret = cppc_get_perf_caps(cpu, caps); > + list_for_each_entry(domain, &domain_list, node) { > + if (cpumask_test_cpu(cpu, domain->shared_cpu_map)) > + return domain; > + } > > + domain = kzalloc(sizeof(struct psd_data), GFP_KERNEL); > + if (!domain) > + return NULL; > + if (!zalloc_cpumask_var(&domain->shared_cpu_map, GFP_KERNEL)) > + goto free_domain; > + INIT_LIST_HEAD(&domain->cpu_list); > + > + ret = acpi_get_psd_map(cpu, domain); > if (ret) { > - pr_debug("Err reading CPU%d perf capabilities. ret:%d\n", > - cpu, ret); > - return ret; > + pr_err("Error parsing PSD data for CPU%d.\n", cpu); > + goto free_mask; > + } > + > + list_add(&domain->node, &domain_list); > + > + return domain; > + > +free_mask: > + free_cpumask_var(domain->shared_cpu_map); > +free_domain: > + kfree(domain); > + > + return NULL; > +} > + > +static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu) > +{ > + struct cppc_cpudata *cpu_data; > + struct psd_data *domain; > + int ret; > + > + domain = cppc_cpufreq_get_domain(cpu); > + if (!domain) { > + pr_err("Error acquiring domain for CPU.\n"); > + return NULL; > } > > + list_for_each_entry(cpu_data, &domain->cpu_list, node) { > + if (cpu_data->cpu == cpu) > + return cpu_data; > + } > + > + if ((domain->shared_type == CPUFREQ_SHARED_TYPE_ANY) && > + !list_empty(&domain->cpu_list)) > + return list_first_entry(&domain->cpu_list, > + struct cppc_cpudata, > + node); > + > + cpu_data = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL); > + if (!cpu_data) > + return NULL; > + > + cpu_data->cpu = cpu; > + cpu_data->domain = domain; > + > + ret = cppc_get_perf_caps(cpu, &cpu_data->perf_caps); > + if (ret) { > + pr_err("Err reading CPU%d perf capabilities. ret:%d\n", > + cpu, ret); > + goto free; > + } > /* Convert the lowest and nominal freq from MHz to KHz */ > - caps->lowest_freq *= 1000; > - caps->nominal_freq *= 1000; > + cpu_data->perf_caps.lowest_freq *= 1000; > + cpu_data->perf_caps.nominal_freq *= 1000; > + > + list_add(&cpu_data->node, &domain->cpu_list); > + > + return cpu_data; > +free: > + kfree(cpu_data); > + > + return NULL; > +} > + > +static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) > +{ > + struct cppc_cpudata *cpu_data = NULL; > + struct psd_data *domain = NULL; > + unsigned int cpu = policy->cpu; > + struct cppc_perf_caps *caps; > + int ret = 0; > + > + cpu_data = cppc_cpufreq_get_cpu_data(cpu); > + if (!cpu_data) { > + pr_err("Error in acquiring _CPC/_PSD data for CPU.\n"); > + return -ENODEV; > + } > + > + domain = cpu_data->domain; > + caps = &cpu_data->perf_caps; > + policy->driver_data = cpu_data; > > /* > * Set min to lowest nonlinear perf to avoid any efficiency penalty (see > @@ -278,20 +360,10 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) > caps->nominal_perf); > > policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu); > - policy->shared_type = cpu_data->shared_type; > + policy->shared_type = domain->shared_type; > > if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) { > - int i; > - > - cpumask_copy(policy->cpus, cpu_data->shared_cpu_map); > - > - for_each_cpu(i, policy->cpus) { > - if (unlikely(i == cpu)) > - continue; > - > - memcpy(&all_cpu_data[i]->perf_caps, caps, > - sizeof(cpu_data->perf_caps)); > - } > + cpumask_copy(policy->cpus, domain->shared_cpu_map); > } else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) { > /* Support only SW_ANY for now. */ > pr_debug("Unsupported CPU co-ord type\n"); > @@ -354,9 +426,12 @@ static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu_data, > static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > { > struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0}; > - struct cppc_cpudata *cpu_data = all_cpu_data[cpu]; > + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); > + struct cppc_cpudata *cpu_data = policy->driver_data; > int ret; > > + cpufreq_cpu_put(policy); > + > ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); > if (ret) > return ret; > @@ -372,7 +447,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) > > static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) > { > - struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu]; > + struct cppc_cpudata *cpu_data = policy->driver_data; > struct cppc_perf_caps *caps = &cpu_data->perf_caps; > int ret; > > @@ -415,10 +490,13 @@ static struct cpufreq_driver cppc_cpufreq_driver = { > */ > static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu) > { > - struct cppc_cpudata *cpu_data = all_cpu_data[cpu]; > + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); > + struct cppc_cpudata *cpu_data = policy->driver_data; > u64 desired_perf; > int ret; > > + cpufreq_cpu_put(policy); > + > ret = cppc_get_desired_perf(cpu, &desired_perf); > if (ret < 0) > return -EIO; > @@ -451,68 +529,41 @@ static void cppc_check_hisi_workaround(void) > > static int __init cppc_cpufreq_init(void) > { > - struct cppc_cpudata *cpu_data; > - int i, ret = 0; > - > if (acpi_disabled) > return -ENODEV; > > - all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *), > - GFP_KERNEL); > - if (!all_cpu_data) > - return -ENOMEM; > - > - for_each_possible_cpu(i) { > - all_cpu_data[i] = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL); > - if (!all_cpu_data[i]) > - goto out; > - > - cpu_data = all_cpu_data[i]; > - if (!zalloc_cpumask_var(&cpu_data->shared_cpu_map, GFP_KERNEL)) > - goto out; > - } > - > - ret = acpi_get_psd_map(all_cpu_data); > - if (ret) { > - pr_debug("Error parsing PSD data. Aborting cpufreq registration.\n"); > - goto out; > - } > - > cppc_check_hisi_workaround(); > > - ret = cpufreq_register_driver(&cppc_cpufreq_driver); > - if (ret) > - goto out; > + return cpufreq_register_driver(&cppc_cpufreq_driver); > +} > > - return ret; > +static inline void free_cpu_data(struct psd_data *domain) > +{ > + struct cppc_cpudata *iter, *tmp; > > -out: > - for_each_possible_cpu(i) { > - cpu_data = all_cpu_data[i]; > - if (!cpu_data) > - break; > - free_cpumask_var(cpu_data->shared_cpu_map); > - kfree(cpu_data); > + list_for_each_entry_safe(iter, tmp, &domain->cpu_list, node) { > + list_del(&iter->node); > + kfree(iter); > } > - > - kfree(all_cpu_data); > - return -ENODEV; > } > > +static inline void free_domain_data(void) > +{ > + struct psd_data *iter, *tmp; > + > + list_for_each_entry_safe(iter, tmp, &domain_list, node) { > + list_del(&iter->node); > + if (!list_empty(&iter->cpu_list)) > + free_cpu_data(iter); > + free_cpumask_var(iter->shared_cpu_map); > + kfree(iter); > + } > +} > static void __exit cppc_cpufreq_exit(void) > { > - struct cppc_cpudata *cpu_data; > - int i; > - > cpufreq_unregister_driver(&cppc_cpufreq_driver); > > - for_each_possible_cpu(i) { > - cpu_data = all_cpu_data[i]; > - free_cpumask_var(cpu_data->shared_cpu_map); > - kfree(cpu_data); > - } > - > - kfree(all_cpu_data); > + free_domain_data(); > } > > module_exit(cppc_cpufreq_exit); > diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h > index a6a9373ab863..c0081fb6032e 100644 > --- a/include/acpi/cppc_acpi.h > +++ b/include/acpi/cppc_acpi.h > @@ -122,22 +122,30 @@ struct cppc_perf_fb_ctrs { > u64 wraparound_time; > }; > > +/* Container of performance state domain data */ > +struct psd_data { > + struct list_head node; > + unsigned int shared_type; > + struct list_head cpu_list; > + cpumask_var_t shared_cpu_map; > +}; > + > /* Per CPU container for runtime CPPC management. */ > struct cppc_cpudata { > int cpu; > + struct list_head node; > + struct psd_data *domain; > struct cppc_perf_caps perf_caps; > struct cppc_perf_ctrls perf_ctrls; > struct cppc_perf_fb_ctrs perf_fb_ctrs; > struct cpufreq_policy *cur_policy; > - unsigned int shared_type; > - cpumask_var_t shared_cpu_map; > }; > > extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf); > extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs); > extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls); > extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps); > -extern int acpi_get_psd_map(struct cppc_cpudata **); > +extern int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain); > extern unsigned int cppc_get_transition_latency(int cpu); > extern bool cpc_ffh_supported(void); > extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val); >
Hi Jeremy, On Thursday 05 Nov 2020 at 09:50:30 (-0600), Jeremy Linton wrote: > Hi, > > On 11/5/20 6:55 AM, Ionela Voinescu wrote: > > The cppc_cpudata per-cpu storage was inefficient (1) additional to causing > > functional issues (2) when CPUs are hotplugged out, due to per-cpu data > > being improperly initialised. > > > > (1) The amount of information needed for CPPC performance control in its > > cpufreq driver depends on the domain (PSD) coordination type: > > > > ANY: One set of CPPC control and capability data (e.g desired > > performance, highest/lowest performance, etc) applies to all > > CPUs in the domain. > > > > ALL: Same as ANY. To be noted that this type is not currently > > supported. When supported, information about which CPUs > > belong to a domain is needed in order for frequency change > > requests to be sent to each of them. > > > > HW: It's necessary to store CPPC control and capability > > information for all the CPUs. HW will then coordinate the > > performance state based on their limitations and requests. > > > > NONE: Same as HW. No HW coordination is expected. > > > > Despite this, the previous initialisation code would indiscriminately > > allocate memory for all CPUs (all_cpu_data) and unnecessarily > > duplicate performance capabilities and the domain sharing mask and type > > for each possible CPU. > > I should have mentioned this on the last set. > > If the common case on arm/acpi machines is a single core per _PSD (which I > believe it is), then you are actually increasing the overhead doing this. > Thanks for taking another look and pointing this out. Yes, that would be quite inefficient as I'd be holding both CPU and domain information uselessly, for that case. I could drop the domain information without actually losing anything (shared type and shared cpu map have no purpose for single CPUs in a domain). Also, I don't actually need a list of CPUs in the domain, an array will work just as well, as I know the number of CPUs when I allocate the domain - that will allow me to remove the node from cppc_cpudata and save me some pointers. Also, I now remember I wanted to get rid of cpu and cur_policy from cppc_cpudata as well, as they serve no purpose. Let me know if you guys see a reason against this. All of this should at least bring things on par for HW and NONE types, while improving ANY and ALL types. Thanks again for bringing this up. Regards, Ionela.
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 7a99b19bb893..75e36b909ae6 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -414,108 +414,72 @@ static int acpi_get_psd(struct cpc_desc *cpc_ptr, acpi_handle handle) } /** - * acpi_get_psd_map - Map the CPUs in a common freq domain. - * @all_cpu_data: Ptrs to CPU specific CPPC data including PSD info. + * acpi_get_psd_map - Map the CPUs in the freq domain of a given cpu + * @cpu: Find all CPUs that share a domain with cpu. + * @domain: Pointer to given domain data storage * * Return: 0 for success or negative value for err. */ -int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data) +int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain) { - int count_target; - int retval = 0; - unsigned int i, j; - cpumask_var_t covered_cpus; - struct cppc_cpudata *pr, *match_pr; - struct acpi_psd_package *pdomain; - struct acpi_psd_package *match_pdomain; struct cpc_desc *cpc_ptr, *match_cpc_ptr; - - if (!zalloc_cpumask_var(&covered_cpus, GFP_KERNEL)) - return -ENOMEM; + struct acpi_psd_package *match_pdomain; + struct acpi_psd_package *pdomain; + int count_target, i; /* * Now that we have _PSD data from all CPUs, let's setup P-state * domain info. */ - for_each_possible_cpu(i) { - if (cpumask_test_cpu(i, covered_cpus)) - continue; + cpc_ptr = per_cpu(cpc_desc_ptr, cpu); + if (!cpc_ptr) + return -EFAULT; - pr = all_cpu_data[i]; - cpc_ptr = per_cpu(cpc_desc_ptr, i); - if (!cpc_ptr) { - retval = -EFAULT; - goto err_ret; - } + pdomain = &(cpc_ptr->domain_info); + cpumask_set_cpu(cpu, domain->shared_cpu_map); + if (pdomain->num_processors <= 1) + return 0; - pdomain = &(cpc_ptr->domain_info); - cpumask_set_cpu(i, pr->shared_cpu_map); - cpumask_set_cpu(i, covered_cpus); - if (pdomain->num_processors <= 1) - continue; + /* Validate the Domain info */ + count_target = pdomain->num_processors; + if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL) + domain->shared_type = CPUFREQ_SHARED_TYPE_ALL; + else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL) + domain->shared_type = CPUFREQ_SHARED_TYPE_HW; + else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY) + domain->shared_type = CPUFREQ_SHARED_TYPE_ANY; - /* Validate the Domain info */ - count_target = pdomain->num_processors; - if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ALL) - pr->shared_type = CPUFREQ_SHARED_TYPE_ALL; - else if (pdomain->coord_type == DOMAIN_COORD_TYPE_HW_ALL) - pr->shared_type = CPUFREQ_SHARED_TYPE_HW; - else if (pdomain->coord_type == DOMAIN_COORD_TYPE_SW_ANY) - pr->shared_type = CPUFREQ_SHARED_TYPE_ANY; - - for_each_possible_cpu(j) { - if (i == j) - continue; - - match_cpc_ptr = per_cpu(cpc_desc_ptr, j); - if (!match_cpc_ptr) { - retval = -EFAULT; - goto err_ret; - } - - match_pdomain = &(match_cpc_ptr->domain_info); - if (match_pdomain->domain != pdomain->domain) - continue; + for_each_possible_cpu(i) { + if (i == cpu) + continue; - /* Here i and j are in the same domain */ - if (match_pdomain->num_processors != count_target) { - retval = -EFAULT; - goto err_ret; - } + match_cpc_ptr = per_cpu(cpc_desc_ptr, i); + if (!match_cpc_ptr) + goto err_fault; - if (pdomain->coord_type != match_pdomain->coord_type) { - retval = -EFAULT; - goto err_ret; - } + match_pdomain = &(match_cpc_ptr->domain_info); + if (match_pdomain->domain != pdomain->domain) + continue; - cpumask_set_cpu(j, covered_cpus); - cpumask_set_cpu(j, pr->shared_cpu_map); - } + /* Here i and cpu are in the same domain */ + if (match_pdomain->num_processors != count_target) + goto err_fault; - for_each_cpu(j, pr->shared_cpu_map) { - if (i == j) - continue; + if (pdomain->coord_type != match_pdomain->coord_type) + goto err_fault; - match_pr = all_cpu_data[j]; - match_pr->shared_type = pr->shared_type; - cpumask_copy(match_pr->shared_cpu_map, - pr->shared_cpu_map); - } + cpumask_set_cpu(i, domain->shared_cpu_map); } - goto out; -err_ret: - for_each_possible_cpu(i) { - pr = all_cpu_data[i]; + return 0; - /* Assume no coordination on any error parsing domain info */ - cpumask_clear(pr->shared_cpu_map); - cpumask_set_cpu(i, pr->shared_cpu_map); - pr->shared_type = CPUFREQ_SHARED_TYPE_ALL; - } -out: - free_cpumask_var(covered_cpus); - return retval; +err_fault: + /* Assume no coordination on any error parsing domain info */ + cpumask_clear(domain->shared_cpu_map); + cpumask_set_cpu(cpu, domain->shared_cpu_map); + domain->shared_type = CPUFREQ_SHARED_TYPE_ALL; + + return -EFAULT; } EXPORT_SYMBOL_GPL(acpi_get_psd_map); diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c index 7cc9bd8568de..ac95b4424a2e 100644 --- a/drivers/cpufreq/cppc_cpufreq.c +++ b/drivers/cpufreq/cppc_cpufreq.c @@ -30,13 +30,12 @@ #define DMI_PROCESSOR_MAX_SPEED 0x14 /* - * These structs contain information parsed from per CPU - * ACPI _CPC structures. - * e.g. For each CPU the highest, lowest supported - * performance capabilities, desired performance level - * requested etc. + * This list contains information parsed from per CPU ACPI _CPC and _PSD + * structures: this is a list of domain data which in turn contains a list + * of cpus with their controls and capabilities, belonging to the domain. */ -static struct cppc_cpudata **all_cpu_data; +static LIST_HEAD(domain_list); + static bool boost_supported; struct cppc_workaround_oem_info { @@ -148,8 +147,9 @@ static unsigned int cppc_cpufreq_khz_to_perf(struct cppc_cpudata *cpu_data, static int cppc_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation) + { - struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu]; + struct cppc_cpudata *cpu_data = policy->driver_data; struct cpufreq_freqs freqs; u32 desired_perf; int ret = 0; @@ -182,7 +182,7 @@ static int cppc_verify_policy(struct cpufreq_policy_data *policy) static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy) { - struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu]; + struct cppc_cpudata *cpu_data = policy->driver_data; struct cppc_perf_caps *caps = &cpu_data->perf_caps; unsigned int cpu = policy->cpu; int ret; @@ -238,25 +238,107 @@ static unsigned int cppc_cpufreq_get_transition_delay_us(unsigned int cpu) } #endif -static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) +static struct psd_data *cppc_cpufreq_get_domain(unsigned int cpu) { - struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu]; - struct cppc_perf_caps *caps = &cpu_data->perf_caps; - unsigned int cpu = policy->cpu; - int ret = 0; + struct psd_data *domain; + int ret; - cpu_data->cpu = cpu; - ret = cppc_get_perf_caps(cpu, caps); + list_for_each_entry(domain, &domain_list, node) { + if (cpumask_test_cpu(cpu, domain->shared_cpu_map)) + return domain; + } + domain = kzalloc(sizeof(struct psd_data), GFP_KERNEL); + if (!domain) + return NULL; + if (!zalloc_cpumask_var(&domain->shared_cpu_map, GFP_KERNEL)) + goto free_domain; + INIT_LIST_HEAD(&domain->cpu_list); + + ret = acpi_get_psd_map(cpu, domain); if (ret) { - pr_debug("Err reading CPU%d perf capabilities. ret:%d\n", - cpu, ret); - return ret; + pr_err("Error parsing PSD data for CPU%d.\n", cpu); + goto free_mask; + } + + list_add(&domain->node, &domain_list); + + return domain; + +free_mask: + free_cpumask_var(domain->shared_cpu_map); +free_domain: + kfree(domain); + + return NULL; +} + +static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu) +{ + struct cppc_cpudata *cpu_data; + struct psd_data *domain; + int ret; + + domain = cppc_cpufreq_get_domain(cpu); + if (!domain) { + pr_err("Error acquiring domain for CPU.\n"); + return NULL; } + list_for_each_entry(cpu_data, &domain->cpu_list, node) { + if (cpu_data->cpu == cpu) + return cpu_data; + } + + if ((domain->shared_type == CPUFREQ_SHARED_TYPE_ANY) && + !list_empty(&domain->cpu_list)) + return list_first_entry(&domain->cpu_list, + struct cppc_cpudata, + node); + + cpu_data = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL); + if (!cpu_data) + return NULL; + + cpu_data->cpu = cpu; + cpu_data->domain = domain; + + ret = cppc_get_perf_caps(cpu, &cpu_data->perf_caps); + if (ret) { + pr_err("Err reading CPU%d perf capabilities. ret:%d\n", + cpu, ret); + goto free; + } /* Convert the lowest and nominal freq from MHz to KHz */ - caps->lowest_freq *= 1000; - caps->nominal_freq *= 1000; + cpu_data->perf_caps.lowest_freq *= 1000; + cpu_data->perf_caps.nominal_freq *= 1000; + + list_add(&cpu_data->node, &domain->cpu_list); + + return cpu_data; +free: + kfree(cpu_data); + + return NULL; +} + +static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) +{ + struct cppc_cpudata *cpu_data = NULL; + struct psd_data *domain = NULL; + unsigned int cpu = policy->cpu; + struct cppc_perf_caps *caps; + int ret = 0; + + cpu_data = cppc_cpufreq_get_cpu_data(cpu); + if (!cpu_data) { + pr_err("Error in acquiring _CPC/_PSD data for CPU.\n"); + return -ENODEV; + } + + domain = cpu_data->domain; + caps = &cpu_data->perf_caps; + policy->driver_data = cpu_data; /* * Set min to lowest nonlinear perf to avoid any efficiency penalty (see @@ -278,20 +360,10 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy) caps->nominal_perf); policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu); - policy->shared_type = cpu_data->shared_type; + policy->shared_type = domain->shared_type; if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) { - int i; - - cpumask_copy(policy->cpus, cpu_data->shared_cpu_map); - - for_each_cpu(i, policy->cpus) { - if (unlikely(i == cpu)) - continue; - - memcpy(&all_cpu_data[i]->perf_caps, caps, - sizeof(cpu_data->perf_caps)); - } + cpumask_copy(policy->cpus, domain->shared_cpu_map); } else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) { /* Support only SW_ANY for now. */ pr_debug("Unsupported CPU co-ord type\n"); @@ -354,9 +426,12 @@ static int cppc_get_rate_from_fbctrs(struct cppc_cpudata *cpu_data, static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) { struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0}; - struct cppc_cpudata *cpu_data = all_cpu_data[cpu]; + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); + struct cppc_cpudata *cpu_data = policy->driver_data; int ret; + cpufreq_cpu_put(policy); + ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0); if (ret) return ret; @@ -372,7 +447,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu) static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state) { - struct cppc_cpudata *cpu_data = all_cpu_data[policy->cpu]; + struct cppc_cpudata *cpu_data = policy->driver_data; struct cppc_perf_caps *caps = &cpu_data->perf_caps; int ret; @@ -415,10 +490,13 @@ static struct cpufreq_driver cppc_cpufreq_driver = { */ static unsigned int hisi_cppc_cpufreq_get_rate(unsigned int cpu) { - struct cppc_cpudata *cpu_data = all_cpu_data[cpu]; + struct cpufreq_policy *policy = cpufreq_cpu_get(cpu); + struct cppc_cpudata *cpu_data = policy->driver_data; u64 desired_perf; int ret; + cpufreq_cpu_put(policy); + ret = cppc_get_desired_perf(cpu, &desired_perf); if (ret < 0) return -EIO; @@ -451,68 +529,41 @@ static void cppc_check_hisi_workaround(void) static int __init cppc_cpufreq_init(void) { - struct cppc_cpudata *cpu_data; - int i, ret = 0; - if (acpi_disabled) return -ENODEV; - all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *), - GFP_KERNEL); - if (!all_cpu_data) - return -ENOMEM; - - for_each_possible_cpu(i) { - all_cpu_data[i] = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL); - if (!all_cpu_data[i]) - goto out; - - cpu_data = all_cpu_data[i]; - if (!zalloc_cpumask_var(&cpu_data->shared_cpu_map, GFP_KERNEL)) - goto out; - } - - ret = acpi_get_psd_map(all_cpu_data); - if (ret) { - pr_debug("Error parsing PSD data. Aborting cpufreq registration.\n"); - goto out; - } - cppc_check_hisi_workaround(); - ret = cpufreq_register_driver(&cppc_cpufreq_driver); - if (ret) - goto out; + return cpufreq_register_driver(&cppc_cpufreq_driver); +} - return ret; +static inline void free_cpu_data(struct psd_data *domain) +{ + struct cppc_cpudata *iter, *tmp; -out: - for_each_possible_cpu(i) { - cpu_data = all_cpu_data[i]; - if (!cpu_data) - break; - free_cpumask_var(cpu_data->shared_cpu_map); - kfree(cpu_data); + list_for_each_entry_safe(iter, tmp, &domain->cpu_list, node) { + list_del(&iter->node); + kfree(iter); } - - kfree(all_cpu_data); - return -ENODEV; } +static inline void free_domain_data(void) +{ + struct psd_data *iter, *tmp; + + list_for_each_entry_safe(iter, tmp, &domain_list, node) { + list_del(&iter->node); + if (!list_empty(&iter->cpu_list)) + free_cpu_data(iter); + free_cpumask_var(iter->shared_cpu_map); + kfree(iter); + } +} static void __exit cppc_cpufreq_exit(void) { - struct cppc_cpudata *cpu_data; - int i; - cpufreq_unregister_driver(&cppc_cpufreq_driver); - for_each_possible_cpu(i) { - cpu_data = all_cpu_data[i]; - free_cpumask_var(cpu_data->shared_cpu_map); - kfree(cpu_data); - } - - kfree(all_cpu_data); + free_domain_data(); } module_exit(cppc_cpufreq_exit); diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h index a6a9373ab863..c0081fb6032e 100644 --- a/include/acpi/cppc_acpi.h +++ b/include/acpi/cppc_acpi.h @@ -122,22 +122,30 @@ struct cppc_perf_fb_ctrs { u64 wraparound_time; }; +/* Container of performance state domain data */ +struct psd_data { + struct list_head node; + unsigned int shared_type; + struct list_head cpu_list; + cpumask_var_t shared_cpu_map; +}; + /* Per CPU container for runtime CPPC management. */ struct cppc_cpudata { int cpu; + struct list_head node; + struct psd_data *domain; struct cppc_perf_caps perf_caps; struct cppc_perf_ctrls perf_ctrls; struct cppc_perf_fb_ctrs perf_fb_ctrs; struct cpufreq_policy *cur_policy; - unsigned int shared_type; - cpumask_var_t shared_cpu_map; }; extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf); extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs); extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls); extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps); -extern int acpi_get_psd_map(struct cppc_cpudata **); +extern int acpi_get_psd_map(unsigned int cpu, struct psd_data *domain); extern unsigned int cppc_get_transition_latency(int cpu); extern bool cpc_ffh_supported(void); extern int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val);
The cppc_cpudata per-cpu storage was inefficient (1) additional to causing functional issues (2) when CPUs are hotplugged out, due to per-cpu data being improperly initialised. (1) The amount of information needed for CPPC performance control in its cpufreq driver depends on the domain (PSD) coordination type: ANY: One set of CPPC control and capability data (e.g desired performance, highest/lowest performance, etc) applies to all CPUs in the domain. ALL: Same as ANY. To be noted that this type is not currently supported. When supported, information about which CPUs belong to a domain is needed in order for frequency change requests to be sent to each of them. HW: It's necessary to store CPPC control and capability information for all the CPUs. HW will then coordinate the performance state based on their limitations and requests. NONE: Same as HW. No HW coordination is expected. Despite this, the previous initialisation code would indiscriminately allocate memory for all CPUs (all_cpu_data) and unnecessarily duplicate performance capabilities and the domain sharing mask and type for each possible CPU. (2) With the current per-cpu structure, when having ANY coordination, the cppc_cpudata cpu information is not initialised (will remain 0) for all CPUs in a policy, other than policy->cpu. When policy->cpu is hotplugged out, the driver will incorrectly use the uninitialised (0) value of the other CPUs when making frequency changes. Additionally, the previous values stored in the perf_ctrls.desired_perf will be lost when policy->cpu changes. Due to the current storage of driver data not being fit for purpose, replace the per-cpu CPPC/PSD data with a list of domains which in turn will point to a list of CPUs with their controls and capabilities, belonging to the domain. This new structure has the following benefits: - Clearly separates PSD (domain) data from CPPC (performance monitoring and control) data and stores one mask of CPUs belonging to a domain and its type in a single structure, for each domain. - For ANY domain coordination, a single cppc_cpudata set of capabilities and controls are stored, for policy->cpu, and used for describing performance controls and capabilities even when policy->cpu changes as a result of hotplug. All the CPUs in the domain will belong to the same policy. - For HW or lack of coordination, a full map of domains and CPUs is obtained. Each CPU will have its own policy, but for HW, as a result of PSD information, a shared_cpu_map mask could be used to identify the domain CPU content. Additional changes: - A pointer to the policy's cppc_cpudata is stored in policy->driver_data - All data allocation is done from the driver's init function. At that point the domain is either created or retrieved. For this purpose acpi_get_psd_map() was changed to create a map of the domain of a single CPU, rather than the full system domain map. - When CPU's are hotplugged out and in, old information can be retrieved. Signed-off-by: Ionela Voinescu <ionela.voinescu@arm.com> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> Cc: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/acpi/cppc_acpi.c | 126 +++++++------------ drivers/cpufreq/cppc_cpufreq.c | 217 ++++++++++++++++++++------------- include/acpi/cppc_acpi.h | 14 ++- 3 files changed, 190 insertions(+), 167 deletions(-)