Re: [PATCH] powerpc: move epapr paravirt init of power_save to an initcall

[Tudor Laurentiu <b10716@freescale.com>]

On 05/05/2014 03:21 PM, Alexander Graf wrote:
> On 05/05/2014 02:17 PM, Tudor Laurentiu wrote:
>> On 04/30/2014 11:09 PM, Alexander Graf wrote:
>>>
>>> On 30.04.14 22:03, Stuart Yoder wrote:
>>>>
>>>>> -----Original Message-----
>>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>>> Sent: Wednesday, April 30, 2014 2:56 PM
>>>>> To: Yoder Stuart-B08248; benh@kernel.crashing.org; Wood Scott-B07421
>>>>> Cc: linuxppc-dev@lists.ozlabs.org
>>>>> Subject: Re: [PATCH] powerpc: move epapr paravirt init of
>>>>> power_save to
>>>>> an initcall
>>>>>
>>>>>
>>>>> On 30.04.14 21:54, Stuart Yoder wrote:
>>>>>> From: Stuart Yoder <stuart.yoder@freescale.com>
>>>>>>
>>>>>> some restructuring of epapr paravirt init resulted in
>>>>>> ppc_md.power_save being set, and then overwritten to
>>>>>> NULL during machine_init.  This patch splits the
>>>>>> initialization of ppc_md.power_save out into a postcore
>>>>>> init call.
>>>>>>
>>>>>> Signed-off-by: Stuart Yoder <stuart.yoder@freescale.com>
>>>>>> ---
>>>>>>    arch/powerpc/kernel/epapr_paravirt.c |   25
>>>>>> ++++++++++++++++++++-----
>>>>>>    1 file changed, 20 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/powerpc/kernel/epapr_paravirt.c
>>>>> b/arch/powerpc/kernel/epapr_paravirt.c
>>>>>> index 6300c13..c49b69c 100644
>>>>>> --- a/arch/powerpc/kernel/epapr_paravirt.c
>>>>>> +++ b/arch/powerpc/kernel/epapr_paravirt.c
>>>>>> @@ -52,11 +52,6 @@ static int __init
>>>>>> early_init_dt_scan_epapr(unsigned
>>>>> long node,
>>>>>>    #endif
>>>>>>        }
>>>>>>
>>>>>> -#if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64)
>>>>>> -    if (of_get_flat_dt_prop(node, "has-idle", NULL))
>>>>>> -        ppc_md.power_save = epapr_ev_idle;
>>>>>> -#endif
>>>>>> -
>>>>>>        epapr_paravirt_enabled = true;
>>>>>>
>>>>>>        return 1;
>>>>>> @@ -69,3 +64,23 @@ int __init epapr_paravirt_early_init(void)
>>>>>>        return 0;
>>>>>>    }
>>>>>>
>>>>>> +static int __init epapr_idle_init_dt_scan(unsigned long node,
>>>>>> +                       const char *uname,
>>>>>> +                       int depth, void *data)
>>>>>> +{
>>>>>> +#if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64)
>>>>>> +    if (of_get_flat_dt_prop(node, "has-idle", NULL))
>>>>>> +        ppc_md.power_save = epapr_ev_idle;
>>>>>> +#endif
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int __init epapr_idle_init(void)
>>>>>> +{
>>>>>> +    if (epapr_paravirt_enabled)
>>>>>> +        of_scan_flat_dt(epapr_idle_init_dt_scan, NULL);
>>>>> Doesn't this scan all nodes? We only want to match on
>>>>> /hypervisor/has-idle, no?
>>>> I cut/pasted from  the approach the existing code in that file
>>>> took, but yes you're right we just need the one property.
>>>> Let me respin that to look at the hypervisor node only.
>>>
>>> Yeah, the same commit that introduced the breakage on has-idle also
>>> removed the explicit check for /hypervisor.
>>>
>>> Laurentiu, was this change on purpose?
>>>
>>
>> Alex,
>>
>> IIRC, at that time i had to switch from the normal "of" functions to a
>> completely different api that's available in early init stage. This
>> early "of" api is pretty limited (e.g. doesn't have a way to address a
>> specific node) and i had to use that function that scans the whole tree.
>
> Ok, so it is an accident. Could you please post a patch that checks that
> the node we're looking at is called "hypervisor"? The simple API should
> give you enough information for that at least. Maybe you could even
> check that the parent node is the root node.
>

Just had a quick look and it looks that that early fdt api was improved 
with a function that allows specifying a starting path for the scan 
(of_scan_flat_dt_by_path() added in commit 
57d74bcf3072b65bde5aa540cedc976a75c48e5c). So i think we can simply use 
that instead.

---
Best Regards, Laurentiu