Re: [PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Michael Ellerman]

Gautham R Shenoy  writes:

> Hi Michael,
>
> On Wed, Dec 06, 2017 at 09:54:27PM +1100, Michael Ellerman wrote:
>> Shilpasri G Bhat  writes:
>> 
>> > From: "Gautham R. Shenoy" 
>> >
>> > Pstates are 8bit values but on POWER8 they are negative and on POWER9
>> > they are positive. This patch adds helper routines to differentiate
>> > the sign to read the correct pstate value.
>> 
>> This sounds like it could be a bad bug, but I can't really tell from the
>> change log. What is the actual impact of not having this patch?
>
> On some POWER9 platforms, there can be more than 128 pstates. 
>
> Without this patch, on such platforms, if the value of the current
> frequency corresponds to a pstate greater than 128, then the code will
> interpret it as a negative number, and report that the "pstate is out
> of bound" while returning a nominal frequency.

OK, that's good detail to have in the change log, please include it.

>> Should it have a Fixes/Cc-stable tag?
>
> This doesn't fix any prior commit, but is fixes a newly discovered
> bug.

OK. You could say it "fixes" the commit that added Power9 support to the
driver, but it seems there wasn't really a commit that did that
specifically.

> I will resend the patch Cc'ing stable.

You don't have to Cc stable, that was just a suggestion. Though it
sounds like the symptoms are probably bad enough to warrant it.

cheers

Re: [PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Michael Ellerman]

Gautham R Shenoy  writes:

> Hi Michael,
>
> On Wed, Dec 06, 2017 at 09:54:27PM +1100, Michael Ellerman wrote:
>> Shilpasri G Bhat  writes:
>> 
>> > From: "Gautham R. Shenoy" 
>> >
>> > Pstates are 8bit values but on POWER8 they are negative and on POWER9
>> > they are positive. This patch adds helper routines to differentiate
>> > the sign to read the correct pstate value.
>> 
>> This sounds like it could be a bad bug, but I can't really tell from the
>> change log. What is the actual impact of not having this patch?
>
> On some POWER9 platforms, there can be more than 128 pstates. 
>
> Without this patch, on such platforms, if the value of the current
> frequency corresponds to a pstate greater than 128, then the code will
> interpret it as a negative number, and report that the "pstate is out
> of bound" while returning a nominal frequency.

OK, that's good detail to have in the change log, please include it.

>> Should it have a Fixes/Cc-stable tag?
>
> This doesn't fix any prior commit, but is fixes a newly discovered
> bug.

OK. You could say it "fixes" the commit that added Power9 support to the
driver, but it seems there wasn't really a commit that did that
specifically.

> I will resend the patch Cc'ing stable.

You don't have to Cc stable, that was just a suggestion. Though it
sounds like the symptoms are probably bad enough to warrant it.

cheers

Re: [PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Gautham R Shenoy]

Hi Michael,

On Wed, Dec 06, 2017 at 09:54:27PM +1100, Michael Ellerman wrote:
> Shilpasri G Bhat  writes:
> 
> > From: "Gautham R. Shenoy" 
> >
> > Pstates are 8bit values but on POWER8 they are negative and on POWER9
> > they are positive. This patch adds helper routines to differentiate
> > the sign to read the correct pstate value.
> 
> This sounds like it could be a bad bug, but I can't really tell from the
> change log. What is the actual impact of not having this patch?

On some POWER9 platforms, there can be more than 128 pstates. 

Without this patch, on such platforms, if the value of the current
frequency corresponds to a pstate greater than 128, then the code will
interpret it as a negative number, and report that the "pstate is out
of bound" while returning a nominal frequency.

> 
> Should it have a Fixes/Cc-stable tag?

This doesn't fix any prior commit, but is fixes a newly discovered
bug.

I will resend the patch Cc'ing stable.

> 
> cheers
> 

Re: [PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Gautham R Shenoy]

Hi Michael,

On Wed, Dec 06, 2017 at 09:54:27PM +1100, Michael Ellerman wrote:
> Shilpasri G Bhat  writes:
> 
> > From: "Gautham R. Shenoy" 
> >
> > Pstates are 8bit values but on POWER8 they are negative and on POWER9
> > they are positive. This patch adds helper routines to differentiate
> > the sign to read the correct pstate value.
> 
> This sounds like it could be a bad bug, but I can't really tell from the
> change log. What is the actual impact of not having this patch?

On some POWER9 platforms, there can be more than 128 pstates. 

