| Message ID | 20160906170457.32393-13-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Sep 6, 2016 at 7:04 PM, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > Install the callbacks via the state machine. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Cc: linux-pm@vger.kernel.org > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > drivers/cpufreq/cpufreq.c | 38 ++++++++++++-------------------------- > 1 file changed, 12 insertions(+), 26 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 3dd4884c6f9e..e0bc632a259e 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1358,7 +1358,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) > return add_cpu_dev_symlink(policy, cpu); > } > > -static void cpufreq_offline(unsigned int cpu) > +static int cpufreq_offline(unsigned int cpu) > { > struct cpufreq_policy *policy; > int ret; > @@ -1368,7 +1368,7 @@ static void cpufreq_offline(unsigned int cpu) > policy = cpufreq_cpu_get_raw(cpu); > if (!policy) { > pr_debug("%s: No cpu_data found\n", __func__); > - return; > + return 0; > } > > down_write(&policy->rwsem); > @@ -1417,6 +1417,7 @@ static void cpufreq_offline(unsigned int cpu) > > unlock: > up_write(&policy->rwsem); > + return 0; > } > > /** > @@ -2332,28 +2333,6 @@ int cpufreq_update_policy(unsigned int cpu) > } > EXPORT_SYMBOL(cpufreq_update_policy); > > -static int cpufreq_cpu_callback(struct notifier_block *nfb, > - unsigned long action, void *hcpu) > -{ > - unsigned int cpu = (unsigned long)hcpu; > - > - switch (action & ~CPU_TASKS_FROZEN) { > - case CPU_ONLINE: > - case CPU_DOWN_FAILED: > - cpufreq_online(cpu); > - break; > - > - case CPU_DOWN_PREPARE: > - cpufreq_offline(cpu); > - break; > - } > - return NOTIFY_OK; > -} > - > -static struct notifier_block __refdata cpufreq_cpu_notifier = { > - .notifier_call = cpufreq_cpu_callback, > -}; > - > /********************************************************************* > * BOOST * > *********************************************************************/ > @@ -2455,6 +2434,7 @@ EXPORT_SYMBOL_GPL(cpufreq_boost_enabled); > /********************************************************************* > * REGISTER / UNREGISTER CPUFREQ DRIVER * > *********************************************************************/ > +static enum cpuhp_state hp_online; > > /** > * cpufreq_register_driver - register a CPU Frequency driver > @@ -2517,7 +2497,13 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) > goto err_if_unreg; > } > > - register_hotcpu_notifier(&cpufreq_cpu_notifier); > + ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "cpufreq:online", > + cpufreq_online, > + cpufreq_offline); > + if (ret < 0) > + goto err_if_unreg; > + hp_online = ret; hp_online is enum cpuhp_state (and we pass it to cpuhp_remove_state_nocalls(() later on), but cpuhp_setup_state_nocalls() returns an int (and that should be 0 if it is not an error code AFAICS), so is this actually correct? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-09-06 23:27:46 [+0200], Rafael J. Wysocki wrote: > > + ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "cpufreq:online", > > + cpufreq_online, > > + cpufreq_offline); > > + if (ret < 0) > > + goto err_if_unreg; > > + hp_online = ret; > > hp_online is enum cpuhp_state (and we pass it to > cpuhp_remove_state_nocalls(() later on), but > cpuhp_setup_state_nocalls() returns an int (and that should be 0 if it > is not an error code AFAICS), so is this actually correct? Not sure what you are pointing out here. Let me try to cover it. cpuhp_setup_state_nocalls() return <0 for errors. Those are are not assigned to hp_online. It returns 0 for success on ID was != CPUHP_AP_ONLINE_DYN and >= 0 for success if ID was CPUHP_AP_ONLINE_DYN. In the latter case the dynamic assigned ID is returned which should be used if you plan to remove the callbacks. Assigning an unsigned int to enum is okay because enumeration constants itself should be an int. > Thanks, > Rafael Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, September 07, 2016 04:18:29 PM Sebastian Andrzej Siewior wrote: > On 2016-09-06 23:27:46 [+0200], Rafael J. Wysocki wrote: > > > + ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "cpufreq:online", > > > + cpufreq_online, > > > + cpufreq_offline); > > > + if (ret < 0) > > > + goto err_if_unreg; > > > + hp_online = ret; > > > > hp_online is enum cpuhp_state (and we pass it to > > cpuhp_remove_state_nocalls(() later on), but > > cpuhp_setup_state_nocalls() returns an int (and that should be 0 if it > > is not an error code AFAICS), so is this actually correct? > > Not sure what you are pointing out here. Let me try to cover it. > cpuhp_setup_state_nocalls() return <0 for errors. Those are are not > assigned to hp_online. It returns 0 for success on ID was != > CPUHP_AP_ONLINE_DYN and >= 0 for success if ID was CPUHP_AP_ONLINE_DYN. > In the latter case the dynamic assigned ID is returned which should be > used if you plan to remove the callbacks. OK, that last part wasn't clear to me. The kerneldoc comment for __cpuhp_setup_state() doesn't mention the possible non-zero return values on success, which is a bit confusing IMHO. > Assigning an unsigned int to enum is okay because enumeration constants > itself should be an int. But the unsigned int still may be out of range for the given enum, so I wouldn't call it particularly clean. :-) Anyway, please feel free to add Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> to the patch. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 3dd4884c6f9e..e0bc632a259e 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1358,7 +1358,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) return add_cpu_dev_symlink(policy, cpu); } -static void cpufreq_offline(unsigned int cpu) +static int cpufreq_offline(unsigned int cpu) { struct cpufreq_policy *policy; int ret; @@ -1368,7 +1368,7 @@ static void cpufreq_offline(unsigned int cpu) policy = cpufreq_cpu_get_raw(cpu); if (!policy) { pr_debug("%s: No cpu_data found\n", __func__); - return; + return 0; } down_write(&policy->rwsem); @@ -1417,6 +1417,7 @@ static void cpufreq_offline(unsigned int cpu) unlock: up_write(&policy->rwsem); + return 0; } /** @@ -2332,28 +2333,6 @@ int cpufreq_update_policy(unsigned int cpu) } EXPORT_SYMBOL(cpufreq_update_policy); -static int cpufreq_cpu_callback(struct notifier_block *nfb, - unsigned long action, void *hcpu) -{ - unsigned int cpu = (unsigned long)hcpu; - - switch (action & ~CPU_TASKS_FROZEN) { - case CPU_ONLINE: - case CPU_DOWN_FAILED: - cpufreq_online(cpu); - break; - - case CPU_DOWN_PREPARE: - cpufreq_offline(cpu); - break; - } - return NOTIFY_OK; -} - -static struct notifier_block __refdata cpufreq_cpu_notifier = { - .notifier_call = cpufreq_cpu_callback, -}; - /********************************************************************* * BOOST * *********************************************************************/ @@ -2455,6 +2434,7 @@ EXPORT_SYMBOL_GPL(cpufreq_boost_enabled); /********************************************************************* * REGISTER / UNREGISTER CPUFREQ DRIVER * *********************************************************************/ +static enum cpuhp_state hp_online; /** * cpufreq_register_driver - register a CPU Frequency driver @@ -2517,7 +2497,13 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data) goto err_if_unreg; } - register_hotcpu_notifier(&cpufreq_cpu_notifier); + ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "cpufreq:online", + cpufreq_online, + cpufreq_offline); + if (ret < 0) + goto err_if_unreg; + hp_online = ret; + pr_debug("driver %s up and running\n", driver_data->name); goto out; @@ -2556,7 +2542,7 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) get_online_cpus(); subsys_interface_unregister(&cpufreq_interface); remove_boost_sysfs_file(); - unregister_hotcpu_notifier(&cpufreq_cpu_notifier); + cpuhp_remove_state_nocalls(hp_online); write_lock_irqsave(&cpufreq_driver_lock, flags);
Install the callbacks via the state machine. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Viresh Kumar <viresh.kumar@linaro.org> Cc: linux-pm@vger.kernel.org Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- drivers/cpufreq/cpufreq.c | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-)