Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Shreyas B Prabhu]

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

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Shreyas B Prabhu]

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

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Benjamin Herrenschmidt]

 <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.

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Benjamin Herrenschmidt]

 <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.

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Benjamin Herrenschmidt]

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.

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Benjamin Herrenschmidt]

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.

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Shreyas B Prabhu]

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

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Shreyas B Prabhu]

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

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Shreyas B Prabhu]

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-...@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

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Shreyas B Prabhu]

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-...@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

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Benjamin Herrenschmidt]

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:

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Benjamin Herrenschmidt]

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:

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Daniel Lezcano]

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-...@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

Re: [PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Daniel Lezcano]

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-...@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

[PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Shreyas B. Prabhu]

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-...@lists.ozlabs.org
Reviewed-by: Gautham R. Shenoy 
Signed-off-by: Shreyas B. Prabhu 
---
Note: Documentation for the device tree bindings is posted here-
http://patchwork.ozlabs.org/patch/629125/

 - No changes in v6

Changes in v5
=
 - 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
 - _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 
 #include 
 
+#define POWERNV_THRESHOLD_LATENCY_NS 20
+
 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.
 

[PATCH v6 10/11] cpuidle/powernv: Add support for POWER ISA v3 idle states

[Shreyas B. Prabhu]

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-...@lists.ozlabs.org
Reviewed-by: Gautham R. Shenoy 
Signed-off-by: Shreyas B. Prabhu 
---
Note: Documentation for the device tree bindings is posted here-
http://patchwork.ozlabs.org/patch/629125/

 - No changes in v6

Changes in v5
=
 - 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
 - _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 
 #include 
 
+#define POWERNV_THRESHOLD_LATENCY_NS 20
+
 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;