Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Thu, 12 May 2022 18:23:52 +0200 | Subject | Re: [PATCH v4 2/2] cpufreq: make interface functions and lock holding state clear |
| |
On Thu, May 12, 2022 at 6:01 PM Schspa Shi <schspa@gmail.com> wrote: > > "Rafael J. Wysocki" <rafael@kernel.org> writes: > > > On Thu, May 12, 2022 at 3:52 PM Schspa Shi <schspa@gmail.com> wrote: > >> > >> cpufreq_offline() calls offline() and exit() under the policy rwsem > >> But they are called outside the rwsem in cpufreq_online(). > >> > >> This patch move the offline(), exit(), online(), init() to be inside > >> of policy rwsem to achieve a clear lock relationship. > >> > >> All the init() online() implement only initialize policy object without > >> holding this lock and won't call cpufreq APIs need to hold this lock. > >> > >> Signed-off-by: Schspa Shi <schspa@gmail.com> > > > > IMV this still addresses 2 different issues and so it should be split > > into 2 different patches. > > > >> --- > >> drivers/cpufreq/cpufreq.c | 5 ++--- > >> 1 file changed, 2 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > >> index 35dffd738580..f242d5488364 100644 > >> --- a/drivers/cpufreq/cpufreq.c > >> +++ b/drivers/cpufreq/cpufreq.c > > > > Patch 1: > > > >> @@ -1343,12 +1343,12 @@ static int cpufreq_online(unsigned int cpu) > >> down_write(&policy->rwsem); > >> policy->cpu = cpu; > >> policy->governor = NULL; > >> - up_write(&policy->rwsem); > >> } else { > >> new_policy = true; > >> policy = cpufreq_policy_alloc(cpu); > >> if (!policy) > >> return -ENOMEM; > >> + down_write(&policy->rwsem); > >> } > >> > >> if (!new_policy && cpufreq_driver->online) { > >> @@ -1388,7 +1388,6 @@ static int cpufreq_online(unsigned int cpu) > >> cpumask_copy(policy->related_cpus, policy->cpus); > >> } > >> > >> - down_write(&policy->rwsem); > >> /* > >> * affected cpus must always be the one, which are online. We aren't > >> * managing offline cpus here. > > > > which addresses the problem that cpufreq_online() updates the > > policy->cpus and related_cpus masks without holding the policy rwsem > > (since the policy kobject has been registered already at this point, > > this is generally unsafe). > > > > A side-effect of it is that ->online() and ->init() will be called > > under the policy rwsem now, but that should be fine and is more > > consistent than the current code too. > > > > Patch 2: > > > >> @@ -1540,7 +1539,6 @@ static int cpufreq_online(unsigned int cpu) > >> remove_cpu_dev_symlink(policy, get_cpu_device(j)); > >> > >> cpumask_clear(policy->cpus); > >> - up_write(&policy->rwsem); > >> > >> out_offline_policy: > >> if (cpufreq_driver->offline) > >> @@ -1549,6 +1547,7 @@ static int cpufreq_online(unsigned int cpu) > >> out_exit_policy: > >> if (cpufreq_driver->exit) > >> cpufreq_driver->exit(policy); > >> + up_write(&policy->rwsem); > >> > >> out_free_policy: > >> cpufreq_policy_free(policy); > >> -- > > > > which addressed the issue of calling ->offline() and ->exit() without > > holding the policy rwsem that is at best inconsistent with > > cpufreq_offline(). > > No, we can't split this into two different patches. which will cause a > uninitialized unlock for policy rwsem. > This will make the git bitsec unusable. > > Which Dan Carpenter reported, and cause the patch of the v1 version to > be reverted. > > Link: https://lore.kernel.org/all/YnKZCGaig+EXSowf@kili/
Ah, OK. Never mind.
| |