| Message ID | 674fe91065034083fd7c8c1810305cd01551bb80.1569319738.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Add dev_pm_qos support | expand |
On 19. 9. 24. 오후 7:11, 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 | 112 ++++++++++++++++++++++---------------- > 1 file changed, 64 insertions(+), 48 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 4a878baa809e..eee403e70c84 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -96,10 +96,54 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) > dev_pm_opp_put(opp); > > return max_freq; > } > > +/** > + * 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 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. Init -> Initialize min/max -> minimum/maximum > + * Drivers can initialize this in either ascending or descending order Drivers -> devfreq drivers > + * and devfreq core supports both. > + */ In result, I prefer to change the comments as following: /* * Initialize the minimum/maximum frequency from freq_table. * The devfreq drivers can initialize freq_table in either * ascending or descending order and devfreq core supports both. */ > + 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 */ 'constraints' -> Constraint because first verb have to be used as the sigular verbs. Also, I think that have to enhance the comments I prefer to use following comments: /* Constraint minimum/maximum frequency from user input via sysfs */ > + *min_freq = max(*min_freq, devfreq->min_freq); > + *max_freq = min(*max_freq, devfreq->max_freq); > + > + /* constraints from OPP interface */ ditto. I prefer to use following comments: /* Constraint minimum/maximum frequency from OPP interface */ > + *min_freq = max(*min_freq, devfreq->scaling_min_freq); > + /* scaling_max_freq can be zero on error */ Please remove it. > + if (devfreq->scaling_max_freq) As I knew, devfreq->scaling_max_freq is never zero. So, it is always true. This if statement is needed. > + *max_freq = min(*max_freq, devfreq->scaling_max_freq); > + > + /* max_freq takes precedence over min_freq */ As I said, almost people know that min_freq have be under than max_freq. Please remove it. And until finishing the discussion on mailing list, please don't send the next version. If you just replied from my comment and then wait my next comment, we can save the time without replying the repetitive and same comment for same point. > + 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,20 +393,11 @@ int update_devfreq(struct devfreq *devfreq) > > /* Reevaluate the proper frequency */ > err = devfreq->governor->get_target_freq(devfreq, &freq); > if (err) > return err; > - > - /* > - * 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); > + get_freq_range(devfreq, &min_freq, &max_freq); > > if (freq < min_freq) { > freq = min_freq; > flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ > } > @@ -1298,40 +1333,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; > + > + mutex_lock(&df->lock); > + get_freq_range(df, &min_freq, &max_freq); > + mutex_unlock(&df->lock); > > - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); > + 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) > { > @@ -1343,40 +1366,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 it. Also, the detailed comment for user have to add the documentation file. > + 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); > + 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 25.09.2019 04:32, Chanwoo Choi wrote: > On 19. 9. 24. 오후 7:11, 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 | 112 ++++++++++++++++++++++---------------- >> 1 file changed, 64 insertions(+), 48 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 4a878baa809e..eee403e70c84 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -96,10 +96,54 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >> dev_pm_opp_put(opp); >> >> return max_freq; >> } >> >> +/** >> + * 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 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. > > Init -> Initialize > min/max -> minimum/maximum > >> + * Drivers can initialize this in either ascending or descending order > > Drivers -> devfreq drivers > >> + * and devfreq core supports both. >> + */ > > > In result, I prefer to change the comments as following: > /* > * Initialize the minimum/maximum frequency from freq_table. > * The devfreq drivers can initialize freq_table in either > * ascending or descending order and devfreq core supports both. > */ OK >> + 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 */ > > 'constraints' -> Constraint because first verb have to be used > as the sigular verbs. Also, I think that have to enhance the comments > I prefer to use following comments: > > /* Constraint minimum/maximum frequency from user input via sysfs */ > > > >> + *min_freq = max(*min_freq, devfreq->min_freq); >> + *max_freq = min(*max_freq, devfreq->max_freq); >> + >> + /* constraints from OPP interface */ > > ditto. I prefer to use following comments: > > /* Constraint minimum/maximum frequency from OPP interface */ > > >> + *min_freq = max(*min_freq, devfreq->scaling_min_freq); >> + /* scaling_max_freq can be zero on error */ > > Please remove it. > >> + if (devfreq->scaling_max_freq) > > As I knew, devfreq->scaling_max_freq is never zero. > So, it is always true. This if statement is needed. It can happen if find_available_max_freq encounters an error when called from devfreq_notifier_call. Maybe that should be separately fixed to set scaling_max_freq to a neutral value such as "ULONG_MAX" instead? BTW: the devfreq_notifier_call function returns -EINVAL on error but it should return one of the NOTIFY_OK/DONE/STOP values instead. The OPP framework ignores notifier results but (-EINVAL & NOTIFY_STOP) evaluates as true so other notifiers will be skipped unintentionally. >> + *max_freq = min(*max_freq, devfreq->scaling_max_freq); >> + >> + /* max_freq takes precedence over min_freq */ > > As I said, almost people know that min_freq have be under than max_freq. > Please remove it. And until finishing the discussion on mailing list, > please don't send the next version. If you just replied from my comment > and then wait my next comment, we can save the time without replying > the repetitive and same comment for same point. This series makes it possible to set a min_freq higher than max_freq (for example via PM QoS from various devices). It is not obvious that min_freq takes precedence over max_freq but the code is self-evident so I will remove the 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,20 +393,11 @@ int update_devfreq(struct devfreq *devfreq) >> >> /* Reevaluate the proper frequency */ >> err = devfreq->governor->get_target_freq(devfreq, &freq); >> if (err) >> return err; >> - >> - /* >> - * 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); >> + get_freq_range(devfreq, &min_freq, &max_freq); >> >> if (freq < min_freq) { >> freq = min_freq; >> flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ >> } >> @@ -1298,40 +1333,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; >> + >> + mutex_lock(&df->lock); >> + get_freq_range(df, &min_freq, &max_freq); >> + mutex_unlock(&df->lock); >> >> - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); >> + 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) >> { >> @@ -1343,40 +1366,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 it. Also, the detailed comment for user have to add > the documentation file. OK > >> + 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); >> + 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,
Hi, On 19. 9. 26. 오전 5:55, Leonard Crestez wrote: > On 25.09.2019 04:32, Chanwoo Choi wrote: >> On 19. 9. 24. 오후 7:11, 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 | 112 ++++++++++++++++++++++---------------- >>> 1 file changed, 64 insertions(+), 48 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 4a878baa809e..eee403e70c84 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -96,10 +96,54 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>> dev_pm_opp_put(opp); >>> >>> return max_freq; >>> } >>> >>> +/** >>> + * 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 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. >> >> Init -> Initialize >> min/max -> minimum/maximum >> >>> + * Drivers can initialize this in either ascending or descending order >> >> Drivers -> devfreq drivers >> >>> + * and devfreq core supports both. >>> + */ >> >> >> In result, I prefer to change the comments as following: >> /* >> * Initialize the minimum/maximum frequency from freq_table. >> * The devfreq drivers can initialize freq_table in either >> * ascending or descending order and devfreq core supports both. >> */ > > OK > >>> + 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 */ >> >> 'constraints' -> Constraint because first verb have to be used >> as the sigular verbs. Also, I think that have to enhance the comments >> I prefer to use following comments: >> >> /* Constraint minimum/maximum frequency from user input via sysfs */ >> >> >> >>> + *min_freq = max(*min_freq, devfreq->min_freq); >>> + *max_freq = min(*max_freq, devfreq->max_freq); >>> + >>> + /* constraints from OPP interface */ >> >> ditto. I prefer to use following comments: >> >> /* Constraint minimum/maximum frequency from OPP interface */ >> >> >>> + *min_freq = max(*min_freq, devfreq->scaling_min_freq); >>> + /* scaling_max_freq can be zero on error */ >> >> Please remove it. >> >>> + if (devfreq->scaling_max_freq) >> >> As I knew, devfreq->scaling_max_freq is never zero. >> So, it is always true. This if statement is needed. > > It can happen if find_available_max_freq encounters an error when called > from devfreq_notifier_call. If you are wondering this case, I think that have to fix the possible issue on there instead of this point. > > Maybe that should be separately fixed to set scaling_max_freq to a > neutral value such as "ULONG_MAX" instead? OK. > > BTW: the devfreq_notifier_call function returns -EINVAL on error but it > should return one of the NOTIFY_OK/DONE/STOP values instead. The OPP > framework ignores notifier results but (-EINVAL & NOTIFY_STOP) evaluates > as true so other notifiers will be skipped unintentionally. I agree. It is needed to fix the return value type. > >>> + *max_freq = min(*max_freq, devfreq->scaling_max_freq); >>> + >>> + /* max_freq takes precedence over min_freq */ >> >> As I said, almost people know that min_freq have be under than max_freq. >> Please remove it. And until finishing the discussion on mailing list, >> please don't send the next version. If you just replied from my comment >> and then wait my next comment, we can save the time without replying >> the repetitive and same comment for same point. > > This series makes it possible to set a min_freq higher than max_freq > (for example via PM QoS from various devices). > > It is not obvious that min_freq takes precedence over max_freq but the > code is self-evident so I will remove the 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,20 +393,11 @@ int update_devfreq(struct devfreq *devfreq) >>> >>> /* Reevaluate the proper frequency */ >>> err = devfreq->governor->get_target_freq(devfreq, &freq); >>> if (err) >>> return err; >>> - >>> - /* >>> - * 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); >>> + get_freq_range(devfreq, &min_freq, &max_freq); >>> >>> if (freq < min_freq) { >>> freq = min_freq; >>> flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ >>> } >>> @@ -1298,40 +1333,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; >>> + >>> + mutex_lock(&df->lock); >>> + get_freq_range(df, &min_freq, &max_freq); >>> + mutex_unlock(&df->lock); >>> >>> - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); >>> + 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) >>> { >>> @@ -1343,40 +1366,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 it. Also, the detailed comment for user have to add >> the documentation file. > > OK > >> >>> + 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); >>> + 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-26 4:02 AM, Chanwoo Choi wrote: > Hi, > > On 19. 9. 26. 오전 5:55, Leonard Crestez wrote: >> On 25.09.2019 04:32, Chanwoo Choi wrote: >>> On 19. 9. 24. 오후 7:11, 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 | 112 ++++++++++++++++++++++---------------- >>>> 1 file changed, 64 insertions(+), 48 deletions(-) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index 4a878baa809e..eee403e70c84 100644 >>>> --- a/drivers/devfreq/devfreq.c >>>> +++ b/drivers/devfreq/devfreq.c >>>> @@ -96,10 +96,54 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) >>>> dev_pm_opp_put(opp); >>>> >>>> return max_freq; >>>> } >>>> >>>> +/** >>>> + * 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 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. >>> >>> Init -> Initialize >>> min/max -> minimum/maximum >>> >>>> + * Drivers can initialize this in either ascending or descending order >>> >>> Drivers -> devfreq drivers >>> >>>> + * and devfreq core supports both. >>>> + */ >>> >>> >>> In result, I prefer to change the comments as following: >>> /* >>> * Initialize the minimum/maximum frequency from freq_table. >>> * The devfreq drivers can initialize freq_table in either >>> * ascending or descending order and devfreq core supports both. >>> */ >> >> OK >> >>>> + 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 */ >>> >>> 'constraints' -> Constraint because first verb have to be used >>> as the sigular verbs. Also, I think that have to enhance the comments >>> I prefer to use following comments: >>> >>> /* Constraint minimum/maximum frequency from user input via sysfs */ >>> >>> >>> >>>> + *min_freq = max(*min_freq, devfreq->min_freq); >>>> + *max_freq = min(*max_freq, devfreq->max_freq); >>>> + >>>> + /* constraints from OPP interface */ >>> >>> ditto. I prefer to use following comments: >>> >>> /* Constraint minimum/maximum frequency from OPP interface */ >>> >>> >>>> + *min_freq = max(*min_freq, devfreq->scaling_min_freq); >>>> + /* scaling_max_freq can be zero on error */ >>> >>> Please remove it. >>> >>>> + if (devfreq->scaling_max_freq) >>> >>> As I knew, devfreq->scaling_max_freq is never zero. >>> So, it is always true. This if statement is needed. >> >> It can happen if find_available_max_freq encounters an error when called >> from devfreq_notifier_call. > > If you are wondering this case, I think that have to fix > the possible issue on there instead of this point. > >> >> Maybe that should be separately fixed to set scaling_max_freq to a >> neutral value such as "ULONG_MAX" instead? > > OK. > >> >> BTW: the devfreq_notifier_call function returns -EINVAL on error but it >> should return one of the NOTIFY_OK/DONE/STOP values instead. The OPP >> framework ignores notifier results but (-EINVAL & NOTIFY_STOP) evaluates >> as true so other notifiers will be skipped unintentionally. > > I agree. It is needed to fix the return value type. Will include as separate patches in v9. >>>> + *max_freq = min(*max_freq, devfreq->scaling_max_freq); >>>> + >>>> + /* max_freq takes precedence over min_freq */ >>> >>> As I said, almost people know that min_freq have be under than max_freq. >>> Please remove it. And until finishing the discussion on mailing list, >>> please don't send the next version. If you just replied from my comment >>> and then wait my next comment, we can save the time without replying >>> the repetitive and same comment for same point. >> >> This series makes it possible to set a min_freq higher than max_freq >> (for example via PM QoS from various devices). >> >> It is not obvious that min_freq takes precedence over max_freq but the >> code is self-evident so I will remove the 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,20 +393,11 @@ int update_devfreq(struct devfreq *devfreq) >>>> >>>> /* Reevaluate the proper frequency */ >>>> err = devfreq->governor->get_target_freq(devfreq, &freq); >>>> if (err) >>>> return err; >>>> - >>>> - /* >>>> - * 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); >>>> + get_freq_range(devfreq, &min_freq, &max_freq); >>>> >>>> if (freq < min_freq) { >>>> freq = min_freq; >>>> flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ >>>> } >>>> @@ -1298,40 +1333,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; >>>> + >>>> + mutex_lock(&df->lock); >>>> + get_freq_range(df, &min_freq, &max_freq); >>>> + mutex_unlock(&df->lock); >>>> >>>> - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); >>>> + 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) >>>> { >>>> @@ -1343,40 +1366,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 it. Also, the detailed comment for user have to add >>> the documentation file. >> >> OK >> >>> >>>> + 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); >>>> + 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 4a878baa809e..eee403e70c84 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -96,10 +96,54 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq) dev_pm_opp_put(opp); return max_freq; } +/** + * 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 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. + * Drivers can initialize this in either ascending or descending order + * and devfreq core supports both. + */ + 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,20 +393,11 @@ int update_devfreq(struct devfreq *devfreq) /* Reevaluate the proper frequency */ err = devfreq->governor->get_target_freq(devfreq, &freq); if (err) return err; - - /* - * 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); + get_freq_range(devfreq, &min_freq, &max_freq); if (freq < min_freq) { freq = min_freq; flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */ } @@ -1298,40 +1333,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; + + mutex_lock(&df->lock); + get_freq_range(df, &min_freq, &max_freq); + mutex_unlock(&df->lock); - return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq)); + 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) { @@ -1343,40 +1366,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); + 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,