S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Jarzmik, Robert]

Hi Rafael,

I came across that little odd behavior in "Suspend to RAM" :
 1) I do a "chmod audio:audio /sys/devices/system/cpu1/cpufreq/scaling_max_freq (I know, stupid, but it's for the example)
 2) I do a "echo mem > /sys/power/state"
 3) I resume the kernel
     => here the permissions of /sys/devices/system/cpu1/cpufreq/scaling_max_freq have turned back to root:root

OK, fair well, I add a UDEV rule which does a "chown audio:audio /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq" upon udev event "online, /sys/...".
And here it becomes interesting :
 - the udev event "offline" or "online" never comes with the "Suspend to RAM" testcase
 - but the same udev event comes when I manually do a "echo 0 > /sys/devices/system/cpu/cpu1/online"

So my question is : is it intended behavior that no udev offline/online event happens on S3 for non-boot CPUs, or is it something I should try to fix ?

Cheers.

--
Robert

PS: I have a rather old kernel (3.4), so forgive me if the question is outdated ...
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Srivatsa S. Bhat]

On 05/13/2013 06:31 PM, Jarzmik, Robert wrote:
> Hi Rafael,
> 
> I came across that little odd behavior in "Suspend to RAM" :
>  1) I do a "chmod audio:audio /sys/devices/system/cpu1/cpufreq/scaling_max_freq (I know, stupid, but it's for the example)
>  2) I do a "echo mem > /sys/power/state"
>  3) I resume the kernel
>      => here the permissions of /sys/devices/system/cpu1/cpufreq/scaling_max_freq have turned back to root:root
> 
> OK, fair well, I add a UDEV rule which does a "chown audio:audio /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq" upon udev event "online, /sys/...".
> And here it becomes interesting :
>  - the udev event "offline" or "online" never comes with the "Suspend to RAM" testcase
>  - but the same udev event comes when I manually do a "echo 0 > /sys/devices/system/cpu/cpu1/online"
> 
> So my question is : is it intended behavior that no udev offline/online event happens on S3 for non-boot CPUs, or is it something I should try to fix ?
>

IMHO, using CPU hotplug (offline/online of CPUs) in the S3 path is
supposed to be totally internal to the suspend/resume code.
It is not intended for userspace to know that we are internally
offlining/onlining CPUs. So I'm not surprised that udev events
are not triggered for hotplug in S3 path. And that's not a bug,
if you ask me. (And yes, even code-wise, we use a slightly different
path in the S3 code, to initiate hotplug. That's why the uevents
are by-passed.)

The user initiated an S3 operation, not CPU hotplug. So there is
no reason to surprise the user with unexpected events.
Put another way, in the future, if we change the kernel code
to do S3 without using hotplug, then there should be no visible
change in userspace, because how S3 is handled in the kernel is
intended to be an "internal" operation. 

Regards,
Srivatsa S. Bhat

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Rafael J. Wysocki]

On Tuesday, May 14, 2013 02:10:35 AM Srivatsa S. Bhat wrote:
> On 05/13/2013 06:31 PM, Jarzmik, Robert wrote:
> > Hi Rafael,
> > 
> > I came across that little odd behavior in "Suspend to RAM" :
> >  1) I do a "chmod audio:audio /sys/devices/system/cpu1/cpufreq/scaling_max_freq (I know, stupid, but it's for the example)
> >  2) I do a "echo mem > /sys/power/state"
> >  3) I resume the kernel
> >      => here the permissions of /sys/devices/system/cpu1/cpufreq/scaling_max_freq have turned back to root:root
> > 
> > OK, fair well, I add a UDEV rule which does a "chown audio:audio /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq" upon udev event "online, /sys/...".
> > And here it becomes interesting :
> >  - the udev event "offline" or "online" never comes with the "Suspend to RAM" testcase
> >  - but the same udev event comes when I manually do a "echo 0 > /sys/devices/system/cpu/cpu1/online"
> > 
> > So my question is : is it intended behavior that no udev offline/online event happens on S3 for non-boot CPUs, or is it something I should try to fix ?
> >
> 
> IMHO, using CPU hotplug (offline/online of CPUs) in the S3 path is
> supposed to be totally internal to the suspend/resume code.
> It is not intended for userspace to know that we are internally
> offlining/onlining CPUs. So I'm not surprised that udev events
> are not triggered for hotplug in S3 path. And that's not a bug,
> if you ask me. (And yes, even code-wise, we use a slightly different
> path in the S3 code, to initiate hotplug. That's why the uevents
> are by-passed.)
> 
> The user initiated an S3 operation, not CPU hotplug. So there is
> no reason to surprise the user with unexpected events.
> Put another way, in the future, if we change the kernel code
> to do S3 without using hotplug, then there should be no visible
> change in userspace, because how S3 is handled in the kernel is
> intended to be an "internal" operation. 

Yes, that's correct.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Jarzmik, Robert]

-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@sisk.pl] 
Sent: Tuesday, May 14, 2013 1:33 AM
To: Srivatsa S. Bhat; Jarzmik, Robert
Cc: linux-pm@vger.kernel.org
Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

> > So my question is : is it intended behavior that no udev offline/online event happens on S3 for non-boot CPUs, or is it something I should try to fix ?
> 
> IMHO, using CPU hotplug (offline/online of CPUs) in the S3 path is 
> supposed to be totally internal to the suspend/resume code.
> It is not intended for userspace to know that we are internally 
> offlining/onlining CPUs. So I'm not surprised that udev events are not 
> triggered for hotplug in S3 path. And that's not a bug, if you ask me. 
> (And yes, even code-wise, we use a slightly different path in the S3 
> code, to initiate hotplug. That's why the uevents are by-passed.)
> 
> The user initiated an S3 operation, not CPU hotplug. So there is no 
> reason to surprise the user with unexpected events.
Hi Srivatsa,

But the user actually *is* very surprised to have "lost" his
permissions to write to scaling_max_freq. Or, told differently,
if a userspace process handles this scaling_max_freq based on
thermal values for example, a suspend/resume will impede his
capacity to throttle the CPU, and the CPU will overheat.

A good example is an Android phone : imagine the thermal will not
be able anymore to throttle the CPU, the phone will become hot
and possibly burn the user (yeah, that's not very probable but
still ...).

So you say "there is no reason to surprise the user with
unexpected events", and I say "this is a bug that permissions of
a /sys file change because a suspend/resume happened". Now if I'm
right, what could be done to keep these permissions, other than
adding a udev event ?

> Put another way, in the future, if we change the kernel code to do S3 
> without using hotplug, then there should be no visible change in 
> userspace, because how S3 is handled in the kernel is intended to be 
> an "internal" operation.
Yep.


Cheers.

--
Robert

PS: My first experience with Outlook instead of my usual Gnus doesn't
promise a great future cooperation. I'm sorry for mail formatting ...

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Srivatsa S. Bhat]

On 05/14/2013 02:36 PM, Jarzmik, Robert wrote:
> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl] 
> Sent: Tuesday, May 14, 2013 1:33 AM
> To: Srivatsa S. Bhat; Jarzmik, Robert
> Cc: linux-pm@vger.kernel.org
> Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq
> 
>>> So my question is : is it intended behavior that no udev offline/online event happens on S3 for non-boot CPUs, or is it something I should try to fix ?
>>
>> IMHO, using CPU hotplug (offline/online of CPUs) in the S3 path is 
>> supposed to be totally internal to the suspend/resume code.
>> It is not intended for userspace to know that we are internally 
>> offlining/onlining CPUs. So I'm not surprised that udev events are not 
>> triggered for hotplug in S3 path. And that's not a bug, if you ask me. 
>> (And yes, even code-wise, we use a slightly different path in the S3 
>> code, to initiate hotplug. That's why the uevents are by-passed.)
>>
>> The user initiated an S3 operation, not CPU hotplug. So there is no 
>> reason to surprise the user with unexpected events.
> Hi Srivatsa,
> 
> But the user actually *is* very surprised to have "lost" his
> permissions to write to scaling_max_freq.

And losing permissions isn't specific to suspend-to-ram right?
You said that yourself, that on regular cpu online/offline you lose
the changed permissions. So just write a resume hook to restore whatever
permissions you want after suspend-resume. See /usr/lib64/pm-utils/sleep.d
for some sample resume scripts.

> Or, told differently,
> if a userspace process handles this scaling_max_freq based on
> thermal values for example, a suspend/resume will impede his
> capacity to throttle the CPU, and the CPU will overheat.
> 
> A good example is an Android phone : imagine the thermal will not
> be able anymore to throttle the CPU, the phone will become hot
> and possibly burn the user (yeah, that's not very probable but
> still ...).
> 
> So you say "there is no reason to surprise the user with
> unexpected events", and I say "this is a bug that permissions of
> a /sys file change because a suspend/resume happened". Now if I'm
> right, what could be done to keep these permissions, other than
> adding a udev event ?
> 

The point I was trying to make is that expecting udev events for
CPU online/offline during S3 is wrong. Nobody is supposed to know that
we are offlining/onlining CPUs under-the-hood.
If you want to do something special on resume, use a resume script
that is automatically invoked for you upon system resume.

Regards,
Srivatsa S. Bhat

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Viresh Kumar]

