On 1/13/2023 11:18 AM, Mario Limonciello wrote:
> On 1/12/23 23:21, Wyes Karny wrote:
>> amd_pstate driver's `status` sysfs entry helps to control the driver's
>> mode dynamically by user. After the addition of guided mode the
>> combinations of mode transitions have been increased (16 combinations).
>> Therefore optimise the amd_pstate_update_status function by implementing
>> a state transition table.
>>
>> There are 4 states amd_pstate supports, namely: 'disable', 'passive',
>> 'active', and 'guided'. The transition from any state to any other
>> state is possible after this change. Only if the state requested matches
>> with the current state then -EBUSY value is returned.
>>
>> Sysfs interface:
>>
>> To disable amd_pstate driver:
>> # echo disable > /sys/devices/system/cpu/amd_pstate/status
>>
>> To enable passive mode:
>> # echo passive > /sys/devices/system/cpu/amd_pstate/status
>>
>> To change mode to active:
>> # echo active > /sys/devices/system/cpu/amd_pstate/status
>>
>> To change mode to guided:
>> # echo guided > /sys/devices/system/cpu/amd_pstate/status
>>
>> Signed-off-by: Wyes Karny <wyes.karny@xxxxxxx>
>> ---
>> drivers/cpufreq/amd-pstate.c | 144 ++++++++++++++++++++++++++---------
>> 1 file changed, 109 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 20d78dad712d..4a2b559fd712 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -65,6 +65,8 @@ static struct cpufreq_driver amd_pstate_epp_driver;
>> static int cppc_state = AMD_PSTATE_DISABLE;
>> struct kobject *amd_pstate_kobj;
>> +typedef int (*cppc_mode_transition_fn)(int);
>> +
>> static inline int get_mode_idx_from_str(const char *str, size_t size)
>> {
>> int i;
>> @@ -806,55 +808,127 @@ static ssize_t amd_pstate_show_status(char *buf)
>> static void amd_pstate_driver_cleanup(void)
>> {
>> + amd_pstate_enable(false);
>> + cppc_state = AMD_PSTATE_DISABLE;
>> default_pstate_driver = NULL;
>> }
>> -static int amd_pstate_update_status(const char *buf, size_t size)
>> +static int amd_pstate_register_driver(int mode)
>> {
>> int ret;
>> - int mode_idx;
>> - if (size > 7 || size < 3)
>> + if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED)
>> + default_pstate_driver = &amd_pstate_driver;
>> + else if (mode == AMD_PSTATE_ACTIVE)
>> + default_pstate_driver = &amd_pstate_epp_driver;
>> + else
>> return -EINVAL;
>> - mode_idx = get_mode_idx_from_str(buf, size);
>> - switch(mode_idx) {
>> - case AMD_PSTATE_DISABLE:
>> - if (!default_pstate_driver)
>> - return -EINVAL;
>> - if (cppc_state == AMD_PSTATE_ACTIVE)
>> - return -EBUSY;
>> - ret = cpufreq_unregister_driver(default_pstate_driver);
>> + ret = cpufreq_register_driver(default_pstate_driver);
>> + if (ret) {
>> amd_pstate_driver_cleanup();
>> - break;
>> - case AMD_PSTATE_PASSIVE:
>> - if (default_pstate_driver) {
>> - if (default_pstate_driver == &amd_pstate_driver)
>> - return 0;
>> - cpufreq_unregister_driver(default_pstate_driver);
>> - cppc_state = AMD_PSTATE_PASSIVE;
>> - default_pstate_driver = &amd_pstate_driver;
>> - }
>> + return ret;
>> + }
>> - ret = cpufreq_register_driver(default_pstate_driver);
>> - break;
>> - case AMD_PSTATE_ACTIVE:
>> - if (default_pstate_driver) {
>> - if (default_pstate_driver == &amd_pstate_epp_driver)
>> - return 0;
>> - cpufreq_unregister_driver(default_pstate_driver);
>> - default_pstate_driver = &amd_pstate_epp_driver;
>> - cppc_state = AMD_PSTATE_ACTIVE;
>> + cppc_state = mode;
>> + return 0;
>> +}
>> +
>> +static int amd_pstate_unregister_driver(int dummy)
>> +{
>> + int ret;
>> +
>> + ret = cpufreq_unregister_driver(default_pstate_driver);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + amd_pstate_driver_cleanup();
>> + return 0;
>> +}
>> +
>> +static void amd_pstate_change_mode_without_dvr_change(int mode)
>> +{
>> + int cpu = 0;
>> +
>> + cppc_state = mode;
>> + if (!boot_cpu_has(X86_FEATURE_CPPC)) {
>
> You can save some indentation by inverting the check and calling "return
Makes sense. Thanks!
>
>> + if (cppc_state == AMD_PSTATE_PASSIVE) {
>> + for_each_present_cpu(cpu) {
>> + cppc_set_auto_sel(cpu, 0);
>> + }
>> + } else if (cppc_state == AMD_PSTATE_GUIDED) {
>> + for_each_present_cpu(cpu) {
>> + cppc_set_auto_sel(cpu, 1);
>> + }
>> }
>> + }
>> +}
>> - ret = cpufreq_register_driver(default_pstate_driver);
>> - break;
>> - default:
>> - break;
>> - ret = -EINVAL;
>> +static int amd_pstate_change_driver_mode(int mode)
>> +{
>> + int ret;
>> +
>> + if ((mode == AMD_PSTATE_GUIDED && cppc_state == AMD_PSTATE_PASSIVE) ||
>> + (mode == AMD_PSTATE_PASSIVE && cppc_state == AMD_PSTATE_GUIDED)) {
>> + amd_pstate_change_mode_without_dvr_change(mode);
>> + return 0;
>
> This feels like you're duplicating your logic of your mode transition table. How about having the mode transition table call the function for changing modes without a driver change directly instead for those cases?
Makes sense. Will add this in the table.
>
>> }
>> - return ret;
>> + ret = amd_pstate_unregister_driver(0);
>> + if (ret)
>> + return ret;
>> +
>> + ret = amd_pstate_register_driver(mode);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +/* Mode transition table */
>> +cppc_mode_transition_fn mode_state_machine[AMD_PSTATE_MAX][AMD_PSTATE_MAX] = {
>> + [AMD_PSTATE_DISABLE] = {
>> + [AMD_PSTATE_DISABLE] = NULL,
>> + [AMD_PSTATE_PASSIVE] = amd_pstate_register_driver,
>> + [AMD_PSTATE_ACTIVE] = amd_pstate_register_driver,
>> + [AMD_PSTATE_GUIDED] = amd_pstate_register_driver,
>> + },
>> + [AMD_PSTATE_PASSIVE] = {
>> + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver,
>> + [AMD_PSTATE_PASSIVE] = NULL,
>> + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode,
>> + [AMD_PSTATE_GUIDED] = amd_pstate_change_driver_mode,
>> + },
>> + [AMD_PSTATE_ACTIVE] = {
>> + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver,
>> + [AMD_PSTATE_PASSIVE] = amd_pstate_change_driver_mode,
>> + [AMD_PSTATE_ACTIVE] = NULL,
>> + [AMD_PSTATE_GUIDED] = amd_pstate_change_driver_mode,
>> + },
>> + [AMD_PSTATE_GUIDED] = {
>> + [AMD_PSTATE_DISABLE] = amd_pstate_unregister_driver,
>> + [AMD_PSTATE_PASSIVE] = amd_pstate_change_driver_mode,
>> + [AMD_PSTATE_ACTIVE] = amd_pstate_change_driver_mode,
>> + [AMD_PSTATE_GUIDED] = NULL,
>> + },
>> +};
>> +
>> +static int amd_pstate_update_status(const char *buf, size_t size)
>> +{
>> + int mode_idx;
>> +
>> + if (size > 7 || size < 3)
>> + return -EINVAL;
>
> The compiler typically inlines strlen. Perhaps to make this more obvious what these boundaries are maybe do this:
>
> size > strlen("passive") || size < strlen("off")
>
>> + mode_idx = get_mode_idx_from_str(buf, size);
>> +
>> + if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX)
>> + return -EINVAL;
>> +
>> + if (mode_state_machine[cppc_state][mode_idx])
>> + return mode_state_machine[cppc_state][mode_idx](mode_idx);
>> +
>> + return -EBUSY;
>> }
>> static ssize_t show_status(struct kobject *kobj,
>
--
Thanks & Regards,
Wyes
