Re: [PATCH Resend] cpufreq: remove sysfs files for CPU which failed to come back after resume

["Bjørn Mork" <bjorn@mork.no>]

Viresh Kumar <viresh.kumar@linaro.org> writes:
> On 23 December 2013 16:27, Bjørn Mork <bjorn@mork.no> wrote:
>> I could be missing something, but I haven't noticed any attempt to
>> preserve anything except the sysfs files.
>
> What do you mean by sysfs here? Doesn't the below files mentioned
> by you also come in sysfs?

My apologies here.  I see that you *do* try to preserve the policy over
the suspend.  So I guess it should have worked...

>> I tried modifying the max frequency, using
>>
>>  echo 800000 >/sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
>>  echo 800000 >/sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
>>
>> After supend + resume the boot CPU still had the modifed maximum, while
>> the non-boot core was reset to the default value.
>
> This is all we were doing. i.e. not removing or putting the kobject which has
> all these files and so shouldn't get reallocated at all..
>
> So, has resumed passed on the first go only? As it was failing for the first
> time in your case and hence this thread. In that case we are going to get
> new files and so all values will be restored to default values.
>
> Otherwise I don't see why we should loose any values here..

Looking at the code I don't see it either.  But the value is reset.
This is with both your patches:

  cpufreq: remove sysfs files for CPUs which failed to come back after resume
  cpufreq: try to resume policies which failed on last resume

applied on top of v3.13-rc5.  I.e. also including Jason's

  cpufreq: Use CONFIG_CPU_FREQ_DEFAULT_* to set initial policy for setpolicy drivers

since -rc4.  I don't know if that confuses the picture or not.  But
these are the results:

 nemi:/tmp# ls -l /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq
 -rw-rw-r-- 1 root bjorn 4096 Dec 23 12:00 /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
 -rw-rw-r-- 1 root bjorn 4096 Dec 23 11:59 /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
 nemi:/tmp# grep . /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq
 /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq:1401000
 /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq:1401000
 nemi:/tmp# echo 800000 > /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
 nemi:/tmp# echo 800000 > /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
 nemi:/tmp# grep . /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq
 /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq:800000
 /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq:800000
 nemi:/tmp# s2ram 
 KMS graphics driver is in use, skipping quirks.

 ### resume, and then:

 nemi:/tmp# ls -l /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq
 -rw-rw-r-- 1 root bjorn 4096 Dec 23 12:33 /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
 -rw-rw-r-- 1 root bjorn 4096 Dec 23 12:33 /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq
 nemi:/tmp# grep . /sys/devices/system/cpu/cpu*/cpufreq/scaling_max_freq
 /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq:800000
 /sys/devices/system/cpu/cpu1/cpufreq/scaling_max_freq:1401000


The driver and governor is

 nemi:/tmp# grep . /sys/devices/system/cpu/cpu*/cpufreq/scaling_{driver,governor}
 /sys/devices/system/cpu/cpu0/cpufreq/scaling_driver:acpi-cpufreq
 /sys/devices/system/cpu/cpu1/cpufreq/scaling_driver:acpi-cpufreq
 /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor:ondemand
 /sys/devices/system/cpu/cpu1/cpufreq/scaling_governor:ondemand


>>  I changed the gid of
>> both files too, verifying that they were saved and restored as expected.
>> But the value will change to default.
>
> For both boot and non-boot CPUs? I am asking because things should
> be very plain for boot CPU atleast as that is never hot unplugged..
>
> Have you tested this with the latest patches I gave?

See above.  Yes, this is tested with both the two patches in flight and
without any failures on suspend. The non-boot CPU have its settings
reset to default.  The boot CPU keeps the modified values.

>> IMHO it would still be a lot better if this was handled as a true
>> hotplug event, allowing userspace to reset values/modes/owners on
>> resume. Hiding the hotplug event and saving part of the userspace
>> controlled environment is worse than not doing anything at all.
>
> We should be saving everything correctly with the current code,
> with the patches I have sent.. And so things should work as far
> as I can comment.
>
> If you can confirm that these happened despite of latest patches
> then probably I need to test that again on my thinkpad.

That would be great.  This could be just me.  I am quite good at
breaking stuff.


> But I was quite sure this worked :)

Sorry for breaking the illusion :-)



Bjørn