On 14 May 2013 15:39, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> And losing permissions isn't specific to suspend-to-ram right?
> You said that yourself, that on regular cpu online/offline you lose
> the changed permissions. So just write a resume hook to restore whatever
> permissions you want after suspend-resume. See /usr/lib64/pm-utils/sleep.d
> for some sample resume scripts.

This is what i feel about losing chages on cpu hotplug/unplug:
- On normal online/offline, we aren't required to preserve earlier settings.
- But on suspend/resume, user shouldn't know that cpus have been
  removed/added and so permissions should stay as is.

What do you say?

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Srivatsa S. Bhat]

On 05/14/2013 03:52 PM, Viresh Kumar wrote:
> On 14 May 2013 15:39, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> And losing permissions isn't specific to suspend-to-ram right?
>> You said that yourself, that on regular cpu online/offline you lose
>> the changed permissions. So just write a resume hook to restore whatever
>> permissions you want after suspend-resume. See /usr/lib64/pm-utils/sleep.d
>> for some sample resume scripts.
> 
> This is what i feel about losing chages on cpu hotplug/unplug:
> - On normal online/offline, we aren't required to preserve earlier settings.

>From Robert's descriptions, we _are_ required to preserve the settings. But
this is somewhat acceptable because the user _knows_ he is doing hotplug.

> - But on suspend/resume, user shouldn't know that cpus have been
>   removed/added and so permissions should stay as is.
>

Hmm, you are right. It is wrong for us to expect the user to use custom scripts
to preserve permissions across S3, since he is supposed to remain ignorant of
CPU hotplug in this path.

> What do you say?
> 

I agree, we should try to fix it for the S3/S4 case. Robert, I'm sorry, you were
right - if S3 is supposed to hide CPU hotplug, it should hide it completely or
not at all. And since we go with the former option, a fix for the permissions
is in order, IMHO.

Regards,
Srivatsa S. Bhat

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[R, Durgadoss]

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: Tuesday, May 14, 2013 4:53 PM
> To: Srivatsa S. Bhat
> Cc: Viresh Kumar; Jarzmik, Robert; linux-pm@vger.kernel.org
> Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-
> 9]/cpufreq/scaling_max_freq
> 
> On Tuesday, May 14, 2013 03:57:39 PM Srivatsa S. Bhat wrote:
> > On 05/14/2013 03:52 PM, Viresh Kumar wrote:
> > > On 14 May 2013 15:39, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> > >> And losing permissions isn't specific to suspend-to-ram right?
> > >> You said that yourself, that on regular cpu online/offline you lose
> > >> the changed permissions. So just write a resume hook to restore
> whatever
> > >> permissions you want after suspend-resume. See /usr/lib64/pm-
> utils/sleep.d
> > >> for some sample resume scripts.
> > >
> > > This is what i feel about losing chages on cpu hotplug/unplug:
> > > - On normal online/offline, we aren't required to preserve earlier
> settings.
> >
> > From Robert's descriptions, we _are_ required to preserve the settings.
> But
> > this is somewhat acceptable because the user _knows_ he is doing
> hotplug.
> >
> > > - But on suspend/resume, user shouldn't know that cpus have been
> > >   removed/added and so permissions should stay as is.
> > >
> >
> > Hmm, you are right. It is wrong for us to expect the user to use custom
> scripts
> > to preserve permissions across S3, since he is supposed to remain ignorant
> of
> > CPU hotplug in this path.
> >
> > > What do you say?
> > >
> >
> > I agree, we should try to fix it for the S3/S4 case. Robert, I'm sorry, you
> were
> > right - if S3 is supposed to hide CPU hotplug, it should hide it completely or
> > not at all. And since we go with the former option, a fix for the permissions
> > is in order, IMHO.
> 
> I agree.

Now that we agree on this, how should go about the fix ?
I was debugging this to an extent, and asked a question on [1].

Thanks,
Durga
[1] https://lkml.org/lkml/2013/5/13/190

> 
> Thanks,
> Rafael
> 
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Rafael J. Wysocki]

On Tuesday, May 14, 2013 03:57:39 PM Srivatsa S. Bhat wrote:
> On 05/14/2013 03:52 PM, Viresh Kumar wrote:
> > On 14 May 2013 15:39, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> >> And losing permissions isn't specific to suspend-to-ram right?
> >> You said that yourself, that on regular cpu online/offline you lose
> >> the changed permissions. So just write a resume hook to restore whatever
> >> permissions you want after suspend-resume. See /usr/lib64/pm-utils/sleep.d
> >> for some sample resume scripts.
> > 
> > This is what i feel about losing chages on cpu hotplug/unplug:
> > - On normal online/offline, we aren't required to preserve earlier settings.
> 
> From Robert's descriptions, we _are_ required to preserve the settings. But
> this is somewhat acceptable because the user _knows_ he is doing hotplug.
> 
> > - But on suspend/resume, user shouldn't know that cpus have been
> >   removed/added and so permissions should stay as is.
> >
> 
> Hmm, you are right. It is wrong for us to expect the user to use custom scripts
> to preserve permissions across S3, since he is supposed to remain ignorant of
> CPU hotplug in this path.
> 
> > What do you say?
> > 
> 
> I agree, we should try to fix it for the S3/S4 case. Robert, I'm sorry, you were
> right - if S3 is supposed to hide CPU hotplug, it should hide it completely or
> not at all. And since we go with the former option, a fix for the permissions
> is in order, IMHO.

I agree.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Rafael J. Wysocki]

On Tuesday, May 14, 2013 11:20:30 AM R, Durgadoss wrote:
> > -----Original Message-----
> > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > Sent: Tuesday, May 14, 2013 4:53 PM
> > To: Srivatsa S. Bhat
> > Cc: Viresh Kumar; Jarzmik, Robert; linux-pm@vger.kernel.org
> > Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-
> > 9]/cpufreq/scaling_max_freq
> > 
> > On Tuesday, May 14, 2013 03:57:39 PM Srivatsa S. Bhat wrote:
> > > On 05/14/2013 03:52 PM, Viresh Kumar wrote:
> > > > On 14 May 2013 15:39, Srivatsa S. Bhat
> > <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> > > >> And losing permissions isn't specific to suspend-to-ram right?
> > > >> You said that yourself, that on regular cpu online/offline you lose
> > > >> the changed permissions. So just write a resume hook to restore
> > whatever
> > > >> permissions you want after suspend-resume. See /usr/lib64/pm-
> > utils/sleep.d
> > > >> for some sample resume scripts.
> > > >
> > > > This is what i feel about losing chages on cpu hotplug/unplug:
> > > > - On normal online/offline, we aren't required to preserve earlier
> > settings.
> > >
> > > From Robert's descriptions, we _are_ required to preserve the settings.
> > But
> > > this is somewhat acceptable because the user _knows_ he is doing
> > hotplug.
> > >
> > > > - But on suspend/resume, user shouldn't know that cpus have been
> > > >   removed/added and so permissions should stay as is.
> > > >
> > >
> > > Hmm, you are right. It is wrong for us to expect the user to use custom
> > scripts
> > > to preserve permissions across S3, since he is supposed to remain ignorant
> > of
> > > CPU hotplug in this path.
> > >
> > > > What do you say?
> > > >
> > >
> > > I agree, we should try to fix it for the S3/S4 case. Robert, I'm sorry, you
> > were
> > > right - if S3 is supposed to hide CPU hotplug, it should hide it completely or
> > > not at all. And since we go with the former option, a fix for the permissions
> > > is in order, IMHO.
> > 
> > I agree.
> 
> Now that we agree on this, how should go about the fix ?
> I was debugging this to an extent, and asked a question on [1].

This wasn't the right question to ask.  We need the permissions to be preserved
by the kernel, not to be restored by user space.

Thanks,
Rafael


> Thanks,
> Durga
> [1] https://lkml.org/lkml/2013/5/13/190

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[R, Durgadoss]

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> Sent: Tuesday, May 14, 2013 5:03 PM
> To: R, Durgadoss
> Cc: Srivatsa S. Bhat; Viresh Kumar; Jarzmik, Robert; linux-
> pm@vger.kernel.org
> Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-
> 9]/cpufreq/scaling_max_freq
> 
> On Tuesday, May 14, 2013 11:20:30 AM R, Durgadoss wrote:
> > > -----Original Message-----
> > > From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> > > owner@vger.kernel.org] On Behalf Of Rafael J. Wysocki
> > > Sent: Tuesday, May 14, 2013 4:53 PM
> > > To: Srivatsa S. Bhat
> > > Cc: Viresh Kumar; Jarzmik, Robert; linux-pm@vger.kernel.org
> > > Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-
> > > 9]/cpufreq/scaling_max_freq
> > >
> > > On Tuesday, May 14, 2013 03:57:39 PM Srivatsa S. Bhat wrote:
> > > > On 05/14/2013 03:52 PM, Viresh Kumar wrote:
> > > > > On 14 May 2013 15:39, Srivatsa S. Bhat
> > > <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> > > > >> And losing permissions isn't specific to suspend-to-ram right?
> > > > >> You said that yourself, that on regular cpu online/offline you lose
> > > > >> the changed permissions. So just write a resume hook to restore
> > > whatever
> > > > >> permissions you want after suspend-resume. See /usr/lib64/pm-
> > > utils/sleep.d
> > > > >> for some sample resume scripts.
> > > > >
> > > > > This is what i feel about losing chages on cpu hotplug/unplug:
> > > > > - On normal online/offline, we aren't required to preserve earlier
> > > settings.
> > > >
> > > > From Robert's descriptions, we _are_ required to preserve the settings.
> > > But
> > > > this is somewhat acceptable because the user _knows_ he is doing
> > > hotplug.
> > > >
> > > > > - But on suspend/resume, user shouldn't know that cpus have been
> > > > >   removed/added and so permissions should stay as is.
> > > > >
> > > >
> > > > Hmm, you are right. It is wrong for us to expect the user to use custom
> > > scripts
> > > > to preserve permissions across S3, since he is supposed to remain
> ignorant
> > > of
> > > > CPU hotplug in this path.
> > > >
> > > > > What do you say?
> > > > >
> > > >
> > > > I agree, we should try to fix it for the S3/S4 case. Robert, I'm sorry, you
> > > were
> > > > right - if S3 is supposed to hide CPU hotplug, it should hide it completely
> or
> > > > not at all. And since we go with the former option, a fix for the
> permissions
> > > > is in order, IMHO.
> > >
> > > I agree.
> >
> > Now that we agree on this, how should go about the fix ?
> > I was debugging this to an extent, and asked a question on [1].
> 
> This wasn't the right question to ask.  We need the permissions to be
> preserved
> by the kernel, not to be restored by user space.

