Re: [PATCH] cpufreq: fix garbage kobj on errors during suspend/resume

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

"Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com> writes:
> On 12/09/2013 08:29 AM, Lan Tianyu wrote:
>> 2013/12/5 Rafael J. Wysocki <rjw@rjwysocki.net>:
>>> On Wednesday, December 04, 2013 04:02:18 PM viresh kumar wrote:
>>>> On Tuesday 03 December 2013 04:44 PM, Bjørn Mork wrote:
>>>>> This is effectively a revert of commit 5302c3fb2e62 ("cpufreq: Perform
>>>>> light-weight init/teardown during suspend/resume"), which enabled
>>>>> suspend/resume optimizations leaving the sysfs files in place.
> [...]
>>> I took the Bjorn's patch for 3.13 and this one I can queued up for 3.14,
>>> but for that I guess it should contain a revert of the change made by the
>>> Bjorn's patch.
>> 
>> This patch causes a s3 regression. Cc:Martin Ziegler
>> https://bugzilla.kernel.org/show_bug.cgi?id=66751
>> 
>
> Hmm.. With Bjorn's patch applied, the cpufreq hotplug callback should become
> identical to what happens during regular CPU hotplug.

Yes, I also wondered how that could have happened.

Apparently this is due to bad interaction between two patches. Commit 

  5a87182aa21d ("cpufreq: suspend governors on system suspend/hibernate")

added an implicit dependency on the suspend/resume code which commit

  2167e2399dc5 ("cpufreq: fix garbage kobjects on errors during suspend/resume")

disabled.

This would make the last patch applied of these two come out of the
bisect, which is 2167e2399dc5 in this case.  I can confirm that
reverting only this patch also fixes my hibernate problem.

BUT: It reintroduces the problem it was supposed to fix.  AND: As you
note, it really does nothing but revert to the assumed safe regular CPU
hotplug operations.  Which means that the other patch somehow has made
regular CPU hotplugging fail *if suspending*.  It won't make it fail
unless suspending, so there is no need to test CPU hotplugging
separately. 

In any case, my claim is that the real bug here still is in commit
5a87182aa21d, which added an undocumented implicit dependency on the
special cpufreq suspend/resume code.  There is no way in hell that
anyone could have guessed that the seemingly innocent changes in commit
2167e2399dc5 would fail because of this. Which should be more than
enough to understand why the continues sprinkling of suspend/resume code
all over has to stop.  Where did all the nice and clean pm hooks design
disappear?

My opinion is that commit 2167e2399dc5 still is the correct short term
fix, and it should be reapplied to v3.13-rcX and resubmitted for
3.12-stable.

I anticipate the real cleanup of this mess.  But I don't think any
additional "if suspending" tests has any place in it.  Test *once* and
fork to whatever you want to do differently when suspending .
Sprinkling these tests all over, having separate code blocks implicitly
depending on each other, is nothing but a recipe for hard to track bugs.

Just my € .015 (yes, I'm cheap)


Bjørn