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

2017-12-06 Thread 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

2017-12-06 Thread 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

2017-12-06 Thread 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

2017-12-06 Thread 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

2017-12-06 Thread 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

2017-12-06 Thread 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

2017-12-04 Thread 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

2017-12-04 Thread 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