[PATCH] CPPC: Initialize CPUFreq CPU co-ord type correctly

[PATCH] CPPC: Initialize CPUFreq CPU co-ord type correctly

[Ashwin Chaugule]

The CPU policy struct indicates the co-ordination type
for all CPUs of a common freq domain. Initialize it
correctly using the CPU specific data gathered from
CPPC ACPI lib via acpi_get_psd_map().

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/cpufreq/cppc_cpufreq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 93c219f..508809b 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -98,6 +98,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	policy->max = cpu->perf_caps.highest_perf;
 	policy->cpuinfo.min_freq = policy->min;
 	policy->cpuinfo.max_freq = policy->max;
+	policy->shared_type = cpu->shared_type;
 
 	if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY)
 		cpumask_copy(policy->cpus, cpu->shared_cpu_map);
-- 
1.9.1

Re: [PATCH] CPPC: Initialize CPUFreq CPU co-ord type correctly

[Ashwin Chaugule]

Hi Rafael,

On 12 November 2015 at 19:52, Ashwin Chaugule
<ashwin.chaugule@linaro.org> wrote:
> The CPU policy struct indicates the co-ordination type
> for all CPUs of a common freq domain. Initialize it
> correctly using the CPU specific data gathered from
> CPPC ACPI lib via acpi_get_psd_map().
>
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 93c219f..508809b 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -98,6 +98,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>         policy->max = cpu->perf_caps.highest_perf;
>         policy->cpuinfo.min_freq = policy->min;
>         policy->cpuinfo.max_freq = policy->max;
> +       policy->shared_type = cpu->shared_type;
>
>         if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY)
>                 cpumask_copy(policy->cpus, cpu->shared_cpu_map);
> --
> 1.9.1
>

I apologize for not spotting this earlier, but I seem to have gotten
overzealous cleaning up code here. Can this make it into v4.4-rc1
along with what you've already pushed out?

Thanks,
Ashwin.

Re: [PATCH] CPPC: Initialize CPUFreq CPU co-ord type correctly

[Rafael J. Wysocki]

On Friday, November 13, 2015 03:47:43 PM Ashwin Chaugule wrote:
> Hi Rafael,
> 
> On 12 November 2015 at 19:52, Ashwin Chaugule
> <ashwin.chaugule@linaro.org> wrote:
> > The CPU policy struct indicates the co-ordination type
> > for all CPUs of a common freq domain. Initialize it
> > correctly using the CPU specific data gathered from
> > CPPC ACPI lib via acpi_get_psd_map().
> >
> > Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> > ---
> >  drivers/cpufreq/cppc_cpufreq.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > index 93c219f..508809b 100644
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -98,6 +98,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >         policy->max = cpu->perf_caps.highest_perf;
> >         policy->cpuinfo.min_freq = policy->min;
> >         policy->cpuinfo.max_freq = policy->max;
> > +       policy->shared_type = cpu->shared_type;
> >
> >         if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY)
> >                 cpumask_copy(policy->cpus, cpu->shared_cpu_map);
> > --
> > 1.9.1
> >
> 
> I apologize for not spotting this earlier, but I seem to have gotten
> overzealous cleaning up code here. Can this make it into v4.4-rc1
> along with what you've already pushed out?

It's too late for -rc1, but I'll queue them up for -rc2.

Thanks,
Rafael

Re: [PATCH] CPPC: Initialize CPUFreq CPU co-ord type correctly

[Ashwin Chaugule]

Hi Rafael,

On 13 November 2015 at 19:27, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, November 13, 2015 03:47:43 PM Ashwin Chaugule wrote:
>> Hi Rafael,
>>
>> On 12 November 2015 at 19:52, Ashwin Chaugule
>> <ashwin.chaugule@linaro.org> wrote:
>> > The CPU policy struct indicates the co-ordination type
>> > for all CPUs of a common freq domain. Initialize it
>> > correctly using the CPU specific data gathered from
>> > CPPC ACPI lib via acpi_get_psd_map().
>> >
>> > Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
>> > ---
>> >  drivers/cpufreq/cppc_cpufreq.c | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> > index 93c219f..508809b 100644
>> > --- a/drivers/cpufreq/cppc_cpufreq.c
>> > +++ b/drivers/cpufreq/cppc_cpufreq.c
>> > @@ -98,6 +98,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> >         policy->max = cpu->perf_caps.highest_perf;
>> >         policy->cpuinfo.min_freq = policy->min;
>> >         policy->cpuinfo.max_freq = policy->max;
>> > +       policy->shared_type = cpu->shared_type;
>> >
>> >         if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY)
>> >                 cpumask_copy(policy->cpus, cpu->shared_cpu_map);
>> > --
>> > 1.9.1
>> >
>>
>> I apologize for not spotting this earlier, but I seem to have gotten
>> overzealous cleaning up code here. Can this make it into v4.4-rc1
>> along with what you've already pushed out?
>
> It's too late for -rc1, but I'll queue them up for -rc2.

