Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1373307

Re: [PATCH] cpufreq: Skip all governor-related actions for cpufreq_suspended set

Show all headers | View raw

On Thu, Apr 7, 2016 at 1:32 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 07-04-16, 13:22, Rafael J. Wysocki wrote:
>> On Thu, Apr 7, 2016 at 6:28 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > On 07-04-16, 03:29, Rafael J. Wysocki wrote:
>> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >>
>> >> Since governor operations are generally skipped if cpufreq_suspended
>> >> is set, do nothing at all in cpufreq_start_governor() and
>> >> cpufreq_exit_governor() in that case.
>> >>
>> >> In particular, this prevents fast frequency switching from being
>> >> disabled after a suspend-to-RAM cycle on all CPUs except for the
>> >> boot one.
>> >
>> > static int cpufreq_governor(struct cpufreq_policy *policy, unsigned int event)
>> > {
>> >         int ret;
>> >
>> >         /* Don't start any governor operations if we are entering suspend */
>> >         if (cpufreq_suspended)
>> >                 return 0;
>> >
>> >         ...
>> >
>> > }
>> >
>> > Above already guarantees that we would start/stop governors. Why do we
>> > need this change then ?
>>
>> Because we do extra stuff in cpufreq_start_governor() and
>> cpufreq_exit_governor() that *also* shouldn't be done if
>> cpufreq_suspended is set.
>
> The only extra thing done by cpufreq_exit_governor() is
> cpufreq_disable_fast_switch(), which just plays with
> cpufreq_fast_switch_count and policy->fast_switch_enabled.
>
> That should be done even if we have started to suspend.

Well, no.

The problem here is that fast switch is enabled by the governor init
and that's not called if cpufreq_suspend is set.

> The exit-governor path is be called while we hot-unplug all non-boot
> CPUs during suspend. Which would eventually mean that at least
> cpufreq_fast_switch_count will stay positive for ever now, and we just
> can't recover from this situation.

I'm not sure I'm following.

Without this patch fast switch is disabled when we offline the nonboot
CPUs during suspend, because cpufreq_exit_governor() runs then, but
the cpufreq_governor() called by it does nothing.  Also
cpufreq_governor() during nonboot CPUs online does nothing.

That has to be made consistent somehow.  This patch is one way.
Another way would be to disable fast switch from the governor ->exit
callback, but the net result would be the same.

> Similarly for cpufreq_start_governor(), we call
> cpufreq_update_current_freq(). I think we should move the check to
> this routine instead.
>
> IOW, we are using this cpufreq_suspended flag to return early in cases
> where its not safe to try to access the hardware registers, as they
> might be accessible via a device that has suspended now, like I2C or
> SPI.

Right.  So checking cpufreq_suspended upfront in
cpufreq_start_governor() and cpufreq_exit_governor() addresses that.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] cpufreq: Skip all governor-related actions for cpufreq_suspended set "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-04-07 03:30 +0200
  Re: [PATCH] cpufreq: Skip all governor-related actions for  cpufreq_suspended set Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-07 06:30 +0200
    Re: [PATCH] cpufreq: Skip all governor-related actions for  cpufreq_suspended set "Rafael J. Wysocki" <rafael@kernel.org> - 2016-04-07 13:30 +0200
      Re: [PATCH] cpufreq: Skip all governor-related actions for  cpufreq_suspended set Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-07 13:40 +0200
        Re: [PATCH] cpufreq: Skip all governor-related actions for  cpufreq_suspended set "Rafael J. Wysocki" <rafael@kernel.org> - 2016-04-07 13:50 +0200
          Re: [PATCH] cpufreq: Skip all governor-related actions for  cpufreq_suspended set Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-07 14:10 +0200
            Re: [PATCH] cpufreq: Skip all governor-related actions for cpufreq_suspended set "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-04-08 00:10 +0200
              Re: [PATCH] cpufreq: Skip all governor-related actions for  cpufreq_suspended set Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-08 07:50 +0200
                Re: [PATCH] cpufreq: Skip all governor-related actions for cpufreq_suspended set "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-04-09 00:00 +0200
  Re: [PATCH] cpufreq: Skip all governor-related actions for  cpufreq_suspended set Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-08 07:50 +0200
    Re: [PATCH] cpufreq: Skip all governor-related actions for cpufreq_suspended set "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-04-09 00:00 +0200
      Re: [PATCH] cpufreq: Skip all governor-related actions for  cpufreq_suspended set Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-10 05:20 +0200
        Re: [PATCH] cpufreq: Skip all governor-related actions for  cpufreq_suspended set "Rafael J. Wysocki" <rafael@kernel.org> - 2016-04-10 05:50 +0200
          [PATCH] cpufreq: Abort cpufreq_update_current_freq() for cpufreq_suspended set "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-04-10 06:10 +0200
            Re: [PATCH] cpufreq: Abort cpufreq_update_current_freq() for  cpufreq_suspended set Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-10 06:20 +0200

csiph-web