2013/7/11 Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>:
> On 07/11/2013 11:10 AM, Lan Tianyu wrote:
>> 2013/7/11 Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>:
> Oops! You are right. Hmm, this looks quite difficult to get right :(
> There are multiple challenges here:
If I understand correctly, the most concern is how to deal with per cpus'
cpufreq_policy structure. If something wrong , please correct me.
>
> 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.
For this case, cpufreq_policy must be reserved since all related cpufreq data
and kobj is also store into it. We can't release it.
> 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.
>
cpufreq_add_dev() checks whether the cpu has been attached cpufreq_cpu_data
and associated kobj has been created. If yes, it would try to release it by
cpufreq_cpu_put() and return directly. This seems to be conflicted
with the above.
> 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.
>
Even we remove the previous check, cpu scaling driver's init() also will change
some data of struct cpufreq_policy and this is also what we don't like to see.
> 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 ;-(
>
How about implement scaling driver's suspend/resume callback()? Although this
needs to be dealt with case by case. If one's callbacks hasn't been implemented,
it would have to follow current rule.
>>> + unlock_policy_rwsem_read(cpu);
>>> + kobject_put(kobj);
>>> +
>>> + pr_debug("waiting for dropping of refcount\n");
>>> + wait_for_completion(cmp);
>>> + pr_debug("wait complete\n");
>>> + }
>>> +
>>> } else if (cpufreq_driver->target) {
>>> __cpufreq_governor(data, CPUFREQ_GOV_START);
>>> __cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
>>> @@ -1221,7 +1230,7 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
>>> if (cpu_is_offline(cpu))
>>> return 0;
>>>
>>> - retval = __cpufreq_remove_dev(dev, sif);
>>> + retval = __cpufreq_remove_dev(dev, sif, true);
>>> return retval;
>>> }
>
> Regards,
> Srivatsa S. Bhat
>
--
Best regards
Tianyu Lan
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
