Date:   Tue, 31 Jan 2023 17:48:51 +0000
From:   Wyes Karny <wyes.karny@amd.com>
To:     Mario Limonciello <mario.limonciello@amd.com>
Cc:     Rafael J Wysocki <rafael@kernel.org>,
        Huang Rui <ray.huang@amd.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Viresh Kumar <viresh.kumar@linaro.org>, Perry.Yuan@amd.com,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org, Bagas Sanjaya <bagasdotme@gmail.com>,
        santosh.shukla@amd.com, Len Brown <lenb@kernel.org>,
        Robert Moore <robert.moore@intel.com>,
        Borislav Petkov <bp@alien8.de>,
        Ananth Narayan <ananth.narayan@amd.com>,
        gautham.shenoy@amd.com, Tor Vic <torvic9@mailbox.org>
Subject: Re: [PATCH v4 5/6] cpufreq: amd_pstate: Add guided mode control
 support via sysfs
Message-ID: <Y9lUgxevhdlv840K@beas>
References: <20230131052141.96475-1-wyes.karny@amd.com>
 <20230131052141.96475-6-wyes.karny@amd.com>
 <661a55c4-5703-ef84-728a-229997737416@amd.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <661a55c4-5703-ef84-728a-229997737416@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?PSYiIKNj76+RBs9y2gL2IqliBKugQ079BkCr0DVPRqpog1g41G3ogBj5iobQ?=
 =?us-ascii?Q?IcvZ0zlMm22m3DCZGRJu0tZH9jD75WosZGRfZTkmMsxnv+zzyErZ4KlP3VCC?=
 =?us-ascii?Q?FBMtX4Z3ahof8TpgHKmvlSywe4k6+/7D57RJbkz6yEmBx3agDhSevnvwoo1z?=
 =?us-ascii?Q?Ll1oYZbaV6njv+6vWRkeru+g9idSRiziIwn/bJmNobStQ6PgFgzN8CWP4SIP?=
 =?us-ascii?Q?mItf9yJJoeKBAdw/gTE8zV2eZaMQT89OUW9S4fu4F2EBc+l7uo5NEW3PJIav?=
 =?us-ascii?Q?QJKzXLmkHyvpS+qT7iUvY4VpeFtafU22neQSeQvEFzzslku3x7V/3SbCsY7C?=
 =?us-ascii?Q?lWML97fzdXSM112PP5mvwABq4d594PuOrn8uZV/y1X9uTFaS5KrzIYG2VGNp?=
 =?us-ascii?Q?lDjFRZ8SdyQuZHICOXuZjtFStKMbLS2pWIlefcJCqpevmjakD4weQv/X9zCI?=
 =?us-ascii?Q?GolAcWLNBr1p5iuT5RFokSAZSOPeINU5qKsiNh/B1ULr0Me3HupfiIr/xje8?=
 =?us-ascii?Q?dwbk7+qOXEzgl8VNfifw9+VD1mnk0Lxqs+mCxiIqLJw7BTIbzxrzJhk0LH3+?=
 =?us-ascii?Q?/nHCpjfJHIDZekkvi7tt8KVEUCBZqg9nvu4jzriS17XywVUsFj7U0TJLRQ59?=
 =?us-ascii?Q?M/GMFmhDnuSrPDOflxlX4trvSjm/rXrxiiCjObTsPjuGOLoKZZ1KuIpZfg9u?=
 =?us-ascii?Q?lu17o8zImcpS2JcFjBKvZkmlZnTtDP0X4SNm4Xq3bx4jNmzQGlTfpq7mtLkj?=
 =?us-ascii?Q?z4nahmO6zSU8/JWkugkQYls19HjeBgHuPPRPVTHn3ReR9zTryrxYD/ZrIjUG?=
 =?us-ascii?Q?Cx5dSk/AYV2jyJFjWhY6qrHq4kGbkPZdznwSygnCbcpDFlPdLk8Jq11lTxtU?=
 =?us-ascii?Q?4grgUhuYQZl2lWYd7S2c/L+609XyBASnN60F5WuGBPZVj9gOdMSHHEF4hXRy?=
 =?us-ascii?Q?q7ZG/VAR2v6/DDhhaiO4Jy6uBRaw9n6zXMUdc6sBfNbYZbuUmlzQyYYA+tyy?=
 =?us-ascii?Q?ALshgpUOs6YOZnsoeqJskmQHjCPv/hyTcEPSucitelmDFPmh294apT/01XDv?=
 =?us-ascii?Q?cwUTWiIiahfsSX8PS0a5FW+qLXeReHBLLsHy+OkH4r+8NoxRtPkZ2d5fQg5D?=
 =?us-ascii?Q?mJRoXMMq5UtPzCRd3CkJZyJqqM2QkX8i7NMoYjReiA2MwGOMkCGuuH+BM7ri?=
 =?us-ascii?Q?yfbKu0xOIm/z5Wi7Es9h7Z6b7UCoXMECtRW/yElvfobnMRKMnQtoBXk21rJL?=
 =?us-ascii?Q?XPYyIuWYzfmCNig+4h7coNGrCgGbGKjIXZcGKbvJKIQe/zwgKDraG5747h9d?=
 =?us-ascii?Q?Ngf7XphSumiOO5JIaO2cUp/tYdutm9/H674NoIq28oJWQQiK071cOh3vi2jb?=
 =?us-ascii?Q?rNB6b8oKWjOUZKcTEBBkTMgnE4vk/Sa+CyKcyWcVQVPzJKyxOMhHHIGRKZsj?=
 =?us-ascii?Q?oSX+3ienFrEV7Fn642Fd3jIVrt5vHvJtcQ+3bzBWNBPUbyK85XJtp0RknEw1?=
 =?us-ascii?Q?taj79bPrUXPKGARWv04QXaETnQPdsL1WtTlWPZc4sgEv0loBN3VPaoBINCX0?=
 =?us-ascii?Q?RzDD5VrQgBJOOAtf9jYPRXLjkwyU1C0ZIeiPfu06?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ec70a19d-8270-4911-2e86-08db03b3721d
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB3876.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2023 17:49:05.7226
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: zVCxHBky/L4O9yXP9XXl/qZzbEGDGT8m1pccXHiraNOqpow6TVC802g4hd8yOeQsD5WnNvLJKR3WTnFehWXtqw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB4325
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@amd.com>
> > ---
> >   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@amd.com>