linux-kernel - RE: [PATCH v11 3/9] cpufreq: introduce init_boost callback to initialize boost state for pstate drivers

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: Fri, 14 Jun 2024 02:43:18 +0000
From: "Yuan, Perry" <Perry.Yuan@....com>
To: "Limonciello, Mario" <Mario.Limonciello@....com>, "Shenoy, Gautham Ranjal"
	<gautham.shenoy@....com>, "Petkov, Borislav" <Borislav.Petkov@....com>
CC: "rafael.j.wysocki@...el.com" <rafael.j.wysocki@...el.com>,
	"viresh.kumar@...aro.org" <viresh.kumar@...aro.org>, "Deucher, Alexander"
	<Alexander.Deucher@....com>, "Huang, Shimmer" <Shimmer.Huang@....com>, "Du,
 Xiaojian" <Xiaojian.Du@....com>, "Meng, Li (Jassmine)" <Li.Meng@....com>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v11 3/9] cpufreq: introduce init_boost callback to
 initialize boost state for pstate drivers

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@....com>
> Sent: Friday, June 14, 2024 1:55 AM
> To: Yuan, Perry <Perry.Yuan@....com>; Shenoy, Gautham Ranjal
> <gautham.shenoy@....com>; Petkov, Borislav <Borislav.Petkov@....com>
> Cc: rafael.j.wysocki@...el.com; viresh.kumar@...aro.org; Deucher, Alexander
> <Alexander.Deucher@....com>; Huang, Shimmer
> <Shimmer.Huang@....com>; Du, Xiaojian <Xiaojian.Du@....com>; Meng,
> Li (Jassmine) <Li.Meng@....com>; linux-pm@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH v11 3/9] cpufreq: introduce init_boost callback to
> initialize boost state for pstate drivers
>
> On 6/13/2024 02:25, Perry Yuan wrote:
> > Introduce a new init_boost callback in cpufreq to initialize the boost
> > state for specific pstate drivers. This initialization is required
> > before calling the set_boost interface for each CPU.
> >
> > The init_boost callback will set up and synchronize each CPU's current
> > boost state before invoking the set_boost function. Without this step,
> > the boost state may be inconsistent across CPUs.
> >
> > Signed-off-by: Perry Yuan <perry.yuan@....com>
> > ---
> >   drivers/cpufreq/cpufreq.c | 12 ++++++++++--
> >   include/linux/cpufreq.h   |  2 ++
> >   2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 1fdabb660231..e1a4730f4f8c 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1429,8 +1429,16 @@ static int cpufreq_online(unsigned int cpu)
> >                     goto out_free_policy;
> >             }
> >
> > -           /* Let the per-policy boost flag mirror the cpufreq_driver
> boost during init */
> > -           policy->boost_enabled = cpufreq_boost_enabled() &&
> policy_has_boost_freq(policy);
> > +           /* init boost state to prepare set_boost callback for each CPU
> */
> > +           if (cpufreq_driver->init_boost) {
> > +                   ret = cpufreq_driver->init_boost(policy);
> > +                   if (ret)
> > +                           pr_debug("%s: %d: initialization boost
> failed\n", __func__,
> > +                                   __LINE__);
>
> The message should have the subject at the beginning.  IE:
>
> "boost initialization failed\n"
>
> Also, isn't this fatal if init failed?  IE shouldn't failing have a:


Firstly, I also add the " goto out_free_policy", and removed later,  because I think it is a little risky to fail the whole online process if driver init boost callback failed.
If driver init boost failed, it just let boost control function lost, but online initialize failed and go to free the policy,  it is a big problem.
I am ok to add the error handling if maintainer agree to see the potential online function aborting. 😊

Perry.

>
>       goto out_free_policy;
>
> > +           } else {
> > +                   /* Let the per-policy boost flag mirror the
> cpufreq_driver boost during init */
> > +                   policy->boost_enabled = cpufreq_boost_enabled()
> && policy_has_boost_freq(policy);
> > +           }
> >
> >             /*
> >              * The initialization has succeeded and the policy is online.
> > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index
> > 20f7e98ee8af..0698c0292d8f 100644
> > --- a/include/linux/cpufreq.h
> > +++ b/include/linux/cpufreq.h
> > @@ -409,6 +409,8 @@ struct cpufreq_driver {
> >     bool            boost_enabled;
> >     int             (*set_boost)(struct cpufreq_policy *policy, int state);
> >
> > +   /* initialize boost state to be consistent before calling set_boost */
> > +   int             (*init_boost)(struct cpufreq_policy *policy);
> >     /*
> >      * Set by drivers that want to register with the energy model after the
> >      * policy is properly initialized, but before the governor is started.