On 09/04/2014 02:46 PM, Viresh Kumar wrote:
> On 4 September 2014 14:40, Preeti U Murthy <pre...@linux.vnet.ibm.com> wrote:
>> cpufreq: Allow stop CPU callback to be used by all cpufreq drivers
>>
>> Commit 367dc4aa introduced the stop CPU callback for intel_pstate
>> drivers. During the CPU_DOWN_PREPARE stage, this callback is invoked
>> so that drivers can take some action on the pstate of the cpu
>> before it is taken offline. This callback was assumed to be useful
>> only for those drivers which have implemented the set_policy CPU
>> callback because they have no other way to take action about the
>> cpufreq of a CPU which is being hotplugged out except in the exit
>> callback which is called very late in the offline process.
>>
>> The drivers which implement the target/target_index callbacks were
>> expected to take care of requirements like the ones that commit
>> 367dc4aa addresses in the GOV_STOP notification event. But there
>> are disadvantages to restricting the usage of stop CPU callback
>> to cpufreq drivers that implement the set_policy callbacks and who
>> want to take explicit action on the setting the cpufreq during a
>> hotplug operation.
>>
>> 1.GOV_STOP gets called for every CPU offline and drivers would usually
>> want to take action when the last cpu in the policy->cpus mask
>> is taken offline. As long as there is more than one cpu in the
>> policy->cpus mask, cpufreq core itself makes sure that the freq
>> for the other cpus in this mask is set according to the maximum load.
>> This is sensible and drivers which implement the target_index callback
>> would mostly not want to modify that. However the cpufreq core leaves a
>> loose end when the cpu in the policy->cpus mask is the last one to go
>> offline;
>> it does nothing explicit to the frequency of the core. Drivers may need
>> a way to take some action here and stop CPU callback mechanism is the
>> best way to do it today.
>>
>> 2.We cannot implement driver specific actions in the GOV_STOP mechanism.
>> So we will need another driver callback which is invoked from here which is
>> unnecessary.
>>
>> Therefore this patch extends the usage of stop CPU callback to be used
>> by all cpufreq drivers as long as they have this callback implemented
>> and irrespective of whether they are set_policy/target_index drivers.
>> The assumption is if the drivers find the GOV_STOP path to be a suitable
>> way of implementing what they want to do with the freq of the cpu
>> going offine,they will not implement the stop CPU callback at all.
>>
>> Signed-off-by: Preeti U Murthy <pre...@linux.vnet.ibm.com>
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index d9fdedd..6463f35 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1380,7 +1380,7 @@ static int __cpufreq_remove_dev_prepare(struct device
>> *dev,
>> if (!cpufreq_suspended)
>> pr_debug("%s: policy Kobject moved to cpu: %d from:
>> %d\n",
>> __func__, new_cpu, cpu);
>> - } else if (cpufreq_driver->stop_cpu && cpufreq_driver->setpolicy) {
>> + } else if (cpufreq_driver->stop_cpu) {
>> cpufreq_driver->stop_cpu(policy);
>> }
>
> Rafael explicitly said earlier that he want to see a separate callback for
> ->target() drivers, don't know why..
I think Rafael's point was that since no driver that had implemented the
target_index callback was using it at the time that this patch was
proposed, it was be best to couple the check on existence of stop_cpu
callback with the the check on the kind of cpufreq driver. However
powerpc is also in need of this today and we implement the target_index
callback and find it convenient to use the stop CPU callback.
Rafael, in which case would it not make sense to remove the check on
driver->setpolicy above?
Besides, I don't understand very well why we had this double check in
the first place. Only if the drivers are in need of the functionality
like stop_cpu, would they have implemented this callback right? If we
are to assume that the drivers which have implemented the target_index
callback should never be needing it, they would not have implemented the
stop CPU callback either. So what was that, which was blatantly wrong
with just having a check on stop_cpu? I did go through the discussion
but did not find a convincing answer to this.
Rafael?
Regards
Preeti U Murthy
>
> It looks fine to me though.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
