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;
Date:   Thu, 12 May 2022 12:26:23 +0530
From:   Viresh Kumar <viresh.kumar@linaro.org>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     Schspa Shi <schspa@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v3] cpufreq: fix race on cpufreq online
Message-ID: <20220512065623.q4aa6y52pst3zpxu@vireshk-i7>
References: <20220510035259.5ep52sgahd2a6rie@vireshk-i7>
 <20220510154236.88753-1-schspa@gmail.com>
 <20220511043515.fn2gz6q3kcpdai5p@vireshk-i7>
 <CAMA88TpefB=rnqea2u1zEvNUJNE_kdj4mYito7SGCuMj-o071Q@mail.gmail.com>
 <20220511122114.wccgyur6g3qs6fps@vireshk-i7>
 <CAJZ5v0gN_yDFpvCXRXv8rN-i3TugCi-HKpBKK2z4eWU0Zm1GUg@mail.gmail.com>
 <CAJZ5v0id+7vkqMQEyVRe29oF_dRtzZ0EhoYUn8=yzeENDeABJw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJZ5v0id+7vkqMQEyVRe29oF_dRtzZ0EhoYUn8=yzeENDeABJw@mail.gmail.com>
User-Agent: NeoMutt/20180716-391-311a52
Precedence: bulk

On 11-05-22, 15:19, Rafael J. Wysocki wrote:
> On Wed, May 11, 2022 at 2:59 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > Hmm, I don't think depending on the values of policy->cpus is a good idea to be
> > > honest. This design is inviting bugs to come in at another place. We need a
> > > clear flag for this, a new flag or something like policy_list.
> 
> Why?

Because it doesn't mean anything unless we have code elsewhere which checks this
specifically. It should be fine though after using policy_is_inactive() in
show/store as you suggested, which I too tried to do in a patch :)

> > > Also I see the same bug happening while the policy is removed. The kobject is
> > > put after the rwsem is dropped.
> 
> This shouldn't be a problem because of the wait_for_completion() in
> cpufreq_policy_put_kobj().  It is known that cpufreq_sysfs_release()
> has run when cpufreq_policy_put_kobj() returns, so it is safe to free
> the policy then.

I agree to that, but the destruction of stuff happens right in
cpufreq_policy_free() where it starts removing the policy from the list and
clears cpufreq_cpu_data. I don't know if it will break anything or not, but we
should disallow any further sysfs operations once we have reached
cpufreq_policy_free().

> TBH, I'm not sure why show() doesn't check policy_is_inactive() under the rwsem.

I agree, both show/store should have it.

> Moreover, I'm not sure why the locking dance in store() is necessary.

commit fdd320da84c6 ("cpufreq: Lock CPU online/offline in cpufreq_register_driver()")

-- 
viresh