[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[Prashanth Prakash]

On ACPI based systems where the topology is setup using the API
store_cpu_topology, at the moment we do not have necessary code
to handle a cpufreq notifier, thus resulting in a crash.

Skip register_cpufreq_notifier if raw_capacity is not allocated
as part of topology initialization.

Stack:
        init_cpu_capacity_callback+0xb4/0x1c8
        notifier_call_chain+0x5c/0xa0
        __blocking_notifier_call_chain+0x58/0xa0
        blocking_notifier_call_chain+0x3c/0x50
        cpufreq_set_policy+0xe4/0x328
        cpufreq_init_policy+0x80/0x100
        cpufreq_online+0x418/0x710
        cpufreq_add_dev+0x118/0x180
        subsys_interface_register+0xa4/0xf8
        cpufreq_register_driver+0x1c0/0x298
        cppc_cpufreq_init+0xdc/0x1000 [cppc_cpufreq]
        do_one_initcall+0x5c/0x168
        do_init_module+0x64/0x1e4
        load_module+0x130c/0x14d0
        SyS_finit_module+0x108/0x120
        el0_svc_naked+0x24/0x28

Patch that added support for popultaing cpu capacity for DT:
https://patchwork.codeaurora.org/patch/98353/

Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
---
 arch/arm64/kernel/topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 23e9e13..3f175ce 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -209,7 +209,7 @@ static void normalize_cpu_capacity(void)
 
 static int __init register_cpufreq_notifier(void)
 {
-	if (cap_parsing_failed)
+	if (cap_parsing_failed || !raw_capacity)
 		return -EINVAL;
 
 	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
-- 
Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[Juri Lelli]

Hi,

On 24/01/17 17:43, Prashanth Prakash wrote:
> On ACPI based systems where the topology is setup using the API
> store_cpu_topology, at the moment we do not have necessary code
> to handle a cpufreq notifier, thus resulting in a crash.
> 
> Skip register_cpufreq_notifier if raw_capacity is not allocated
> as part of topology initialization.
> 
> Stack:
>         init_cpu_capacity_callback+0xb4/0x1c8
>         notifier_call_chain+0x5c/0xa0
>         __blocking_notifier_call_chain+0x58/0xa0
>         blocking_notifier_call_chain+0x3c/0x50
>         cpufreq_set_policy+0xe4/0x328
>         cpufreq_init_policy+0x80/0x100
>         cpufreq_online+0x418/0x710
>         cpufreq_add_dev+0x118/0x180
>         subsys_interface_register+0xa4/0xf8
>         cpufreq_register_driver+0x1c0/0x298
>         cppc_cpufreq_init+0xdc/0x1000 [cppc_cpufreq]
>         do_one_initcall+0x5c/0x168
>         do_init_module+0x64/0x1e4
>         load_module+0x130c/0x14d0
>         SyS_finit_module+0x108/0x120
>         el0_svc_naked+0x24/0x28
> 
> Patch that added support for popultaing cpu capacity for DT:
> https://patchwork.codeaurora.org/patch/98353/
> 
> Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
> ---
>  arch/arm64/kernel/topology.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 23e9e13..3f175ce 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -209,7 +209,7 @@ static void normalize_cpu_capacity(void)
>  
>  static int __init register_cpufreq_notifier(void)
>  {
> -	if (cap_parsing_failed)
> +	if (cap_parsing_failed || !raw_capacity)

Looks good. I couldn't really test it to check if it fixes your problem
(I assume it does), but it doesn't seem to introduce regressions on my
boxes.

Reviewed-by: Juri Lelli <juri.lelli@arm.com>

Thanks,

- Juri

[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[Christopher Covington]

On 01/25/2017 07:03 AM, Juri Lelli wrote:
> Hi,
> 
> On 24/01/17 17:43, Prashanth Prakash wrote:
>> On ACPI based systems where the topology is setup using the API
>> store_cpu_topology, at the moment we do not have necessary code
>> to handle a cpufreq notifier, thus resulting in a crash.
>>
>> Skip register_cpufreq_notifier if raw_capacity is not allocated
>> as part of topology initialization.
>>
>> Stack:
>>         init_cpu_capacity_callback+0xb4/0x1c8
>>         notifier_call_chain+0x5c/0xa0
>>         __blocking_notifier_call_chain+0x58/0xa0
>>         blocking_notifier_call_chain+0x3c/0x50
>>         cpufreq_set_policy+0xe4/0x328
>>         cpufreq_init_policy+0x80/0x100
>>         cpufreq_online+0x418/0x710
>>         cpufreq_add_dev+0x118/0x180
>>         subsys_interface_register+0xa4/0xf8
>>         cpufreq_register_driver+0x1c0/0x298
>>         cppc_cpufreq_init+0xdc/0x1000 [cppc_cpufreq]
>>         do_one_initcall+0x5c/0x168
>>         do_init_module+0x64/0x1e4
>>         load_module+0x130c/0x14d0
>>         SyS_finit_module+0x108/0x120
>>         el0_svc_naked+0x24/0x28
>>
>> Patch that added support for popultaing cpu capacity for DT:
>> https://patchwork.codeaurora.org/patch/98353/

In other words,

Fixes: 7202bde8b7ae ("arm64: parse cpu capacity-dmips-mhz from DT")

>> Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
>> ---
>>  arch/arm64/kernel/topology.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 23e9e13..3f175ce 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -209,7 +209,7 @@ static void normalize_cpu_capacity(void)
>>  
>>  static int __init register_cpufreq_notifier(void)
>>  {
>> -	if (cap_parsing_failed)
>> +	if (cap_parsing_failed || !raw_capacity)
> 
> Looks good. I couldn't really test it to check if it fixes your problem
> (I assume it does), but it doesn't seem to introduce regressions on my
> boxes.

Are you testing on QEMU or Fast Models? I was under the impression that the
former had ACPI support. For the latter, wouldn't it be nice to have ACPI
support in the bootwrapper. Out of curiosity, is Mark's bootwrapper
repository on kernel.org the best one to use these days?

http://git.kernel.org/cgit/linux/kernel/git/mark/boot-wrapper-aarch64.git/

Thanks,
Cov

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[Juri Lelli]

On 25/01/17 10:49, Christopher Covington wrote:
> On 01/25/2017 07:03 AM, Juri Lelli wrote:
> > Hi,
> > 
> > On 24/01/17 17:43, Prashanth Prakash wrote:
> >> On ACPI based systems where the topology is setup using the API
> >> store_cpu_topology, at the moment we do not have necessary code
> >> to handle a cpufreq notifier, thus resulting in a crash.
> >>
> >> Skip register_cpufreq_notifier if raw_capacity is not allocated
> >> as part of topology initialization.
> >>
> >> Stack:
> >>         init_cpu_capacity_callback+0xb4/0x1c8
> >>         notifier_call_chain+0x5c/0xa0
> >>         __blocking_notifier_call_chain+0x58/0xa0
> >>         blocking_notifier_call_chain+0x3c/0x50
> >>         cpufreq_set_policy+0xe4/0x328
> >>         cpufreq_init_policy+0x80/0x100
> >>         cpufreq_online+0x418/0x710
> >>         cpufreq_add_dev+0x118/0x180
> >>         subsys_interface_register+0xa4/0xf8
> >>         cpufreq_register_driver+0x1c0/0x298
> >>         cppc_cpufreq_init+0xdc/0x1000 [cppc_cpufreq]
> >>         do_one_initcall+0x5c/0x168
> >>         do_init_module+0x64/0x1e4
> >>         load_module+0x130c/0x14d0
> >>         SyS_finit_module+0x108/0x120
> >>         el0_svc_naked+0x24/0x28
> >>
> >> Patch that added support for popultaing cpu capacity for DT:
> >> https://patchwork.codeaurora.org/patch/98353/
> 
> In other words,
> 
> Fixes: 7202bde8b7ae ("arm64: parse cpu capacity-dmips-mhz from DT")
> 
> >> Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
> >> ---
> >>  arch/arm64/kernel/topology.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> >> index 23e9e13..3f175ce 100644
> >> --- a/arch/arm64/kernel/topology.c
> >> +++ b/arch/arm64/kernel/topology.c
> >> @@ -209,7 +209,7 @@ static void normalize_cpu_capacity(void)
> >>  
> >>  static int __init register_cpufreq_notifier(void)
> >>  {
> >> -	if (cap_parsing_failed)
> >> +	if (cap_parsing_failed || !raw_capacity)
> > 
> > Looks good. I couldn't really test it to check if it fixes your problem
> > (I assume it does), but it doesn't seem to introduce regressions on my
> > boxes.
> 
> Are you testing on QEMU or Fast Models?

Neither, Juno R2. Let me see if I can quickly test it in ACPI mode.

> I was under the impression that the
> former had ACPI support. For the latter, wouldn't it be nice to have ACPI
> support in the bootwrapper. Out of curiosity, is Mark's bootwrapper
> repository on kernel.org the best one to use these days?
> 
> http://git.kernel.org/cgit/linux/kernel/git/mark/boot-wrapper-aarch64.git/
> 

Not sure, Mark, others?

Thanks,

- Juri

[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[Will Deacon]

[adding Sudeep and Lorenzo for comment]

On Tue, Jan 24, 2017 at 05:43:12PM -0700, Prashanth Prakash wrote:
> On ACPI based systems where the topology is setup using the API
> store_cpu_topology, at the moment we do not have necessary code
> to handle a cpufreq notifier, thus resulting in a crash.

What is the "necessary code" that we're missing? Wouldn't it be better
to add that, or explicitly avoid the cpufreq notifier registration if
we're using ACPI?

Will


> Skip register_cpufreq_notifier if raw_capacity is not allocated
> as part of topology initialization.
> 
> Stack:
>         init_cpu_capacity_callback+0xb4/0x1c8
>         notifier_call_chain+0x5c/0xa0
>         __blocking_notifier_call_chain+0x58/0xa0
>         blocking_notifier_call_chain+0x3c/0x50
>         cpufreq_set_policy+0xe4/0x328
>         cpufreq_init_policy+0x80/0x100
>         cpufreq_online+0x418/0x710
>         cpufreq_add_dev+0x118/0x180
>         subsys_interface_register+0xa4/0xf8
>         cpufreq_register_driver+0x1c0/0x298
>         cppc_cpufreq_init+0xdc/0x1000 [cppc_cpufreq]
>         do_one_initcall+0x5c/0x168
>         do_init_module+0x64/0x1e4
>         load_module+0x130c/0x14d0
>         SyS_finit_module+0x108/0x120
>         el0_svc_naked+0x24/0x28
> 
> Patch that added support for popultaing cpu capacity for DT:
> https://patchwork.codeaurora.org/patch/98353/
> 
> Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
> ---
>  arch/arm64/kernel/topology.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 23e9e13..3f175ce 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -209,7 +209,7 @@ static void normalize_cpu_capacity(void)
>  
>  static int __init register_cpufreq_notifier(void)
>  {
> -	if (cap_parsing_failed)
> +	if (cap_parsing_failed || !raw_capacity)
>  		return -EINVAL;
>  
>  	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
> -- 
> Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[Lorenzo Pieralisi]

On Thu, Jan 26, 2017 at 12:18:11PM +0000, Will Deacon wrote:
> [adding Sudeep and Lorenzo for comment]
> 
> On Tue, Jan 24, 2017 at 05:43:12PM -0700, Prashanth Prakash wrote:
> > On ACPI based systems where the topology is setup using the API
> > store_cpu_topology, at the moment we do not have necessary code
> > to handle a cpufreq notifier, thus resulting in a crash.
> 
> What is the "necessary code" that we're missing? Wouldn't it be better
> to add that, or explicitly avoid the cpufreq notifier registration if
> we're using ACPI?

Necessary code is, in DT, code parsing bindings to provide capacity
values and allocate the raw_capacity array; there is no ACPI counterpart
for those bindings (well..there is a byte length field in the GICC MADT
entry "Processor Power Efficiency Class" but as far as I understand,
currently, it would be more reliable as a random seed than a useful
capacity scale, I just do not know why it is there) so the whole CPUfreq
notifier thing is basically useless when booting with ACPI.

How about using (or put the ACPI bit in a separate line with a
comment):

	if (!acpi_disabled || cap_parsing_failed)

to prevent registering the CPUfreq notifier ? Using raw_capacity check
this patch achieves the same in a much more opaque way (you will have to
include <linux/acpi.h> to make use of acpi_disabled).

We can work to turn "Processor Power Efficiency Class" into something
useful but that's for another series anyway.

Lorenzo

> 
> Will
> 
> 
> > Skip register_cpufreq_notifier if raw_capacity is not allocated
> > as part of topology initialization.
> > 
> > Stack:
> >         init_cpu_capacity_callback+0xb4/0x1c8
> >         notifier_call_chain+0x5c/0xa0
> >         __blocking_notifier_call_chain+0x58/0xa0
> >         blocking_notifier_call_chain+0x3c/0x50
> >         cpufreq_set_policy+0xe4/0x328
> >         cpufreq_init_policy+0x80/0x100
> >         cpufreq_online+0x418/0x710
> >         cpufreq_add_dev+0x118/0x180
> >         subsys_interface_register+0xa4/0xf8
> >         cpufreq_register_driver+0x1c0/0x298
> >         cppc_cpufreq_init+0xdc/0x1000 [cppc_cpufreq]
> >         do_one_initcall+0x5c/0x168
> >         do_init_module+0x64/0x1e4
> >         load_module+0x130c/0x14d0
> >         SyS_finit_module+0x108/0x120
> >         el0_svc_naked+0x24/0x28
> > 
> > Patch that added support for popultaing cpu capacity for DT:
> > https://patchwork.codeaurora.org/patch/98353/
> > 
> > Signed-off-by: Prashanth Prakash <pprakash@codeaurora.org>
> > ---
> >  arch/arm64/kernel/topology.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index 23e9e13..3f175ce 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -209,7 +209,7 @@ static void normalize_cpu_capacity(void)
> >  
> >  static int __init register_cpufreq_notifier(void)
> >  {
> > -	if (cap_parsing_failed)
> > +	if (cap_parsing_failed || !raw_capacity)
> >  		return -EINVAL;
> >  
> >  	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
> > -- 
> > Qualcomm Datacenter Technologies on behalf of Qualcomm Technologies, Inc.
> > Qualcomm Technologies, Inc. is a member of the
> > Code Aurora Forum, a Linux Foundation Collaborative Project.
> > 

[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[Will Deacon]

On Thu, Jan 26, 2017 at 03:52:29PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Jan 26, 2017 at 12:18:11PM +0000, Will Deacon wrote:
> > [adding Sudeep and Lorenzo for comment]
> > 
> > On Tue, Jan 24, 2017 at 05:43:12PM -0700, Prashanth Prakash wrote:
> > > On ACPI based systems where the topology is setup using the API
> > > store_cpu_topology, at the moment we do not have necessary code
> > > to handle a cpufreq notifier, thus resulting in a crash.
> > 
> > What is the "necessary code" that we're missing? Wouldn't it be better
> > to add that, or explicitly avoid the cpufreq notifier registration if
> > we're using ACPI?
> 
> Necessary code is, in DT, code parsing bindings to provide capacity
> values and allocate the raw_capacity array; there is no ACPI counterpart
> for those bindings (well..there is a byte length field in the GICC MADT
> entry "Processor Power Efficiency Class" but as far as I understand,
> currently, it would be more reliable as a random seed than a useful
> capacity scale, I just do not know why it is there) so the whole CPUfreq
> notifier thing is basically useless when booting with ACPI.

Ok, thanks for the explanation.

> How about using (or put the ACPI bit in a separate line with a
> comment):
> 
> 	if (!acpi_disabled || cap_parsing_failed)
> 
> to prevent registering the CPUfreq notifier ? Using raw_capacity check
> this patch achieves the same in a much more opaque way (you will have to
> include <linux/acpi.h> to make use of acpi_disabled).

That looks good to me. If somebody sends a v2, I can apply it for 4.11.

Thanks,

Will

[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[Mark Rutland]

On Wed, Jan 25, 2017 at 10:49:36AM -0500, Christopher Covington wrote:

> Are you testing on QEMU or Fast Models? I was under the impression that the
> former had ACPI support. For the latter, wouldn't it be nice to have ACPI
> support in the bootwrapper.

One goal of the boot wrapper is to be as simple as reasonably possible.
ACPI support in the boot wrapper would be far from trivial, at minimum
requiring EFI support (requiring either a whole reimplementation, or
work to make EDK2 run atop of the boot wrapper).

Given that it is already possible to use ARM Trusted Firmware and EDK2
in a model environment, and this can already be used to handover ACPI
tables, one would be served better by those.

> Out of curiosity, is Mark's bootwrapper repository on kernel.org the
> best one to use these days?
> 
> http://git.kernel.org/cgit/linux/kernel/git/mark/boot-wrapper-aarch64.git/

That is the canonical repo these days, yes.

Thanks,
Mark.

[PATCH] arm64: skip register_cpufreq_notifier on ACPI-based systems

[Prakash, Prashanth]

On 1/26/2017 8:57 AM, Will Deacon wrote:
> On Thu, Jan 26, 2017 at 03:52:29PM +0000, Lorenzo Pieralisi wrote:
>> On Thu, Jan 26, 2017 at 12:18:11PM +0000, Will Deacon wrote:
>>> [adding Sudeep and Lorenzo for comment]
>>>
>>> On Tue, Jan 24, 2017 at 05:43:12PM -0700, Prashanth Prakash wrote:
>>>> On ACPI based systems where the topology is setup using the API
>>>> store_cpu_topology, at the moment we do not have necessary code
>>>> to handle a cpufreq notifier, thus resulting in a crash.
>>> What is the "necessary code" that we're missing? Wouldn't it be better
>>> to add that, or explicitly avoid the cpufreq notifier registration if
>>> we're using ACPI?
>> Necessary code is, in DT, code parsing bindings to provide capacity
>> values and allocate the raw_capacity array; there is no ACPI counterpart
>> for those bindings (well..there is a byte length field in the GICC MADT
>> entry "Processor Power Efficiency Class" but as far as I understand,
>> currently, it would be more reliable as a random seed than a useful
>> capacity scale, I just do not know why it is there) so the whole CPUfreq
>> notifier thing is basically useless when booting with ACPI.
> Ok, thanks for the explanation.
>
>> How about using (or put the ACPI bit in a separate line with a
>> comment):
>>
>> 	if (!acpi_disabled || cap_parsing_failed)
>>
>> to prevent registering the CPUfreq notifier ? Using raw_capacity check
>> this patch achieves the same in a much more opaque way (you will have to
>> include <linux/acpi.h> to make use of acpi_disabled).
> That looks good to me. If somebody sends a v2, I can apply it for 4.11.

Thanks for the inputs. I will post v2 in next couple of hours.

Any chances this can make it to one of the 4-10 release candidates as it results
in a panic without this change?

--
Thanks,
Prashanth