Okay, agreed after looking at this discussion ;)
I am fine with the approach of kernel preserving permissions too.

Thanks,
Durga

> 
> Thanks,
> Rafael
> 
> 
> > Thanks,
> > Durga
> > [1] https://lkml.org/lkml/2013/5/13/190
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Jarzmik, Robert]

> Okay, agreed after looking at this discussion ;) I am fine with the approach of kernel preserving permissions too.

Ok, that's great, yet I don't see a clean solution here.  This is
the path I see and that bothers me in the S3 suspend path:
cpufreq_sysfs_release
  kobject_cleanup
    kobject_release
      kobject_put
        __cpufreq_remove_dev
          cpufreq_cpu_callback
            notifier_call_chain
              __raw_notifier_call_chain
                __cpu_notify
                  _cpu_down

Here the sysfs attributes are destroyed. Cpu onlining will
recreate them with root permissions. I don't see a good clean way
to preserve permissions in sysfs across suspend/resume for
deleted nodes. Do you ?

And here we come to my actual worry : what is the technical clean
way to solve this problem ?

So far I have no idea (the cpufreq example is only a subset of
the cases where it could happen). So if someone comes up with a
good idea I'll volunteer to implement it :)

Cheers.

--
Robert

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Srivatsa S. Bhat]

On 05/14/2013 05:24 PM, Jarzmik, Robert wrote:
>> Okay, agreed after looking at this discussion ;) I am fine with the approach of kernel preserving permissions too.
> 
> Ok, that's great, yet I don't see a clean solution here.  This is
> the path I see and that bothers me in the S3 suspend path:
> cpufreq_sysfs_release
>   kobject_cleanup
>     kobject_release
>       kobject_put
>         __cpufreq_remove_dev
>           cpufreq_cpu_callback
>             notifier_call_chain
>               __raw_notifier_call_chain
>                 __cpu_notify
>                   _cpu_down
> 
> Here the sysfs attributes are destroyed. Cpu onlining will
> recreate them with root permissions. I don't see a good clean way
> to preserve permissions in sysfs across suspend/resume for
> deleted nodes. Do you ?
> 
> And here we come to my actual worry : what is the technical clean
> way to solve this problem ?
> 
> So far I have no idea (the cpufreq example is only a subset of
> the cases where it could happen). So if someone comes up with a
> good idea I'll volunteer to implement it :)
> 

Does the below (untested) patch help? I haven't spent time investigating
whether not doing the add_dev/remove_dev stuff during CPU hotplug in S3 path
will break something else. But it would be great if this works, since
its the simplest solution that I can think of.

---------------------------------------------------------------------->

From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH] cpufreq: Preserve sysfs file permissions across suspend/resume

The file permissions of cpufreq per-cpu sysfs files are not preserved across
suspend/resume because we internally go through the CPU Hotplug path which
reinitializes the file permissions on CPU online.

But the user is not supposed to know that we are using CPU hotplug
internally within suspend/resume (IOW, the kernel should not silently wreck
the user-set file permissions across a suspend cycle). Therefore, we need to
preserve the file permissions as it is, across suspend/resume.

The simplest way to achieve that is to just not touch the sysfs files at
all - ie., just ignore the CPU hotplug events in the suspend/resume path
(_FROZEN) in the cpufreq hotplug callback.

Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
Reported-by: Durgadoss R <durgadoss.r@intel.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---

 drivers/cpufreq/cpufreq.c       |    3 ---
 drivers/cpufreq/cpufreq_stats.c |    3 ---
 2 files changed, 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1b8a48e..a7b0910 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1832,15 +1832,12 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
 	if (dev) {
 		switch (action) {
 		case CPU_ONLINE:
-		case CPU_ONLINE_FROZEN:
 			cpufreq_add_dev(dev, NULL);
 			break;
 		case CPU_DOWN_PREPARE:
-		case CPU_DOWN_PREPARE_FROZEN:
 			__cpufreq_remove_dev(dev, NULL);
 			break;
 		case CPU_DOWN_FAILED:
-		case CPU_DOWN_FAILED_FROZEN:
 			cpufreq_add_dev(dev, NULL);
 			break;
 		}
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index bfd6273..fc1ec57 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -349,15 +349,12 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
 
 	switch (action) {
 	case CPU_ONLINE:
-	case CPU_ONLINE_FROZEN:
 		cpufreq_update_policy(cpu);
 		break;
 	case CPU_DOWN_PREPARE:
-	case CPU_DOWN_PREPARE_FROZEN:
 		cpufreq_stats_free_sysfs(cpu);
 		break;
 	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
 		cpufreq_stats_free_table(cpu);
 		break;
 	}

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Rafael J. Wysocki]

On Tuesday, May 14, 2013 11:54:23 AM Jarzmik, Robert wrote:
> > Okay, agreed after looking at this discussion ;) I am fine with the approach of kernel preserving permissions too.
> 
> Ok, that's great, yet I don't see a clean solution here.  This is
> the path I see and that bothers me in the S3 suspend path:
> cpufreq_sysfs_release
>   kobject_cleanup
>     kobject_release
>       kobject_put
>         __cpufreq_remove_dev
>           cpufreq_cpu_callback
>             notifier_call_chain
>               __raw_notifier_call_chain
>                 __cpu_notify
>                   _cpu_down
> 
> Here the sysfs attributes are destroyed. Cpu onlining will
> recreate them with root permissions. I don't see a good clean way
> to preserve permissions in sysfs across suspend/resume for
> deleted nodes. Do you ?

No, you can't preserve permissions for a deleted node.  [I suppose it's
generally true for all filesystems.]  You can only create a new node with the
same permissions as the old one had *or* you can avoid deleting the original
node in the first place.  Whichever is simpler to implement.

> And here we come to my actual worry : what is the technical clean
> way to solve this problem ?

Well, first, a fair question to ask is whether or not this is a real problem.
It is kind of ugly, but who honestly changes the ownership/permissions of
cpufreq directories in non-exotic usage scenarios? Is it needed for anything
actually useful?

> So far I have no idea (the cpufreq example is only a subset of
> the cases where it could happen). So if someone comes up with a
> good idea I'll volunteer to implement it :)

One can argue that we're doing too much during system suspend, because what
we really need is the _cpu_down() down the road.  That said we need to let
cpufreq (and other subsystems too) know that this CPU is gone temporarily and
it is good to have a clean state in case it doesn't come up later.

So I think that the cleanest way to address this issue would be to rearrange
the code so that the sysfs is not modified by suspend/resume at all, unless the
resume fails, but I don't think it's going to be easy to make that happen.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Rafael J. Wysocki]

On Tuesday, May 14, 2013 06:04:48 PM Srivatsa S. Bhat wrote:
> On 05/14/2013 05:24 PM, Jarzmik, Robert wrote:
> >> Okay, agreed after looking at this discussion ;) I am fine with the approach of kernel preserving permissions too.
> > 
> > Ok, that's great, yet I don't see a clean solution here.  This is
> > the path I see and that bothers me in the S3 suspend path:
> > cpufreq_sysfs_release
> >   kobject_cleanup
> >     kobject_release
> >       kobject_put
> >         __cpufreq_remove_dev
> >           cpufreq_cpu_callback
> >             notifier_call_chain
> >               __raw_notifier_call_chain
> >                 __cpu_notify
> >                   _cpu_down
> > 
> > Here the sysfs attributes are destroyed. Cpu onlining will
> > recreate them with root permissions. I don't see a good clean way
> > to preserve permissions in sysfs across suspend/resume for
> > deleted nodes. Do you ?
> > 
> > And here we come to my actual worry : what is the technical clean
> > way to solve this problem ?
> > 
> > So far I have no idea (the cpufreq example is only a subset of
> > the cases where it could happen). So if someone comes up with a
> > good idea I'll volunteer to implement it :)
> > 
> 
> Does the below (untested) patch help? I haven't spent time investigating
> whether not doing the add_dev/remove_dev stuff during CPU hotplug in S3 path
> will break something else. But it would be great if this works, since
> its the simplest solution that I can think of.

