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

[Lan Tianyu]

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