| Message ID | 15e599188c5b50edad3efc37787dc2b70d6efead.1569272883.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
Hi, On 19. 9. 24. 오전 6:10, Leonard Crestez wrote: > Moving handling of min/max freq to a single function and call it from > update_devfreq and for printing min/max freq values in sysfs. > > This changes the behavior of out-of-range min_freq/max_freq: clamping > is now done at evaluation time. This means that if an out-of-range > constraint is imposed by sysfs and it later becomes valid then it will > be enforced. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/devfreq/devfreq.c | 111 +++++++++++++++++++++----------------- > 1 file changed, 63 insertions(+), 48 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 1cec816d3d00..7f152a582e78 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -24,10 +24,12 @@ > #include <linux/printk.h> > #include <linux/hrtimer.h> > #include <linux/of.h> > #include "governor.h" > > +#define HZ_PER_KHZ 1000 It is not used on this patch. If it is used on later patch, move it to other patch. > + > #define CREATE_TRACE_POINTS > #include <trace/events/devfreq.h> > > static struct class *devfreq_class; > > @@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) > dev_pm_opp_put(opp); > > return max_freq; > } > > +/** > + * devfreq_get_freq_range() - Get the current freq range > + * @devfreq: the devfreq instance > + * @min_freq: the min frequency > + * @max_freq: the max frequency > + * > + * This takes into consideration all constraints. > + */ > +static void devfreq_get_freq_range(struct devfreq *devfreq, This function is used in the only devfreq core. I think that the internal function doesn't need to add 'devfreq' prefix. Also, when I developed the patches, I didn't use 'devfreq' prefix for internal function. > + unsigned long *min_freq, > + unsigned long *max_freq) > +{ > + unsigned long *freq_table = devfreq->profile->freq_table; > + > + lockdep_assert_held(&devfreq->lock); > + > + /* Init min/max frequency from freq table */ I think that the comments in the devfreq_get_freq_range(), it is not necessary. But if you think that it is necessary, please add more detailed description like as following: /* * The devfreq recommend that freq_table must be generated * in ascending order but, some devfreq driver used the descending order * for freq_table. In order to support all cases, check the order * of freq_table and then initialize the min/max frequency from freq_table. */ > + if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) { > + *min_freq = freq_table[0]; > + *max_freq = freq_table[devfreq->profile->max_state - 1]; > + } else { > + *min_freq = freq_table[devfreq->profile->max_state - 1]; > + *max_freq = freq_table[0]; > + } > + > + /* constraints from sysfs */ ditto. Need to add more detailed comment by keeping the comment style of devfreq. > + *min_freq = max(*min_freq, devfreq->min_freq); > + *max_freq = min(*max_freq, devfreq->max_freq); > + > + /* constraints from OPP interface */ ditto. > + *min_freq = max(*min_freq, devfreq->scaling_min_freq); > + /* scaling_max_freq can be zero on error */ > + if (devfreq->scaling_max_freq) > + *max_freq = min(*max_freq, devfreq->scaling_max_freq); > + > + /* max_freq takes precedence over min_freq */ It is not necessary. We can know that min_freq have to be under max_freq without any comment. > + if (*min_freq > *max_freq) > + *min_freq = *max_freq; > +} > + > /** > * devfreq_get_freq_level() - Lookup freq_table for the frequency > * @devfreq: the devfreq instance > * @freq: the target frequency > */ > @@ -349,21 +391,13 @@ int update_devfreq(struct devfreq *devfreq) > > /* Reevaluate the proper frequency */ > err = devfreq->governor->get_target_freq(devfreq, &freq); > if (err) > return err; > + devfreq_get_freq_range(devfreq, &min_freq, &max_freq); > > - /* > - * Adjust the frequency with user freq, QoS and available freq. > - * > - * 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 takes priority over min freq */ Please remove it. > if (freq < min_freq) { > freq = min_freq; > flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ > } > if (freq > max_freq) { > @@ -1278,40 +1312,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, > ret = sscanf(buf, "%lu", &value); > if (ret != 1) > return -EINVAL; > > mutex_lock(&df->lock); > - > - if (value) { > - if (value > df->max_freq) { > - ret = -EINVAL; > - goto unlock; > - } > - } else { > - unsigned long *freq_table = df->profile->freq_table; > - > - /* Get minimum frequency according to sorting order */ > - if (freq_table[0] < freq_table[df->profile->max_state - 1]) > - value = freq_table[0]; > - else > - value = freq_table[df->profile->max_state - 1]; > - } > -> df->min_freq = value;> update_devfreq(df); > - ret = count; > -unlock: > mutex_unlock(&df->lock); > - return ret; > + > + return count; > } > > static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > struct devfreq *df = to_devfreq(dev); > + unsigned long min_freq, max_freq; > > - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); > + mutex_lock(&df->lock); > + devfreq_get_freq_range(df, &min_freq, &max_freq); > + mutex_unlock(&df->lock); > + > + return sprintf(buf, "%lu\n", min_freq); > } > > static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t count) > { > @@ -1323,40 +1345,33 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > if (ret != 1) > return -EINVAL; > > mutex_lock(&df->lock); > > - if (value) { > - if (value < df->min_freq) { > - ret = -EINVAL; > - goto unlock; > - } > - } else { > - unsigned long *freq_table = df->profile->freq_table; > - > - /* Get maximum frequency according to sorting order */ > - if (freq_table[0] < freq_table[df->profile->max_state - 1]) > - value = freq_table[df->profile->max_state - 1]; > - else > - value = freq_table[0]; > - } > + /* Interpret zero as "don't care" */ Please remove this comment. > + if (!value) > + value = ULONG_MAX; > > df->max_freq = value; > update_devfreq(df); > - ret = count; > -unlock: > mutex_unlock(&df->lock); > - return ret; > + > + return count; > } > 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); > + unsigned long min_freq, max_freq; > + > + mutex_lock(&df->lock); > + devfreq_get_freq_range(df, &min_freq, &max_freq); > + mutex_unlock(&df->lock); > > - return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq)); > + return sprintf(buf, "%lu\n", max_freq); > } > static DEVICE_ATTR_RW(max_freq); > > static ssize_t available_frequencies_show(struct device *d, > struct device_attribute *attr, >
On 2019-09-24 8:09 AM, Chanwoo Choi wrote: > Hi, > > On 19. 9. 24. 오전 6:10, Leonard Crestez wrote: >> Moving handling of min/max freq to a single function and call it from >> update_devfreq and for printing min/max freq values in sysfs. >> >> This changes the behavior of out-of-range min_freq/max_freq: clamping >> is now done at evaluation time. This means that if an out-of-range >> constraint is imposed by sysfs and it later becomes valid then it will >> be enforced. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >> --- >> drivers/devfreq/devfreq.c | 111 +++++++++++++++++++++----------------- >> 1 file changed, 63 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 1cec816d3d00..7f152a582e78 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -24,10 +24,12 @@ >> #include <linux/printk.h> >> #include <linux/hrtimer.h> >> #include <linux/of.h> >> #include "governor.h" >> >> +#define HZ_PER_KHZ 1000 > > It is not used on this patch. If it is used on later patch, > move it to other patch. Good catch. >> #define CREATE_TRACE_POINTS >> #include <trace/events/devfreq.h> >> >> static struct class *devfreq_class; >> >> @@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >> dev_pm_opp_put(opp); >> >> return max_freq; >> } >> >> +/** >> + * devfreq_get_freq_range() - Get the current freq range >> + * @devfreq: the devfreq instance >> + * @min_freq: the min frequency >> + * @max_freq: the max frequency >> + * >> + * This takes into consideration all constraints. >> + */ >> +static void devfreq_get_freq_range(struct devfreq *devfreq, > > This function is used in the only devfreq core. > I think that the internal function doesn't need to add 'devfreq' prefix. > Also, when I developed the patches, I didn't use 'devfreq' prefix > for internal function. This does not appear to a clear convention, there are other internal functions with a devfreq prefix. I usually keep prefixes always but it does result in extra verbosity. >> + unsigned long *min_freq, >> + unsigned long *max_freq) >> +{ >> + unsigned long *freq_table = devfreq->profile->freq_table; >> + >> + lockdep_assert_held(&devfreq->lock); >> + >> + /* Init min/max frequency from freq table */ > > I think that the comments in the devfreq_get_freq_range(), > it is not necessary. But if you think that it is necessary, > please add more detailed description like as following: > > /* > * The devfreq recommend that freq_table must be generated > * in ascending order but, some devfreq driver used the descending order > * for freq_table. In order to support all cases, check the order > * of freq_table and then initialize the min/max frequency from freq_table. > */ OK. Not strictly related to this series: wouldn't it make sense to just fix the drivers? In linux kernel API compatibility hacks are usually avoided. >> + if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) { >> + *min_freq = freq_table[0]; >> + *max_freq = freq_table[devfreq->profile->max_state - 1]; >> + } else { >> + *min_freq = freq_table[devfreq->profile->max_state - 1]; >> + *max_freq = freq_table[0]; >> + } >> + >> + /* constraints from sysfs */ > > ditto. Need to add more detailed comment by keeping the comment style of devfreq. What comment style, capitalization and punctuation? This is similar to the rest of the file. >> + *min_freq = max(*min_freq, devfreq->min_freq); >> + *max_freq = min(*max_freq, devfreq->max_freq); >> + >> + /* constraints from OPP interface */ > > ditto. > >> + *min_freq = max(*min_freq, devfreq->scaling_min_freq); >> + /* scaling_max_freq can be zero on error */ >> + if (devfreq->scaling_max_freq) >> + *max_freq = min(*max_freq, devfreq->scaling_max_freq); >> + >> + /* max_freq takes precedence over min_freq */ > > It is not necessary. We can know that min_freq have to be > under max_freq without any comment. > >> + if (*min_freq > *max_freq) >> + *min_freq = *max_freq; This is a deliberate change introduced by this patch: instead of ensuring min_freq <= max_freq at sysfs write time we make min_freq take precedence at evaluation time. With PM QoS there is no way to ensure that the value coming through DEV_PM_QOS_MIN_FREQ is less than through DEV_PM_QOS_MAX_FREQ. >> +} >> + >> /** >> * devfreq_get_freq_level() - Lookup freq_table for the frequency >> * @devfreq: the devfreq instance >> * @freq: the target frequency >> */ >> @@ -349,21 +391,13 @@ int update_devfreq(struct devfreq *devfreq) >> >> /* Reevaluate the proper frequency */ >> err = devfreq->governor->get_target_freq(devfreq, &freq); >> if (err) >> return err; >> + devfreq_get_freq_range(devfreq, &min_freq, &max_freq); >> >> - /* >> - * Adjust the frequency with user freq, QoS and available freq. >> - * >> - * 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 takes priority over min freq */ > > Please remove it. OK >> if (freq < min_freq) { >> freq = min_freq; >> flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ >> } >> if (freq > max_freq) { >> @@ -1278,40 +1312,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, >> ret = sscanf(buf, "%lu", &value); >> if (ret != 1) >> return -EINVAL; >> >> mutex_lock(&df->lock); >> - >> - if (value) { >> - if (value > df->max_freq) { >> - ret = -EINVAL; >> - goto unlock; >> - } >> - } else { >> - unsigned long *freq_table = df->profile->freq_table; >> - >> - /* Get minimum frequency according to sorting order */ >> - if (freq_table[0] < freq_table[df->profile->max_state - 1]) >> - value = freq_table[0]; >> - else >> - value = freq_table[df->profile->max_state - 1]; >> - } >> -> df->min_freq = value;> update_devfreq(df); >> - ret = count; >> -unlock: >> mutex_unlock(&df->lock); >> - return ret; >> + >> + return count; >> } >> >> static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr, >> char *buf) >> { >> struct devfreq *df = to_devfreq(dev); >> + unsigned long min_freq, max_freq; >> >> - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); >> + mutex_lock(&df->lock); >> + devfreq_get_freq_range(df, &min_freq, &max_freq); >> + mutex_unlock(&df->lock); >> + >> + return sprintf(buf, "%lu\n", min_freq); >> } >> >> static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, >> const char *buf, size_t count) >> { >> @@ -1323,40 +1345,33 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, >> if (ret != 1) >> return -EINVAL; >> >> mutex_lock(&df->lock); >> >> - if (value) { >> - if (value < df->min_freq) { >> - ret = -EINVAL; >> - goto unlock; >> - } >> - } else { >> - unsigned long *freq_table = df->profile->freq_table; >> - >> - /* Get maximum frequency according to sorting order */ >> - if (freq_table[0] < freq_table[df->profile->max_state - 1]) >> - value = freq_table[df->profile->max_state - 1]; >> - else >> - value = freq_table[0]; >> - } >> + /* Interpret zero as "don't care" */ > > Please remove this comment. This is documented in Documentation/ABI/testing/sysfs-class-devfreq but not immediately obvious. Setting max to "zero" could have meant "min possible" instead of "don't care". >> + if (!value) >> + value = ULONG_MAX; >> >> df->max_freq = value; >> update_devfreq(df); >> - ret = count; >> -unlock: >> mutex_unlock(&df->lock); >> - return ret; >> + >> + return count; >> } >> 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); >> + unsigned long min_freq, max_freq; >> + >> + mutex_lock(&df->lock); >> + devfreq_get_freq_range(df, &min_freq, &max_freq); >> + mutex_unlock(&df->lock); >> >> - return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq)); >> + return sprintf(buf, "%lu\n", max_freq); >> } >> static DEVICE_ATTR_RW(max_freq); >> >> static ssize_t available_frequencies_show(struct device *d, >> struct device_attribute *attr,
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 1cec816d3d00..7f152a582e78 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -24,10 +24,12 @@ #include <linux/printk.h> #include <linux/hrtimer.h> #include <linux/of.h> #include "governor.h" +#define HZ_PER_KHZ 1000 + #define CREATE_TRACE_POINTS #include <trace/events/devfreq.h> static struct class *devfreq_class; @@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) dev_pm_opp_put(opp); return max_freq; } +/** + * devfreq_get_freq_range() - Get the current freq range + * @devfreq: the devfreq instance + * @min_freq: the min frequency + * @max_freq: the max frequency + * + * This takes into consideration all constraints. + */ +static void devfreq_get_freq_range(struct devfreq *devfreq, + unsigned long *min_freq, + unsigned long *max_freq) +{ + unsigned long *freq_table = devfreq->profile->freq_table; + + lockdep_assert_held(&devfreq->lock); + + /* Init min/max frequency from freq table */ + if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) { + *min_freq = freq_table[0]; + *max_freq = freq_table[devfreq->profile->max_state - 1]; + } else { + *min_freq = freq_table[devfreq->profile->max_state - 1]; + *max_freq = freq_table[0]; + } + + /* constraints from sysfs */ + *min_freq = max(*min_freq, devfreq->min_freq); + *max_freq = min(*max_freq, devfreq->max_freq); + + /* constraints from OPP interface */ + *min_freq = max(*min_freq, devfreq->scaling_min_freq); + /* scaling_max_freq can be zero on error */ + if (devfreq->scaling_max_freq) + *max_freq = min(*max_freq, devfreq->scaling_max_freq); + + /* max_freq takes precedence over min_freq */ + if (*min_freq > *max_freq) + *min_freq = *max_freq; +} + /** * devfreq_get_freq_level() - Lookup freq_table for the frequency * @devfreq: the devfreq instance * @freq: the target frequency */ @@ -349,21 +391,13 @@ int update_devfreq(struct devfreq *devfreq) /* Reevaluate the proper frequency */ err = devfreq->governor->get_target_freq(devfreq, &freq); if (err) return err; + devfreq_get_freq_range(devfreq, &min_freq, &max_freq); - /* - * Adjust the frequency with user freq, QoS and available freq. - * - * 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 takes priority over min freq */ if (freq < min_freq) { freq = min_freq; flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ } if (freq > max_freq) { @@ -1278,40 +1312,28 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, ret = sscanf(buf, "%lu", &value); if (ret != 1) return -EINVAL; mutex_lock(&df->lock); - - if (value) { - if (value > df->max_freq) { - ret = -EINVAL; - goto unlock; - } - } else { - unsigned long *freq_table = df->profile->freq_table; - - /* Get minimum frequency according to sorting order */ - if (freq_table[0] < freq_table[df->profile->max_state - 1]) - value = freq_table[0]; - else - value = freq_table[df->profile->max_state - 1]; - } - df->min_freq = value; update_devfreq(df); - ret = count; -unlock: mutex_unlock(&df->lock); - return ret; + + return count; } static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr, char *buf) { struct devfreq *df = to_devfreq(dev); + unsigned long min_freq, max_freq; - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); + mutex_lock(&df->lock); + devfreq_get_freq_range(df, &min_freq, &max_freq); + mutex_unlock(&df->lock); + + return sprintf(buf, "%lu\n", min_freq); } static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { @@ -1323,40 +1345,33 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, if (ret != 1) return -EINVAL; mutex_lock(&df->lock); - if (value) { - if (value < df->min_freq) { - ret = -EINVAL; - goto unlock; - } - } else { - unsigned long *freq_table = df->profile->freq_table; - - /* Get maximum frequency according to sorting order */ - if (freq_table[0] < freq_table[df->profile->max_state - 1]) - value = freq_table[df->profile->max_state - 1]; - else - value = freq_table[0]; - } + /* Interpret zero as "don't care" */ + if (!value) + value = ULONG_MAX; df->max_freq = value; update_devfreq(df); - ret = count; -unlock: mutex_unlock(&df->lock); - return ret; + + return count; } 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); + unsigned long min_freq, max_freq; + + mutex_lock(&df->lock); + devfreq_get_freq_range(df, &min_freq, &max_freq); + mutex_unlock(&df->lock); - return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq)); + return sprintf(buf, "%lu\n", max_freq); } static DEVICE_ATTR_RW(max_freq); static ssize_t available_frequencies_show(struct device *d, struct device_attribute *attr,