Well, what if the CPU doesn't come up during resume?

Rafael


> ---------------------------------------------------------------------->
> 
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH] cpufreq: Preserve sysfs file permissions across suspend/resume
> 
> The file permissions of cpufreq per-cpu sysfs files are not preserved across
> suspend/resume because we internally go through the CPU Hotplug path which
> reinitializes the file permissions on CPU online.
> 
> But the user is not supposed to know that we are using CPU hotplug
> internally within suspend/resume (IOW, the kernel should not silently wreck
> the user-set file permissions across a suspend cycle). Therefore, we need to
> preserve the file permissions as it is, across suspend/resume.
> 
> The simplest way to achieve that is to just not touch the sysfs files at
> all - ie., just ignore the CPU hotplug events in the suspend/resume path
> (_FROZEN) in the cpufreq hotplug callback.
> 
> Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
> Reported-by: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> 
>  drivers/cpufreq/cpufreq.c       |    3 ---
>  drivers/cpufreq/cpufreq_stats.c |    3 ---
>  2 files changed, 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1b8a48e..a7b0910 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1832,15 +1832,12 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
>  	if (dev) {
>  		switch (action) {
>  		case CPU_ONLINE:
> -		case CPU_ONLINE_FROZEN:
>  			cpufreq_add_dev(dev, NULL);
>  			break;
>  		case CPU_DOWN_PREPARE:
> -		case CPU_DOWN_PREPARE_FROZEN:
>  			__cpufreq_remove_dev(dev, NULL);
>  			break;
>  		case CPU_DOWN_FAILED:
> -		case CPU_DOWN_FAILED_FROZEN:
>  			cpufreq_add_dev(dev, NULL);
>  			break;
>  		}
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index bfd6273..fc1ec57 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -349,15 +349,12 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
>  
>  	switch (action) {
>  	case CPU_ONLINE:
> -	case CPU_ONLINE_FROZEN:
>  		cpufreq_update_policy(cpu);
>  		break;
>  	case CPU_DOWN_PREPARE:
> -	case CPU_DOWN_PREPARE_FROZEN:
>  		cpufreq_stats_free_sysfs(cpu);
>  		break;
>  	case CPU_DEAD:
> -	case CPU_DEAD_FROZEN:
>  		cpufreq_stats_free_table(cpu);
>  		break;
>  	}
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Srivatsa S. Bhat]

On 05/14/2013 06:30 PM, Rafael J. Wysocki wrote:
> On Tuesday, May 14, 2013 06:04:48 PM Srivatsa S. Bhat wrote:
>> On 05/14/2013 05:24 PM, Jarzmik, Robert wrote:
>>>> Okay, agreed after looking at this discussion ;) I am fine with the approach of kernel preserving permissions too.
>>>
>>> Ok, that's great, yet I don't see a clean solution here.  This is
>>> the path I see and that bothers me in the S3 suspend path:
>>> cpufreq_sysfs_release
>>>   kobject_cleanup
>>>     kobject_release
>>>       kobject_put
>>>         __cpufreq_remove_dev
>>>           cpufreq_cpu_callback
>>>             notifier_call_chain
>>>               __raw_notifier_call_chain
>>>                 __cpu_notify
>>>                   _cpu_down
>>>
>>> Here the sysfs attributes are destroyed. Cpu onlining will
>>> recreate them with root permissions. I don't see a good clean way
>>> to preserve permissions in sysfs across suspend/resume for
>>> deleted nodes. Do you ?
>>>
>>> And here we come to my actual worry : what is the technical clean
>>> way to solve this problem ?
>>>
>>> So far I have no idea (the cpufreq example is only a subset of
>>> the cases where it could happen). So if someone comes up with a
>>> good idea I'll volunteer to implement it :)
>>>
>>
>> Does the below (untested) patch help? I haven't spent time investigating
>> whether not doing the add_dev/remove_dev stuff during CPU hotplug in S3 path
>> will break something else. But it would be great if this works, since
>> its the simplest solution that I can think of.
> 
> Well, what if the CPU doesn't come up during resume?
> 

Hmm, that's a good point. We will need to remove the sysfs files in that
case. The updated patch below uses CPU_UP_CANCELED_FROZEN notification
to do that.

Regards,
Srivatsa S. Bhat

----------------------------------------------------------------------->

From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Subject: [PATCH v2] cpufreq: Preserve sysfs file permissions across suspend/resume

The file permissions of cpufreq per-cpu sysfs files are not preserved across
suspend/resume because we internally go through the CPU Hotplug path which
reinitializes the file permissions on CPU online.

But the user is not supposed to know that we are using CPU hotplug
internally within suspend/resume (IOW, the kernel should not silently wreck
the user-set file permissions across a suspend cycle). Therefore, we need to
preserve the file permissions as it is, across suspend/resume.

The simplest way to achieve that is to just not touch the sysfs files at
all - ie., just ignore the CPU hotplug events in the suspend/resume path
(_FROZEN) in the cpufreq hotplug callback.

Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
Reported-by: Durgadoss R <durgadoss.r@intel.com>
Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
---
v2: Use UP_CANCELED_FROZEN notification to delete the sysfs files if the
CPUs don't come up during resume.

 drivers/cpufreq/cpufreq.c       |    4 +---
 drivers/cpufreq/cpufreq_stats.c |    7 ++++---
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1b8a48e..284ba63 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1832,15 +1832,13 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
 	if (dev) {
 		switch (action) {
 		case CPU_ONLINE:
-		case CPU_ONLINE_FROZEN:
 			cpufreq_add_dev(dev, NULL);
 			break;
 		case CPU_DOWN_PREPARE:
-		case CPU_DOWN_PREPARE_FROZEN:
+		case CPU_UP_CANCELED_FROZEN:
 			__cpufreq_remove_dev(dev, NULL);
 			break;
 		case CPU_DOWN_FAILED:
-		case CPU_DOWN_FAILED_FROZEN:
 			cpufreq_add_dev(dev, NULL);
 			break;
 		}
diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index bfd6273..fb65dec 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -349,15 +349,16 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
 
 	switch (action) {
 	case CPU_ONLINE:
-	case CPU_ONLINE_FROZEN:
 		cpufreq_update_policy(cpu);
 		break;
 	case CPU_DOWN_PREPARE:
-	case CPU_DOWN_PREPARE_FROZEN:
 		cpufreq_stats_free_sysfs(cpu);
 		break;
 	case CPU_DEAD:
-	case CPU_DEAD_FROZEN:
+		cpufreq_stats_free_table(cpu);
+		break;
+	case CPU_UP_CANCELED_FROZEN:
+		cpufreq_stats_free_sysfs(cpu);
 		cpufreq_stats_free_table(cpu);
 		break;
 	}

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Jarzmik, Robert]

-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@sisk.pl] 
Sent: Tuesday, May 14, 2013 2:59 PM
To: Jarzmik, Robert
Cc: R, Durgadoss; Srivatsa S. Bhat; Viresh Kumar; linux-pm@vger.kernel.org; Greg Kroah-Hartman
Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

>> Here the sysfs attributes are destroyed. Cpu onlining will recreate 
>> them with root permissions. I don't see a good clean way to preserve 
>> permissions in sysfs across suspend/resume for deleted nodes. Do you ?

>No, you can't preserve permissions for a deleted node.  [I suppose it's generally true for all filesystems.]  You can only create a new node with the same permissions as the old one had *or* you can avoid deleting the original node in the first place.  >Whichever is simpler to implement.
I think Srivatsa already replied which one was simpler :) And I
really really like his solution if it is correct (well it works for me,
it's the corner cases that could be nasty).

> Well, first, a fair question to ask is whether or not this is a real problem.
> It is kind of ugly, but who honestly changes the ownership/permissions of cpufreq directories in non-exotic usage scenarios? Is it needed for anything actually useful?
It is (a real problem and really ugly). The example I gave with
thermal conditions implying a userspace process (not root)
storing a new value in scaling_max_freq is something done in
Android's thermal management, when a thermal trip point is
reached. CPU is throttled by inputing a scaling_max_freq value
lower than the CPU's max possible frequency.

If the phone then enters S3 and exists several seconds
later (ie. it doesn't have the time to cool down), the
scaling_max_freq can't be modified anymore by ther userspace
process, and will keep its maximal value. Therefore, the phone
will overheat, assuming the original heating was due to something
silly (like "while (1);" in a userspace process).

So the "who changes" will be "android init process",
the "non-exotic usage scenario" is a bit ... exotic, and
the "useful" part is a question of point of view.

> One can argue that we're doing too much during system suspend, because what we really need is the _cpu_down() down the road.  That said we need to let cpufreq (and other subsystems too) know that this CPU is gone temporarily and it is good to have a clean state in case it doesn't come up later.
Agreed.

> So I think that the cleanest way to address this issue would be to rearrange the code so that the sysfs is not modified by suspend/resume at all, unless the resume fails, but I don't think it's going to be easy to make that happen.
I think I miss the point in there, as a failing resume path
should be catchable in the cpufreq framework. Well,
I should have a closer look in here.

Cheers.

--
Robert

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Alan Stern]

On Tue, 14 May 2013, Jarzmik, Robert wrote:

> > Okay, agreed after looking at this discussion ;) I am fine with the approach of kernel preserving permissions too.
> 
> Ok, that's great, yet I don't see a clean solution here.  This is
> the path I see and that bothers me in the S3 suspend path:
> cpufreq_sysfs_release
>   kobject_cleanup
>     kobject_release
>       kobject_put
>         __cpufreq_remove_dev
>           cpufreq_cpu_callback
>             notifier_call_chain
>               __raw_notifier_call_chain
>                 __cpu_notify
>                   _cpu_down
> 
> Here the sysfs attributes are destroyed. Cpu onlining will
> recreate them with root permissions. I don't see a good clean way
> to preserve permissions in sysfs across suspend/resume for
> deleted nodes. Do you ?
> 
> And here we come to my actual worry : what is the technical clean
> way to solve this problem ?
> 
> So far I have no idea (the cpufreq example is only a subset of
> the cases where it could happen). So if someone comes up with a
> good idea I'll volunteer to implement it :)

