Re: [PATCH 2/2] CPUFreq: Add new sysfs attribute freqdomain_cpus for acpi-freq driver

[Lan Tianyu <tianyu.lan@intel.com>]

Hi Viresh
	Thanks for your review.

On 2013年06月25日 11:56, Viresh Kumar wrote:
> On 25 June 2013 07:36, Lan Tianyu <tianyu.lan@intel.com> wrote:
>> diff --git a/Documentation/cpu-freq/user-guide.txt b/Documentation/cpu-freq/user-guide.txt
>> index ff2f283..0cc72f7 100644
>> --- a/Documentation/cpu-freq/user-guide.txt
>> +++ b/Documentation/cpu-freq/user-guide.txt
>> @@ -196,6 +196,10 @@ affected_cpus :                    List of Online CPUs that require software
>>  related_cpus :                 List of Online + Offline CPUs that need software
>>                                 coordination of frequency.
>>
>> +freqdomain_cpus :              List of Online + Offline CPUs in same CPU dependency
>> +                               domain. (This is only available for acpi-cpufreq
>> +                               driver)
>> +
> 
> This is generic file, don't add this information here. Add this in
> acpi-cpufreq file.
I don't find acpi-cpufreq.txt under
Documentation/cpu-freq/acpi-cpufreq.txt. So I should create it?

> 
>>  scaling_driver :               Hardware driver for cpufreq.
>>
>>  scaling_cur_freq :             Current frequency of the CPU as determined by
>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index 17e3496..b859997 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -176,6 +176,16 @@ static struct global_attr global_boost = __ATTR(boost, 0644,
>>                                                 show_global_boost,
>>                                                 store_global_boost);
>>
>> +static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
>> +{
>> +       struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
>> +       struct acpi_processor_performance *perf = data->acpi_data;
>> +
>> +       return cpufreq_show_cpus(perf->shared_cpu_map, buf);
>> +}
> 
> I am not sure if this is enough. Check this commit:
> 
> aa77a52764a92216b61a6c8079b5c01937c046cd
> 
> It had these changes:

Please see the acpi_processor_preregister_performance() in the
drivers/acpi/processor_perlib.c. All cpus in the same dependency domain
are stored in the perf->shared_cpu_map(including the reference cpu the
perf belongs to). Original code will copy shared_cpu_map to
policy->related_cpus, do cpumask_or between policy->related_cpus and
policy->cpus in the cpufreq_add_dev() and store the result into
policy->related_cpus. Expose the data via related_cpus.

For acpi-cpufreq driver, the shared_cpu_map is the biggest subset since
it regardless the coordination type.

After the commit aa77a5, only the cpus in the software or
software&hardware coordination type dependency domain will copy to
policy->cpus and finally store into policy->related_cpus in the
cpufreq_add_dev() by cpumask_or. And then we miss the cpus in the
hardware coordination type domain. But these info is still stored in the
policy->shared_cpu_map. Does this make sense?

> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 937bc28..57a8774 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -730,7 +730,6 @@ static int acpi_cpufreq_cpu_init(struct
> cpufreq_policy *policy)
>             policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
>                 cpumask_copy(policy->cpus, perf->shared_cpu_map);
>         }
> -       cpumask_copy(policy->related_cpus, perf->shared_cpu_map);
> 
>  #ifdef CONFIG_SMP
>         dmi_check_system(sw_any_bug_dmi_table);
> @@ -742,7 +741,6 @@ static int acpi_cpufreq_cpu_init(struct
> cpufreq_policy *policy)
>         if (check_amd_hwpstate_cpu(cpu) && !acpi_pstate_strict) {
>                 cpumask_clear(policy->cpus);
>                 cpumask_set_cpu(cpu, policy->cpus);
> -               cpumask_copy(policy->related_cpus, cpu_sibling_mask(cpu));
>                 policy->shared_type = CPUFREQ_SHARED_TYPE_HW;
>                 pr_info_once(PFX "overriding BIOS provided _PSD data\n");
> 


-- 
Best regards
Tianyu Lan