Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue

[xufuhai <xufuhai1992@gmail.com>]

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