| Message ID | 1398889209-10350-1-git-send-email-stuart.yoder@freescale.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 30.04.14 22:20, 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> Looks pretty good to me and IMHO should get a CC on stable when it gets into the tree. One minor nit below. We still need to clarify whether the omitted check on early_init_dt_scan_epapr() is on purpose and if not reintroduce it, but that's a separate issue. > --- > > -v2: don't iterate over the entire DT, just look at > the hypervisor node > > 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 7898be9..a01df5e 100644 > --- a/arch/powerpc/kernel/epapr_paravirt.c > +++ b/arch/powerpc/kernel/epapr_paravirt.c > @@ -53,11 +53,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; > @@ -70,3 +65,23 @@ int __init epapr_paravirt_early_init(void) > return 0; > } > v > +static int __init epapr_idle_init(void) > +{ > + struct device_node *node; > + > + if (!epapr_paravirt_enabled) > + return 0; > + > + node = of_find_node_by_path("/hypervisor"); > + if (!node) > + return -ENODEV; > + > +#if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64) Please move the #if scope from v to ^. That way we don't waste space / time / anything on systems that don't bother with the idle hcall. > + if (of_get_property(node, "has-idle", NULL)) > + ppc_md.power_save = epapr_ev_idle; > +#endif > + > + return 0; > +} > + > +postcore_initcall(epapr_idle_init); ^ Alex
On Wed, 2014-04-30 at 15:20 -0500, 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> > --- > > -v2: don't iterate over the entire DT, just look at > the hypervisor node > > 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 7898be9..a01df5e 100644 > --- a/arch/powerpc/kernel/epapr_paravirt.c > +++ b/arch/powerpc/kernel/epapr_paravirt.c > @@ -53,11 +53,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; > @@ -70,3 +65,23 @@ int __init epapr_paravirt_early_init(void) > return 0; > } > > +static int __init epapr_idle_init(void) > +{ > + struct device_node *node; > + > + if (!epapr_paravirt_enabled) > + return 0; > + > + node = of_find_node_by_path("/hypervisor"); > + if (!node) > + return -ENODEV; > + > +#if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64) > + if (of_get_property(node, "has-idle", NULL)) > + ppc_md.power_save = epapr_ev_idle; > +#endif > + > + return 0; > +} Why duplicate the search-for-hv-node code? Just have the early init func set a flag indicating whether has-idle was found, and have epapr_idle_init act on that flag. -Scott
> -----Original Message----- > From: Wood Scott-B07421 > Sent: Wednesday, April 30, 2014 5:49 PM > To: Yoder Stuart-B08248 > Cc: benh@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org; > agraf@suse.de > Subject: Re: [PATCH][v2] powerpc: move epapr paravirt init of power_save > to an initcall > > On Wed, 2014-04-30 at 15:20 -0500, 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> > > --- > > > > -v2: don't iterate over the entire DT, just look at > > the hypervisor node > > > > 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 7898be9..a01df5e 100644 > > --- a/arch/powerpc/kernel/epapr_paravirt.c > > +++ b/arch/powerpc/kernel/epapr_paravirt.c > > @@ -53,11 +53,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; > > @@ -70,3 +65,23 @@ int __init epapr_paravirt_early_init(void) > > return 0; > > } > > > > +static int __init epapr_idle_init(void) > > +{ > > + struct device_node *node; > > + > > + if (!epapr_paravirt_enabled) > > + return 0; > > + > > + node = of_find_node_by_path("/hypervisor"); > > + if (!node) > > + return -ENODEV; > > + > > +#if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64) > > + if (of_get_property(node, "has-idle", NULL)) > > + ppc_md.power_save = epapr_ev_idle; > > +#endif > > + > > + return 0; > > +} > > Why duplicate the search-for-hv-node code? Just have the early init > func set a flag indicating whether has-idle was found, and have > epapr_idle_init act on that flag. That's a good idea, and the patch would be quite a bit smaller. I'll do that and leave the stuff in early_init_dt_scan_epapr() alone mostly. Then as a separate step we could put back the code that looks at only /hypervisor (based on feedback from Laurentiu). Stuart
diff --git a/arch/powerpc/kernel/epapr_paravirt.c b/arch/powerpc/kernel/epapr_paravirt.c index 7898be9..a01df5e 100644 --- a/arch/powerpc/kernel/epapr_paravirt.c +++ b/arch/powerpc/kernel/epapr_paravirt.c @@ -53,11 +53,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; @@ -70,3 +65,23 @@ int __init epapr_paravirt_early_init(void) return 0; } +static int __init epapr_idle_init(void) +{ + struct device_node *node; + + if (!epapr_paravirt_enabled) + return 0; + + node = of_find_node_by_path("/hypervisor"); + if (!node) + return -ENODEV; + +#if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64) + if (of_get_property(node, "has-idle", NULL)) + ppc_md.power_save = epapr_ev_idle; +#endif + + return 0; +} + +postcore_initcall(epapr_idle_init);