The right way to solve this problem is to add a new state.  As I
understand it, right now CPUs can be either present (active) or not
present (inactive).  What you want is a third state in which the CPU is
present but inactive.  Then all the device-core stuff (including sysfs
attributes) can remain unchanged, and eventually the CPU is either
reactivated or removed.

Alan Stern

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Srivatsa S. Bhat]

On 05/14/2013 07:31 PM, Jarzmik, Robert wrote:
> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl] 
> Sent: Tuesday, May 14, 2013 2:59 PM
> To: Jarzmik, Robert
> Cc: R, Durgadoss; Srivatsa S. Bhat; Viresh Kumar; linux-pm@vger.kernel.org; Greg Kroah-Hartman
> Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq
> 
[...]
>> One can argue that we're doing too much during system suspend, because what we really need is the _cpu_down() down the road.  That said we need to let cpufreq (and other subsystems too) know that this CPU is gone temporarily and it is good to have a clean state in case it doesn't come up later.
> Agreed.
> 
>> So I think that the cleanest way to address this issue would be to rearrange the code so that the sysfs is not modified by suspend/resume at all, unless the resume fails, but I don't think it's going to be easy to make that happen.
> I think I miss the point in there, as a failing resume path
> should be catchable in the cpufreq framework. Well,
> I should have a closer look in here.
> 

Please take a look at the v2 of the patch that I sent out
just now. IMHO it addresses Rafael's concern adequately.

Regards,
Srivatsa S. Bhat

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Rafael J. Wysocki]

On Tuesday, May 14, 2013 07:24:41 PM Srivatsa S. Bhat wrote:
> On 05/14/2013 06:30 PM, Rafael J. Wysocki wrote:
> > On Tuesday, May 14, 2013 06:04:48 PM Srivatsa S. Bhat wrote:
> >> On 05/14/2013 05:24 PM, Jarzmik, Robert wrote:
> >>>> Okay, agreed after looking at this discussion ;) I am fine with the approach of kernel preserving permissions too.
> >>>
> >>> Ok, that's great, yet I don't see a clean solution here.  This is
> >>> the path I see and that bothers me in the S3 suspend path:
> >>> cpufreq_sysfs_release
> >>>   kobject_cleanup
> >>>     kobject_release
> >>>       kobject_put
> >>>         __cpufreq_remove_dev
> >>>           cpufreq_cpu_callback
> >>>             notifier_call_chain
> >>>               __raw_notifier_call_chain
> >>>                 __cpu_notify
> >>>                   _cpu_down
> >>>
> >>> Here the sysfs attributes are destroyed. Cpu onlining will
> >>> recreate them with root permissions. I don't see a good clean way
> >>> to preserve permissions in sysfs across suspend/resume for
> >>> deleted nodes. Do you ?
> >>>
> >>> And here we come to my actual worry : what is the technical clean
> >>> way to solve this problem ?
> >>>
> >>> So far I have no idea (the cpufreq example is only a subset of
> >>> the cases where it could happen). So if someone comes up with a
> >>> good idea I'll volunteer to implement it :)
> >>>
> >>
> >> Does the below (untested) patch help? I haven't spent time investigating
> >> whether not doing the add_dev/remove_dev stuff during CPU hotplug in S3 path
> >> will break something else. But it would be great if this works, since
> >> its the simplest solution that I can think of.
> > 
> > Well, what if the CPU doesn't come up during resume?
> > 
> 
> Hmm, that's a good point. We will need to remove the sysfs files in that
> case. The updated patch below uses CPU_UP_CANCELED_FROZEN notification
> to do that.
> 
> Regards,
> Srivatsa S. Bhat
> 
> ----------------------------------------------------------------------->
> 
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH v2] cpufreq: Preserve sysfs file permissions across suspend/resume
> 
> The file permissions of cpufreq per-cpu sysfs files are not preserved across
> suspend/resume because we internally go through the CPU Hotplug path which
> reinitializes the file permissions on CPU online.
> 
> But the user is not supposed to know that we are using CPU hotplug
> internally within suspend/resume (IOW, the kernel should not silently wreck
> the user-set file permissions across a suspend cycle). Therefore, we need to
> preserve the file permissions as it is, across suspend/resume.
> 
> The simplest way to achieve that is to just not touch the sysfs files at
> all - ie., just ignore the CPU hotplug events in the suspend/resume path
> (_FROZEN) in the cpufreq hotplug callback.
> 
> Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
> Reported-by: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

It makes sense to me.

Robert, Durga, does this work for you?

Rafael


> ---
> v2: Use UP_CANCELED_FROZEN notification to delete the sysfs files if the
> CPUs don't come up during resume.
> 
>  drivers/cpufreq/cpufreq.c       |    4 +---
>  drivers/cpufreq/cpufreq_stats.c |    7 ++++---
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1b8a48e..284ba63 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1832,15 +1832,13 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
>  	if (dev) {
>  		switch (action) {
>  		case CPU_ONLINE:
> -		case CPU_ONLINE_FROZEN:
>  			cpufreq_add_dev(dev, NULL);
>  			break;
>  		case CPU_DOWN_PREPARE:
> -		case CPU_DOWN_PREPARE_FROZEN:
> +		case CPU_UP_CANCELED_FROZEN:
>  			__cpufreq_remove_dev(dev, NULL);
>  			break;
>  		case CPU_DOWN_FAILED:
> -		case CPU_DOWN_FAILED_FROZEN:
>  			cpufreq_add_dev(dev, NULL);
>  			break;
>  		}
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index bfd6273..fb65dec 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -349,15 +349,16 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
>  
>  	switch (action) {
>  	case CPU_ONLINE:
> -	case CPU_ONLINE_FROZEN:
>  		cpufreq_update_policy(cpu);
>  		break;
>  	case CPU_DOWN_PREPARE:
> -	case CPU_DOWN_PREPARE_FROZEN:
>  		cpufreq_stats_free_sysfs(cpu);
>  		break;
>  	case CPU_DEAD:
> -	case CPU_DEAD_FROZEN:
> +		cpufreq_stats_free_table(cpu);
> +		break;
> +	case CPU_UP_CANCELED_FROZEN:
> +		cpufreq_stats_free_sysfs(cpu);
>  		cpufreq_stats_free_table(cpu);
>  		break;
>  	}
> 
> 
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Viresh Kumar]

On 14 May 2013 19:24, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH v2] cpufreq: Preserve sysfs file permissions across suspend/resume
>
> The file permissions of cpufreq per-cpu sysfs files are not preserved across
> suspend/resume because we internally go through the CPU Hotplug path which
> reinitializes the file permissions on CPU online.
>
> But the user is not supposed to know that we are using CPU hotplug
> internally within suspend/resume (IOW, the kernel should not silently wreck
> the user-set file permissions across a suspend cycle). Therefore, we need to
> preserve the file permissions as it is, across suspend/resume.
>
> The simplest way to achieve that is to just not touch the sysfs files at
> all - ie., just ignore the CPU hotplug events in the suspend/resume path
> (_FROZEN) in the cpufreq hotplug callback.
>
> Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
> Reported-by: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> v2: Use UP_CANCELED_FROZEN notification to delete the sysfs files if the
> CPUs don't come up during resume.

Is it also possible that more cpus come up after resume? i.e. we had 2 cpus
at suspend and 3 at resume?

