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

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Monday, December 09, 2013 11:04:53 AM Bjørn Mork wrote:
> "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.

I suspected so, but then I was about to jump on a plane to another continent
in several hours, so I preferred to simply revert both commits and start over
after the dust settled.

> 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?

cpufreq has always had problems with suspend/resume in the first place,
but it just didn't have so much testing coverage before.

> 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.

First of all, I'm not going to send any pull requests this week and even
the next week may be too early to reintroduce that commit.  However, the
second next week will be the -rc6 time frame, so I'm not sure.  It may
end up in 3.14-rc1.

> 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.

Yes, that's pretty much the case, but it looks like we need to do a major
redesign of stuff to really fix those problems.

Thanks,
Rafael