Without this patch, on such platforms, if the value of the current
frequency corresponds to a pstate greater than 128, then the code will
interpret it as a negative number, and report that the "pstate is out
of bound" while returning a nominal frequency.

> 
> Should it have a Fixes/Cc-stable tag?

This doesn't fix any prior commit, but is fixes a newly discovered
bug.

I will resend the patch Cc'ing stable.

> 
> cheers
> 

Re: [PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Michael Ellerman]

Shilpasri G Bhat  writes:

> From: "Gautham R. Shenoy" 
>
> Pstates are 8bit values but on POWER8 they are negative and on POWER9
> they are positive. This patch adds helper routines to differentiate
> the sign to read the correct pstate value.

This sounds like it could be a bad bug, but I can't really tell from the
change log. What is the actual impact of not having this patch?

Should it have a Fixes/Cc-stable tag?

cheers

Re: [PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Michael Ellerman]

Shilpasri G Bhat  writes:

> From: "Gautham R. Shenoy" 
>
> Pstates are 8bit values but on POWER8 they are negative and on POWER9
> they are positive. This patch adds helper routines to differentiate
> the sign to read the correct pstate value.

This sounds like it could be a bad bug, but I can't really tell from the
change log. What is the actual impact of not having this patch?

Should it have a Fixes/Cc-stable tag?

cheers

Re: [PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Viresh Kumar]

On 30-11-17, 10:13, Shilpasri G Bhat wrote:
> From: "Gautham R. Shenoy" 
> 
> Pstates are 8bit values but on POWER8 they are negative and on POWER9
> they are positive. This patch adds helper routines to differentiate
> the sign to read the correct pstate value.
> 
> Signed-off-by: Gautham R. Shenoy 
> Tested-by: Shilpasri G Bhat 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 43 
> ++-
>  1 file changed, 33 insertions(+), 10 deletions(-)

Acked-by: Viresh Kumar 

-- 
viresh

Re: [PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Viresh Kumar]

On 30-11-17, 10:13, Shilpasri G Bhat wrote:
> From: "Gautham R. Shenoy" 
> 
> Pstates are 8bit values but on POWER8 they are negative and on POWER9
> they are positive. This patch adds helper routines to differentiate
> the sign to read the correct pstate value.
> 
> Signed-off-by: Gautham R. Shenoy 
> Tested-by: Shilpasri G Bhat 
> ---
>  drivers/cpufreq/powernv-cpufreq.c | 43 
> ++-
>  1 file changed, 33 insertions(+), 10 deletions(-)

Acked-by: Viresh Kumar 

-- 
viresh

[PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Shilpasri G Bhat]

From: "Gautham R. Shenoy" 

Pstates are 8bit values but on POWER8 they are negative and on POWER9
they are positive. This patch adds helper routines to differentiate
the sign to read the correct pstate value.

Signed-off-by: Gautham R. Shenoy 
Tested-by: Shilpasri G Bhat 
---
 drivers/cpufreq/powernv-cpufreq.c | 43 ++-
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index b6d7c4c..bb7586e 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -41,11 +41,14 @@
 #define POWERNV_MAX_PSTATES256
 #define PMSR_PSAFE_ENABLE  (1UL << 30)
 #define PMSR_SPR_EM_DISABLE(1UL << 31)
-#define PMSR_MAX(x)((x >> 32) & 0xFF)
+#define EXTRACT_BYTE(x, shift) (((x) >> shift) & 0xFF)
+#define MAX_SHIFT  32
 #define LPSTATE_SHIFT  48
 #define GPSTATE_SHIFT  56
-#define GET_LPSTATE(x) (((x) >> LPSTATE_SHIFT) & 0xFF)
-#define GET_GPSTATE(x) (((x) >> GPSTATE_SHIFT) & 0xFF)
+#define GET_PMSR_MAX(x)EXTRACT_BYTE(x, MAX_SHIFT)
+#define GET_LPSTATE(x) EXTRACT_BYTE(x, LPSTATE_SHIFT)
+#define GET_GPSTATE(x) EXTRACT_BYTE(x, GPSTATE_SHIFT)
+
 
 #define MAX_RAMP_DOWN_TIME 5120
 /*
@@ -64,6 +67,12 @@
 
 /* Interval after which the timer is queued to bring down global pstate */
 #define GPSTATE_TIMER_INTERVAL 2000
