| Message ID | 1465404871-5406-11-git-send-email-shreyas@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Jun 08, 2016 at 11:54:30AM -0500, Shreyas B. Prabhu wrote: > POWER ISA v3 defines a new idle processor core mechanism. In summary, > a) new instruction named stop is added. > b) new per thread SPR named PSSCR is added which controls the behavior > of stop instruction. > > Supported idle states and value to be written to PSSCR register to enter > any idle state is exposed via ibm,cpu-idle-state-names and > ibm,cpu-idle-state-psscr respectively. To enter an idle state, > platform provided power_stop() needs to be invoked with the appropriate > PSSCR value. > > This patch adds support for this new mechanism in cpuidle powernv driver. > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com> > Cc: linux-pm@vger.kernel.org > Cc: Michael Ellerman <mpe@ellerman.id.au> > Cc: Paul Mackerras <paulus@ozlabs.org> > Cc: linuxppc-dev@lists.ozlabs.org > Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> > Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com> > --- [ ... ] > + rc = of_property_read_string_array(power_mgt, > + "ibm,cpu-idle-state-names", names, > + dt_idle_states); > + if (rc < 0) { > + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n"); > + goto out_free_latency; > + } > + > + /* > + * If the idle states use stop instruction, probe for psscr values > + * which are necessary to specify required stop level. > + */ > + if (flags[0] & (OPAL_PM_STOP_INST_FAST | OPAL_PM_STOP_INST_DEEP)) { > + psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), > + GFP_KERNEL); if (!psscr_val) check missing. > + rc = of_property_read_u64_array(power_mgt, > + "ibm,cpu-idle-state-psscr", > + psscr_val, dt_idle_states); > + if (rc) { > + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-states-psscr in DT\n"); > + goto out_free_psscr; > + } > + } > residency_ns = kzalloc(sizeof(*residency_ns) * dt_idle_states, GFP_KERNEL); if (!residency_ns) check missing. I suppose the code is relying on 'of_property_read_u32_array' to check it, right ? -- Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2016-06-08 at 11:54 -0500, Shreyas B. Prabhu wrote: > > /* > * States for dedicated partition case. > */ > @@ -167,6 +183,8 @@ static int powernv_add_idle_states(void) > int nr_idle_states = 1; /* Snooze */ > int dt_idle_states; > u32 *latency_ns, *residency_ns, *flags; > + u64 *psscr_val = NULL; > + const char *names[CPUIDLE_STATE_MAX]; > int i, rc; > > /* Currently we have snooze statically defined */ > @@ -199,12 +217,41 @@ static int powernv_add_idle_states(void) > goto out_free_latency; > } > > + rc = of_property_read_string_array(power_mgt, > + "ibm,cpu-idle-state-names", names, > + dt_idle_states); Ok so from this I assume that dt_idle_states is the number of entries, which has been checked properly to be < CPUIDLE_STATE_MAX correct ? Beause ... > + if (rc < 0) { > + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n"); > + goto out_free_latency; > + } > + > + /* > + * If the idle states use stop instruction, probe for psscr values > + * which are necessary to specify required stop level. > + */ > + if (flags[0] & (OPAL_PM_STOP_INST_FAST | OPAL_PM_STOP_INST_DEEP)) { > + psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), > + GFP_KERNEL); > + rc = of_property_read_u64_array(power_mgt, > + "ibm,cpu-idle-state-psscr", > + psscr_val, dt_idle_states); Here, psscr val is only one u64 ... shouldn't you kmalloc sizeof(..) * dt_idle_states ? > + if (rc) { > + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-states-psscr in DT\n"); > + goto out_free_psscr; > + } > + } > residency_ns = kzalloc(sizeof(*residency_ns) * dt_idle_states, GFP_KERNEL); Just like we do here > rc = of_property_read_u32_array(power_mgt, "ibm,cpu-idle-state-residency-ns", residency_ns, dt_idle_states); > for (i = 0; i < dt_idle_states; i++) { > - > + /* > + * If an idle state has exit latency beyond > + * POWERNV_THRESHOLD_LATENCY_NS then don't use it > + * in cpu-idle. > + */ > + if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS) > + continue; > /* > * Cpuidle accepts exit_latency and target_residency in us. > * Use default target_residency values if f/w does not expose it. > @@ -216,6 +263,16 @@ static int powernv_add_idle_states(void) > powernv_states[nr_idle_states].flags = 0; > powernv_states[nr_idle_states].target_residency = 100; > powernv_states[nr_idle_states].enter = &nap_loop; > + } else if ((flags[i] & OPAL_PM_STOP_INST_FAST) && > + !(flags[i] & OPAL_PM_TIMEBASE_STOP)) { > + strncpy(powernv_states[nr_idle_states].name, > + names[i], CPUIDLE_NAME_LEN); > + strncpy(powernv_states[nr_idle_states].desc, > + names[i], CPUIDLE_NAME_LEN); > + powernv_states[nr_idle_states].flags = 0; > + > + powernv_states[nr_idle_states].enter = stop_loop; > + stop_psscr_table[nr_idle_states] = psscr_val[i]; > } > > /* > @@ -231,6 +288,16 @@ static int powernv_add_idle_states(void) > powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP; > powernv_states[nr_idle_states].target_residency = 300000; > powernv_states[nr_idle_states].enter = &fastsleep_loop; > + } else if ((flags[i] & OPAL_PM_STOP_INST_DEEP) && > + (flags[i] & OPAL_PM_TIMEBASE_STOP)) { > + strncpy(powernv_states[nr_idle_states].name, > + names[i], CPUIDLE_NAME_LEN); > + strncpy(powernv_states[nr_idle_states].desc, > + names[i], CPUIDLE_NAME_LEN); > + > + powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP; > + powernv_states[nr_idle_states].enter = stop_loop; > + stop_psscr_table[nr_idle_states] = psscr_val[i]; > } > #endif > powernv_states[nr_idle_states].exit_latency = > @@ -245,6 +312,8 @@ static int powernv_add_idle_states(void) > } > > kfree(residency_ns); > +out_free_psscr: > + kfree(psscr_val); > out_free_latency: > kfree(latency_ns); > out_free_flags: -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/13/2016 09:04 PM, Daniel Lezcano wrote: > On Wed, Jun 08, 2016 at 11:54:30AM -0500, Shreyas B. Prabhu wrote: >> POWER ISA v3 defines a new idle processor core mechanism. In summary, >> a) new instruction named stop is added. >> b) new per thread SPR named PSSCR is added which controls the behavior >> of stop instruction. >> >> Supported idle states and value to be written to PSSCR register to enter >> any idle state is exposed via ibm,cpu-idle-state-names and >> ibm,cpu-idle-state-psscr respectively. To enter an idle state, >> platform provided power_stop() needs to be invoked with the appropriate >> PSSCR value. >> >> This patch adds support for this new mechanism in cpuidle powernv driver. >> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> Cc: Daniel Lezcano <daniel.lezcano@linaro.org> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com> >> Cc: linux-pm@vger.kernel.org >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Cc: Paul Mackerras <paulus@ozlabs.org> >> Cc: linuxppc-dev@lists.ozlabs.org >> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com> >> Signed-off-by: Shreyas B. Prabhu <shreyas@linux.vnet.ibm.com> >> --- > > [ ... ] > >> + rc = of_property_read_string_array(power_mgt, >> + "ibm,cpu-idle-state-names", names, >> + dt_idle_states); >> + if (rc < 0) { >> + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n"); >> + goto out_free_latency; >> + } >> + >> + /* >> + * If the idle states use stop instruction, probe for psscr values >> + * which are necessary to specify required stop level. >> + */ >> + if (flags[0] & (OPAL_PM_STOP_INST_FAST | OPAL_PM_STOP_INST_DEEP)) { >> + psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), >> + GFP_KERNEL); > > if (!psscr_val) check missing. I ignored adding this check because this is part of initcall and we are unlikely to run out of memory at this state. But I'll add the check in next version. > >> + rc = of_property_read_u64_array(power_mgt, >> + "ibm,cpu-idle-state-psscr", >> + psscr_val, dt_idle_states); >> + if (rc) { >> + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-states-psscr in DT\n"); >> + goto out_free_psscr; >> + } >> + } >> residency_ns = kzalloc(sizeof(*residency_ns) * dt_idle_states, GFP_KERNEL); > > if (!residency_ns) check missing. > > I suppose the code is relying on 'of_property_read_u32_array' to check it, > right ? I'll add the NULL check for existing kzalloc's in the file as well in the next version. Thanks, Shreyas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/14/2016 03:18 AM, Benjamin Herrenschmidt wrote: > On Wed, 2016-06-08 at 11:54 -0500, Shreyas B. Prabhu wrote: >> >> /* >> * States for dedicated partition case. >> */ >> @@ -167,6 +183,8 @@ static int powernv_add_idle_states(void) >> int nr_idle_states = 1; /* Snooze */ >> int dt_idle_states; >> u32 *latency_ns, *residency_ns, *flags; >> + u64 *psscr_val = NULL; >> + const char *names[CPUIDLE_STATE_MAX]; >> int i, rc; >> >> /* Currently we have snooze statically defined */ >> @@ -199,12 +217,41 @@ static int powernv_add_idle_states(void) >> goto out_free_latency; >> } >> >> + rc = of_property_read_string_array(power_mgt, >> + "ibm,cpu-idle-state-names", names, >> + dt_idle_states); > > Ok so from this I assume that dt_idle_states is the number of entries, > which has been checked properly to be < CPUIDLE_STATE_MAX correct ? > > Beause ... > While dt_idle_states should not be > CPUIDLE_STATE_MAX, if that were the case we will end up corrupting memory while updating powernv_states[]. I'll add a WARN_ON for such a case and handle adding idle states to powernv_states accordingly. Thanks for pointing this out. >> + if (rc < 0) { >> + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n"); >> + goto out_free_latency; >> + } >> + >> + /* >> + * If the idle states use stop instruction, probe for psscr values >> + * which are necessary to specify required stop level. >> + */ >> + if (flags[0] & (OPAL_PM_STOP_INST_FAST | OPAL_PM_STOP_INST_DEEP)) { >> + psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), >> + GFP_KERNEL); >> + rc = of_property_read_u64_array(power_mgt, >> + "ibm,cpu-idle-state-psscr", >> + psscr_val, dt_idle_states); > > Here, psscr val is only one u64 ... shouldn't you kmalloc sizeof(..) * > dt_idle_states ? I'm using kcalloc here since checkpatch script suggested kcalloc over kzalloc for allocating memory for arrays. I'll also include a patch to use kcalloc throughout the file for uniformity in next version. I was originally planning to post that cleanup separately. Thanks, Shreyas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2016-06-14 at 16:17 +0530, Shreyas B Prabhu wrote: > > I ignored adding this check because this is part of initcall and we are > unlikely to run out of memory at this state. But I'll add the check in > next version. Why do you malloc the u64 array and not the string pointer array ? Shouldn't you either have both on stack or both allocated ? Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
<1465404871-5406-11-git-send-email-shreyas@linux.vnet.ibm.com> <1465854492.3022.30.camel@au1.ibm.com> <575FE64C.9080107@linux.vnet.ibm.com> Organization: IBM Australia Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.3 (3.20.3-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit On Tue, 2016-06-14 at 16:41 +0530, Shreyas B Prabhu wrote: > >> + psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), > >> + GFP_KERNEL); > >> + rc = of_property_read_u64_array(power_mgt, > >> + "ibm,cpu-idle-state-psscr", > >> + psscr_val, dt_idle_states); > > > > Here, psscr val is only one u64 ... shouldn't you kmalloc sizeof(..) * > > dt_idle_states ? > > I'm using kcalloc here since checkpatch script suggested kcalloc over > kzalloc for allocating memory for arrays. > I'll also include a patch to use kcalloc throughout the file for > uniformity in next version. I was originally planning to post that > cleanup separately. Ah ok, I missed the use of kcalloc (I didn't even know its existence), my brain just read kmalloc :-) Still, I find it inconsistent that you allocate here while you use the stack for the names. Any reason for that ? Cheers, Ben. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/14/2016 04:59 PM, Benjamin Herrenschmidt wrote: > On Tue, 2016-06-14 at 16:17 +0530, Shreyas B Prabhu wrote: > >> >> I ignored adding this check because this is part of initcall and we are >> unlikely to run out of memory at this state. But I'll add the check in >> next version. > > Why do you malloc the u64 array and not the string pointer array ? > Shouldn't you either have both on stack or both allocated ? > Yes. I'll make this consistent. Thanks, Shreyas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
============= - Use generic cpuidle constant CPUIDLE_NAME_LEN - Fix return code handling for of_property_read_string_array - Use DT flags to determine if are using stop instruction, instead of cpu_has_feature - Removed uncessary cast with names - &stop_loop -> stop_loop - Added POWERNV_THRESHOLD_LATENCY_NS to filter out idle states with high latency drivers/cpuidle/cpuidle-powernv.c | 71 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c index 3a763a8..c74a020 100644 --- a/drivers/cpuidle/cpuidle-powernv.c +++ b/drivers/cpuidle/cpuidle-powernv.c @@ -20,6 +20,8 @@ #include <asm/opal.h> #include <asm/runlatch.h> +#define POWERNV_THRESHOLD_LATENCY_NS 200000 + struct cpuidle_driver powernv_idle_driver = { .name = "powernv_idle", .owner = THIS_MODULE, @@ -27,6 +29,9 @@ struct cpuidle_driver powernv_idle_driver = { static int max_idle_state; static struct cpuidle_state *cpuidle_state_table; + +static u64 stop_psscr_table[CPUIDLE_STATE_MAX]; + static u64 snooze_timeout; static bool snooze_timeout_en; @@ -91,6 +96,17 @@ static int fastsleep_loop(struct cpuidle_device *dev, return index; } #endif + +static int stop_loop(struct cpuidle_device *dev, + struct cpuidle_driver *drv, + int index) +{ + ppc64_runlatch_off(); + power_stop(stop_psscr_table[index]); + ppc64_runlatch_on(); + return index; +} + /* * States for dedicated partition case. */ @@ -167,6 +183,8 @@ static int powernv_add_idle_states(void) int nr_idle_states = 1; /* Snooze */ int dt_idle_states; u32 *latency_ns, *residency_ns, *flags; + u64 *psscr_val = NULL; + const char *names[CPUIDLE_STATE_MAX]; int i, rc; /* Currently we have snooze statically defined */ @@ -199,12 +217,41 @@ static int powernv_add_idle_states(void) goto out_free_latency; } + rc = of_property_read_string_array(power_mgt, + "ibm,cpu-idle-state-names", names, + dt_idle_states); + if (rc < 0) { + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n"); + goto out_free_latency; + } + + /* + * If the idle states use stop instruction, probe for psscr values + * which are necessary to specify required stop level. + */ + if (flags[0] & (OPAL_PM_STOP_INST_FAST | OPAL_PM_STOP_INST_DEEP)) { + psscr_val = kcalloc(dt_idle_states, sizeof(*psscr_val), + GFP_KERNEL); + rc = of_property_read_u64_array(power_mgt, + "ibm,cpu-idle-state-psscr", + psscr_val, dt_idle_states); + if (rc) { + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-states-psscr in DT\n"); + goto out_free_psscr; + } + } residency_ns = kzalloc(sizeof(*residency_ns) * dt_idle_states, GFP_KERNEL); rc = of_property_read_u32_array(power_mgt, "ibm,cpu-idle-state-residency-ns", residency_ns, dt_idle_states); for (i = 0; i < dt_idle_states; i++) { - + /* + * If an idle state has exit latency beyond + * POWERNV_THRESHOLD_LATENCY_NS then don't use it + * in cpu-idle. + */ + if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS) + continue; /* * Cpuidle accepts exit_latency and target_residency in us. * Use default target_residency values if f/w does not expose it. @@ -216,6 +263,16 @@ static int powernv_add_idle_states(void) powernv_states[nr_idle_states].flags = 0; powernv_states[nr_idle_states].target_residency = 100; powernv_states[nr_idle_states].enter = &nap_loop; + } else if ((flags[i] & OPAL_PM_STOP_INST_FAST) && + !(flags[i] & OPAL_PM_TIMEBASE_STOP)) { + strncpy(powernv_states[nr_idle_states].name, + names[i], CPUIDLE_NAME_LEN); + strncpy(powernv_states[nr_idle_states].desc, + names[i], CPUIDLE_NAME_LEN); + powernv_states[nr_idle_states].flags = 0; + + powernv_states[nr_idle_states].enter = stop_loop; + stop_psscr_table[nr_idle_states] = psscr_val[i]; } /* @@ -231,6 +288,16 @@ static int powernv_add_idle_states(void) powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP; powernv_states[nr_idle_states].target_residency = 300000; powernv_states[nr_idle_states].enter = &fastsleep_loop; + } else if ((flags[i] & OPAL_PM_STOP_INST_DEEP) && + (flags[i] & OPAL_PM_TIMEBASE_STOP)) { + strncpy(powernv_states[nr_idle_states].name, + names[i], CPUIDLE_NAME_LEN); + strncpy(powernv_states[nr_idle_states].desc, + names[i], CPUIDLE_NAME_LEN); + + powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP; + powernv_states[nr_idle_states].enter = stop_loop; + stop_psscr_table[nr_idle_states] = psscr_val[i]; } #endif powernv_states[nr_idle_states].exit_latency = @@ -245,6 +312,8 @@ static int powernv_add_idle_states(void) } kfree(residency_ns); +out_free_psscr: + kfree(psscr_val); out_free_latency: kfree(latency_ns); out_free_flags: