| Message ID | 3b93af7e61a573ea2a123c353255645b5ad2a805.1566314535.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
Hi, On 19. 8. 21. 오전 12:24, Leonard Crestez wrote: > Add dev_pm_qos notifies to devfreq core in order to support frequency > limits via the dev_pm_qos_add_request. > > Unlike the rest of devfreq the dev_pm_qos frequency is measured in Khz, > this is consistent with current dev_pm_qos usage for cpufreq and > allows frequencies above 2Ghz (pm_qos expresses limits as s32). > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/devfreq.c | 95 ++++++++++++++++++++++++++++++++++++--- > include/linux/devfreq.h | 5 +++ > 2 files changed, 95 insertions(+), 5 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 784c08e4f931..58deffa52a37 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -22,10 +22,11 @@ > #include <linux/platform_device.h> > #include <linux/list.h> > #include <linux/printk.h> > #include <linux/hrtimer.h> > #include <linux/of.h> > +#include <linux/pm_qos.h> > #include "governor.h" > > #define CREATE_TRACE_POINTS > #include <trace/events/devfreq.h> > > @@ -96,10 +97,30 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) > dev_pm_opp_put(opp); > > return max_freq; > } > > +static unsigned long get_effective_min_freq(struct devfreq *devfreq) I'm not sure that 'effective' expression is correct. From this function, the devfreq can get the final target frequency. I think that we need to use the more correct expression to give the meaning of function as following: get_min_freq get_max_freq or get_biggest_min_freq get_biggest_max_freq or others if there are more proper function name. > +{ > + lockdep_assert_held(&devfreq->lock); > + > + return max3(devfreq->scaling_min_freq, devfreq->min_freq, > + 1000 * (unsigned long)dev_pm_qos_read_value( I prefer to use 'KHZ' defintion instead of 1000. The constant definition is more easy to inform the correct meaning of constant. > + devfreq->dev.parent, > + DEV_PM_QOS_MIN_FREQUENCY)); > +} > + > +static unsigned long get_effective_max_freq(struct devfreq *devfreq) ditto. > +{ > + lockdep_assert_held(&devfreq->lock); > + > + return min3(devfreq->scaling_max_freq, devfreq->max_freq, > + 1000 * (unsigned long)dev_pm_qos_read_value( ditto. > + devfreq->dev.parent, > + DEV_PM_QOS_MAX_FREQUENCY)); > +} > + > /** > * devfreq_get_freq_level() - Lookup freq_table for the frequency > * @devfreq: the devfreq instance > * @freq: the target frequency > */ > @@ -356,12 +377,12 @@ int update_devfreq(struct devfreq *devfreq) > * > * List from the highest priority > * max_freq > * min_freq > */ > - max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq); > - min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq); > + max_freq = get_effective_max_freq(devfreq); > + min_freq = get_effective_min_freq(devfreq); > > if (freq < min_freq) { > freq = min_freq; > flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ > } > @@ -570,10 +591,37 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, > mutex_unlock(&devfreq->lock); > > return ret; > } > > +static int devfreq_qos_notifier_call(struct devfreq *devfreq) > +{ > + int ret; > + > + mutex_lock(&devfreq->lock); > + ret = update_devfreq(devfreq); > + mutex_unlock(&devfreq->lock); > + > + return ret; > +} > + > +static int devfreq_qos_min_notifier_call(struct notifier_block *nb, > + unsigned long val, void *ptr) > +{ > + struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min); > + > + return devfreq_qos_notifier_call(devfreq); > +} > + > +static int devfreq_qos_max_notifier_call(struct notifier_block *nb, > + unsigned long val, void *ptr) > +{ > + struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max); > + > + return devfreq_qos_notifier_call(devfreq); > +} Although the all functions has not the function description in devfreq.c, You need to add the function description for newly added functions. > + > /** > * devfreq_dev_release() - Callback for struct device to release the device. > * @dev: the devfreq device > * > * Remove devfreq from the list and release its resources. > @@ -592,10 +640,14 @@ static void devfreq_dev_release(struct device *dev) > mutex_unlock(&devfreq_list_lock); > > if (devfreq->profile->exit) > devfreq->profile->exit(devfreq->dev.parent); > > + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, > + DEV_PM_QOS_MAX_FREQUENCY); > + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, > + DEV_PM_QOS_MIN_FREQUENCY); > mutex_destroy(&devfreq->lock); > kfree(devfreq); > } > > /** > @@ -636,21 +688,40 @@ struct devfreq *devfreq_add_device(struct device *dev, > err = -ENOMEM; > goto err_out; > } > > mutex_init(&devfreq->lock); > - mutex_lock(&devfreq->lock); Basically, I think that it is safe to lock when touch the variable of the devfreq. it is not proper way for the dev_pm_qos because it breaks the existing locking reason of devfreq's variables. > devfreq->dev.parent = dev; > devfreq->dev.class = devfreq_class; > devfreq->dev.release = devfreq_dev_release; > devfreq->profile = profile; > strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN); > devfreq->previous_freq = profile->initial_freq; > devfreq->last_status.current_frequency = profile->initial_freq; > devfreq->data = data; > devfreq->nb.notifier_call = devfreq_notifier_call; > > + /* > + * notifier from pm_qos > + * > + * initialized outside of devfreq->lock to avoid circular warning > + * between devfreq->lock and dev_pm_qos_mtx > + */ > + devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call; > + devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call; > + > + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, > + DEV_PM_QOS_MIN_FREQUENCY); > + if (err) > + goto err_dev; > + > + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, > + DEV_PM_QOS_MAX_FREQUENCY); > + if (err) > + goto err_dev; > + > + mutex_lock(&devfreq->lock); > if (!devfreq->profile->max_state && !devfreq->profile->freq_table) { > mutex_unlock(&devfreq->lock); > err = set_freq_table(devfreq); > if (err < 0) > goto err_dev; > @@ -741,10 +812,14 @@ struct devfreq *devfreq_add_device(struct device *dev, > mutex_unlock(&devfreq_list_lock); > err_devfreq: > devfreq_remove_device(devfreq); > devfreq = NULL; > err_dev: > + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, > + DEV_PM_QOS_MAX_FREQUENCY); > + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, > + DEV_PM_QOS_MIN_FREQUENCY); > kfree(devfreq); > err_out: > return ERR_PTR(err); > } > EXPORT_SYMBOL(devfreq_add_device); > @@ -1312,12 +1387,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, > > static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > struct devfreq *df = to_devfreq(dev); > + ssize_t ret; > + > + mutex_lock(&df->lock); > + ret = sprintf(buf, "%lu\n", get_effective_min_freq(df)); > + mutex_unlock(&df->lock); > > - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); > + return ret; > } > > static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t count) > { > @@ -1357,12 +1437,17 @@ static DEVICE_ATTR_RW(min_freq); > > static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > struct devfreq *df = to_devfreq(dev); > + ssize_t ret; > > - return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq)); > + mutex_lock(&df->lock); > + ret = sprintf(buf, "%lu\n", get_effective_max_freq(df)); > + mutex_unlock(&df->lock); > + > + return ret; > } > static DEVICE_ATTR_RW(max_freq); > > static ssize_t available_frequencies_show(struct device *d, > struct device_attribute *attr, > diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h > index 2bae9ed3c783..8b92ccbd1962 100644 > --- a/include/linux/devfreq.h > +++ b/include/linux/devfreq.h > @@ -134,10 +134,12 @@ struct devfreq_dev_profile { > * @total_trans: Number of devfreq transitions > * @trans_table: Statistics of devfreq transitions > * @time_in_state: Statistics of devfreq states > * @last_stat_updated: The last time stat updated > * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier > + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY > + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY > * > * This structure stores the devfreq information for a give device. > * > * Note that when a governor accesses entries in struct devfreq in its > * functions except for the context of callbacks defined in struct > @@ -176,10 +178,13 @@ struct devfreq { > unsigned int *trans_table; > unsigned long *time_in_state; > unsigned long last_stat_updated; > > struct srcu_notifier_head transition_notifier_list; > + > + struct notifier_block nb_min; > + struct notifier_block nb_max; > }; > > struct devfreq_freqs { > unsigned long old; > unsigned long new; >
On 21.08.2019 04:40, Chanwoo Choi wrote: > On 19. 8. 21. 오전 12:24, Leonard Crestez wrote: >> Add dev_pm_qos notifies to devfreq core in order to support frequency >> limits via the dev_pm_qos_add_request. >> >> +static unsigned long get_effective_min_freq(struct devfreq *devfreq) > > I'm not sure that 'effective' expression is correct. > From this function, the devfreq can get the final target frequency. > > I think that we need to use the more correct expression > to give the meaning of function as following: > > get_min_freq > get_max_freq OK, will rename to get_min_freq and get_max_freq >> @@ -636,21 +688,40 @@ struct devfreq *devfreq_add_device(struct device *dev, >> err = -ENOMEM; >> goto err_out; >> } >> >> mutex_init(&devfreq->lock); >> - mutex_lock(&devfreq->lock); > > Basically, I think that it is safe to lock when touch > the variable of the devfreq. > > it is not proper way for the dev_pm_qos because > it breaks the existing locking reason of devfreq's variables. I don't understand what you mean. I'm initializing some stuff outside the lock to avoid circular lock warning between: (&devfreq->lock){+.+.}, at: devfreq_qos_min_notifier_call+0x24/0x48 (&(n)->rwsem){++++}, at: blocking_notifier_call_chain+0x3c/0x78 In general you don't need to lock an object while initializing except after it becomes accessible from the outside so devfreq_add_device doesn't need to take the lock so early. The QOS notifiers are registered on the parent device so in theory it's possible for somebody to add QOS requests while devfreq_add_device is executing. Maybe notifier registration should be moved at the end after unlock? -- Regards, Leonard
On 19. 8. 21. 오후 10:00, Leonard Crestez wrote: > On 21.08.2019 04:40, Chanwoo Choi wrote: > >> On 19. 8. 21. 오전 12:24, Leonard Crestez wrote: >>> Add dev_pm_qos notifies to devfreq core in order to support frequency >>> limits via the dev_pm_qos_add_request. >>> >>> +static unsigned long get_effective_min_freq(struct devfreq *devfreq) >> >> I'm not sure that 'effective' expression is correct. >> From this function, the devfreq can get the final target frequency. >> >> I think that we need to use the more correct expression >> to give the meaning of function as following: >> >> get_min_freq >> get_max_freq > > OK, will rename to get_min_freq and get_max_freq > >>> @@ -636,21 +688,40 @@ struct devfreq *devfreq_add_device(struct device *dev, >>> err = -ENOMEM; >>> goto err_out; >>> } >>> >>> mutex_init(&devfreq->lock); >>> - mutex_lock(&devfreq->lock); >> >> Basically, I think that it is safe to lock when touch >> the variable of the devfreq. >> >> it is not proper way for the dev_pm_qos because >> it breaks the existing locking reason of devfreq's variables. > > I don't understand what you mean. I'm initializing some stuff outside > the lock to avoid circular lock warning between: > > (&devfreq->lock){+.+.}, at: devfreq_qos_min_notifier_call+0x24/0x48 > (&(n)->rwsem){++++}, at: blocking_notifier_call_chain+0x3c/0x78 > > In general you don't need to lock an object while initializing except > after it becomes accessible from the outside so devfreq_add_device > doesn't need to take the lock so early. > > The QOS notifiers are registered on the parent device so in theory it's > possible for somebody to add QOS requests while devfreq_add_device is > executing. Maybe notifier registration should be moved at the end after > unlock? I think that it is more clear to add notifier after mutex_unlock(&devfreq->lock) if there are no any issue. > > -- > Regards, > Leonard >
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 784c08e4f931..58deffa52a37 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -22,10 +22,11 @@ #include <linux/platform_device.h> #include <linux/list.h> #include <linux/printk.h> #include <linux/hrtimer.h> #include <linux/of.h> +#include <linux/pm_qos.h> #include "governor.h" #define CREATE_TRACE_POINTS #include <trace/events/devfreq.h> @@ -96,10 +97,30 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) dev_pm_opp_put(opp); return max_freq; } +static unsigned long get_effective_min_freq(struct devfreq *devfreq) +{ + lockdep_assert_held(&devfreq->lock); + + return max3(devfreq->scaling_min_freq, devfreq->min_freq, + 1000 * (unsigned long)dev_pm_qos_read_value( + devfreq->dev.parent, + DEV_PM_QOS_MIN_FREQUENCY)); +} + +static unsigned long get_effective_max_freq(struct devfreq *devfreq) +{ + lockdep_assert_held(&devfreq->lock); + + return min3(devfreq->scaling_max_freq, devfreq->max_freq, + 1000 * (unsigned long)dev_pm_qos_read_value( + devfreq->dev.parent, + DEV_PM_QOS_MAX_FREQUENCY)); +} + /** * devfreq_get_freq_level() - Lookup freq_table for the frequency * @devfreq: the devfreq instance * @freq: the target frequency */ @@ -356,12 +377,12 @@ int update_devfreq(struct devfreq *devfreq) * * List from the highest priority * max_freq * min_freq */ - max_freq = min(devfreq->scaling_max_freq, devfreq->max_freq); - min_freq = max(devfreq->scaling_min_freq, devfreq->min_freq); + max_freq = get_effective_max_freq(devfreq); + min_freq = get_effective_min_freq(devfreq); if (freq < min_freq) { freq = min_freq; flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ } @@ -570,10 +591,37 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type, mutex_unlock(&devfreq->lock); return ret; } +static int devfreq_qos_notifier_call(struct devfreq *devfreq) +{ + int ret; + + mutex_lock(&devfreq->lock); + ret = update_devfreq(devfreq); + mutex_unlock(&devfreq->lock); + + return ret; +} + +static int devfreq_qos_min_notifier_call(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min); + + return devfreq_qos_notifier_call(devfreq); +} + +static int devfreq_qos_max_notifier_call(struct notifier_block *nb, + unsigned long val, void *ptr) +{ + struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max); + + return devfreq_qos_notifier_call(devfreq); +} + /** * devfreq_dev_release() - Callback for struct device to release the device. * @dev: the devfreq device * * Remove devfreq from the list and release its resources. @@ -592,10 +640,14 @@ static void devfreq_dev_release(struct device *dev) mutex_unlock(&devfreq_list_lock); if (devfreq->profile->exit) devfreq->profile->exit(devfreq->dev.parent); + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, + DEV_PM_QOS_MAX_FREQUENCY); + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, + DEV_PM_QOS_MIN_FREQUENCY); mutex_destroy(&devfreq->lock); kfree(devfreq); } /** @@ -636,21 +688,40 @@ struct devfreq *devfreq_add_device(struct device *dev, err = -ENOMEM; goto err_out; } mutex_init(&devfreq->lock); - mutex_lock(&devfreq->lock); devfreq->dev.parent = dev; devfreq->dev.class = devfreq_class; devfreq->dev.release = devfreq_dev_release; devfreq->profile = profile; strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN); devfreq->previous_freq = profile->initial_freq; devfreq->last_status.current_frequency = profile->initial_freq; devfreq->data = data; devfreq->nb.notifier_call = devfreq_notifier_call; + /* + * notifier from pm_qos + * + * initialized outside of devfreq->lock to avoid circular warning + * between devfreq->lock and dev_pm_qos_mtx + */ + devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call; + devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call; + + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min, + DEV_PM_QOS_MIN_FREQUENCY); + if (err) + goto err_dev; + + err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max, + DEV_PM_QOS_MAX_FREQUENCY); + if (err) + goto err_dev; + + mutex_lock(&devfreq->lock); if (!devfreq->profile->max_state && !devfreq->profile->freq_table) { mutex_unlock(&devfreq->lock); err = set_freq_table(devfreq); if (err < 0) goto err_dev; @@ -741,10 +812,14 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_unlock(&devfreq_list_lock); err_devfreq: devfreq_remove_device(devfreq); devfreq = NULL; err_dev: + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max, + DEV_PM_QOS_MAX_FREQUENCY); + dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min, + DEV_PM_QOS_MIN_FREQUENCY); kfree(devfreq); err_out: return ERR_PTR(err); } EXPORT_SYMBOL(devfreq_add_device); @@ -1312,12 +1387,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr, char *buf) { struct devfreq *df = to_devfreq(dev); + ssize_t ret; + + mutex_lock(&df->lock); + ret = sprintf(buf, "%lu\n", get_effective_min_freq(df)); + mutex_unlock(&df->lock); - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); + return ret; } static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { @@ -1357,12 +1437,17 @@ static DEVICE_ATTR_RW(min_freq); static ssize_t max_freq_show(struct device *dev, struct device_attribute *attr, char *buf) { struct devfreq *df = to_devfreq(dev); + ssize_t ret; - return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq)); + mutex_lock(&df->lock); + ret = sprintf(buf, "%lu\n", get_effective_max_freq(df)); + mutex_unlock(&df->lock); + + return ret; } static DEVICE_ATTR_RW(max_freq); static ssize_t available_frequencies_show(struct device *d, struct device_attribute *attr, diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h index 2bae9ed3c783..8b92ccbd1962 100644 --- a/include/linux/devfreq.h +++ b/include/linux/devfreq.h @@ -134,10 +134,12 @@ struct devfreq_dev_profile { * @total_trans: Number of devfreq transitions * @trans_table: Statistics of devfreq transitions * @time_in_state: Statistics of devfreq states * @last_stat_updated: The last time stat updated * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier + * @nb_min: Notifier block for DEV_PM_QOS_MIN_FREQUENCY + * @nb_max: Notifier block for DEV_PM_QOS_MAX_FREQUENCY * * This structure stores the devfreq information for a give device. * * Note that when a governor accesses entries in struct devfreq in its * functions except for the context of callbacks defined in struct @@ -176,10 +178,13 @@ struct devfreq { unsigned int *trans_table; unsigned long *time_in_state; unsigned long last_stat_updated; struct srcu_notifier_head transition_notifier_list; + + struct notifier_block nb_min; + struct notifier_block nb_max; }; struct devfreq_freqs { unsigned long old; unsigned long new;
Add dev_pm_qos notifies to devfreq core in order to support frequency limits via the dev_pm_qos_add_request. Unlike the rest of devfreq the dev_pm_qos frequency is measured in Khz, this is consistent with current dev_pm_qos usage for cpufreq and allows frequencies above 2Ghz (pm_qos expresses limits as s32). Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- drivers/devfreq/devfreq.c | 95 ++++++++++++++++++++++++++++++++++++--- include/linux/devfreq.h | 5 +++ 2 files changed, 95 insertions(+), 5 deletions(-)