Re: [PATCH v4 5/6] cpufreq: amd_pstate: Add guided mode control support via sysfs

[Wyes Karny]

On 31 Jan 07:45, Mario Limonciello wrote:
> On 1/30/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.
> 
> I realized this after I finished reviewing doc patch, but you probably want
> to explain -EBUSY return code in documentation patch too.

Yes, I'll add this to documentation. Thanks!

> 
> > 
> > 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 | 150 +++++++++++++++++++++++++----------
> >   1 file changed, 108 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > index 48ab4684c3a5..6c522dec6967 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;
> > @@ -797,6 +799,105 @@ static ssize_t show_energy_performance_preference(
> >   	return sysfs_emit(buf, "%s\n", energy_perf_strings[preference]);
> >   }
> > +static void amd_pstate_driver_cleanup(void)
> > +{
> > +	amd_pstate_enable(false);
> > +	cppc_state = AMD_PSTATE_DISABLE;
> > +	current_pstate_driver = NULL;
> > +}
> > +
> > +static int amd_pstate_register_driver(int mode)
> > +{
> > +	int ret;
> > +
> > +	if (mode == AMD_PSTATE_PASSIVE || mode == AMD_PSTATE_GUIDED)
> > +		current_pstate_driver = &amd_pstate_driver;
> > +	else if (mode == AMD_PSTATE_ACTIVE)
> > +		current_pstate_driver = &amd_pstate_epp_driver;
> > +	else
> > +		return -EINVAL;
> > +
> > +	cppc_state = mode;
> > +	ret = cpufreq_register_driver(current_pstate_driver);
> > +	if (ret) {
> > +		amd_pstate_driver_cleanup();
> > +		return ret;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int amd_pstate_unregister_driver(int dummy)
> > +{
> > +	int ret;
> > +
> > +	ret = cpufreq_unregister_driver(current_pstate_driver);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	amd_pstate_driver_cleanup();
> > +	return 0;
> > +}
> > +
> > +static int amd_pstate_change_mode_without_dvr_change(int mode)
> > +{
> > +	int cpu = 0;
> > +
> > +	cppc_state = mode;
> > +
> > +	if (boot_cpu_has(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE)
> > +		return 0;
> > +
> > +	for_each_present_cpu(cpu) {
> > +		cppc_set_auto_sel(cpu, (cppc_state == AMD_PSTATE_PASSIVE) ? 0 : 1);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int amd_pstate_change_driver_mode(int mode)
> > +{
> > +	int 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 */
> 
> This seems to be a pointless comment to me.
> 
> > +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_mode_without_dvr_change,
> > +	},
> > +	[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_mode_without_dvr_change,
> > +		[AMD_PSTATE_ACTIVE]      = amd_pstate_change_driver_mode,
> > +		[AMD_PSTATE_GUIDED]      = NULL,
> > +	},
> > +};
> > +
> >   static ssize_t amd_pstate_show_status(char *buf)
> >   {
> >   	if (!current_pstate_driver)
> > @@ -805,57 +906,22 @@ static ssize_t amd_pstate_show_status(char *buf)
> >   	return sysfs_emit(buf, "%s\n", amd_pstate_mode_string[cppc_state]);
> >   }
> > -static void amd_pstate_driver_cleanup(void)
> > -{
> > -	current_pstate_driver = NULL;
> > -}
> > -
> >   static int amd_pstate_update_status(const char *buf, size_t size)
> >   {
> > -	int ret;
> >   	int mode_idx;
> > -	if (size > 7 || size < 6)
> > +	if (size > strlen("passive") || size < strlen("active"))
> >   		return -EINVAL;
> > -	mode_idx = get_mode_idx_from_str(buf, size);
> > -	switch(mode_idx) {
> > -	case AMD_PSTATE_DISABLE:
> > -		if (!current_pstate_driver)
> > -			return -EINVAL;
> > -		if (cppc_state == AMD_PSTATE_ACTIVE)
> > -			return -EBUSY;
> > -		ret = cpufreq_unregister_driver(current_pstate_driver);
> > -		amd_pstate_driver_cleanup();
> > -		break;
> > -	case AMD_PSTATE_PASSIVE:
> > -		if (current_pstate_driver) {
> > -			if (current_pstate_driver == &amd_pstate_driver)
> > -				return 0;
> > -			cpufreq_unregister_driver(current_pstate_driver);
> > -			cppc_state = AMD_PSTATE_PASSIVE;
> > -			current_pstate_driver = &amd_pstate_driver;
> > -		}
> > +	mode_idx = get_mode_idx_from_str(buf, size);
> > -		ret = cpufreq_register_driver(current_pstate_driver);
> > -		break;
> > -	case AMD_PSTATE_ACTIVE:
> > -		if (current_pstate_driver) {
> > -			if (current_pstate_driver == &amd_pstate_epp_driver)
> > -				return 0;
> > -			cpufreq_unregister_driver(current_pstate_driver);
> > -			current_pstate_driver = &amd_pstate_epp_driver;
> > -			cppc_state = AMD_PSTATE_ACTIVE;
> > -		}
> > +	if (mode_idx < 0 || mode_idx >= AMD_PSTATE_MAX)
> > +		return -EINVAL;
> > -		ret = cpufreq_register_driver(current_pstate_driver);
> > -		break;
> > -	default:
> > -		ret = -EINVAL;
> > -		break;
> > -	}
> > +	if (mode_state_machine[cppc_state][mode_idx])
> > +		return mode_state_machine[cppc_state][mode_idx](mode_idx);
> > -	return ret;
> > +	return -EBUSY;
> >   }
> >   static ssize_t show_status(struct kobject *kobj,
> 
> With one nit fixed,
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>