Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <8c3d50faf8811e86136fb3f9c459e43fc3c50bc0.1653565641.git.viresh.kumar@linaro.org>
 <426bf6edc80b2e944d459fa7b8dffbe8b73bb3d9.1653623526.git.viresh.kumar@linaro.org>
 <CAJZ5v0gswDEdK9-gC1dPr9BFPv3G9rs+JYe-7=1JZ11OMoBb-g@mail.gmail.com> <20220615045957.55ocdyddcac3vwct@vireshk-i7>
In-Reply-To: <20220615045957.55ocdyddcac3vwct@vireshk-i7>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 6 Jul 2022 15:49:09 +0200
Message-ID: <CAJZ5v0gDCS8mkoFjnqOAi9gGuhTheE79FwvRWgeoxagMj3Ok0Q@mail.gmail.com>
Subject: Re: [PATCH V2 2/3] cpufreq: Panic if policy is active in cpufreq_policy_free()
To:     Viresh Kumar <viresh.kumar@linaro.org>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        kernel test robot <oliver.sang@intel.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 15, 2022 at 7:00 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 14-06-22, 15:59, Rafael J. Wysocki wrote:
> > On Fri, May 27, 2022 at 5:53 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > With the new design in place, to avoid potential races show() and
> > > store() callbacks check if the policy is active or not before proceeding
> > > any further. And in order to guarantee that cpufreq_policy_free() must
> > > be called after clearing the policy->cpus mask, i.e. by marking it
> > > inactive.
> > >
> > > Lets make sure we don't get a bug around this later and catch this early
> > > by putting a BUG_ON() within cpufreq_policy_free().
> > >
> > > Also update cpufreq_online() a bit to make sure we clear the cpus mask
> > > for each error case before calling cpufreq_policy_free().
> > >
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > > V2: Update cpufreq_online() and changelog.
> > >
> > >  drivers/cpufreq/cpufreq.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index e24aa5d4bca5..0f8245731783 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -1284,6 +1284,12 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
> > >         unsigned long flags;
> > >         int cpu;
> > >
> > > +       /*
> > > +        * The callers must ensure the policy is inactive by now, to avoid any
> > > +        * races with show()/store() callbacks.
> > > +        */
> > > +       BUG_ON(!policy_is_inactive(policy));
> >
> > I'm not a super-big fan of this change.
> >
> > First off, crashing the kernel outright here because of possible races
> > appears a bit excessive to me.
> >
> > Second, it looks like we are worrying about the code running before
> > the wait_for_completion() call in cpufreq_policy_put_kobj(), because
> > after that call no one can be running show() or store().  So why don't
> > we reorder the wait_for_completion() call with respect to the code in
> > question instead?
>
> No, I am not worrying about that race. I am just trying to make sure some change
> in future doesn't break this assumption (that policy should be inactive by this
> point). That's all. It all looks good for now.
>
> May be a WARN instead of BUG if we don't want to crash.

WARN_ON() would be somewhat better, but then I'm not sure if having a
full call trace in this case is really useful, because we know when
cpufreq_policy_free() can be called anyway.

Maybe just print a warning message.