>  drivers/cpufreq/cpufreq.c       |    4 +---
>  drivers/cpufreq/cpufreq_stats.c |    7 ++++---
>  2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1b8a48e..284ba63 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1832,15 +1832,13 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
>         if (dev) {
>                 switch (action) {
>                 case CPU_ONLINE:
> -               case CPU_ONLINE_FROZEN:
>                         cpufreq_add_dev(dev, NULL);
>                         break;
>                 case CPU_DOWN_PREPARE:
> -               case CPU_DOWN_PREPARE_FROZEN:
> +               case CPU_UP_CANCELED_FROZEN:
>                         __cpufreq_remove_dev(dev, NULL);
>                         break;
>                 case CPU_DOWN_FAILED:
> -               case CPU_DOWN_FAILED_FROZEN:
>                         cpufreq_add_dev(dev, NULL);
>                         break;

I have forgotten meaning of these flags now :(
So, CPU_ONLINE, CPU_DOWN_PREPARE and CPU_DOWN_FAILED
are called when cpus are added/removed (but not for cpu_down in suspend
resume)? and _FROZEN ones are only used for s2r/s2d?

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Srivatsa S. Bhat]

On 05/15/2013 11:46 AM, Viresh Kumar wrote:
> On 14 May 2013 19:24, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Subject: [PATCH v2] cpufreq: Preserve sysfs file permissions across suspend/resume
>>
>> The file permissions of cpufreq per-cpu sysfs files are not preserved across
>> suspend/resume because we internally go through the CPU Hotplug path which
>> reinitializes the file permissions on CPU online.
>>
>> But the user is not supposed to know that we are using CPU hotplug
>> internally within suspend/resume (IOW, the kernel should not silently wreck
>> the user-set file permissions across a suspend cycle). Therefore, we need to
>> preserve the file permissions as it is, across suspend/resume.
>>
>> The simplest way to achieve that is to just not touch the sysfs files at
>> all - ie., just ignore the CPU hotplug events in the suspend/resume path
>> (_FROZEN) in the cpufreq hotplug callback.
>>
>> Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
>> Reported-by: Durgadoss R <durgadoss.r@intel.com>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>> v2: Use UP_CANCELED_FROZEN notification to delete the sysfs files if the
>> CPUs don't come up during resume.
> 
> Is it also possible that more cpus come up after resume? i.e. we had 2 cpus
> at suspend and 3 at resume?

Nope. That's not possible. While suspending we note down the online non-boot
CPUs we have, in a cpumask, and take them offline. On resume, we try to online
the CPUs in that cpumask. So there is no chance for more CPUs to come up during
resume.

> 
>>  drivers/cpufreq/cpufreq.c       |    4 +---
>>  drivers/cpufreq/cpufreq_stats.c |    7 ++++---
>>  2 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 1b8a48e..284ba63 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1832,15 +1832,13 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
>>         if (dev) {
>>                 switch (action) {
>>                 case CPU_ONLINE: 
>> -               case CPU_ONLINE_FROZEN:
>>                         cpufreq_add_dev(dev, NULL);
>>                         break;
>>                 case CPU_DOWN_PREPARE:
>> -               case CPU_DOWN_PREPARE_FROZEN:
>> +               case CPU_UP_CANCELED_FROZEN:
>>                         __cpufreq_remove_dev(dev, NULL);
>>                         break;
>>                 case CPU_DOWN_FAILED:
>> -               case CPU_DOWN_FAILED_FROZEN:
>>                         cpufreq_add_dev(dev, NULL);
>>                         break;
> 
> I have forgotten meaning of these flags now :(

Take a quick peek at kernel/cpu.c and include/linux/cpu.h and all those
memories will come rushing back ;-)

> So, CPU_ONLINE, CPU_DOWN_PREPARE and CPU_DOWN_FAILED
> are called when cpus are added/removed (but not for cpu_down in suspend
> resume)? and _FROZEN ones are only used for s2r/s2d?
> 

Yes, that's right. For the suspend/resume and hibernate/restore case, all
the notifiers use the _FROZEN flags. The regular hotplug notifications use
the flags without the _FROZEN suffix.

Regards,
Srivatsa S. Bhat

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Viresh Kumar]

On 14 May 2013 19:24, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH v2] cpufreq: Preserve sysfs file permissions across suspend/resume
>
> The file permissions of cpufreq per-cpu sysfs files are not preserved across
> suspend/resume because we internally go through the CPU Hotplug path which
> reinitializes the file permissions on CPU online.
>
> But the user is not supposed to know that we are using CPU hotplug
> internally within suspend/resume (IOW, the kernel should not silently wreck
> the user-set file permissions across a suspend cycle). Therefore, we need to
> preserve the file permissions as it is, across suspend/resume.
>
> The simplest way to achieve that is to just not touch the sysfs files at
> all - ie., just ignore the CPU hotplug events in the suspend/resume path
> (_FROZEN) in the cpufreq hotplug callback.
>
> Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
> Reported-by: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> ---
> v2: Use UP_CANCELED_FROZEN notification to delete the sysfs files if the
> CPUs don't come up during resume.
>
>  drivers/cpufreq/cpufreq.c       |    4 +---
>  drivers/cpufreq/cpufreq_stats.c |    7 ++++---
>  2 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1b8a48e..284ba63 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1832,15 +1832,13 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
>         if (dev) {
>                 switch (action) {
>                 case CPU_ONLINE:
> -               case CPU_ONLINE_FROZEN:
>                         cpufreq_add_dev(dev, NULL);
>                         break;
>                 case CPU_DOWN_PREPARE:
> -               case CPU_DOWN_PREPARE_FROZEN:
> +               case CPU_UP_CANCELED_FROZEN:
>                         __cpufreq_remove_dev(dev, NULL);
>                         break;
>                 case CPU_DOWN_FAILED:
> -               case CPU_DOWN_FAILED_FROZEN:
>                         cpufreq_add_dev(dev, NULL);
>                         break;
>                 }
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index bfd6273..fb65dec 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -349,15 +349,16 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
>
>         switch (action) {
>         case CPU_ONLINE:
> -       case CPU_ONLINE_FROZEN:
>                 cpufreq_update_policy(cpu);
>                 break;
>         case CPU_DOWN_PREPARE:
> -       case CPU_DOWN_PREPARE_FROZEN:
>                 cpufreq_stats_free_sysfs(cpu);
>                 break;
>         case CPU_DEAD:
> -       case CPU_DEAD_FROZEN:
> +               cpufreq_stats_free_table(cpu);
> +               break;
> +       case CPU_UP_CANCELED_FROZEN:
> +               cpufreq_stats_free_sysfs(cpu);
>                 cpufreq_stats_free_table(cpu);
>                 break;
>         }

I accept that the patch makes things work but honestly speaking I
didn't like it much :(

As it looks like more of a hack than a proper solution. We are off-lining
cpus but we are still keeping cpufreq directories (cpu/cpu*/cpufreq)...
That goes against the basic rules...

This works because there is no potential user between suspend/resume
who can notice above issue. But that doesn't mean we write such code.

On the other side, it looks difficult to preserve permissions by writing some
complicated code to preserve and restore original settings for
Suspend/resume.

For now I can't imaging this breaking some stuff in cpufreq core.

I don't want to but I have to (As i don't have a better solution):

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Srivatsa S. Bhat]

On 05/15/2013 12:15 PM, Viresh Kumar wrote:
> On 14 May 2013 19:24, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> Subject: [PATCH v2] cpufreq: Preserve sysfs file permissions across suspend/resume
>>
>> The file permissions of cpufreq per-cpu sysfs files are not preserved across
>> suspend/resume because we internally go through the CPU Hotplug path which
>> reinitializes the file permissions on CPU online.
>>
>> But the user is not supposed to know that we are using CPU hotplug
>> internally within suspend/resume (IOW, the kernel should not silently wreck
>> the user-set file permissions across a suspend cycle). Therefore, we need to
>> preserve the file permissions as it is, across suspend/resume.
>>
>> The simplest way to achieve that is to just not touch the sysfs files at
>> all - ie., just ignore the CPU hotplug events in the suspend/resume path
>> (_FROZEN) in the cpufreq hotplug callback.
>>
>> Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
>> Reported-by: Durgadoss R <durgadoss.r@intel.com>
>> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
>> ---
>> v2: Use UP_CANCELED_FROZEN notification to delete the sysfs files if the
>> CPUs don't come up during resume.
>>
>>  drivers/cpufreq/cpufreq.c       |    4 +---
>>  drivers/cpufreq/cpufreq_stats.c |    7 ++++---
>>  2 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 1b8a48e..284ba63 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1832,15 +1832,13 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
>>         if (dev) {
>>                 switch (action) {
>>                 case CPU_ONLINE:
>> -               case CPU_ONLINE_FROZEN:
>>                         cpufreq_add_dev(dev, NULL);
>>                         break;
>>                 case CPU_DOWN_PREPARE:
>> -               case CPU_DOWN_PREPARE_FROZEN:
>> +               case CPU_UP_CANCELED_FROZEN:
>>                         __cpufreq_remove_dev(dev, NULL);
>>                         break;
>>                 case CPU_DOWN_FAILED:
>> -               case CPU_DOWN_FAILED_FROZEN:
>>                         cpufreq_add_dev(dev, NULL);
>>                         break;
>>                 }
>> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
>> index bfd6273..fb65dec 100644
>> --- a/drivers/cpufreq/cpufreq_stats.c
>> +++ b/drivers/cpufreq/cpufreq_stats.c
>> @@ -349,15 +349,16 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
>>
>>         switch (action) {
>>         case CPU_ONLINE:
>> -       case CPU_ONLINE_FROZEN:
>>                 cpufreq_update_policy(cpu);
>>                 break;
>>         case CPU_DOWN_PREPARE:
>> -       case CPU_DOWN_PREPARE_FROZEN:
>>                 cpufreq_stats_free_sysfs(cpu);
>>                 break;
>>         case CPU_DEAD:
>> -       case CPU_DEAD_FROZEN:
>> +               cpufreq_stats_free_table(cpu);
>> +               break;
>> +       case CPU_UP_CANCELED_FROZEN:
>> +               cpufreq_stats_free_sysfs(cpu);
>>                 cpufreq_stats_free_table(cpu);
>>                 break;
>>         }
> 
> I accept that the patch makes things work but honestly speaking I
> didn't like it much :(
> 
> As it looks like more of a hack than a proper solution. We are off-lining
> cpus but we are still keeping cpufreq directories (cpu/cpu*/cpufreq)...
> That goes against the basic rules...
>

Why do you say that? We want to do *suspend/resume*, not *CPU hotplug*.
So we do a "light" form of hotplug where only the most necessary things
are done. Deleting all the sysfs files and recreating them on resume is
a total waste of time anyway (and adds to the suspend/resume latency).
The _FROZEN flags have been introduced and used for a long time now,
for this very purpose : provide a way to make hotplug as light, and as
smart as possible in the suspend/resume path, because our intention is
to do *suspend/resume* and *not* hotplug per-se. It is only coincidental
that we use hotplug internally in the those paths to get the job done easily.

> This works because there is no potential user between suspend/resume
> who can notice above issue. But that doesn't mean we write such code.
> 

I disagree. There is absolutely nothing wrong here - we have *frozen* the
userspace much before we did any of that! Why do you think we do that?
IOW, it is not purely by chance that nobody is observing what we are doing
in the suspend/resume path - we intentionally *froze* everybody! And until
we thaw them in the resume path, nobody can see anything. And that is
by design. Of course, freezing of tasks is important to restore them back
on resume to the same state, but it serves higher-level purposes too, like
this (ie., not surprising the user with unrelated events). That's part of
the reason why its almost the first thing we do in the suspend path.

> On the other side, it looks difficult to preserve permissions by writing some
> complicated code to preserve and restore original settings for
> Suspend/resume.
> 

Think about it - nobody asked us to delete the sysfs files or wreck their
settings and then worry about restoring them back; we were just asked to suspend
and resume the system. Wrecking the settings is a side-effect of doing hotplug
internally in the suspend/resume path. And that's exactly why we have the
_FROZEN flags, to help us avoid shooting ourselves in the foot like that.

We have used this solution of ignoring stuff to give the effect of
preserving and restoring state, for much more serious stuff before.
Take a look at commit 8f2f748b0656 (CPU hotplug, cpusets, suspend: Don't
touch cpusets during suspend/resume). (But that commit had to be reworked later
to handle a different problem, but that is not relevant here). There have been
some clever optimizations too, that used similar methods : commit 3fb82d56ad0
(x86, suspend: Avoid unnecessary smp alternatives switch during suspend/resume).

So IMHO this is a perfectly sane way to deal with suspend/resume with the
infrastructure we have today. There is nothing wrong in doing this.
Using the _FROZEN flags to speed up suspend/resume (and handle problems like
this) is the closest approximation of the "inactive" state that Alan Stern
described, that we have today in the kernel.

> For now I can't imaging this breaking some stuff in cpufreq core.
> 

What I was concerned was if there were any hidden dependencies between these
functions which deal with sysfs and functions that deal with putting back
the CPUs to the previously set frequencies upon resume (in case there are such
things to be done at resume). Good to know that there are no such dependencies.

> I don't want to but I have to (As i don't have a better solution):
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 

Thank you! I wish you had ACKed it more happily though ;-)

