Re: [PATCH] cpufreq: Do not schedule policy update work in cpufreq_resume()

[Rafael J. Wysocki]

On Wed, Mar 16, 2016 at 5:47 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
> On 15-03-16, 13:11, Rafael J. Wysocki wrote:
>> On Tue, Mar 15, 2016 at 7:10 AM, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>> > On 12-03-16, 03:05, Rafael J. Wysocki wrote:
>> >> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> >>
>> >> 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.

No, it won't.  This might be applicable to other governors, but not to
"performance" (look at what it does on _START instead of just
guessing).

> So, your patch break things for sure.

I'm not actually sure it breaks anything.

Theoretically, it may, but practically?  Is there any system out there
where it makes any difference?