Re: [PATCH] cpufreq: Fix cpufreq regression after suspend/resume

[Srivatsa S. Bhat]

On 07/11/2013 07:53 PM, Alan Stern wrote:
> On Thu, 11 Jul 2013, Srivatsa S. Bhat wrote:
> 
>> Oops! You are right. Hmm, this looks quite difficult to get right :(
>> There are multiple challenges here:
>>
>> 1. The sysfs files must not be removed during cpu_down, and not initialized
>>
>>    during cpu_up. That would help us preserve the file permissions.
>> 2. But we should ensure that we really do the cpufreq-core parts of the cpu
>>    initialization during cpu_up. If we fail to free some of the data-structures
>>    during cpu_down, the cpu_up callback will think that a full-init is not
>>    required and not do its job. That will make cpufreq behave erratically after
>>    suspend/resume and take us back to square one.
>>
>> 3. A full re-init in the cpu_up callback also involves memory allocations.
>>    So if we don't release the memory in the cpu_down callback, we'll end up
>>    in a memory leak.
>>
>> I tried to address all these in this patch, but you found yet another serious
>> loop-hole. I guess I'm out of ideas now... if anybody has any thoughts on how
>> to get this right, then I'm all ears. Else, we'll just revert the original
>> commit like Rafael suggested and leave it upto userspace to save and restore
>> the permissions across suspend/resume if it wants ;-(
> 
> Asking as a naive outsider who is completely unfamiliar with the code,
> why are any of these things at all troublesome?
> 
> 	Can't cpu_up tell the difference between activating a brand-new
> 	CPU and reactivating one that was present before but was
> 	temporarily disabled?
> 
> 	Doesn't cpu_up know which data structures get freed when an
> 	active CPU is temporarily deactivated?
> 
> 	Doesn't cpu_down know what memory gets allocated in cpu_up?
> 	Can't it deallocate just the right parts for the type of
> 	transition it is doing?
> 
> It sounds like you're really asking how to make sure that cpu_up and 
> cpu_down both know what the other is doing, so that each can do the 
> opposite of the other.  That doesn't sound hard.
>

It would not have been hard if the code had been already structured that
way. Currently, the code is quite a bit entangled, and it doesn't distinguish
the "temporarily deactivated" and the "fully torn-down" cases. To add to the
mess, the code is generously sprinkled with notifiers that are invoked every
now and then (which depend on previous init steps etc).

But the bottomline is that this is purely a code-reorganization issue,
nothing more than that. And hopefully we'll be able to separate out the
"temporarily deactivated" and the "fully down" cases reasonably well, such
that we can preserve the sysfs file permissions easily across suspend/resume.
That's the code reorganization I'm working on at the moment; I'll post it
out as soon as I'm done.

Regards,
Srivatsa S. Bhat

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/