I was working on a rather quirky system with only one CPU in a domain.
In such a case the _PSD object doesn't make sense but its also
supposed to be an optional object. This system had a PSD package with
Num CPUs = 1. The acpi_get_psd_map() routine (which I adapted from
acpi_processor_preregister_performance()) rightfully skips trying to
match PSDs when there is only one CPU. But it also skips reading the
co-ord type from the PSD if there is one. So what ends up happening is
that cpu->shared_type is always 0 and the cppc_cpufreq driver will
fail init on this CPU, since I only support TYPE_ANY. This will happen
even if there is no PSD object for this CPU.

So I think a better way to deal with this is to explicitly check if
shared_type == CPUFREQ_SHARED_TYPE_ALL then print out the Unsupported
co-ord type message. I'll send a patch along with this fix as a reply
to this mail.

Regards,
Ashwin.

[PATCH] CPPC: Initialize and check CPUFreq CPU co-ord type correctly

[Ashwin Chaugule]

The CPU policy struct indicates the co-ordination type
for all CPUs of a common freq domain. Initialize it
correctly using the CPU specific data gathered from
CPPC ACPI lib via acpi_get_psd_map().

The PSD object is optional, so the cpu->shared_type
can also be 0. So instead of assuming any value other
than SW_ANY(0xFD) is unsupported, explictly check
if shared_type is SW_ALL and then bail.

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
---
 drivers/cpufreq/cppc_cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index e8cb334..7c0bdfb 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -98,10 +98,11 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	policy->max = cpu->perf_caps.highest_perf;
 	policy->cpuinfo.min_freq = policy->min;
 	policy->cpuinfo.max_freq = policy->max;
+	policy->shared_type = cpu->shared_type;
 
 	if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY)
 		cpumask_copy(policy->cpus, cpu->shared_cpu_map);
-	else {
+	else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) {
 		/* Support only SW_ANY for now. */
 		pr_debug("Unsupported CPU co-ord type\n");
 		return -EFAULT;
-- 
1.9.1

Re: [PATCH] CPPC: Initialize and check CPUFreq CPU co-ord type correctly

[Rafael J. Wysocki]

On Thursday, November 19, 2015 10:40:07 AM Ashwin Chaugule wrote:
> The CPU policy struct indicates the co-ordination type
> for all CPUs of a common freq domain. Initialize it
> correctly using the CPU specific data gathered from
> CPPC ACPI lib via acpi_get_psd_map().
> 
> The PSD object is optional, so the cpu->shared_type
> can also be 0. So instead of assuming any value other
> than SW_ANY(0xFD) is unsupported, explictly check
> if shared_type is SW_ALL and then bail.
> 
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>

I'm assiming that this is a replacement for https://patchwork.kernel.org/patch/7607581/
so I'm going to replace that patch with this one, but that means it will go
into -rc3 instead.

> ---
>  drivers/cpufreq/cppc_cpufreq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index e8cb334..7c0bdfb 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -98,10 +98,11 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  	policy->max = cpu->perf_caps.highest_perf;
>  	policy->cpuinfo.min_freq = policy->min;
>  	policy->cpuinfo.max_freq = policy->max;
> +	policy->shared_type = cpu->shared_type;
>  
>  	if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY)
>  		cpumask_copy(policy->cpus, cpu->shared_cpu_map);
> -	else {
> +	else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) {
>  		/* Support only SW_ANY for now. */
>  		pr_debug("Unsupported CPU co-ord type\n");
>  		return -EFAULT;
> 

Thanks,
Rafael

Re: [PATCH] CPPC: Initialize and check CPUFreq CPU co-ord type correctly

[Ashwin Chaugule]

On 19 November 2015 at 17:17, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, November 19, 2015 10:40:07 AM Ashwin Chaugule wrote:
>> The CPU policy struct indicates the co-ordination type
>> for all CPUs of a common freq domain. Initialize it
>> correctly using the CPU specific data gathered from
>> CPPC ACPI lib via acpi_get_psd_map().
>>
>> The PSD object is optional, so the cpu->shared_type
>> can also be 0. So instead of assuming any value other
>> than SW_ANY(0xFD) is unsupported, explictly check
>> if shared_type is SW_ALL and then bail.
>>
>> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
>
> I'm assiming that this is a replacement for https://patchwork.kernel.org/patch/7607581/
> so I'm going to replace that patch with this one, but that means it will go
> into -rc3 instead.
>

Replacement is correct. I guess I didn't realize you had pulled it in
already. :)

Thanks,
Ashwin.