Re: [PATCH 2/3] powernv:idle: Don't override default/deepest directly in kernel
Hi Nick,
Thanks for reviewing the patch.
On Wed, Mar 15, 2017 at 12:05:43AM +1000, Nicholas Piggin wrote:
> On Mon, 13 Mar 2017 11:31:27 +0530
> "Gautham R. Shenoy" wrote:
>
> > From: "Gautham R. Shenoy"
> >
> > Currently during idle-init on power9, if we don't find suitable stop
> > states in the device tree that can be used as the
> > default_stop/deepest_stop, we set stop0 (ESL=1,EC=1) as the default
> > stop state psscr to be used by power9_idle and deepest stop state
> > which is used by CPU-Hotplug.
> >
> > However, if the platform firmware has not configured or enabled a stop
> > state, the kernel should not make any assumptions and fallback to a
> > default choice.
> >
> > If the kernel uses a stop state that is not configured by the platform
> > firmware, it may lead to further failures which should be avoided.
> >
> > In this patch, we modify the init code to ensure that the kernel uses
> > only the stop states exposed by the firmware through the device
> > tree. When a suitable default stop state isn't found, we disable
> > ppc_md.power_save for power9. Similarly, when a suitable
> > deepest_stop_state is not found in the device tree exported by the
> > firmware, fall back to the default busy-wait loop in the CPU-Hotplug
> > code.
>
> Seems reasonable. I have a few comments that you may consider. Nothing
> too major.
>
> Btw., it would be nice to move this hotplug idling selection code to
> idle.c. Have the hotplug just ask to enter the best available idle mode
> and that's it. I'm not asking you to do that for this series, but perhaps
> consider it for the future.
That's not a bad idea. I will do it in the respin of the patchset.
>
>
> > diff --git a/arch/powerpc/platforms/powernv/idle.c
> > b/arch/powerpc/platforms/powernv/idle.c
> > index 4ee837e..9fde6e4 100644
> > --- a/arch/powerpc/platforms/powernv/idle.c
> > +++ b/arch/powerpc/platforms/powernv/idle.c
> > @@ -147,7 +147,6 @@ u32 pnv_get_supported_cpuidle_states(void)
> > }
> > EXPORT_SYMBOL_GPL(pnv_get_supported_cpuidle_states);
> >
> > -
> > static void pnv_fastsleep_workaround_apply(void *info)
> >
> > {
> > @@ -243,6 +242,7 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600,
> > */
> > u64 pnv_default_stop_val;
> > u64 pnv_default_stop_mask;
> > +bool default_stop_found;
> >
> > /*
> > * Used for ppc_md.power_save which needs a function with no parameters
> > @@ -264,6 +264,7 @@ static void power9_idle(void)
> > */
> > u64 pnv_deepest_stop_psscr_val;
> > u64 pnv_deepest_stop_psscr_mask;
> > +bool deepest_stop_found;
> >
> > /*
> > * Power ISA 3.0 idle initialization.
>
> If the hotplug idle code was in idle.c, then all this deepest/default stop
> logic and register settings would be static to idle.c, which would be nice.
>
> If you have a function to check if deepest stop is found, then you don't need
> a non-static variable here (or for default_stop_found AFAIKS).
Sure!
>
>
> > @@ -352,7 +353,6 @@ static int __init pnv_power9_idle_init(struct
> > device_node *np, u32 *flags,
> > u32 *residency_ns = NULL;
> > u64 max_residency_ns = 0;
> > int rc = 0, i;
> > - bool default_stop_found = false, deepest_stop_found = false;
> >
> > psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), GFP_KERNEL);
> > psscr_mask = kcalloc(dt_idle_states, sizeof(*psscr_mask), GFP_KERNEL);
> > @@ -433,20 +433,22 @@ static int __init pnv_power9_idle_init(struct
> > device_node *np, u32 *flags,
> > }
> >
> > if (!default_stop_found) {
> > - pnv_default_stop_val = PSSCR_HV_DEFAULT_VAL;
> > - pnv_default_stop_mask = PSSCR_HV_DEFAULT_MASK;
> > - pr_warn("Setting default stop psscr
> > val=0x%016llx,mask=0x%016llx\n",
> > + pr_warn("powernv:idle: Default stop not found. Disabling
> > ppcmd.powersave\n");
> > + } else {
> > + pr_info("powernv:idle: Default stop: psscr =
> > 0x%016llx,mask=0x%016llx\n",
> > pnv_default_stop_val, pnv_default_stop_mask);
> > }
> >
> > if (!deepest_stop_found) {
> > - pnv_deepest_stop_psscr_val = PSSCR_HV_DEFAULT_VAL;
> > - pnv_deepest_stop_psscr_mask = PSSCR_HV_DEFAULT_MASK;
> > - pr_warn("Setting default stop psscr
> > val=0x%016llx,mask=0x%016llx\n",
> > + pr_warn("powernv:idle: Deepest stop not found. CPU-Hotplug is
> > affected\n");
>
> I guess these warnings are meant for developers, but it might be nice
> to make them slightly more meaningful? Prefix of choice seems to be
> "cpuidle-powernv:"
>
> Then you could have "no suitable stop state provided by firmware. Disabling
> idle power saving" and "no suitable stop state provided by firmware. Offlined
> CPUs will busy-wait", perhaps?
How about
pr_warn("cpuidle-powernv: No suitable stop for CPU-Hotplug.
Offlined CPUs will busy wait\n");
>
> Just a suggestion.
>
> > + } else {
> > + pr_info("powernv:idle: Deepest stop: psscr =
>
Re: [PATCH 2/3] powernv:idle: Don't override default/deepest directly in kernel
On Mon, 13 Mar 2017 11:31:27 +0530
"Gautham R. Shenoy" wrote:
> From: "Gautham R. Shenoy"
>
> Currently during idle-init on power9, if we don't find suitable stop
> states in the device tree that can be used as the
> default_stop/deepest_stop, we set stop0 (ESL=1,EC=1) as the default
> stop state psscr to be used by power9_idle and deepest stop state
> which is used by CPU-Hotplug.
>
> However, if the platform firmware has not configured or enabled a stop
> state, the kernel should not make any assumptions and fallback to a
> default choice.
>
> If the kernel uses a stop state that is not configured by the platform
> firmware, it may lead to further failures which should be avoided.
>
> In this patch, we modify the init code to ensure that the kernel uses
> only the stop states exposed by the firmware through the device
> tree. When a suitable default stop state isn't found, we disable
> ppc_md.power_save for power9. Similarly, when a suitable
> deepest_stop_state is not found in the device tree exported by the
> firmware, fall back to the default busy-wait loop in the CPU-Hotplug
> code.
Seems reasonable. I have a few comments that you may consider. Nothing
too major.
Btw., it would be nice to move this hotplug idling selection code to
idle.c. Have the hotplug just ask to enter the best available idle mode
and that's it. I'm not asking you to do that for this series, but perhaps
consider it for the future.
> diff --git a/arch/powerpc/platforms/powernv/idle.c
> b/arch/powerpc/platforms/powernv/idle.c
> index 4ee837e..9fde6e4 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -147,7 +147,6 @@ u32 pnv_get_supported_cpuidle_states(void)
> }
> EXPORT_SYMBOL_GPL(pnv_get_supported_cpuidle_states);
>
> -
> static void pnv_fastsleep_workaround_apply(void *info)
>
> {
> @@ -243,6 +242,7 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600,
> */
> u64 pnv_default_stop_val;
> u64 pnv_default_stop_mask;
> +bool default_stop_found;
>
> /*
> * Used for ppc_md.power_save which needs a function with no parameters
> @@ -264,6 +264,7 @@ static void power9_idle(void)
> */
> u64 pnv_deepest_stop_psscr_val;
> u64 pnv_deepest_stop_psscr_mask;
> +bool deepest_stop_found;
>
> /*
> * Power ISA 3.0 idle initialization.
If the hotplug idle code was in idle.c, then all this deepest/default stop
logic and register settings would be static to idle.c, which would be nice.
If you have a function to check if deepest stop is found, then you don't need
a non-static variable here (or for default_stop_found AFAIKS).
> @@ -352,7 +353,6 @@ static int __init pnv_power9_idle_init(struct device_node
> *np, u32 *flags,
> u32 *residency_ns = NULL;
> u64 max_residency_ns = 0;
> int rc = 0, i;
> - bool default_stop_found = false, deepest_stop_found = false;
>
> psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), GFP_KERNEL);
> psscr_mask = kcalloc(dt_idle_states, sizeof(*psscr_mask), GFP_KERNEL);
> @@ -433,20 +433,22 @@ static int __init pnv_power9_idle_init(struct
> device_node *np, u32 *flags,
> }
>
> if (!default_stop_found) {
> - pnv_default_stop_val = PSSCR_HV_DEFAULT_VAL;
> - pnv_default_stop_mask = PSSCR_HV_DEFAULT_MASK;
> - pr_warn("Setting default stop psscr
> val=0x%016llx,mask=0x%016llx\n",
> + pr_warn("powernv:idle: Default stop not found. Disabling
> ppcmd.powersave\n");
> + } else {
> + pr_info("powernv:idle: Default stop: psscr =
> 0x%016llx,mask=0x%016llx\n",
> pnv_default_stop_val, pnv_default_stop_mask);
> }
>
> if (!deepest_stop_found) {
> - pnv_deepest_stop_psscr_val = PSSCR_HV_DEFAULT_VAL;
> - pnv_deepest_stop_psscr_mask = PSSCR_HV_DEFAULT_MASK;
> - pr_warn("Setting default stop psscr
> val=0x%016llx,mask=0x%016llx\n",
> + pr_warn("powernv:idle: Deepest stop not found. CPU-Hotplug is
> affected\n");
I guess these warnings are meant for developers, but it might be nice
to make them slightly more meaningful? Prefix of choice seems to be
"cpuidle-powernv:"
Then you could have "no suitable stop state provided by firmware. Disabling
idle power saving" and "no suitable stop state provided by firmware. Offlined
CPUs will busy-wait", perhaps?
Just a suggestion.
> + } else {
> + pr_info("powernv:idle: Deepest stop: psscr =
> 0x%016llx,mask=0x%016llx\n",
> pnv_deepest_stop_psscr_val,
> pnv_deepest_stop_psscr_mask);
> }
>
> + pr_info("powernv:idle: RL value of first deep stop = 0x%llx\n",
> + pnv_first_deep_stop_state);
cpuidle-powernv: prefix for these too?
> out:
> kfree(psscr_val);
> kfree(psscr_mask);
> @@ -454,6 +456,12 @@ static int __init pnv_power9_idle_init(struct
> device_node *np, u32 *flags,
> return rc;
> }
>
> +
[PATCH 2/3] powernv:idle: Don't override default/deepest directly in kernel
From: "Gautham R. Shenoy"
Currently during idle-init on power9, if we don't find suitable stop
states in the device tree that can be used as the
default_stop/deepest_stop, we set stop0 (ESL=1,EC=1) as the default
stop state psscr to be used by power9_idle and deepest stop state
which is used by CPU-Hotplug.
However, if the platform firmware has not configured or enabled a stop
state, the kernel should not make any assumptions and fallback to a
default choice.
If the kernel uses a stop state that is not configured by the platform
firmware, it may lead to further failures which should be avoided.
In this patch, we modify the init code to ensure that the kernel uses
only the stop states exposed by the firmware through the device
tree. When a suitable default stop state isn't found, we disable
ppc_md.power_save for power9. Similarly, when a suitable
deepest_stop_state is not found in the device tree exported by the
firmware, fall back to the default busy-wait loop in the CPU-Hotplug
code.
[Changelog written with inputs from sva...@linux.vnet.ibm.com]
Signed-off-by: Gautham R. Shenoy
---
arch/powerpc/platforms/powernv/idle.c| 27 ++-
arch/powerpc/platforms/powernv/powernv.h | 1 +
arch/powerpc/platforms/powernv/smp.c | 8 +++-
3 files changed, 26 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/idle.c
b/arch/powerpc/platforms/powernv/idle.c
index 4ee837e..9fde6e4 100644
--- a/arch/powerpc/platforms/powernv/idle.c
+++ b/arch/powerpc/platforms/powernv/idle.c
@@ -147,7 +147,6 @@ u32 pnv_get_supported_cpuidle_states(void)
}
EXPORT_SYMBOL_GPL(pnv_get_supported_cpuidle_states);
-
static void pnv_fastsleep_workaround_apply(void *info)
{
@@ -243,6 +242,7 @@ static DEVICE_ATTR(fastsleep_workaround_applyonce, 0600,
*/
u64 pnv_default_stop_val;
u64 pnv_default_stop_mask;
+bool default_stop_found;
/*
* Used for ppc_md.power_save which needs a function with no parameters
@@ -264,6 +264,7 @@ static void power9_idle(void)
*/
u64 pnv_deepest_stop_psscr_val;
u64 pnv_deepest_stop_psscr_mask;
+bool deepest_stop_found;
/*
* Power ISA 3.0 idle initialization.
@@ -352,7 +353,6 @@ static int __init pnv_power9_idle_init(struct device_node
*np, u32 *flags,
u32 *residency_ns = NULL;
u64 max_residency_ns = 0;
int rc = 0, i;
- bool default_stop_found = false, deepest_stop_found = false;
psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), GFP_KERNEL);
psscr_mask = kcalloc(dt_idle_states, sizeof(*psscr_mask), GFP_KERNEL);
@@ -433,20 +433,22 @@ static int __init pnv_power9_idle_init(struct device_node
*np, u32 *flags,
}
if (!default_stop_found) {
- pnv_default_stop_val = PSSCR_HV_DEFAULT_VAL;
- pnv_default_stop_mask = PSSCR_HV_DEFAULT_MASK;
- pr_warn("Setting default stop psscr
val=0x%016llx,mask=0x%016llx\n",
+ pr_warn("powernv:idle: Default stop not found. Disabling
ppcmd.powersave\n");
+ } else {
+ pr_info("powernv:idle: Default stop: psscr =
0x%016llx,mask=0x%016llx\n",
pnv_default_stop_val, pnv_default_stop_mask);
}
if (!deepest_stop_found) {
- pnv_deepest_stop_psscr_val = PSSCR_HV_DEFAULT_VAL;
- pnv_deepest_stop_psscr_mask = PSSCR_HV_DEFAULT_MASK;
- pr_warn("Setting default stop psscr
val=0x%016llx,mask=0x%016llx\n",
+ pr_warn("powernv:idle: Deepest stop not found. CPU-Hotplug is
affected\n");
+ } else {
+ pr_info("powernv:idle: Deepest stop: psscr =
0x%016llx,mask=0x%016llx\n",
pnv_deepest_stop_psscr_val,
pnv_deepest_stop_psscr_mask);
}
+ pr_info("powernv:idle: RL value of first deep stop = 0x%llx\n",
+ pnv_first_deep_stop_state);
out:
kfree(psscr_val);
kfree(psscr_mask);
@@ -454,6 +456,12 @@ static int __init pnv_power9_idle_init(struct device_node
*np, u32 *flags,
return rc;
}
+bool pnv_check_deepest_stop(void)
+{
+ return deepest_stop_found;
+}
+EXPORT_SYMBOL_GPL(pnv_check_deepest_stop);
+
/*
* Probe device tree for supported idle states
*/
@@ -526,7 +534,8 @@ static int __init pnv_init_idle_states(void)
if (supported_cpuidle_states & OPAL_PM_NAP_ENABLED)
ppc_md.power_save = power7_idle;
- else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST)
+ else if ((supported_cpuidle_states & OPAL_PM_STOP_INST_FAST) &&
+default_stop_found)
ppc_md.power_save = power9_idle;
out:
diff --git a/arch/powerpc/platforms/powernv/powernv.h
b/arch/powerpc/platforms/powernv/powernv.h
index 6130522..9acd5eb 100644
--- a/arch/powerpc/platforms/powernv/powernv.h
+++ b/arch/powerpc/platforms/powernv/powernv.h
@@ -18,6 +18,7 @@ static inline void pnv_pci_shutdown(void) { }
#endif
extern u32 pnv_g