Regards,
Srivatsa S. Bhat

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Viresh Kumar]

On 15 May 2013 13:03, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Thank you! I wish you had ACKed it more happily though ;-)

Hmm.. Your reasoning looks valid and mine wasn't right. So, consider it
a very happy Ack :)

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Srivatsa S. Bhat]

On 05/15/2013 01:14 PM, Viresh Kumar wrote:
> On 15 May 2013 13:03, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> Thank you! I wish you had ACKed it more happily though ;-)
> 
> Hmm.. Your reasoning looks valid and mine wasn't right. So, consider it
> a very happy Ack :)
> 

Awesome! Thank you :-) 

Regards,
Srivatsa S. Bhat

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Jarzmik, Robert]

-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@sisk.pl] 
Sent: Tuesday, May 14, 2013 10:23 PM
To: Srivatsa S. Bhat; Jarzmik, Robert; R, Durgadoss
Cc: Viresh Kumar; linux-pm@vger.kernel.org
Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

> Robert, Durga, does this work for you?
Yes it does, exactly the fix I was looking for.

Tested-by: Robert Jarzmik <robert.jarzmik@intel.com>

--
Robert

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[R, Durgadoss]

> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@sisk.pl]
> Sent: Wednesday, May 15, 2013 1:53 AM
> To: Srivatsa S. Bhat; Jarzmik, Robert; R, Durgadoss
> Cc: Viresh Kumar; linux-pm@vger.kernel.org
> Subject: Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-
> 9]/cpufreq/scaling_max_freq
> 
> On Tuesday, May 14, 2013 07:24:41 PM Srivatsa S. Bhat wrote:
> > On 05/14/2013 06:30 PM, Rafael J. Wysocki wrote:
> > > On Tuesday, May 14, 2013 06:04:48 PM Srivatsa S. Bhat wrote:
> > >> On 05/14/2013 05:24 PM, Jarzmik, Robert wrote:
> > >>>> Okay, agreed after looking at this discussion ;) I am fine with the
> approach of kernel preserving permissions too.
> > >>>
> > >>> Ok, that's great, yet I don't see a clean solution here.  This is
> > >>> the path I see and that bothers me in the S3 suspend path:
> > >>> cpufreq_sysfs_release
> > >>>   kobject_cleanup
> > >>>     kobject_release
> > >>>       kobject_put
> > >>>         __cpufreq_remove_dev
> > >>>           cpufreq_cpu_callback
> > >>>             notifier_call_chain
> > >>>               __raw_notifier_call_chain
> > >>>                 __cpu_notify
> > >>>                   _cpu_down
> > >>>
> > >>> Here the sysfs attributes are destroyed. Cpu onlining will
> > >>> recreate them with root permissions. I don't see a good clean way
> > >>> to preserve permissions in sysfs across suspend/resume for
> > >>> deleted nodes. Do you ?
> > >>>
> > >>> And here we come to my actual worry : what is the technical clean
> > >>> way to solve this problem ?
> > >>>
> > >>> So far I have no idea (the cpufreq example is only a subset of
> > >>> the cases where it could happen). So if someone comes up with a
> > >>> good idea I'll volunteer to implement it :)
> > >>>
> > >>
> > >> Does the below (untested) patch help? I haven't spent time
> investigating
> > >> whether not doing the add_dev/remove_dev stuff during CPU hotplug
> in S3 path
> > >> will break something else. But it would be great if this works, since
> > >> its the simplest solution that I can think of.
> > >
> > > Well, what if the CPU doesn't come up during resume?
> > >
> >
> > Hmm, that's a good point. We will need to remove the sysfs files in that
> > case. The updated patch below uses CPU_UP_CANCELED_FROZEN
> notification
> > to do that.
> >
> > Regards,
> > Srivatsa S. Bhat
> >
> > ----------------------------------------------------------------------->
> >
> > From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> > Subject: [PATCH v2] cpufreq: Preserve sysfs file permissions across
> suspend/resume
> >
> > The file permissions of cpufreq per-cpu sysfs files are not preserved across
> > suspend/resume because we internally go through the CPU Hotplug path
> which
> > reinitializes the file permissions on CPU online.
> >
> > But the user is not supposed to know that we are using CPU hotplug
> > internally within suspend/resume (IOW, the kernel should not silently
> wreck
> > the user-set file permissions across a suspend cycle). Therefore, we need
> to
> > preserve the file permissions as it is, across suspend/resume.
> >
> > The simplest way to achieve that is to just not touch the sysfs files at
> > all - ie., just ignore the CPU hotplug events in the suspend/resume path
> > (_FROZEN) in the cpufreq hotplug callback.
> >
> > Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
> > Reported-by: Durgadoss R <durgadoss.r@intel.com>
> > Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> 
> It makes sense to me.
> 
> Robert, Durga, does this work for you?

Yes, The v2 version of the patch works.
Thank you Srivatsa ;)

Tested-by: Durgadoss R <durgadoss.r@intel.com>

