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

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 12/05/2013 12:27 PM, Bjørn Mork wrote:
> Viresh Kumar <viresh.kumar@linaro.org> writes:
> 
>> Sending from phone.. html.. so left list.
>>
>> Here is the old thread where we discussed this.. see if this helps..
>>
>> http://marc.info/?t=136845016900003&r=1&w=2
> 
> Thanks.  That helped a lot.
> 
> Unless I miss something, it looks like the permission problem *started*
> with fallout from special suspend code - surprising the user by not
> creating any offline/online event on suspend/resume.  Quoting from
> http://marc.info/?l=linux-pm&m=136847781510358 :
> 
>   (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.)
> 

I hope you didn't miss the main idea I was trying to convey in that
reply: 
"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."

...

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


> So instead of going in the direction of even more special treatment to
> hide the fact that a offline/online is done, you could also have solved
> the problem by removed the existing special treatment.  That would
> likely have simplified the code and made it do what userspace expects.
> 

You seem to be getting confused as to what the *actual* userspace
expectation was, in that mail thread. The expectation was that suspend/
resume is a kernel operation that brings back the system to the same
state (as much as possible) at the end of resume, as it was before
suspend. And that is a perfectly valid expectation, and it is something
that the kernel has to try its level best to honor.

And in this particular case, the specific expectation was that the sysfs
file permissions set by the user for cpufreq files will remain as it is
even after a suspend/resume cycle. That's it. There is _absolutely_ _no_
talk about CPU hotplug here.

Robert _happened_ to dig this further and observe that suspend/resume
actually does offline/online of CPUs, and thought that he should have
also seen the associated udev events as well. But we have purposefully
not exposed the fact that suspend/resume involves CPU hotplug. Today,
suspend/resume uses CPU hotplug internally because we don't have any
other better alternative. The very concept/semantic of suspend/resume
_does_ _not_ imply CPU hotplug - it is just an implementation detail
that userspace should not need to care about or rely on.

Moreover, cpufreq is not the only subsystem that participates in suspend/
resume and CPU hotplug. And fundamentally, regular CPU hotplug has _very_
different semantics and guarantees than the hotplug done in suspend/resume.
For example, if you offline CPUs normally, the cpusets associated with
those CPUs will get destroyed, potentially in ways that _won't_ bring
them back to the same state when you online those exact same CPUs!
And this would have been totally unacceptable to a user innocuously
using suspend/resume. Look at commit d35be8bab9 (CPU hotplug, cpusets,
suspend: Don't modify cpusets during suspend/resume), for more details
on how we fixed that problem.


So, to summarize, suspend/resume should mean one simple thing to
userspace - "the kernel will transparently (to the extent possible)
perform suspend/resume and bring back the system during resume, to a
state as close as possible compared to how it was before suspend".
Any implementation challenges must be handled in the kernel (as far as
possible), and we should avoid burdening userspace with extraneous
events etc.

> You have chosen to do offline+online.  Userspace expects to see the
> offline+online.  Don't try to hide the implementation.  That's only
> confusing.
> 

I hope my above explanation answers your questions. We are not trying
to hide the implementation for no reason. We want to do it to preserve
sane semantics for suspend/resume. And that effectively translates to
"provide userspace with a transparent suspend/resume operation".

 
Regards,
Srivatsa S. Bhat