Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <13269660.K2JYd4sGFX@kreacher> <1876249.M1ZxxmeKtZ@kreacher> <20201110024126.v4yxai5hpguj5p5b@vireshk-i7>
In-Reply-To: <20201110024126.v4yxai5hpguj5p5b@vireshk-i7>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 10 Nov 2020 13:36:02 +0100
Message-ID: <CAJZ5v0jkdF7_JKBA0R0kvhzv-ZaLr3m9MS1g_D=vs3ZObv1VVw@mail.gmail.com>
Subject: Re: [PATCH v2 1/4] cpufreq: Introduce governor flags
To:     Viresh Kumar <viresh.kumar@linaro.org>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Linux PM <linux-pm@vger.kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        Zhang Rui <rui.zhang@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Doug Smythies <dsmythies@telus.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Nov 10, 2020 at 3:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 09-11-20, 17:51, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > A new cpufreq governor flag will be added subsequently, so replace
> > the bool dynamic_switching fleid in struct cpufreq_governor with a
> > flags field and introduce CPUFREQ_GOV_FLAG_DYN_SWITCH to set for
> > the "dynamic switching" governors instead of it.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c          |    2 +-
> >  drivers/cpufreq/cpufreq_governor.h |    2 +-
> >  include/linux/cpufreq.h            |    9 +++++++--
> >  kernel/sched/cpufreq_schedutil.c   |    2 +-
> >  4 files changed, 10 insertions(+), 5 deletions(-)
> >
> > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > @@ -2254,7 +2254,7 @@ static int cpufreq_init_governor(struct
> >               return -EINVAL;
> >
> >       /* Platform doesn't want dynamic frequency switching ? */
> > -     if (policy->governor->dynamic_switching &&
>
> I completely forgot that we had something like this :)
>
> > +     if (policy->governor->flags & CPUFREQ_GOV_FLAG_DYN_SWITCH &&
> >           cpufreq_driver->flags & CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING) {
> >               struct cpufreq_governor *gov = cpufreq_fallback_governor();
> >
> > Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
> > +++ linux-pm/drivers/cpufreq/cpufreq_governor.h
> > @@ -156,7 +156,7 @@ void cpufreq_dbs_governor_limits(struct
> >  #define CPUFREQ_DBS_GOVERNOR_INITIALIZER(_name_)                     \
> >       {                                                               \
> >               .name = _name_,                                         \
> > -             .dynamic_switching = true,                              \
> > +             .flags = CPUFREQ_GOV_FLAG_DYN_SWITCH,                   \
> >               .owner = THIS_MODULE,                                   \
> >               .init = cpufreq_dbs_governor_init,                      \
> >               .exit = cpufreq_dbs_governor_exit,                      \
> > Index: linux-pm/include/linux/cpufreq.h
> > ===================================================================
> > --- linux-pm.orig/include/linux/cpufreq.h
> > +++ linux-pm/include/linux/cpufreq.h
> > @@ -565,12 +565,17 @@ struct cpufreq_governor {
> >                                        char *buf);
> >       int     (*store_setspeed)       (struct cpufreq_policy *policy,
> >                                        unsigned int freq);
> > -     /* For governors which change frequency dynamically by themselves */
> > -     bool                    dynamic_switching;
> >       struct list_head        governor_list;
> >       struct module           *owner;
> > +     u8                      flags;
> >  };
> >
> > +/* Governor flags */
> > +
> > +/* For governors which change frequency dynamically by themselves */
> > +#define CPUFREQ_GOV_FLAG_DYN_SWITCH  BIT(0)
>
> Maybe just drop the FLAG_ part as we don't use it for other cpufreq related
> flags as well. That will also give us space to write DYN as DYNAMIC (it may be
> better as we use the full name in CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING).

OK, I'll rename the flag (and the new one too).

> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks!