linux-kernel - Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq

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]

Message-ID: <20160316044757.GB16250@vireshk-mac-ubuntu>
Date:	Wed, 16 Mar 2016 11:47:57 +0700
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	"Rafael J. Wysocki" <rafael@...nel.org>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Linux PM list <linux-pm@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Subject: Re: [PATCH] cpufreq: Do not schedule policy update work in
 cpufreq_resume()

On 15-03-16, 13:11, Rafael J. Wysocki wrote:
> On Tue, Mar 15, 2016 at 7:10 AM, Viresh Kumar <viresh.kumar@...aro.org> wrote:
> > On 12-03-16, 03:05, Rafael J. Wysocki wrote:
> >> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> >>
> >> cpufreq_resume() attempts to resync the current frequency with
> >> policy->cur for the first online CPU, but first it does that after
> >> restarting governors for all active policies (which means that this
> >> is racy with respect to whatever the governors do) and second it
> >
> > Why? Its doing the update withing policy->rwsem ..
> 
> Which doesn't matter.
> 
> dbs_work_handler() doesn't acquire policy->rwsem and may be executed
> in parallel with this, for example.

Right, so we need to fixup something here.

> >> already is too late for that when cpufreq_resume() is called (that
> >> happens after invoking ->resume callbacks for all devices in the
> >> system).
> >>
> >> Also it doesn't make sense to do that for one CPU only in any case,
> >> because the other CPUs in the system need not share the policy with
> >> it and their policy->cur may be out of sync as well in principle.
> >
> > Its done just for the boot CPU, because that's the only CPU that goes to
> > suspend. All other CPUs are disabled/enabled and so the policies are
> > reinitialized for policy->cur as well.
> >
> > I think, its still important to get things in sync, as some bootloader may
> > change the frequency to something else during resume.
> >
> > And our code may not be safe for the case, the current frequency of the CPU
> > isn't part of the freq-table of the policy.
> 
> Since we're already started the governor at this point (or called the
> driver's ->resume), so the CPU is (or shortly will be) running at a
> frequency that makes sense at this point.
> 
> It might be running at a wrong one before, but not when this code is executed.

Not necessarily.

Consider Performance governor for example. Lets say policy->max is 1 GHz, so
before suspend policy->cur will be 1 GHz. We suspended and resumed, and the
bootloader changed the frequency to 500 MHz (but policy->cur remains the same at
1 GHz). Even after calling START for the governor, it will continue to run at
500 MHz.

So, your patch break things for sure.

> I kind of understand the motivation for this code, but it's too late
> to fix up the frequency of the boot CPU at this point.  If you are
> really worried about it, the time to do that is in syscore ops.

Hmm, so maybe fix policy->cur at the top of this routine? syscore-ops wouldn't
get called for boot-cpu and so it wouldn't matter.

-- 
viresh