+/*
+ * On POWER8 the pstates are negatively numbered. On POWER9, they are
+ * positively numbered.  Use this flag to track whether we have
+ * positive or negative numbered pstates.
+ */
+static bool pos_pstates;
 
 /**
  * struct global_pstate_info - Per policy data structure to maintain history of
@@ -164,7 +173,7 @@ static inline unsigned int pstate_to_idx(int pstate)
int min = powernv_freqs[powernv_pstate_info.min].driver_data;
int max = powernv_freqs[powernv_pstate_info.max].driver_data;
 
-   if (min > 0) {
+   if (pos_pstates) {
if (unlikely((pstate < max) || (pstate > min))) {
pr_warn_once("pstate %d is out of bound\n", pstate);
return powernv_pstate_info.nominal;
@@ -301,6 +310,9 @@ static int init_powernv_pstates(void)
}
}
 
+   if ((int)pstate_min > 0)
+   pos_pstates = true;
+
/* End of list marker entry */
powernv_freqs[i].frequency = CPUFREQ_TABLE_END;
return 0;
@@ -438,7 +450,6 @@ struct powernv_smp_call_data {
 static void powernv_read_cpu_freq(void *arg)
 {
unsigned long pmspr_val;
-   s8 local_pstate_id;
struct powernv_smp_call_data *freq_data = arg;
 
pmspr_val = get_pmspr(SPRN_PMSR);
@@ -447,8 +458,11 @@ static void powernv_read_cpu_freq(void *arg)
 * The local pstate id corresponds bits 48..55 in the PMSR.
 * Note: Watch out for the sign!
 */
-   local_pstate_id = (pmspr_val >> 48) & 0xFF;
-   freq_data->pstate_id = local_pstate_id;
+   if (pos_pstates)
+   freq_data->pstate_id = (u8)GET_LPSTATE(pmspr_val);
+   else
+   freq_data->pstate_id = (s8)GET_LPSTATE(pmspr_val);
+
freq_data->freq = pstate_id_to_freq(freq_data->pstate_id);
 
pr_debug("cpu %d pmsr %016lX pstate_id %d frequency %d kHz\n",
@@ -522,7 +536,10 @@ static void powernv_cpufreq_throttle_check(void *data)
chip = this_cpu_read(chip_info);
 
/* Check for Pmax Capping */
-   pmsr_pmax = (s8)PMSR_MAX(pmsr);
+   if (pos_pstates)
+   pmsr_pmax = (u8)GET_PMSR_MAX(pmsr);
+   else
+   pmsr_pmax = (s8)GET_PMSR_MAX(pmsr);
pmsr_pmax_idx = pstate_to_idx(pmsr_pmax);
if (pmsr_pmax_idx != powernv_pstate_info.max) {
if (chip->throttled)
@@ -645,8 +662,14 @@ void gpstate_timer_handler(struct timer_list *t)
 * value. Hence, read from PMCR to get correct data.
 */
val = get_pmspr(SPRN_PMCR);
-   freq_data.gpstate_id = (s8)GET_GPSTATE(val);
-   freq_data.pstate_id = (s8)GET_LPSTATE(val);
+   if (pos_pstates) {
+   freq_data.gpstate_id = (u8)GET_GPSTATE(val);
+   freq_data.pstate_id = (u8)GET_LPSTATE(val);
+   } else {
+   freq_data.gpstate_id = (s8)GET_GPSTATE(val);
+   freq_data.pstate_id = (s8)GET_LPSTATE(val);
+   }
+
if (freq_data.gpstate_id  == freq_data.pstate_id) {
reset_gpstates(policy);
spin_unlock(>gpstate_lock);
-- 
1.8.3.1

[PATCH] cpufreq: powernv: Define methods to parse positive & negative pstates

[Shilpasri G Bhat]

From: "Gautham R. Shenoy" 

Pstates are 8bit values but on POWER8 they are negative and on POWER9
they are positive. This patch adds helper routines to differentiate
the sign to read the correct pstate value.

Signed-off-by: Gautham R. Shenoy 
Tested-by: Shilpasri G Bhat 
---
 drivers/cpufreq/powernv-cpufreq.c | 43 ++-
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index b6d7c4c..bb7586e 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -41,11 +41,14 @@
 #define POWERNV_MAX_PSTATES256
 #define PMSR_PSAFE_ENABLE  (1UL << 30)
 #define PMSR_SPR_EM_DISABLE(1UL << 31)
-#define PMSR_MAX(x)((x >> 32) & 0xFF)
+#define EXTRACT_BYTE(x, shift) (((x) >> shift) & 0xFF)
+#define MAX_SHIFT  32
 #define LPSTATE_SHIFT  48
 #define GPSTATE_SHIFT  56
-#define GET_LPSTATE(x) (((x) >> LPSTATE_SHIFT) & 0xFF)
-#define GET_GPSTATE(x) (((x) >> GPSTATE_SHIFT) & 0xFF)
+#define GET_PMSR_MAX(x)EXTRACT_BYTE(x, MAX_SHIFT)
+#define GET_LPSTATE(x) EXTRACT_BYTE(x, LPSTATE_SHIFT)
+#define GET_GPSTATE(x) EXTRACT_BYTE(x, GPSTATE_SHIFT)
+
 
 #define MAX_RAMP_DOWN_TIME 5120
 /*
@@ -64,6 +67,12 @@
 
 /* Interval after which the timer is queued to bring down global pstate */
 #define GPSTATE_TIMER_INTERVAL 2000
+/*
+ * On POWER8 the pstates are negatively numbered. On POWER9, they are
+ * positively numbered.  Use this flag to track whether we have
+ * positive or negative numbered pstates.
+ */
+static bool pos_pstates;
 
 /**
  * struct global_pstate_info - Per policy data structure to maintain history of
@@ -164,7 +173,7 @@ static inline unsigned int pstate_to_idx(int pstate)
int min = powernv_freqs[powernv_pstate_info.min].driver_data;
int max = powernv_freqs[powernv_pstate_info.max].driver_data;
 
-   if (min > 0) {
+   if (pos_pstates) {
if (unlikely((pstate < max) || (pstate > min))) {
pr_warn_once("pstate %d is out of bound\n", pstate);
return powernv_pstate_info.nominal;
@@ -301,6 +310,9 @@ static int init_powernv_pstates(void)
}
}
 
+   if ((int)pstate_min > 0)
+   pos_pstates = true;
+
/* End of list marker entry */
powernv_freqs[i].frequency = CPUFREQ_TABLE_END;
return 0;
@@ -438,7 +450,6 @@ struct powernv_smp_call_data {
 static void powernv_read_cpu_freq(void *arg)
 {
unsigned long pmspr_val;
-   s8 local_pstate_id;
struct powernv_smp_call_data *freq_data = arg;
 
pmspr_val = get_pmspr(SPRN_PMSR);
@@ -447,8 +458,11 @@ static void powernv_read_cpu_freq(void *arg)
 * The local pstate id corresponds bits 48..55 in the PMSR.
 * Note: Watch out for the sign!
 */
-   local_pstate_id = (pmspr_val >> 48) & 0xFF;
-   freq_data->pstate_id = local_pstate_id;
+   if (pos_pstates)
+   freq_data->pstate_id = (u8)GET_LPSTATE(pmspr_val);
+   else
+   freq_data->pstate_id = (s8)GET_LPSTATE(pmspr_val);
+
freq_data->freq = pstate_id_to_freq(freq_data->pstate_id);
 
pr_debug("cpu %d pmsr %016lX pstate_id %d frequency %d kHz\n",
@@ -522,7 +536,10 @@ static void powernv_cpufreq_throttle_check(void *data)
chip = this_cpu_read(chip_info);
 
/* Check for Pmax Capping */
-   pmsr_pmax = (s8)PMSR_MAX(pmsr);
+   if (pos_pstates)
+   pmsr_pmax = (u8)GET_PMSR_MAX(pmsr);
+   else
+   pmsr_pmax = (s8)GET_PMSR_MAX(pmsr);
pmsr_pmax_idx = pstate_to_idx(pmsr_pmax);
if (pmsr_pmax_idx != powernv_pstate_info.max) {
if (chip->throttled)
@@ -645,8 +662,14 @@ void gpstate_timer_handler(struct timer_list *t)
 * value. Hence, read from PMCR to get correct data.
 */
val = get_pmspr(SPRN_PMCR);
-   freq_data.gpstate_id = (s8)GET_GPSTATE(val);
-   freq_data.pstate_id = (s8)GET_LPSTATE(val);
+   if (pos_pstates) {
+   freq_data.gpstate_id = (u8)GET_GPSTATE(val);
+   freq_data.pstate_id = (u8)GET_LPSTATE(val);
+   } else {
+   freq_data.gpstate_id = (s8)GET_GPSTATE(val);
+   freq_data.pstate_id = (s8)GET_LPSTATE(val);
+   }
+
if (freq_data.gpstate_id  == freq_data.pstate_id) {
reset_gpstates(policy);
spin_unlock(>gpstate_lock);
-- 
1.8.3.1