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

[Rafael J. Wysocki]

On Sun, Apr 10, 2016 at 5:16 AM, Viresh Kumar <viresh.ku...@linaro.org> wrote:
> On 08-04-16, 23:54, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
>> Subject: [PATCH] cpufreq: Skip all governor-related actions for 
>> cpufreq_suspended set
>>
>> Since governor operations are generally skipped if cpufreq_suspended
>> is set, do nothing at all in cpufreq_start_governor() in that case.
>>
>> That function is called in the cpufreq_online() path, and may also
>> be called from cpufreq_offline() in some cases, which are invoked
>> by the nonboot CPUs disabing/enabling code during system suspend
>> to RAM and resume.  That happens when all devices have been
>> suspended, so if the cpufreq driver relies on things like I2C to
>> get the current frequency, it may not be ready to do that then.
>>
>> The change here prevents problems from happening for this reason.
>>
>> Fixes: 3bbf8fe3ae08 (cpufreq: Always update current frequency before startig 
>> governor)
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
>> Acked-by: Viresh Kumar <viresh.ku...@linaro.org>
>> ---
>>  drivers/cpufreq/cpufreq.c |6 ++
>>  1 file changed, 6 insertions(+)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq.c
>> ===
>> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
>> +++ linux-pm/drivers/cpufreq/cpufreq.c
>> @@ -2053,6 +2053,9 @@ static int cpufreq_start_governor(struct
>>  {
>>   int ret;
>>
>> + if (cpufreq_suspended)
>> + return 0;
>> +
>>   if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
>>   cpufreq_update_current_freq(policy);
>
> Since we no longer have the same check in cpufreq_exit_governor(),
> what about moving it to cpufreq_update_current_freq() instead? That's
> all we are trying to protect here anyway, as cpufreq_governor() is
> already protected.

That can be done too.

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

[Rafael J. Wysocki]

On Sun, Apr 10, 2016 at 5:16 AM, Viresh Kumar  wrote:
> On 08-04-16, 23:54, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki 
>> Subject: [PATCH] cpufreq: Skip all governor-related actions for 
>> cpufreq_suspended set
>>
>> Since governor operations are generally skipped if cpufreq_suspended
>> is set, do nothing at all in cpufreq_start_governor() in that case.
>>
>> That function is called in the cpufreq_online() path, and may also
>> be called from cpufreq_offline() in some cases, which are invoked
>> by the nonboot CPUs disabing/enabling code during system suspend
>> to RAM and resume.  That happens when all devices have been
>> suspended, so if the cpufreq driver relies on things like I2C to
>> get the current frequency, it may not be ready to do that then.
>>
>> The change here prevents problems from happening for this reason.
>>
>> Fixes: 3bbf8fe3ae08 (cpufreq: Always update current frequency before startig 
>> governor)
>> Signed-off-by: Rafael J. Wysocki 
>> Acked-by: Viresh Kumar 
>> ---
>>  drivers/cpufreq/cpufreq.c |6 ++
>>  1 file changed, 6 insertions(+)
>>
>> Index: linux-pm/drivers/cpufreq/cpufreq.c
>> ===
>> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
>> +++ linux-pm/drivers/cpufreq/cpufreq.c
>> @@ -2053,6 +2053,9 @@ static int cpufreq_start_governor(struct
>>  {
>>   int ret;
>>
>> + if (cpufreq_suspended)
>> + return 0;
>> +
>>   if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
>>   cpufreq_update_current_freq(policy);
>
> Since we no longer have the same check in cpufreq_exit_governor(),
> what about moving it to cpufreq_update_current_freq() instead? That's
> all we are trying to protect here anyway, as cpufreq_governor() is
> already protected.

That can be done too.

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

[Viresh Kumar]

On 08-04-16, 23:54, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> Subject: [PATCH] cpufreq: Skip all governor-related actions for 
> cpufreq_suspended set
> 
> Since governor operations are generally skipped if cpufreq_suspended
> is set, do nothing at all in cpufreq_start_governor() in that case.
> 
> That function is called in the cpufreq_online() path, and may also
> be called from cpufreq_offline() in some cases, which are invoked
> by the nonboot CPUs disabing/enabling code during system suspend
> to RAM and resume.  That happens when all devices have been
> suspended, so if the cpufreq driver relies on things like I2C to
> get the current frequency, it may not be ready to do that then.
> 
> The change here prevents problems from happening for this reason.
> 
> Fixes: 3bbf8fe3ae08 (cpufreq: Always update current frequency before startig 
> governor)
> Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> Acked-by: Viresh Kumar <viresh.ku...@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c |6 ++
>  1 file changed, 6 insertions(+)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2053,6 +2053,9 @@ static int cpufreq_start_governor(struct
>  {
>   int ret;
>  
> + if (cpufreq_suspended)
> + return 0;
> +
>   if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
>   cpufreq_update_current_freq(policy);

Since we no longer have the same check in cpufreq_exit_governor(),
what about moving it to cpufreq_update_current_freq() instead? That's
all we are trying to protect here anyway, as cpufreq_governor() is
already protected.

-- 
viresh

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

[Viresh Kumar]

On 08-04-16, 23:54, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> Subject: [PATCH] cpufreq: Skip all governor-related actions for 
> cpufreq_suspended set
> 
> Since governor operations are generally skipped if cpufreq_suspended
> is set, do nothing at all in cpufreq_start_governor() in that case.
> 
> That function is called in the cpufreq_online() path, and may also
> be called from cpufreq_offline() in some cases, which are invoked
> by the nonboot CPUs disabing/enabling code during system suspend
> to RAM and resume.  That happens when all devices have been
> suspended, so if the cpufreq driver relies on things like I2C to
> get the current frequency, it may not be ready to do that then.
> 
> The change here prevents problems from happening for this reason.
> 
> Fixes: 3bbf8fe3ae08 (cpufreq: Always update current frequency before startig 
> governor)
> Signed-off-by: Rafael J. Wysocki 
> Acked-by: Viresh Kumar 
> ---
>  drivers/cpufreq/cpufreq.c |6 ++
>  1 file changed, 6 insertions(+)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2053,6 +2053,9 @@ static int cpufreq_start_governor(struct
>  {
>   int ret;
>  
> + if (cpufreq_suspended)
> + return 0;
> +
>   if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
>   cpufreq_update_current_freq(policy);

Since we no longer have the same check in cpufreq_exit_governor(),
what about moving it to cpufreq_update_current_freq() instead? That's
all we are trying to protect here anyway, as cpufreq_governor() is
already protected.

-- 
viresh

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

[Rafael J. Wysocki]

On Friday, April 08, 2016 11:14:14 AM Viresh Kumar wrote:
> On 08-04-16, 00:05, Rafael J. Wysocki wrote:
> > On Thursday, April 07, 2016 05:35:03 PM Viresh Kumar wrote:
> 
> > > That's *ugly* and it works by chance, unless I am misreading it
> > > completely.
> > 
> > I'm assuming that what you mean by "ugly" here is "not really 
> > straightforward",
> > which I agree with,
> 
> Yeah.
> 
> > but then it is really disappointing to see comments like
> > that from you about the code that you helped to write.
> 
> I was just trying to say that this isn't how I feel it should be done.
> :(

Fair enough.

> > Moreover, runtime CPU offline *also* doesn't have to run the governor 
> > exit/init
> > for the same reason why the policy directory doesn't have to be removed on
> > CPU offline: it is just pointless to do that.  The governor has been stopped
> > already and it won't do anything more.  The only problem here is to prevent
> > governor tunable sysfs attributes from triggering actions in that state,
> > but that shouldn't be too difficult to arrange for.  If that's done,
> 
> Isn't that already guaranteed as userspace should have been frozen by
> by the time we reach cpufreq_suspend()?

For the "offline/online during suspend/resume" case it is guaranteed, but for
the "runtime offline/online" case it isn't.

I essentially would like those two cases to be as similar as reasonably
possible, if not identical.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Friday, April 08, 2016 11:14:14 AM Viresh Kumar wrote:
> On 08-04-16, 00:05, Rafael J. Wysocki wrote:
> > On Thursday, April 07, 2016 05:35:03 PM Viresh Kumar wrote:
> 
> > > That's *ugly* and it works by chance, unless I am misreading it
> > > completely.
> > 
> > I'm assuming that what you mean by "ugly" here is "not really 
> > straightforward",
> > which I agree with,
> 
> Yeah.
> 
> > but then it is really disappointing to see comments like
> > that from you about the code that you helped to write.
> 
> I was just trying to say that this isn't how I feel it should be done.
> :(

Fair enough.

> > Moreover, runtime CPU offline *also* doesn't have to run the governor 
> > exit/init
> > for the same reason why the policy directory doesn't have to be removed on
> > CPU offline: it is just pointless to do that.  The governor has been stopped
> > already and it won't do anything more.  The only problem here is to prevent
> > governor tunable sysfs attributes from triggering actions in that state,
> > but that shouldn't be too difficult to arrange for.  If that's done,
> 
> Isn't that already guaranteed as userspace should have been frozen by
> by the time we reach cpufreq_suspend()?

For the "offline/online during suspend/resume" case it is guaranteed, but for
the "runtime offline/online" case it isn't.

I essentially would like those two cases to be as similar as reasonably
possible, if not identical.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Friday, April 08, 2016 11:15:13 AM Viresh Kumar wrote:
> On 07-04-16, 03:29, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wyso...@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.
> > 
> > Fixes: b7898fda5bc7 (cpufreq: Support for fast frequency switching)
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
> > ---
> >  drivers/cpufreq/cpufreq.c |6 ++
> >  1 file changed, 6 insertions(+)
> 
> Acked-by: Viresh Kumar <viresh.ku...@linaro.org>

Thanks!

However, since I'm going to apply https://patchwork.kernel.org/patch/8777561/
to pm-cpufreq-sched, I will only apply the first hunk of the $subject one,
ie. the patch below.  I assume that the ACK still applies. :-)

I'll take it for v4.6, because it fixes up a commit already in there.

---
From: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
Subject: [PATCH] cpufreq: Skip all governor-related actions for 
cpufreq_suspended set

Since governor operations are generally skipped if cpufreq_suspended
is set, do nothing at all in cpufreq_start_governor() in that case.

That function is called in the cpufreq_online() path, and may also
be called from cpufreq_offline() in some cases, which are invoked
by the nonboot CPUs disabing/enabling code during system suspend
to RAM and resume.  That happens when all devices have been
suspended, so if the cpufreq driver relies on things like I2C to
get the current frequency, it may not be ready to do that then.

The change here prevents problems from happening for this reason.

Fixes: 3bbf8fe3ae08 (cpufreq: Always update current frequency before startig 
governor)
Signed-off-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
Acked-by: Viresh Kumar <viresh.ku...@linaro.org>
---
 drivers/cpufreq/cpufreq.c |6 ++
 1 file changed, 6 insertions(+)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2053,6 +2053,9 @@ static int cpufreq_start_governor(struct
 {
int ret;
 
+   if (cpufreq_suspended)
+   return 0;
+
if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
cpufreq_update_current_freq(policy);
 

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

[Rafael J. Wysocki]

On Friday, April 08, 2016 11:15:13 AM Viresh Kumar wrote:
> On 07-04-16, 03:29, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> > 
> > 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.
> > 
> > Fixes: b7898fda5bc7 (cpufreq: Support for fast frequency switching)
> > Signed-off-by: Rafael J. Wysocki 
> > ---
> >  drivers/cpufreq/cpufreq.c |6 ++
> >  1 file changed, 6 insertions(+)
> 
> Acked-by: Viresh Kumar 

Thanks!

However, since I'm going to apply https://patchwork.kernel.org/patch/8777561/
to pm-cpufreq-sched, I will only apply the first hunk of the $subject one,
ie. the patch below.  I assume that the ACK still applies. :-)

I'll take it for v4.6, because it fixes up a commit already in there.

---
From: Rafael J. Wysocki 
Subject: [PATCH] cpufreq: Skip all governor-related actions for 
cpufreq_suspended set

Since governor operations are generally skipped if cpufreq_suspended
is set, do nothing at all in cpufreq_start_governor() in that case.

That function is called in the cpufreq_online() path, and may also
be called from cpufreq_offline() in some cases, which are invoked
by the nonboot CPUs disabing/enabling code during system suspend
to RAM and resume.  That happens when all devices have been
suspended, so if the cpufreq driver relies on things like I2C to
get the current frequency, it may not be ready to do that then.

The change here prevents problems from happening for this reason.

Fixes: 3bbf8fe3ae08 (cpufreq: Always update current frequency before startig 
governor)
Signed-off-by: Rafael J. Wysocki 
Acked-by: Viresh Kumar 
---
 drivers/cpufreq/cpufreq.c |6 ++
 1 file changed, 6 insertions(+)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2053,6 +2053,9 @@ static int cpufreq_start_governor(struct
 {
int ret;
 
+   if (cpufreq_suspended)
+   return 0;
+
if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
cpufreq_update_current_freq(policy);
 

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

[Viresh Kumar]

On 07-04-16, 03:29, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> 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.
> 
> Fixes: b7898fda5bc7 (cpufreq: Support for fast frequency switching)
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/cpufreq/cpufreq.c |6 ++
>  1 file changed, 6 insertions(+)

Acked-by: Viresh Kumar 

-- 
viresh

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

[Viresh Kumar]

On 07-04-16, 03:29, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> 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.
> 
> Fixes: b7898fda5bc7 (cpufreq: Support for fast frequency switching)
> Signed-off-by: Rafael J. Wysocki 
> ---
>  drivers/cpufreq/cpufreq.c |6 ++
>  1 file changed, 6 insertions(+)

Acked-by: Viresh Kumar 

-- 
viresh

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

[Viresh Kumar]

On 08-04-16, 00:05, Rafael J. Wysocki wrote:
> On Thursday, April 07, 2016 05:35:03 PM Viresh Kumar wrote:

> > That's *ugly* and it works by chance, unless I am misreading it
> > completely.
> 
> I'm assuming that what you mean by "ugly" here is "not really 
> straightforward",
> which I agree with,

Yeah.

> but then it is really disappointing to see comments like
> that from you about the code that you helped to write.

I was just trying to say that this isn't how I feel it should be done.
:(

> Moreover, runtime CPU offline *also* doesn't have to run the governor 
> exit/init
> for the same reason why the policy directory doesn't have to be removed on
> CPU offline: it is just pointless to do that.  The governor has been stopped
> already and it won't do anything more.  The only problem here is to prevent
> governor tunable sysfs attributes from triggering actions in that state,
> but that shouldn't be too difficult to arrange for.  If that's done,

Isn't that already guaranteed as userspace should have been frozen by
by the time we reach cpufreq_suspend()?

> cpufreq_suspended can be dropped, modulo changing cpufreq_start_governor()
> to return immediately if the governor has been started already.
> 
> And if something else is needed to protect driver callbacks from being invoked
> outside of the suspend-resume path, a more robust mechanism has to be added
> for that.
> 
> But in the meantime, I'd like to address the fast switch problem first and
> then you're free to clean up things on top of that.  Or I will clean them up
> if I have the time.

Okay..

-- 
viresh

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

[Viresh Kumar]

On 08-04-16, 00:05, Rafael J. Wysocki wrote:
> On Thursday, April 07, 2016 05:35:03 PM Viresh Kumar wrote:

> > That's *ugly* and it works by chance, unless I am misreading it
> > completely.
> 
> I'm assuming that what you mean by "ugly" here is "not really 
> straightforward",
> which I agree with,

Yeah.

> but then it is really disappointing to see comments like
> that from you about the code that you helped to write.

I was just trying to say that this isn't how I feel it should be done.
:(

> Moreover, runtime CPU offline *also* doesn't have to run the governor 
> exit/init
> for the same reason why the policy directory doesn't have to be removed on
> CPU offline: it is just pointless to do that.  The governor has been stopped
> already and it won't do anything more.  The only problem here is to prevent
> governor tunable sysfs attributes from triggering actions in that state,
> but that shouldn't be too difficult to arrange for.  If that's done,

Isn't that already guaranteed as userspace should have been frozen by
by the time we reach cpufreq_suspend()?

> cpufreq_suspended can be dropped, modulo changing cpufreq_start_governor()
> to return immediately if the governor has been started already.
> 
> And if something else is needed to protect driver callbacks from being invoked
> outside of the suspend-resume path, a more robust mechanism has to be added
> for that.
> 
> But in the meantime, I'd like to address the fast switch problem first and
> then you're free to clean up things on top of that.  Or I will clean them up
> if I have the time.

Okay..

-- 
viresh

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

[Rafael J. Wysocki]

On Thursday, April 07, 2016 05:35:03 PM Viresh Kumar wrote:
> On 07-04-16, 13:44, Rafael J. Wysocki wrote:
> > 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.
> 
> Actually things are working fine today by chance IMO, because we don't
> free the policy structures anymore while we offline CPUs.

Yes, that's why they work.  And that's because the code has been written
that way.  Whether that happened by chance or by design, or because of
a favorable concentration of the Force, I don't care.

> Otherwise, policy->governor_data would have been lost together with
> the policy, and governor wouldn't have worked properly after resume.
> 
> What we are doing today is something like this:
> 
> Suspend
> ---
> 
> -> cpufreq_suspend()
>  -> STOP governor
>  -> cpufreq_suspended = true
> 
> -> Offline non-boot CPUs
>   -> cpufreq_offline()
> -> SKIP calling EXIT governor (governor had allocated few
> resources earlier)
> 
> Resume
> --
> 
> -> Bring back non-boot CPUs
>   -> cpufreq_online()
> -> SKIP calling INIT governor (policy->governor_data doesn't get
> reset, luckily)

policy->governor_data is not reset.  Period.

> 
> -> cpufreq_resume()
>  -> cpufreq_suspended = false
>  -> START governor

Yes, that's what the code flow is.

> 
> That's *ugly* and it works by chance, unless I am misreading it
> completely.

I'm assuming that what you mean by "ugly" here is "not really straightforward",
which I agree with, but then it is really disappointing to see comments like
that from you about the code that you helped to write.

But instead of going for a rant about how disappointed I am, let me focus on
the technical side of things.

As per the code today, the only legitimate role of cpufreq_suspended is to 
prevent
governor operations from being carried out when disabling nonboot CPUs during
system suspend.  My *interpretation* of that is that this is to avoid accessing
hardware resources that may not be available at that point, which is fair 
enough.
It also has a nice side effect that the disabling/enabling nonboot CPUs doesn't
run code that it doesn't have to run (like remmoving/creating governor tunables
directory in sysfs in the tunables-per-policy case).  That is good.

The bad thing is how that is different from the runtime CPU offline.

> One of the solutions to get this cleaned is to stop checking for
> cpufreq_suspended flag in cpufreq_governor() and put that *only* in
> places where we are trying to interact with the hardware. And that
> essentially is the callbacks provided by the cpufreq drivers. So,
> ignore calling cpufreq-driver callbacks if cpufreq_suspended is true.

No, cpufreq_suspended is not sufficient for that, because the setting/clearing
of it is generally racy with respect to pretty much anything except for the
suspend process itself.  Checking it outside of the suspend process would be
a bug.

Moreover, runtime CPU offline *also* doesn't have to run the governor exit/init
for the same reason why the policy directory doesn't have to be removed on
CPU offline: it is just pointless to do that.  The governor has been stopped
already and it won't do anything more.  The only problem here is to prevent
governor tunable sysfs attributes from triggering actions in that state,
but that shouldn't be too difficult to arrange for.  If that's done,
cpufreq_suspended can be dropped, modulo changing cpufreq_start_governor()
to return immediately if the governor has been started already.

And if something else is needed to protect driver callbacks from being invoked
outside of the suspend-resume path, a more robust mechanism has to be added
for that.

But in the meantime, I'd like to address the fast switch problem first and
then you're free to clean up things on top of that.  Or I will clean them up
if I have the time.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thursday, April 07, 2016 05:35:03 PM Viresh Kumar wrote:
> On 07-04-16, 13:44, Rafael J. Wysocki wrote:
> > 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.
> 
> Actually things are working fine today by chance IMO, because we don't
> free the policy structures anymore while we offline CPUs.

Yes, that's why they work.  And that's because the code has been written
that way.  Whether that happened by chance or by design, or because of
a favorable concentration of the Force, I don't care.

> Otherwise, policy->governor_data would have been lost together with
> the policy, and governor wouldn't have worked properly after resume.
> 
> What we are doing today is something like this:
> 
> Suspend
> ---
> 
> -> cpufreq_suspend()
>  -> STOP governor
>  -> cpufreq_suspended = true
> 
> -> Offline non-boot CPUs
>   -> cpufreq_offline()
> -> SKIP calling EXIT governor (governor had allocated few
> resources earlier)
> 
> Resume
> --
> 
> -> Bring back non-boot CPUs
>   -> cpufreq_online()
> -> SKIP calling INIT governor (policy->governor_data doesn't get
> reset, luckily)

policy->governor_data is not reset.  Period.

> 
> -> cpufreq_resume()
>  -> cpufreq_suspended = false
>  -> START governor

Yes, that's what the code flow is.

> 
> That's *ugly* and it works by chance, unless I am misreading it
> completely.

I'm assuming that what you mean by "ugly" here is "not really straightforward",
which I agree with, but then it is really disappointing to see comments like
that from you about the code that you helped to write.

But instead of going for a rant about how disappointed I am, let me focus on
the technical side of things.

As per the code today, the only legitimate role of cpufreq_suspended is to 
prevent
governor operations from being carried out when disabling nonboot CPUs during
system suspend.  My *interpretation* of that is that this is to avoid accessing
hardware resources that may not be available at that point, which is fair 
enough.
It also has a nice side effect that the disabling/enabling nonboot CPUs doesn't
run code that it doesn't have to run (like remmoving/creating governor tunables
directory in sysfs in the tunables-per-policy case).  That is good.

The bad thing is how that is different from the runtime CPU offline.

> One of the solutions to get this cleaned is to stop checking for
> cpufreq_suspended flag in cpufreq_governor() and put that *only* in
> places where we are trying to interact with the hardware. And that
> essentially is the callbacks provided by the cpufreq drivers. So,
> ignore calling cpufreq-driver callbacks if cpufreq_suspended is true.

No, cpufreq_suspended is not sufficient for that, because the setting/clearing
of it is generally racy with respect to pretty much anything except for the
suspend process itself.  Checking it outside of the suspend process would be
a bug.

Moreover, runtime CPU offline *also* doesn't have to run the governor exit/init
for the same reason why the policy directory doesn't have to be removed on
CPU offline: it is just pointless to do that.  The governor has been stopped
already and it won't do anything more.  The only problem here is to prevent
governor tunable sysfs attributes from triggering actions in that state,
but that shouldn't be too difficult to arrange for.  If that's done,
cpufreq_suspended can be dropped, modulo changing cpufreq_start_governor()
to return immediately if the governor has been started already.

And if something else is needed to protect driver callbacks from being invoked
outside of the suspend-resume path, a more robust mechanism has to be added
for that.

But in the meantime, I'd like to address the fast switch problem first and
then you're free to clean up things on top of that.  Or I will clean them up
if I have the time.

Thanks,
Rafael

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

[Viresh Kumar]

On 07-04-16, 13:44, Rafael J. Wysocki wrote:
> 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.

Actually things are working fine today by chance IMO, because we don't
free the policy structures anymore while we offline CPUs.

Otherwise, policy->governor_data would have been lost together with
the policy, and governor wouldn't have worked properly after resume.

What we are doing today is something like this:

Suspend
---

-> cpufreq_suspend()
 -> STOP governor
 -> cpufreq_suspended = true

-> Offline non-boot CPUs
  -> cpufreq_offline()
-> SKIP calling EXIT governor (governor had allocated few
resources earlier)

Resume
--

-> Bring back non-boot CPUs
  -> cpufreq_online()
-> SKIP calling INIT governor (policy->governor_data doesn't get
reset, luckily)

-> cpufreq_resume()
 -> cpufreq_suspended = false
 -> START governor


That's *ugly* and it works by chance, unless I am misreading it
completely.

One of the solutions to get this cleaned is to stop checking for
cpufreq_suspended flag in cpufreq_governor() and put that *only* in
places where we are trying to interact with the hardware. And that
essentially is the callbacks provided by the cpufreq drivers. So,
ignore calling cpufreq-driver callbacks if cpufreq_suspended is true.

-- 
viresh

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

[Viresh Kumar]

On 07-04-16, 13:44, Rafael J. Wysocki wrote:
> 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.

Actually things are working fine today by chance IMO, because we don't
free the policy structures anymore while we offline CPUs.

Otherwise, policy->governor_data would have been lost together with
the policy, and governor wouldn't have worked properly after resume.

What we are doing today is something like this:

Suspend
---

-> cpufreq_suspend()
 -> STOP governor
 -> cpufreq_suspended = true

-> Offline non-boot CPUs
  -> cpufreq_offline()
-> SKIP calling EXIT governor (governor had allocated few
resources earlier)

Resume
--

-> Bring back non-boot CPUs
  -> cpufreq_online()
-> SKIP calling INIT governor (policy->governor_data doesn't get
reset, luckily)

-> cpufreq_resume()
 -> cpufreq_suspended = false
 -> START governor


That's *ugly* and it works by chance, unless I am misreading it
completely.

One of the solutions to get this cleaned is to stop checking for
cpufreq_suspended flag in cpufreq_governor() and put that *only* in
places where we are trying to interact with the hardware. And that
essentially is the callbacks provided by the cpufreq drivers. So,
ignore calling cpufreq-driver callbacks if cpufreq_suspended is true.

-- 
viresh

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

[Rafael J. Wysocki]

On Thu, Apr 7, 2016 at 1:32 PM, Viresh Kumar  wrote:
> On 07-04-16, 13:22, Rafael J. Wysocki wrote:
>> On Thu, Apr 7, 2016 at 6:28 AM, Viresh Kumar  wrote:
>> > On 07-04-16, 03:29, Rafael J. Wysocki wrote:
>> >> From: Rafael J. Wysocki 
>> >>
>> >> 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.

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

[Rafael J. Wysocki]

On Thu, Apr 7, 2016 at 1:32 PM, Viresh Kumar  wrote:
> On 07-04-16, 13:22, Rafael J. Wysocki wrote:
>> On Thu, Apr 7, 2016 at 6:28 AM, Viresh Kumar  wrote:
>> > On 07-04-16, 03:29, Rafael J. Wysocki wrote:
>> >> From: Rafael J. Wysocki 
>> >>
>> >> 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.

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

[Viresh Kumar]

On 07-04-16, 13:22, Rafael J. Wysocki wrote:
> On Thu, Apr 7, 2016 at 6:28 AM, Viresh Kumar  wrote:
> > On 07-04-16, 03:29, Rafael J. Wysocki wrote:
> >> From: Rafael J. Wysocki 
> >>
> >> 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.

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.

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.

-- 
viresh

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

[Viresh Kumar]

On 07-04-16, 13:22, Rafael J. Wysocki wrote:
> On Thu, Apr 7, 2016 at 6:28 AM, Viresh Kumar  wrote:
> > On 07-04-16, 03:29, Rafael J. Wysocki wrote:
> >> From: Rafael J. Wysocki 
> >>
> >> 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.

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.

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.

-- 
viresh

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

[Rafael J. Wysocki]

On Thu, Apr 7, 2016 at 6:28 AM, Viresh Kumar  wrote:
> On 07-04-16, 03:29, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki 
>>
>> 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.

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

[Rafael J. Wysocki]

On Thu, Apr 7, 2016 at 6:28 AM, Viresh Kumar  wrote:
> On 07-04-16, 03:29, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki 
>>
>> 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.

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

[Viresh Kumar]

On 07-04-16, 03:29, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> 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 ?


-- 
viresh

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

[Viresh Kumar]

On 07-04-16, 03:29, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> 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 ?


-- 
viresh

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

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.

Fixes: b7898fda5bc7 (cpufreq: Support for fast frequency switching)
Signed-off-by: Rafael J. Wysocki 
---
 drivers/cpufreq/cpufreq.c |6 ++
 1 file changed, 6 insertions(+)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2053,6 +2053,9 @@ static int cpufreq_start_governor(struct
 {
int ret;
 
+   if (cpufreq_suspended)
+   return 0;
+
if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
cpufreq_update_current_freq(policy);
 
@@ -2062,6 +2065,9 @@ static int cpufreq_start_governor(struct
 
 static int cpufreq_exit_governor(struct cpufreq_policy *policy)
 {
+   if (cpufreq_suspended)
+   return 0;
+
cpufreq_disable_fast_switch(policy);
return cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
 }

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

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

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.

Fixes: b7898fda5bc7 (cpufreq: Support for fast frequency switching)
Signed-off-by: Rafael J. Wysocki 
---
 drivers/cpufreq/cpufreq.c |6 ++
 1 file changed, 6 insertions(+)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2053,6 +2053,9 @@ static int cpufreq_start_governor(struct
 {
int ret;
 
+   if (cpufreq_suspended)
+   return 0;
+
if (cpufreq_driver->get && !cpufreq_driver->setpolicy)
cpufreq_update_current_freq(policy);
 
@@ -2062,6 +2065,9 @@ static int cpufreq_start_governor(struct
 
 static int cpufreq_exit_governor(struct cpufreq_policy *policy)
 {
+   if (cpufreq_suspended)
+   return 0;
+
cpufreq_disable_fast_switch(policy);
return cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT);
 }