* Re: [PATCH 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue [not found] ` <20210324082838.41462-2-xufuhai1992@gmail.com> @ 2021-03-29 10:58 ` Thomas Renninger 2021-03-30 2:46 ` xufuhai 0 siblings, 1 reply; 10+ messages in thread From: Thomas Renninger @ 2021-03-29 10:58 UTC (permalink / raw) To: linux, sherry.hurwitz, linux-pm, xufuhai, xufuhai; +Cc: lishujin Hi, Am Mittwoch, 24. März 2021, 09:28:38 CEST schrieb xufuhai: > From: xufuhai <xufuhai@kuaishou.com> > > If the read_msr function is executed by a non-root user, the function > returns -1, which means that there is no permission to access > /dev/cpu/%d/msr, but cpufreq_has_boost_support should also return -1 > immediately, and should not follow the original logic to return 0, which > will cause amd The cpupower tool returns the turbo active status as 0. Yes, this seem to be buggy. Can you clean this up a bit more, please: > Reproduce procedure: > cpupower frequency-info > > Signed-off-by: xufuhai <xufuhai@kuaishou.com> > --- > tools/power/cpupower/utils/helpers/misc.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/tools/power/cpupower/utils/helpers/misc.c > b/tools/power/cpupower/utils/helpers/misc.c index > fc6e34511721..be96f9ce18eb 100644 > --- a/tools/power/cpupower/utils/helpers/misc.c > +++ b/tools/power/cpupower/utils/helpers/misc.c > @@ -30,10 +30,15 @@ int cpufreq_has_boost_support(unsigned int cpu, int > *support, int *active, */ > > if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { > - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { > + ret = read_msr(cpu, MSR_AMD_HWCR, &val); > + if (!ret) { ret should be initialized. I would initialize it with -1, but as Intel case is always "good"/zero, it may make sense here to set: ret = 0 at the beginning of the func already. At the end of the func, unconditionally returning zero: return 0; should be replace by: return ret; > if (!(val & CPUPOWER_AMD_CPBDIS)) > *active = 1; > - } > + } else > + /* no permission to access /dev/cpu/%d/msr, return -1 immediately, > + * and should not follow the original logic to return 0 > + */ > + return ret; Then this part is not needed anymore, right? Still the comment would be nice to show up, maybe slightly modified in the if condition? Afaik 100% correct comment would be: /* ... */ for one line comment and: /* * ... * ... */ for multiline comment (one more line..). > } else { > ret = amd_pci_get_num_boost_states(active, states); > if (ret) and these 2 lines can vanish as well at this point: if (ret) return ret; What do you think? Thanks for spotting this, Thomas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue 2021-03-29 10:58 ` [PATCH 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue Thomas Renninger @ 2021-03-30 2:46 ` xufuhai 2021-04-08 2:21 ` xufuhai 0 siblings, 1 reply; 10+ messages in thread From: xufuhai @ 2021-03-30 2:46 UTC (permalink / raw) To: Thomas Renninger, linux, sherry.hurwitz, linux-pm, xufuhai; +Cc: lishujin Thanks for your review, Thomas～ as you suggested, I have updated my patch as your advice. Please help me review the patch again. thanks ---------------------------------------------------------------------------------------------------- From: xufuhai <xufuhai@kuaishou.com> If the read_msr function is executed by a non-root user, the function returns -1, which means that there is no permission to access /dev/cpu/%d/msr, but cpufreq_has_boost_support should also return -1 immediately, and should not follow the original logic to return 0, which will cause amd The cpupower tool returns the boost active state as 0. Reproduce procedure: cpupower frequency-info Signed-off-by: xufuhai <xufuhai@kuaishou.com> Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com> Signed-off-by: lishujin <lishujin@kuaishou.com> Reviewed-by: Thomas Renninger <trenn@suse.com> --- tools/power/cpupower/utils/helpers/misc.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c index fc6e34511721..565f8c414396 100644 --- a/tools/power/cpupower/utils/helpers/misc.c +++ b/tools/power/cpupower/utils/helpers/misc.c @@ -16,7 +16,7 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, int *states) { - int ret; + int ret = 0; unsigned long long val; *support = *active = *states = 0; @@ -30,18 +30,21 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, */ if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { + /* + * no permission to access /dev/cpu/%d/msr, return -1 immediately, + * and should not follow the original logic to return 0 + */ + ret = read_msr(cpu, MSR_AMD_HWCR, &val); + if (!ret) { if (!(val & CPUPOWER_AMD_CPBDIS)) *active = 1; } } else { ret = amd_pci_get_num_boost_states(active, states); - if (ret) - return ret; } } else if (cpupower_cpu_info.caps & CPUPOWER_CAP_INTEL_IDA) *support = *active = 1; - return 0; + return ret; } int cpupower_intel_get_perf_bias(unsigned int cpu) -- 2.24.3 (Apple Git-128) 在 2021/3/29 下午6:58, Thomas Renninger 写道: > Hi, > > Am Mittwoch, 24. März 2021, 09:28:38 CEST schrieb xufuhai: >> From: xufuhai <xufuhai@kuaishou.com> >> >> If the read_msr function is executed by a non-root user, the function >> returns -1, which means that there is no permission to access >> /dev/cpu/%d/msr, but cpufreq_has_boost_support should also return -1 >> immediately, and should not follow the original logic to return 0, which >> will cause amd The cpupower tool returns the turbo active status as 0. > > Yes, this seem to be buggy. > Can you clean this up a bit more, please: > >> Reproduce procedure: >> cpupower frequency-info >> >> Signed-off-by: xufuhai <xufuhai@kuaishou.com> >> --- >> tools/power/cpupower/utils/helpers/misc.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/tools/power/cpupower/utils/helpers/misc.c >> b/tools/power/cpupower/utils/helpers/misc.c index >> fc6e34511721..be96f9ce18eb 100644 >> --- a/tools/power/cpupower/utils/helpers/misc.c >> +++ b/tools/power/cpupower/utils/helpers/misc.c >> @@ -30,10 +30,15 @@ int cpufreq_has_boost_support(unsigned int cpu, int >> *support, int *active, */ >> >> if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { >> - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { >> + ret = read_msr(cpu, MSR_AMD_HWCR, &val); >> + if (!ret) { > ret should be initialized. I would initialize it with -1, but as Intel case > is always "good"/zero, it may make sense here to set: > > ret = 0 > at the beginning of the func already. > At the end of the func, unconditionally returning zero: > return 0; > should be replace by: > return ret; > >> if (!(val & CPUPOWER_AMD_CPBDIS)) >> *active = 1; >> - } >> + } else >> + /* no permission to access /dev/cpu/%d/msr, return -1 immediately, >> + * and should not follow the original logic to return 0 >> + */ >> + return ret; > > Then this part is not needed anymore, right? > Still the comment would be nice to show up, maybe slightly modified > in the if condition? > Afaik 100% correct comment would be: > /* ... */ > for one line comment and: > /* > * ... > * ... > */ > for multiline comment (one more line..). > >> } else { >> ret = amd_pci_get_num_boost_states(active, states); >> if (ret) > and these 2 lines can vanish as well at this point: > if (ret) > return ret; > > What do you think? > > Thanks for spotting this, > > Thomas > > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue 2021-03-30 2:46 ` xufuhai @ 2021-04-08 2:21 ` xufuhai 2021-04-15 7:02 ` [PATCH v2 " xufuhai 0 siblings, 1 reply; 10+ messages in thread From: xufuhai @ 2021-04-08 2:21 UTC (permalink / raw) To: Thomas Renninger, linux, sherry.hurwitz, linux-pm, xufuhai; +Cc: lishujin Any reply? Thomas 在 2021/3/30 上午10:46, xufuhai 写道: > Thanks for your review, Thomas～ > as you suggested, I have updated my patch as your advice. > Please help me review the patch again. thanks > > > ---------------------------------------------------------------------------------------------------- > > From: xufuhai <xufuhai@kuaishou.com> > > If the read_msr function is executed by a non-root user, the function returns > -1, which means that there is no permission to access /dev/cpu/%d/msr, but > cpufreq_has_boost_support should also return -1 immediately, and should not > follow the original logic to return 0, which will cause amd The cpupower tool > returns the boost active state as 0. > > Reproduce procedure: > cpupower frequency-info > > Signed-off-by: xufuhai <xufuhai@kuaishou.com> > Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com> > Signed-off-by: lishujin <lishujin@kuaishou.com> > Reviewed-by: Thomas Renninger <trenn@suse.com> > --- > tools/power/cpupower/utils/helpers/misc.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c > index fc6e34511721..565f8c414396 100644 > --- a/tools/power/cpupower/utils/helpers/misc.c > +++ b/tools/power/cpupower/utils/helpers/misc.c > @@ -16,7 +16,7 @@ > int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, > int *states) > { > - int ret; > + int ret = 0; > unsigned long long val; > > *support = *active = *states = 0; > @@ -30,18 +30,21 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, > */ > > if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { > - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { > + /* > + * no permission to access /dev/cpu/%d/msr, return -1 immediately, > + * and should not follow the original logic to return 0 > + */ > + ret = read_msr(cpu, MSR_AMD_HWCR, &val); > + if (!ret) { > if (!(val & CPUPOWER_AMD_CPBDIS)) > *active = 1; > } > } else { > ret = amd_pci_get_num_boost_states(active, states); > - if (ret) > - return ret; > } > } else if (cpupower_cpu_info.caps & CPUPOWER_CAP_INTEL_IDA) > *support = *active = 1; > - return 0; > + return ret; > } > > int cpupower_intel_get_perf_bias(unsigned int cpu) > -- > 2.24.3 (Apple Git-128) > > 在 2021/3/29 下午6:58, Thomas Renninger 写道: >> Hi, >> >> Am Mittwoch, 24. März 2021, 09:28:38 CEST schrieb xufuhai: >>> From: xufuhai <xufuhai@kuaishou.com> >>> >>> If the read_msr function is executed by a non-root user, the function >>> returns -1, which means that there is no permission to access >>> /dev/cpu/%d/msr, but cpufreq_has_boost_support should also return -1 >>> immediately, and should not follow the original logic to return 0, which >>> will cause amd The cpupower tool returns the turbo active status as 0. >> >> Yes, this seem to be buggy. >> Can you clean this up a bit more, please: >> >>> Reproduce procedure: >>> cpupower frequency-info >>> >>> Signed-off-by: xufuhai <xufuhai@kuaishou.com> >>> --- >>> tools/power/cpupower/utils/helpers/misc.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/power/cpupower/utils/helpers/misc.c >>> b/tools/power/cpupower/utils/helpers/misc.c index >>> fc6e34511721..be96f9ce18eb 100644 >>> --- a/tools/power/cpupower/utils/helpers/misc.c >>> +++ b/tools/power/cpupower/utils/helpers/misc.c >>> @@ -30,10 +30,15 @@ int cpufreq_has_boost_support(unsigned int cpu, int >>> *support, int *active, */ >>> >>> if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { >>> - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { >>> + ret = read_msr(cpu, MSR_AMD_HWCR, &val); >>> + if (!ret) { >> ret should be initialized. I would initialize it with -1, but as Intel case >> is always "good"/zero, it may make sense here to set: >> >> ret = 0 >> at the beginning of the func already. >> At the end of the func, unconditionally returning zero: >> return 0; >> should be replace by: >> return ret; >> >>> if (!(val & CPUPOWER_AMD_CPBDIS)) >>> *active = 1; >>> - } >>> + } else >>> + /* no permission to access /dev/cpu/%d/msr, return -1 immediately, >>> + * and should not follow the original logic to return 0 >>> + */ >>> + return ret; >> >> Then this part is not needed anymore, right? >> Still the comment would be nice to show up, maybe slightly modified >> in the if condition? >> Afaik 100% correct comment would be: >> /* ... */ >> for one line comment and: >> /* >> * ... >> * ... >> */ >> for multiline comment (one more line..). >> >>> } else { >>> ret = amd_pci_get_num_boost_states(active, states); >>> if (ret) >> and these 2 lines can vanish as well at this point: >> if (ret) >> return ret; >> >> What do you think? >> >> Thanks for spotting this, >> >> Thomas >> >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue 2021-04-08 2:21 ` xufuhai @ 2021-04-15 7:02 ` xufuhai 2021-04-15 13:45 ` Huang Rui 2021-04-15 23:07 ` Shuah Khan 0 siblings, 2 replies; 10+ messages in thread From: xufuhai @ 2021-04-15 7:02 UTC (permalink / raw) To: Thomas Renninger, linux, sherry.hurwitz, linux-pm, xufuhai; +Cc: lishujin From: xufuhai <xufuhai@kuaishou.com> If the read_msr function is executed by a non-root user, the function returns -1, which means that there is no permission to access /dev/cpu/%d/msr, but cpufreq_has_boost_support should also return -1 immediately, and should not follow the original logic to return 0, which will cause amd The cpupower tool returns the boost active state as 0. Reproduce procedure: cpupower frequency-info Signed-off-by: xufuhai <xufuhai@kuaishou.com> Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com> Signed-off-by: lishujin <lishujin@kuaishou.com> Reviewed-by: Thomas Renninger <trenn@suse.com> --- tools/power/cpupower/utils/helpers/misc.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c index fc6e34511721..565f8c414396 100644 --- a/tools/power/cpupower/utils/helpers/misc.c +++ b/tools/power/cpupower/utils/helpers/misc.c @@ -16,7 +16,7 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, int *states) { - int ret; + int ret = 0; unsigned long long val; *support = *active = *states = 0; @@ -30,18 +30,21 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, */ if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { + /* + * no permission to access /dev/cpu/%d/msr, return -1 immediately, + * and should not follow the original logic to return 0 + */ + ret = read_msr(cpu, MSR_AMD_HWCR, &val); + if (!ret) { if (!(val & CPUPOWER_AMD_CPBDIS)) *active = 1; } } else { ret = amd_pci_get_num_boost_states(active, states); - if (ret) - return ret; } } else if (cpupower_cpu_info.caps & CPUPOWER_CAP_INTEL_IDA) *support = *active = 1; - return 0; + return ret; } 在 2021/4/8 上午10:21, xufuhai 写道: > Any reply? Thomas > > 在 2021/3/30 上午10:46, xufuhai 写道: >> Thanks for your review, Thomas～ >> as you suggested, I have updated my patch as your advice. >> Please help me review the patch again. thanks >> >> >> ---------------------------------------------------------------------------------------------------- >> >> From: xufuhai <xufuhai@kuaishou.com> >> >> If the read_msr function is executed by a non-root user, the function returns >> -1, which means that there is no permission to access /dev/cpu/%d/msr, but >> cpufreq_has_boost_support should also return -1 immediately, and should not >> follow the original logic to return 0, which will cause amd The cpupower tool >> returns the boost active state as 0. >> >> Reproduce procedure: >> cpupower frequency-info >> >> Signed-off-by: xufuhai <xufuhai@kuaishou.com> >> Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com> >> Signed-off-by: lishujin <lishujin@kuaishou.com> >> Reviewed-by: Thomas Renninger <trenn@suse.com> >> --- >> tools/power/cpupower/utils/helpers/misc.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c >> index fc6e34511721..565f8c414396 100644 >> --- a/tools/power/cpupower/utils/helpers/misc.c >> +++ b/tools/power/cpupower/utils/helpers/misc.c >> @@ -16,7 +16,7 @@ >> int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, >> int *states) >> { >> - int ret; >> + int ret = 0; >> unsigned long long val; >> >> *support = *active = *states = 0; >> @@ -30,18 +30,21 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, >> */ >> >> if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { >> - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { >> + /* >> + * no permission to access /dev/cpu/%d/msr, return -1 immediately, >> + * and should not follow the original logic to return 0 >> + */ >> + ret = read_msr(cpu, MSR_AMD_HWCR, &val); >> + if (!ret) { >> if (!(val & CPUPOWER_AMD_CPBDIS)) >> *active = 1; >> } >> } else { >> ret = amd_pci_get_num_boost_states(active, states); >> - if (ret) >> - return ret; >> } >> } else if (cpupower_cpu_info.caps & CPUPOWER_CAP_INTEL_IDA) >> *support = *active = 1; >> - return 0; >> + return ret; >> } >> >> int cpupower_intel_get_perf_bias(unsigned int cpu) >> -- >> 2.24.3 (Apple Git-128) >> >> 在 2021/3/29 下午6:58, Thomas Renninger 写道: >>> Hi, >>> >>> Am Mittwoch, 24. März 2021, 09:28:38 CEST schrieb xufuhai: >>>> From: xufuhai <xufuhai@kuaishou.com> >>>> >>>> If the read_msr function is executed by a non-root user, the function >>>> returns -1, which means that there is no permission to access >>>> /dev/cpu/%d/msr, but cpufreq_has_boost_support should also return -1 >>>> immediately, and should not follow the original logic to return 0, which >>>> will cause amd The cpupower tool returns the turbo active status as 0. >>> >>> Yes, this seem to be buggy. >>> Can you clean this up a bit more, please: >>> >>>> Reproduce procedure: >>>> cpupower frequency-info >>>> >>>> Signed-off-by: xufuhai <xufuhai@kuaishou.com> >>>> --- >>>> tools/power/cpupower/utils/helpers/misc.c | 9 +++++++-- >>>> 1 file changed, 7 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/tools/power/cpupower/utils/helpers/misc.c >>>> b/tools/power/cpupower/utils/helpers/misc.c index >>>> fc6e34511721..be96f9ce18eb 100644 >>>> --- a/tools/power/cpupower/utils/helpers/misc.c >>>> +++ b/tools/power/cpupower/utils/helpers/misc.c >>>> @@ -30,10 +30,15 @@ int cpufreq_has_boost_support(unsigned int cpu, int >>>> *support, int *active, */ >>>> >>>> if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { >>>> - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { >>>> + ret = read_msr(cpu, MSR_AMD_HWCR, &val); >>>> + if (!ret) { >>> ret should be initialized. I would initialize it with -1, but as Intel case >>> is always "good"/zero, it may make sense here to set: >>> >>> ret = 0 >>> at the beginning of the func already. >>> At the end of the func, unconditionally returning zero: >>> return 0; >>> should be replace by: >>> return ret; >>> >>>> if (!(val & CPUPOWER_AMD_CPBDIS)) >>>> *active = 1; >>>> - } >>>> + } else >>>> + /* no permission to access /dev/cpu/%d/msr, return -1 immediately, >>>> + * and should not follow the original logic to return 0 >>>> + */ >>>> + return ret; >>> >>> Then this part is not needed anymore, right? >>> Still the comment would be nice to show up, maybe slightly modified >>> in the if condition? >>> Afaik 100% correct comment would be: >>> /* ... */ >>> for one line comment and: >>> /* >>> * ... >>> * ... >>> */ >>> for multiline comment (one more line..). >>> >>>> } else { >>>> ret = amd_pci_get_num_boost_states(active, states); >>>> if (ret) >>> and these 2 lines can vanish as well at this point: >>> if (ret) >>> return ret; >>> >>> What do you think? >>> >>> Thanks for spotting this, >>> >>> Thomas >>> >>> ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue 2021-04-15 7:02 ` [PATCH v2 " xufuhai @ 2021-04-15 13:45 ` Huang Rui 2021-04-15 23:07 ` Shuah Khan 1 sibling, 0 replies; 10+ messages in thread From: Huang Rui @ 2021-04-15 13:45 UTC (permalink / raw) To: xufuhai Cc: Thomas Renninger, linux, sherry.hurwitz, linux-pm, xufuhai, lishujin On Thu, Apr 15, 2021 at 03:02:54PM +0800, xufuhai wrote: > From: xufuhai <xufuhai@kuaishou.com> > > If the read_msr function is executed by a non-root user, the function returns > -1, which means that there is no permission to access /dev/cpu/%d/msr, but > cpufreq_has_boost_support should also return -1 immediately, and should not > follow the original logic to return 0, which will cause amd The cpupower tool > returns the boost active state as 0. > > Reproduce procedure: > cpupower frequency-info > > Signed-off-by: xufuhai <xufuhai@kuaishou.com> > Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com> > Signed-off-by: lishujin <lishujin@kuaishou.com> > Reviewed-by: Thomas Renninger <trenn@suse.com> > --- > tools/power/cpupower/utils/helpers/misc.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c > index fc6e34511721..565f8c414396 100644 > --- a/tools/power/cpupower/utils/helpers/misc.c > +++ b/tools/power/cpupower/utils/helpers/misc.c > @@ -16,7 +16,7 @@ > int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, > int *states) > { > - int ret; > + int ret = 0; > unsigned long long val; > > *support = *active = *states = 0; > @@ -30,18 +30,21 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, > */ > > if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { > - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { > + /* > + * no permission to access /dev/cpu/%d/msr, return -1 immediately, > + * and should not follow the original logic to return 0 > + */ Looks good for me as well. Acked-by: Huang Rui <ray.huang@amd.com> > + ret = read_msr(cpu, MSR_AMD_HWCR, &val); > + if (!ret) { > if (!(val & CPUPOWER_AMD_CPBDIS)) > *active = 1; > } > } else { > ret = amd_pci_get_num_boost_states(active, states); > - if (ret) > - return ret; > } > } else if (cpupower_cpu_info.caps & CPUPOWER_CAP_INTEL_IDA) > *support = *active = 1; > - return 0; > + return ret; > } > > 在 2021/4/8 上午10:21, xufuhai 写道: > > Any reply? Thomas > > > > 在 2021/3/30 上午10:46, xufuhai 写道: > >> Thanks for your review, Thomas～ > >> as you suggested, I have updated my patch as your advice. > >> Please help me review the patch again. thanks > >> > >> > >> ---------------------------------------------------------------------------------------------------- > >> > >> From: xufuhai <xufuhai@kuaishou.com> > >> > >> If the read_msr function is executed by a non-root user, the function returns > >> -1, which means that there is no permission to access /dev/cpu/%d/msr, but > >> cpufreq_has_boost_support should also return -1 immediately, and should not > >> follow the original logic to return 0, which will cause amd The cpupower tool > >> returns the boost active state as 0. > >> > >> Reproduce procedure: > >> cpupower frequency-info > >> > >> Signed-off-by: xufuhai <xufuhai@kuaishou.com> > >> Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com> > >> Signed-off-by: lishujin <lishujin@kuaishou.com> > >> Reviewed-by: Thomas Renninger <trenn@suse.com> > >> --- > >> tools/power/cpupower/utils/helpers/misc.c | 13 ++++++++----- > >> 1 file changed, 8 insertions(+), 5 deletions(-) > >> > >> diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c > >> index fc6e34511721..565f8c414396 100644 > >> --- a/tools/power/cpupower/utils/helpers/misc.c > >> +++ b/tools/power/cpupower/utils/helpers/misc.c > >> @@ -16,7 +16,7 @@ > >> int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, > >> int *states) > >> { > >> - int ret; > >> + int ret = 0; > >> unsigned long long val; > >> > >> *support = *active = *states = 0; > >> @@ -30,18 +30,21 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active, > >> */ > >> > >> if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { > >> - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { > >> + /* > >> + * no permission to access /dev/cpu/%d/msr, return -1 immediately, > >> + * and should not follow the original logic to return 0 > >> + */ > >> + ret = read_msr(cpu, MSR_AMD_HWCR, &val); > >> + if (!ret) { > >> if (!(val & CPUPOWER_AMD_CPBDIS)) > >> *active = 1; > >> } > >> } else { > >> ret = amd_pci_get_num_boost_states(active, states); > >> - if (ret) > >> - return ret; > >> } > >> } else if (cpupower_cpu_info.caps & CPUPOWER_CAP_INTEL_IDA) > >> *support = *active = 1; > >> - return 0; > >> + return ret; > >> } > >> > >> int cpupower_intel_get_perf_bias(unsigned int cpu) > >> -- > >> 2.24.3 (Apple Git-128) > >> > >> 在 2021/3/29 下午6:58, Thomas Renninger 写道: > >>> Hi, > >>> > >>> Am Mittwoch, 24. März 2021, 09:28:38 CEST schrieb xufuhai: > >>>> From: xufuhai <xufuhai@kuaishou.com> > >>>> > >>>> If the read_msr function is executed by a non-root user, the function > >>>> returns -1, which means that there is no permission to access > >>>> /dev/cpu/%d/msr, but cpufreq_has_boost_support should also return -1 > >>>> immediately, and should not follow the original logic to return 0, which > >>>> will cause amd The cpupower tool returns the turbo active status as 0. > >>> > >>> Yes, this seem to be buggy. > >>> Can you clean this up a bit more, please: > >>> > >>>> Reproduce procedure: > >>>> cpupower frequency-info > >>>> > >>>> Signed-off-by: xufuhai <xufuhai@kuaishou.com> > >>>> --- > >>>> tools/power/cpupower/utils/helpers/misc.c | 9 +++++++-- > >>>> 1 file changed, 7 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/tools/power/cpupower/utils/helpers/misc.c > >>>> b/tools/power/cpupower/utils/helpers/misc.c index > >>>> fc6e34511721..be96f9ce18eb 100644 > >>>> --- a/tools/power/cpupower/utils/helpers/misc.c > >>>> +++ b/tools/power/cpupower/utils/helpers/misc.c > >>>> @@ -30,10 +30,15 @@ int cpufreq_has_boost_support(unsigned int cpu, int > >>>> *support, int *active, */ > >>>> > >>>> if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) { > >>>> - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) { > >>>> + ret = read_msr(cpu, MSR_AMD_HWCR, &val); > >>>> + if (!ret) { > >>> ret should be initialized. I would initialize it with -1, but as Intel case > >>> is always "good"/zero, it may make sense here to set: > >>> > >>> ret = 0 > >>> at the beginning of the func already. > >>> At the end of the func, unconditionally returning zero: > >>> return 0; > >>> should be replace by: > >>> return ret; > >>> > >>>> if (!(val & CPUPOWER_AMD_CPBDIS)) > >>>> *active = 1; > >>>> - } > >>>> + } else > >>>> + /* no permission to access /dev/cpu/%d/msr, return -1 immediately, > >>>> + * and should not follow the original logic to return 0 > >>>> + */ > >>>> + return ret; > >>> > >>> Then this part is not needed anymore, right? > >>> Still the comment would be nice to show up, maybe slightly modified > >>> in the if condition? > >>> Afaik 100% correct comment would be: > >>> /* ... */ > >>> for one line comment and: > >>> /* > >>> * ... > >>> * ... > >>> */ > >>> for multiline comment (one more line..). > >>> > >>>> } else { > >>>> ret = amd_pci_get_num_boost_states(active, states); > >>>> if (ret) > >>> and these 2 lines can vanish as well at this point: > >>> if (ret) > >>> return ret; > >>> > >>> What do you think? > >>> > >>> Thanks for spotting this, > >>> > >>> Thomas > >>> > >>> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue 2021-04-15 7:02 ` [PATCH v2 " xufuhai 2021-04-15 13:45 ` Huang Rui @ 2021-04-15 23:07 ` Shuah Khan [not found] ` <CAOkq_BcdvS1NtKJ9=peRWHc00kGjfvdE+Cyz7vqvH0+kermfhQ@mail.gmail.com> 1 sibling, 1 reply; 10+ messages in thread From: Shuah Khan @ 2021-04-15 23:07 UTC (permalink / raw) To: xufuhai, Thomas Renninger, linux, sherry.hurwitz, linux-pm, xufuhai Cc: lishujin, Shuah Khan On 4/15/21 1:02 AM, xufuhai wrote: > From: xufuhai <xufuhai@kuaishou.com> > > If the read_msr function is executed by a non-root user, the function returns > -1, which means that there is no permission to access /dev/cpu/%d/msr, but > cpufreq_has_boost_support should also return -1 immediately, and should not > follow the original logic to return 0, which will cause amd The cpupower tool > returns the boost active state as 0. > > Reproduce procedure: > cpupower frequency-info > > Signed-off-by: xufuhai <xufuhai@kuaishou.com> > Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com> > Signed-off-by: lishujin <lishujin@kuaishou.com> > Reviewed-by: Thomas Renninger <trenn@suse.com> > --- As I mentioned on the previous version of this patch, please run get_maintainers script and include me in the To: list. The patch has to land in my Inbox for me to apply it. thanks, -- Shuah ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <CAOkq_BcdvS1NtKJ9=peRWHc00kGjfvdE+Cyz7vqvH0+kermfhQ@mail.gmail.com>]
* Re: [PATCH v2 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue [not found] ` <CAOkq_BcdvS1NtKJ9=peRWHc00kGjfvdE+Cyz7vqvH0+kermfhQ@mail.gmail.com> @ 2021-04-16 15:04 ` Shuah Khan 0 siblings, 0 replies; 10+ messages in thread From: Shuah Khan @ 2021-04-16 15:04 UTC (permalink / raw) To: 徐福海 Cc: Thomas Renninger, linux, sherry.hurwitz, linux-pm, xufuhai, lishujin, Shuah Khan On 4/15/21 9:45 PM, 徐福海 wrote: > hi Shuah, > Thanks for applying my patch. > and another whether needing to email a new patch such as v3 to you for > merging into mainline？or v2 patch has been ok? I didn't apply your patch. I don't have it in my Inbox as I mentioned in my email yesterday. Please run get_maintainers.pl - refer to the following for information on how to submit patches: https://www.kernel.org/doc/html/latest/process/submitting-patches.html thanks, -- Shuah ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue [not found] <20210324082838.41462-1-xufuhai1992@gmail.com> [not found] ` <20210324082838.41462-2-xufuhai1992@gmail.com> @ 2021-03-29 11:10 ` Thomas Renninger 2021-03-30 3:47 ` xufuhai 1 sibling, 1 reply; 10+ messages in thread From: Thomas Renninger @ 2021-03-29 11:10 UTC (permalink / raw) To: linux, sherry.hurwitz, linux-pm, xufuhai; +Cc: lishujin, xufuhai Hi, Am Mittwoch, 24. März 2021, 09:28:37 CEST schrieb xufuhai: > From: xufuhai <xufuhai@kuaishou.com> > > For the old AMD processor (family < 0x17), cpupower will call the > amd_pci_get_num_boost_states function, but for the non-root user > pci_read_byte function (implementation comes from the psutil library), > val will be set to 0xff, indicating that there is no read function > callback. At this time, the original logic will set the cpupower turbo > active state to yes. This is an obvious issue~ > > Reproduce procedure: > cpupower frequency-info > > Signed-off-by: xufuhai <xufuhai@kuaishou.com> > --- > tools/power/cpupower/utils/helpers/amd.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/tools/power/cpupower/utils/helpers/amd.c > b/tools/power/cpupower/utils/helpers/amd.c index 97f2c857048e..6f9504906afa > 100644 > --- a/tools/power/cpupower/utils/helpers/amd.c > +++ b/tools/power/cpupower/utils/helpers/amd.c > @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int > *states) return -ENODEV; > > val = pci_read_byte(device, 0x15c); > + > + /* If val is 0xff, meaning has no permisson to > + * get the boost states, return -1 > + */ > + if (val == 0xff) > + return -1; > + There is certainly a cleaner way to do this.., theoretically pci_read_byte can return 0xff in other cases? But I guess this is a sufficient way to handle this for now. Reviewed-by: Thomas Renninger <trenn@suse.de> Thanks, Thomas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue 2021-03-29 11:10 ` [PATCH 1/2] cpupower: fix amd cpu (family < " Thomas Renninger @ 2021-03-30 3:47 ` xufuhai 2021-04-08 2:22 ` xufuhai 0 siblings, 1 reply; 10+ messages in thread From: xufuhai @ 2021-03-30 3:47 UTC (permalink / raw) To: Thomas Renninger, linux, sherry.hurwitz, linux-pm; +Cc: lishujin, xufuhai hi Thomas, Thanks for your reply I believe the only way that pci_read_byte can return 0xff is no permission to access the pci_dev read function. Because for pci_read_byte, the pos offset is 0x15c that the offset has excessed the capacity of pci_dev cache_len, so can't get val via memcpy from cache. And then for read callback function, I think that is read val from pci_dev 0x15c register. I have looked up the amd family 0x15 code, the 0x15c register is called "Core Performance Boost Control Register" and this register base addr is D18F4x15C. We just concern the lower 8bit of D18F4x15C register, the detailed as below: Quote from https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/796af1 7f18554380a49d69d7768ac18ee039d711/src/vendorcode/amd/agesa/f15/Proc/CPU/Family/0x15/cpuF15PowerMgmt.h /* Core Performance Boost Control Register D18F4x15C */ #define CPB_CTRL_REG 0x15C #define CPB_CTRL_PCI_ADDR (MAKE_SBDFO (0, 0, 0x18, FUNC_4, CPB_CTRL_REG)) /// Core Performance Boost Control Register of Family 15h common aceess typedef struct { UINT32 BoostSrc:2; ///< Boost source UINT32 NumBoostStates:3; ///< Number of boosted states UINT32 :2; ///< Reserved UINT32 ApmMasterEn:1; ///< APM master enable UINT32 :23; ///< Reserved UINT32 BoostLock:1; ///< } F15_CPB_CTRL_REGISTER; the amd 0x15 family specification pdf: https://www.amd.com/system/files/TechDocs/42301_15h_Mod_00h-0Fh_BKDG.pdf so I believe that val is nealy impossible to return 0xff unless no permisson to access read callback. what do you think? Thomas And at last I have a new doubt why the amd_pci_get_num_boost_states about setting "*active=1" condition is "val & 3", should not be "val & 1"? I accidently found that "00b Boosting disabled / 01b Boosting enabled / 1*b Reserved" via above amd specification about D18F4x15C register description. I am just curious about this point, maybe I am wrong~ : if (val & 3) *active = 1; else *active = 0; Thanks for your reading, Thomas 在 2021/3/29 下午7:10, Thomas Renninger 写道: > Hi, > > Am Mittwoch, 24. März 2021, 09:28:37 CEST schrieb xufuhai: >> From: xufuhai <xufuhai@kuaishou.com> >> >> For the old AMD processor (family < 0x17), cpupower will call the >> amd_pci_get_num_boost_states function, but for the non-root user >> pci_read_byte function (implementation comes from the psutil library), >> val will be set to 0xff, indicating that there is no read function >> callback. At this time, the original logic will set the cpupower turbo >> active state to yes. This is an obvious issue~ >> >> Reproduce procedure: >> cpupower frequency-info >> >> Signed-off-by: xufuhai <xufuhai@kuaishou.com> >> --- >> tools/power/cpupower/utils/helpers/amd.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/tools/power/cpupower/utils/helpers/amd.c >> b/tools/power/cpupower/utils/helpers/amd.c index 97f2c857048e..6f9504906afa >> 100644 >> --- a/tools/power/cpupower/utils/helpers/amd.c >> +++ b/tools/power/cpupower/utils/helpers/amd.c >> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int >> *states) return -ENODEV; >> >> val = pci_read_byte(device, 0x15c); >> + >> + /* If val is 0xff, meaning has no permisson to >> + * get the boost states, return -1 >> + */ >> + if (val == 0xff) >> + return -1; >> + > There is certainly a cleaner way to do this.., theoretically > pci_read_byte can return 0xff in other cases? > > But I guess this is a sufficient way to handle this for now. > > Reviewed-by: Thomas Renninger <trenn@suse.de> > > Thanks, > > Thomas > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue 2021-03-30 3:47 ` xufuhai @ 2021-04-08 2:22 ` xufuhai 0 siblings, 0 replies; 10+ messages in thread From: xufuhai @ 2021-04-08 2:22 UTC (permalink / raw) To: Thomas Renninger, linux, sherry.hurwitz, linux-pm; +Cc: lishujin, xufuhai Any reply? Thomas 在 2021/3/30 上午11:47, xufuhai 写道: > hi Thomas, > > Thanks for your reply > > I believe the only way that pci_read_byte can return 0xff is no permission to > access the pci_dev read function. Because for pci_read_byte, the pos offset is > 0x15c that the offset has excessed the capacity of pci_dev cache_len, so can't > get val via memcpy from cache. > > And then for read callback function, I think that is read val from pci_dev 0x15c > register. I have looked up the amd family 0x15 code, the 0x15c register is called > "Core Performance Boost Control Register" and this register base addr is D18F4x15C. > We just concern the lower 8bit of D18F4x15C register, the detailed as below: > > Quote from https://chromium.googlesource.com/chromiumos/third_party/coreboot/+/796af1 > 7f18554380a49d69d7768ac18ee039d711/src/vendorcode/amd/agesa/f15/Proc/CPU/Family/0x15/cpuF15PowerMgmt.h > > /* Core Performance Boost Control Register D18F4x15C */ > #define CPB_CTRL_REG 0x15C > #define CPB_CTRL_PCI_ADDR (MAKE_SBDFO (0, 0, 0x18, FUNC_4, CPB_CTRL_REG)) > /// Core Performance Boost Control Register of Family 15h common aceess > typedef struct { > UINT32 BoostSrc:2; ///< Boost source > UINT32 NumBoostStates:3; ///< Number of boosted states > UINT32 :2; ///< Reserved > UINT32 ApmMasterEn:1; ///< APM master enable > UINT32 :23; ///< Reserved > UINT32 BoostLock:1; ///< > } F15_CPB_CTRL_REGISTER; > > the amd 0x15 family specification pdf: > https://www.amd.com/system/files/TechDocs/42301_15h_Mod_00h-0Fh_BKDG.pdf > > so I believe that val is nealy impossible to return 0xff unless no permisson to access read callback. > > what do you think? Thomas > > And at last I have a new doubt why the amd_pci_get_num_boost_states about setting "*active=1" > condition is "val & 3", should not be "val & 1"? I accidently found that "00b Boosting disabled / 01b Boosting enabled / 1*b Reserved" > via above amd specification about D18F4x15C register description. > I am just curious about this point, maybe I am wrong~ : > > if (val & 3) > *active = 1; > else > *active = 0; > > Thanks for your reading, Thomas > > 在 2021/3/29 下午7:10, Thomas Renninger 写道: >> Hi, >> >> Am Mittwoch, 24. März 2021, 09:28:37 CEST schrieb xufuhai: >>> From: xufuhai <xufuhai@kuaishou.com> >>> >>> For the old AMD processor (family < 0x17), cpupower will call the >>> amd_pci_get_num_boost_states function, but for the non-root user >>> pci_read_byte function (implementation comes from the psutil library), >>> val will be set to 0xff, indicating that there is no read function >>> callback. At this time, the original logic will set the cpupower turbo >>> active state to yes. This is an obvious issue~ >>> >>> Reproduce procedure: >>> cpupower frequency-info >>> >>> Signed-off-by: xufuhai <xufuhai@kuaishou.com> >>> --- >>> tools/power/cpupower/utils/helpers/amd.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/tools/power/cpupower/utils/helpers/amd.c >>> b/tools/power/cpupower/utils/helpers/amd.c index 97f2c857048e..6f9504906afa >>> 100644 >>> --- a/tools/power/cpupower/utils/helpers/amd.c >>> +++ b/tools/power/cpupower/utils/helpers/amd.c >>> @@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int >>> *states) return -ENODEV; >>> >>> val = pci_read_byte(device, 0x15c); >>> + >>> + /* If val is 0xff, meaning has no permisson to >>> + * get the boost states, return -1 >>> + */ >>> + if (val == 0xff) >>> + return -1; >>> + >> There is certainly a cleaner way to do this.., theoretically >> pci_read_byte can return 0xff in other cases? >> >> But I guess this is a sufficient way to handle this for now. >> >> Reviewed-by: Thomas Renninger <trenn@suse.de> >> >> Thanks, >> >> Thomas >> >> ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-04-16 15:04 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210324082838.41462-1-xufuhai1992@gmail.com>
[not found] ` <20210324082838.41462-2-xufuhai1992@gmail.com>
2021-03-29 10:58 ` [PATCH 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue Thomas Renninger
2021-03-30 2:46 ` xufuhai
2021-04-08 2:21 ` xufuhai
2021-04-15 7:02 ` [PATCH v2 " xufuhai
2021-04-15 13:45 ` Huang Rui
2021-04-15 23:07 ` Shuah Khan
[not found] ` <CAOkq_BcdvS1NtKJ9=peRWHc00kGjfvdE+Cyz7vqvH0+kermfhQ@mail.gmail.com>
2021-04-16 15:04 ` Shuah Khan
2021-03-29 11:10 ` [PATCH 1/2] cpupower: fix amd cpu (family < " Thomas Renninger
2021-03-30 3:47 ` xufuhai
2021-04-08 2:22 ` xufuhai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox