Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states
<1465404871-5406-11-git-send-email-shre...@linux.vnet.ibm.com> <1465854492.3022.30.ca...@au1.ibm.com> <575fe64c.9080...@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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states
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. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states
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>> Cc: Daniel Lezcano >> Cc: Rob Herring >> Cc: Lorenzo Pieralisi >> Cc: linux...@vger.kernel.org >> Cc: Michael Ellerman >> Cc: Paul Mackerras >> Cc: linuxppc-dev@lists.ozlabs.org >> Reviewed-by: Gautham R. Shenoy >> Signed-off-by: Shreyas B. Prabhu >> --- > > [ ... ] > >> +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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states
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 = _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 = > 30; > powernv_states[nr_idle_states].enter = _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: ___ Linuxppc-dev mailing list
Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states
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> Cc: Daniel Lezcano > Cc: Rob Herring > Cc: Lorenzo Pieralisi > Cc: linux...@vger.kernel.org > Cc: Michael Ellerman > Cc: Paul Mackerras > Cc: linuxppc-dev@lists.ozlabs.org > Reviewed-by: Gautham R. Shenoy > Signed-off-by: Shreyas B. Prabhu > --- [ ... ] > + 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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev