This has been running in my mind since few days... That we have fixed cpufreq core and all other drivers for transition serialization but what about powernow-k8? It is somewhat special (even more than exynos5440).. It queues a work from ->target() and may or maynot send notifications at all..
Finally I have got a solution now (detailed logs in the patch).. These must go with following patchset:
https://lkml.org/lkml/2013/9/12/173
Compile tested only..
Viresh Kumar (2): cpufreq: Create cpufreq_transition_complete() cpufreq: powernow-k8: mark freq transition complete on error cases
drivers/cpufreq/cpufreq.c | 25 ++++++++++++----------- drivers/cpufreq/powernow-k8.c | 47 +++++++++++++++++++++++++++++++------------ include/linux/cpufreq.h | 7 +++++++ 3 files changed, 54 insertions(+), 25 deletions(-)
Following patch "cpufreq: make sure frequency transitions are serialized" guarantees that we don't have any races while changing cpu frequency or sending notifications. It handled a special case with CPUFREQ_ASYNC_NOTIFICATION flag for drivers that don't complete their freq change from ->target() and exynos5440 driver is well adopted to it as well..
There is one more driver powernow-k8 that has similar implementation, schedules a work for doing transitions. All is good if that work function does notifications on every call to it and so the transition_ongoing count stays stable. But there are chances that the work function may return without actually doing the notifications, in which case transition_ongoing count will not be set to zero and so no transitions would be possible after that.
This patch adds another routine cpufreq_transition_complete() which would be used by powernow-k8 (or even exynos5440 if required), that will be used to mark end of transition in such cases.
Later patch will change powernow-k8 to use this routine.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 25 +++++++++++++------------ include/linux/cpufreq.h | 7 +++++++ 2 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index f8b0889..cf283f3 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -265,6 +265,16 @@ static inline void adjust_jiffies(unsigned long val, struct cpufreq_freqs *ci) } #endif
+void cpufreq_transition_complete(struct cpufreq_policy *policy) +{ + unsigned long flags; + + write_lock_irqsave(&cpufreq_driver_lock, flags); + policy->transition_ongoing--; + write_unlock_irqrestore(&cpufreq_driver_lock, flags); +} +EXPORT_SYMBOL_GPL(cpufreq_transition_complete); + static void __cpufreq_notify_transition(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs, unsigned int state) { @@ -350,16 +360,12 @@ void cpufreq_notify_transition(struct cpufreq_policy *policy,
if ((cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION) && (state == CPUFREQ_POSTCHANGE)) { - unsigned long flags; - /* * Some drivers don't send POSTCHANGE notification from their * ->target() but from some kind of bottom half and so we are * ending transaction here.. */ - write_lock_irqsave(&cpufreq_driver_lock, flags); - policy->transition_ongoing--; - write_unlock_irqrestore(&cpufreq_driver_lock, flags); + cpufreq_transition_complete(policy); } } EXPORT_SYMBOL_GPL(cpufreq_notify_transition); @@ -1430,9 +1436,7 @@ static void cpufreq_out_of_sync(unsigned int cpu, unsigned int old_freq, if (cpufreq_driver->flags & CPUFREQ_ASYNC_NOTIFICATION) return;
- write_lock_irqsave(&cpufreq_driver_lock, flags); - policy->transition_ongoing--; - write_unlock_irqrestore(&cpufreq_driver_lock, flags); + cpufreq_transition_complete(policy); }
/** @@ -1754,10 +1758,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, && (retval == -EINPROGRESS)) return retval;
- write_lock_irqsave(&cpufreq_driver_lock, flags); - policy->transition_ongoing--; - write_unlock_irqrestore(&cpufreq_driver_lock, flags); - + cpufreq_transition_complete(policy); return retval; } EXPORT_SYMBOL_GPL(__cpufreq_driver_target); diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index c770bc0..10ab22d 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -231,6 +231,13 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
const char *cpufreq_get_current_driver(void);
+/* + * Only for drivers which have CPUFREQ_ASYNC_NOTIFICATION flag set and they need + * to mark transaction over before starting any notifications, otherwise the + * POSTCHANGE notification already does this. + */ +void cpufreq_transition_complete(struct cpufreq_policy *policy); + static inline void cpufreq_verify_within_limits(struct cpufreq_policy *policy, unsigned int min, unsigned int max) {
Following patch "cpufreq: make sure frequency transitions are serialized" guarantees that we don't have any races while changing cpu frequency or sending notifications. It handled a special case with CPUFREQ_ASYNC_NOTIFICATION flag for drivers that don't complete their freq change from ->target() and exynos5440 driver is well adopted to it as well..
There is one more driver powernow-k8 that has similar implementation, schedules a work for doing transitions. All is good if that work function does notifications on every call to it and so the transition_ongoing count stays stable. But there are chances that the work function may return without actually doing the notifications, in which case transition_ongoing count will not be set to zero and so no transitions would be possible after that.
This patch fixes powernow-k8 driver to get transition_ongoing count stable. It does following to ensure proper working of this driver: - return -EINPROGRESS from ->target() so that core doesn't mark transfer over at the end of ->target(). - mark cpufreq_driver->flags with CPUFREQ_ASYNC_NOTIFICATION, so that core knows that driver would terminate its transition. - call cpufreq_transition_complete() whenever we are returning before sending notifications
Hopefully things will work well after this patch, only compiled tested at my end.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/powernow-k8.c | 47 +++++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 13 deletions(-)
diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c index 2344a9e..8215bbc 100644 --- a/drivers/cpufreq/powernow-k8.c +++ b/drivers/cpufreq/powernow-k8.c @@ -952,7 +952,7 @@ static int transition_frequency_fidvid(struct powernow_k8_data *data, if ((data->currvid == vid) && (data->currfid == fid)) { pr_debug("target matches current values (fid 0x%x, vid 0x%x)\n", fid, vid); - return 0; + return -EALREADY; }
pr_debug("cpu %d, changing to fid 0x%x, vid 0x%x\n", @@ -993,22 +993,27 @@ static long powernowk8_target_fn(void *arg) unsigned int newstate; int ret;
- if (!data) - return -EINVAL; + if (!data) { + ret = -EINVAL; + goto transition_complete; + }
checkfid = data->currfid; checkvid = data->currvid;
if (pending_bit_stuck()) { printk(KERN_ERR PFX "failing targ, change pending bit set\n"); - return -EIO; + ret = -EIO; + goto transition_complete; }
pr_debug("targ: cpu %d, %d kHz, min %d, max %d, relation %d\n", pol->cpu, targfreq, pol->min, pol->max, relation);
- if (query_current_values_with_pending_wait(data)) - return -EIO; + if (query_current_values_with_pending_wait(data)) { + ret = -EIO; + goto transition_complete; + }
pr_debug("targ: curr fid 0x%x, vid 0x%x\n", data->currfid, data->currvid); @@ -1022,25 +1027,36 @@ static long powernowk8_target_fn(void *arg) }
if (cpufreq_frequency_table_target(pol, data->powernow_table, - targfreq, relation, &newstate)) - return -EIO; + targfreq, relation, &newstate)) { + ret = -EIO; + goto transition_complete; + }
mutex_lock(&fidvid_mutex);
powernow_k8_acpi_pst_values(data, newstate);
ret = transition_frequency_fidvid(data, newstate); + mutex_unlock(&fidvid_mutex);
if (ret) { - printk(KERN_ERR PFX "transition frequency failed\n"); - mutex_unlock(&fidvid_mutex); - return 1; + /* Not a error case, just need to mark transition complete */ + if (ret == -EALREADY) { + ret = 0; + } else { + printk(KERN_ERR PFX "transition frequency failed\n"); + ret = 1; + } + goto transition_complete; } - mutex_unlock(&fidvid_mutex);
pol->cur = find_khz_freq_from_fid(data->currfid);
return 0; + +transition_complete: + cpufreq_transition_complete(pol); + return ret; }
/* Driver entry point to switch to the target frequency */ @@ -1049,8 +1065,12 @@ static int powernowk8_target(struct cpufreq_policy *pol, { struct powernowk8_target_arg pta = { .pol = pol, .targfreq = targfreq, .relation = relation }; + int ret;
- return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta); + ret = work_on_cpu(pol->cpu, powernowk8_target_fn, &pta); + if (!ret) + ret = -EINPROGRESS; /* Mark transition as In-progress */ + return ret; }
/* Driver entry point to verify the policy and range of frequencies */ @@ -1233,6 +1253,7 @@ static struct freq_attr *powernow_k8_attr[] = { };
static struct cpufreq_driver cpufreq_amd64_driver = { + .flags = CPUFREQ_ASYNC_NOTIFICATION, .verify = powernowk8_verify, .target = powernowk8_target, .bios_limit = acpi_processor_get_bios_limit,
On Friday, September 13, 2013 06:29:37 PM Viresh Kumar wrote:
This has been running in my mind since few days... That we have fixed cpufreq core and all other drivers for transition serialization but what about powernow-k8? It is somewhat special (even more than exynos5440).. It queues a work from ->target() and may or maynot send notifications at all..
Finally I have got a solution now (detailed logs in the patch).. These must go with following patchset:
The "must go" here is a totally wrong way of speaking about things, because I'm not sure I'll take this patch series yet.
Thanks, Rafael
On 14 September 2013 05:12, Rafael J. Wysocki rjw@sisk.pl wrote:
On Friday, September 13, 2013 06:29:37 PM Viresh Kumar wrote:
This has been running in my mind since few days... That we have fixed cpufreq core and all other drivers for transition serialization but what about powernow-k8? It is somewhat special (even more than exynos5440).. It queues a work from ->target() and may or maynot send notifications at all..
Finally I have got a solution now (detailed logs in the patch).. These must go with following patchset:
The "must go" here is a totally wrong way of speaking about things, because I'm not sure I'll take this patch series yet.
The "must go" was about keeping all four patches together, rather than forcing you to take them all..
Sorry for the confusion..
linaro-kernel@lists.linaro.org