linux-kernel - RE: [PATCH v9 08/13] cpufreq: amd-pstate: implement suspend and resume callbacks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 5 Jan 2023 15:08:58 +0000
From:   "Yuan, Perry" <Perry.Yuan@....com>
To:     Viresh Kumar <viresh.kumar@...aro.org>,
        "Limonciello, Mario" <Mario.Limonciello@....com>
CC:     "rafael.j.wysocki@...el.com" <rafael.j.wysocki@...el.com>,
        "Huang, Ray" <Ray.Huang@....com>,
        "Sharma, Deepak" <Deepak.Sharma@....com>,
        "Fontenot, Nathan" <Nathan.Fontenot@....com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Huang, Shimmer" <Shimmer.Huang@....com>,
        "Du, Xiaojian" <Xiaojian.Du@....com>,
        "Meng, Li (Jassmine)" <Li.Meng@....com>,
        "Karny, Wyes" <Wyes.Karny@....com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v9 08/13] cpufreq: amd-pstate: implement suspend and
 resume callbacks

[AMD Official Use Only - General]

Hi Viresh. 

> -----Original Message-----
> From: Viresh Kumar <viresh.kumar@...aro.org>
> Sent: Tuesday, December 27, 2022 10:52 AM
> To: Yuan, Perry <Perry.Yuan@....com>
> Cc: rafael.j.wysocki@...el.com; Limonciello, Mario
> <Mario.Limonciello@....com>; Huang, Ray <Ray.Huang@....com>;
> Sharma, Deepak <Deepak.Sharma@....com>; Fontenot, Nathan
> <Nathan.Fontenot@....com>; Deucher, Alexander
> <Alexander.Deucher@....com>; Huang, Shimmer
> <Shimmer.Huang@....com>; Du, Xiaojian <Xiaojian.Du@....com>; Meng,
> Li (Jassmine) <Li.Meng@....com>; Karny, Wyes <Wyes.Karny@....com>;
> linux-pm@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v9 08/13] cpufreq: amd-pstate: implement suspend and
> resume callbacks
> 
> On 26-12-22, 00:34, Perry Yuan wrote:
> > From: Perry Yuan <Perry.Yuan@....com>
> >
> > add suspend and resume support for the AMD processors by
> > amd_pstate_epp driver instance.
> >
> > When the CPPC is suspended, EPP driver will set EPP profile to 'power'
> > profile and set max/min perf to lowest perf value.
> > When resume happens, it will restore the MSR registers with previous
> > cached value.
> >
> > Acked-by: Huang Rui <ray.huang@....com>
> > Reviewed-by: Mario Limonciello <Mario.Limonciello@....com>
> > Signed-off-by: Perry Yuan <Perry.Yuan@....com>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 40
> > ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index c671f4955766..e3676d1a85c7 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -1041,6 +1041,44 @@ static int amd_pstate_epp_verify_policy(struct
> cpufreq_policy_data *policy)
> >  	return 0;
> >  }
> >
> > +static int amd_pstate_epp_suspend(struct cpufreq_policy *policy) {
> > +	struct amd_cpudata *cpudata = policy->driver_data;
> > +	int ret;
> > +
> > +	/* avoid suspending when EPP is not enabled */
> > +	if (cppc_state != AMD_PSTATE_ACTIVE)
> > +		return 0;
> > +
> > +	/* set this flag to avoid setting core offline*/
> > +	cpudata->suspended = true;
> > +
> > +	/* disable CPPC in lowlevel firmware */
> > +	ret = amd_pstate_enable(false);
> > +	if (ret)
> > +		pr_err("failed to suspend, return %d\n", ret);
> > +
> > +	return 0;
> > +}
> > +
> > +static int amd_pstate_epp_resume(struct cpufreq_policy *policy) {
> > +	struct amd_cpudata *cpudata = policy->driver_data;
> > +
> > +	if (cpudata->suspended) {
> 
> When will resume get called without being suspended first ?

Sorry for the late reply.
Theoretically the resume() will get called when system suspended firstly.
Checking cpudata->suspended flag to make sure eachtime resume() is called to resume the previous MSR values safely in my view.
Maybe we can drop the checking code, but it will take more time to run testing~  
So to be safe , we can keep this, I will try to do some optimization in future. 


Perry. 

> 
> > +		mutex_lock(&amd_pstate_limits_lock);
> > +
> > +		/* enable amd pstate from suspend state*/
> > +		amd_pstate_epp_reenable(cpudata);
> > +
> > +		mutex_unlock(&amd_pstate_limits_lock);
> > +
> > +		cpudata->suspended = false;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static struct cpufreq_driver amd_pstate_driver = {
> >  	.flags		= CPUFREQ_CONST_LOOPS |
> CPUFREQ_NEED_UPDATE_LIMITS,
> >  	.verify		= amd_pstate_verify,
> > @@ -1062,6 +1100,8 @@ static struct cpufreq_driver
> amd_pstate_epp_driver = {
> >  	.exit		= amd_pstate_epp_cpu_exit,
> >  	.offline	= amd_pstate_epp_cpu_offline,
> >  	.online		= amd_pstate_epp_cpu_online,
> > +	.suspend	= amd_pstate_epp_suspend,
> > +	.resume		= amd_pstate_epp_resume,
> >  	.name		= "amd_pstate_epp",
> >  	.attr		= amd_pstate_epp_attr,
> >  };
> > --
> > 2.34.1
> 
> --
> viresh