Thanks,
Durga
> 
> Rafael
> 
> 
> > ---
> > v2: Use UP_CANCELED_FROZEN notification to delete the sysfs files if the
> > CPUs don't come up during resume.
> >
> >  drivers/cpufreq/cpufreq.c       |    4 +---
> >  drivers/cpufreq/cpufreq_stats.c |    7 ++++---
> >  2 files changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 1b8a48e..284ba63 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1832,15 +1832,13 @@ static int __cpuinit cpufreq_cpu_callback(struct
> notifier_block *nfb,
> >  	if (dev) {
> >  		switch (action) {
> >  		case CPU_ONLINE:
> > -		case CPU_ONLINE_FROZEN:
> >  			cpufreq_add_dev(dev, NULL);
> >  			break;
> >  		case CPU_DOWN_PREPARE:
> > -		case CPU_DOWN_PREPARE_FROZEN:
> > +		case CPU_UP_CANCELED_FROZEN:
> >  			__cpufreq_remove_dev(dev, NULL);
> >  			break;
> >  		case CPU_DOWN_FAILED:
> > -		case CPU_DOWN_FAILED_FROZEN:
> >  			cpufreq_add_dev(dev, NULL);
> >  			break;
> >  		}
> > diff --git a/drivers/cpufreq/cpufreq_stats.c
> b/drivers/cpufreq/cpufreq_stats.c
> > index bfd6273..fb65dec 100644
> > --- a/drivers/cpufreq/cpufreq_stats.c
> > +++ b/drivers/cpufreq/cpufreq_stats.c
> > @@ -349,15 +349,16 @@ static int __cpuinit
> cpufreq_stat_cpu_callback(struct notifier_block *nfb,
> >
> >  	switch (action) {
> >  	case CPU_ONLINE:
> > -	case CPU_ONLINE_FROZEN:
> >  		cpufreq_update_policy(cpu);
> >  		break;
> >  	case CPU_DOWN_PREPARE:
> > -	case CPU_DOWN_PREPARE_FROZEN:
> >  		cpufreq_stats_free_sysfs(cpu);
> >  		break;
> >  	case CPU_DEAD:
> > -	case CPU_DEAD_FROZEN:
> > +		cpufreq_stats_free_table(cpu);
> > +		break;
> > +	case CPU_UP_CANCELED_FROZEN:
> > +		cpufreq_stats_free_sysfs(cpu);
> >  		cpufreq_stats_free_table(cpu);
> >  		break;
> >  	}
> >
> >
> >
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Jarzmik, Robert]

-----Original Message-----
From: Alan Stern [mailto:stern@rowland.harvard.edu] 

> The right way to solve this problem is to add a new state.  As I understand it, right now CPUs can be either present (active) or not present (inactive).  What you want is a third state in which the CPU is present but inactive.  Then all the device-core stuff (including sysfs
> attributes) can remain unchanged, and eventually the CPU is either reactivated or removed.

Ah, my understanding is much more fuzzy unfortunately.
The way I understand CPU states is that each cpu has the following flags (stolen from include/linux/cpumask.h) :
 - cpu_present
   If I understand that one, it's when the CPU is physically inserted into the system bus (as when a blade with CPUs is inserted)
 - cpu_online
  This one represents if the CPU "has power connected" (or another sentence would be if it can service interrupts and schedule tasks)
 - cpu_active
  I think this one is related to the scheduler, which marks a cpu is "active" upon CPU_STARTING notification

>From these 3 masks tuple (present, online, active) I think we can find out if the CPU is really removed or reactivated.
Of course, if you were speaking about the notifications (ie. include/linu/cpu.h, CPU_*), then I didn't see the distinction between "cpu removal" and "cpu activation". Yet I feel that I'm missing something in your "third state" proposition ... 

Cheers.

--
Robert

---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris, 
92196 Meudon Cedex, France
Registration Number:  302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

RE: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Alan Stern]

On Wed, 15 May 2013, Jarzmik, Robert wrote:

> -----Original Message-----
> From: Alan Stern [mailto:stern@rowland.harvard.edu] 
> 
> > The right way to solve this problem is to add a new state.  As I understand it, right now CPUs can be either present (active) or not present (inactive).  What you want is a third state in which the CPU is present but inactive.  Then all the device-core stuff (including sysfs
> > attributes) can remain unchanged, and eventually the CPU is either reactivated or removed.
> 
> Ah, my understanding is much more fuzzy unfortunately.

You probably understand the situation better than I do, though.

> The way I understand CPU states is that each cpu has the following flags (stolen from include/linux/cpumask.h) :
>  - cpu_present
>    If I understand that one, it's when the CPU is physically inserted into the system bus (as when a blade with CPUs is inserted)
>  - cpu_online
>   This one represents if the CPU "has power connected" (or another sentence would be if it can service interrupts and schedule tasks)
>  - cpu_active
>   I think this one is related to the scheduler, which marks a cpu is "active" upon CPU_STARTING notification
> 
> From these 3 masks tuple (present, online, active) I think we can find out if the CPU is really removed or reactivated.
> Of course, if you were speaking about the notifications (ie. include/linu/cpu.h, CPU_*), then I didn't see the distinction between "cpu removal" and "cpu activation". Yet I feel that I'm missing something in your "third state" proposition ... 

Then a suspended CPU should have the cpu_present flag set and the other
two flags clear, right?  And as long as the cpu_present flag is set,
there's no need to remove device- or sysfs-related structures.

Alan Stern

Re: S3, SMP non boot cpus and /sys/devices/system/cpu[1-9]/cpufreq/scaling_max_freq

[Rafael J. Wysocki]

On Tuesday, May 14, 2013 07:24:41 PM Srivatsa S. Bhat wrote:
> On 05/14/2013 06:30 PM, Rafael J. Wysocki wrote:
> > On Tuesday, May 14, 2013 06:04:48 PM Srivatsa S. Bhat wrote:
> >> On 05/14/2013 05:24 PM, Jarzmik, Robert wrote:
> >>>> Okay, agreed after looking at this discussion ;) I am fine with the approach of kernel preserving permissions too.
> >>>
> >>> Ok, that's great, yet I don't see a clean solution here.  This is
> >>> the path I see and that bothers me in the S3 suspend path:
> >>> cpufreq_sysfs_release
> >>>   kobject_cleanup
> >>>     kobject_release
> >>>       kobject_put
> >>>         __cpufreq_remove_dev
> >>>           cpufreq_cpu_callback
> >>>             notifier_call_chain
> >>>               __raw_notifier_call_chain
> >>>                 __cpu_notify
> >>>                   _cpu_down
> >>>
> >>> Here the sysfs attributes are destroyed. Cpu onlining will
> >>> recreate them with root permissions. I don't see a good clean way
> >>> to preserve permissions in sysfs across suspend/resume for
> >>> deleted nodes. Do you ?
> >>>
> >>> And here we come to my actual worry : what is the technical clean
> >>> way to solve this problem ?
> >>>
> >>> So far I have no idea (the cpufreq example is only a subset of
> >>> the cases where it could happen). So if someone comes up with a
> >>> good idea I'll volunteer to implement it :)
> >>>
> >>
> >> Does the below (untested) patch help? I haven't spent time investigating
> >> whether not doing the add_dev/remove_dev stuff during CPU hotplug in S3 path
> >> will break something else. But it would be great if this works, since
> >> its the simplest solution that I can think of.
> > 
> > Well, what if the CPU doesn't come up during resume?
> > 
> 
> Hmm, that's a good point. We will need to remove the sysfs files in that
> case. The updated patch below uses CPU_UP_CANCELED_FROZEN notification
> to do that.
> 
> Regards,
> Srivatsa S. Bhat
> 
> ----------------------------------------------------------------------->
> 
> From: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Subject: [PATCH v2] cpufreq: Preserve sysfs file permissions across suspend/resume
> 
> The file permissions of cpufreq per-cpu sysfs files are not preserved across
> suspend/resume because we internally go through the CPU Hotplug path which
> reinitializes the file permissions on CPU online.
> 
> But the user is not supposed to know that we are using CPU hotplug
> internally within suspend/resume (IOW, the kernel should not silently wreck
> the user-set file permissions across a suspend cycle). Therefore, we need to
> preserve the file permissions as it is, across suspend/resume.
> 
> The simplest way to achieve that is to just not touch the sysfs files at
> all - ie., just ignore the CPU hotplug events in the suspend/resume path
> (_FROZEN) in the cpufreq hotplug callback.
> 
> Reported-by: Robert Jarzmik <robert.jarzmik@intel.com>
> Reported-by: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Applied to linux-pm.git/linux-next as v3.10-rc2 material.

Thanks,
Rafael


> ---
> v2: Use UP_CANCELED_FROZEN notification to delete the sysfs files if the
> CPUs don't come up during resume.
> 
>  drivers/cpufreq/cpufreq.c       |    4 +---
>  drivers/cpufreq/cpufreq_stats.c |    7 ++++---
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1b8a48e..284ba63 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1832,15 +1832,13 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
>  	if (dev) {
>  		switch (action) {
>  		case CPU_ONLINE:
> -		case CPU_ONLINE_FROZEN:
>  			cpufreq_add_dev(dev, NULL);
>  			break;
>  		case CPU_DOWN_PREPARE:
> -		case CPU_DOWN_PREPARE_FROZEN:
> +		case CPU_UP_CANCELED_FROZEN:
>  			__cpufreq_remove_dev(dev, NULL);
>  			break;
>  		case CPU_DOWN_FAILED:
> -		case CPU_DOWN_FAILED_FROZEN:
>  			cpufreq_add_dev(dev, NULL);
>  			break;
>  		}
> diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
> index bfd6273..fb65dec 100644
> --- a/drivers/cpufreq/cpufreq_stats.c
> +++ b/drivers/cpufreq/cpufreq_stats.c
> @@ -349,15 +349,16 @@ static int __cpuinit cpufreq_stat_cpu_callback(struct notifier_block *nfb,
>  
>  	switch (action) {
>  	case CPU_ONLINE:
> -	case CPU_ONLINE_FROZEN:
>  		cpufreq_update_policy(cpu);
>  		break;
>  	case CPU_DOWN_PREPARE:
> -	case CPU_DOWN_PREPARE_FROZEN:
>  		cpufreq_stats_free_sysfs(cpu);
>  		break;
>  	case CPU_DEAD:
> -	case CPU_DEAD_FROZEN:
> +		cpufreq_stats_free_table(cpu);
> +		break;
> +	case CPU_UP_CANCELED_FROZEN:
> +		cpufreq_stats_free_sysfs(cpu);
>  		cpufreq_stats_free_table(cpu);
>  		break;
>  	}
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.