From: Leonard Crestez <leonard.crestez@nxp.com>
To: Chanwoo Choi <cw00.choi@samsung.com>,
MyungJoo Ham <myungjoo.ham@samsung.com>,
Kyungmin Park <kyungmin.park@samsung.com>,
Matthias Kaehlcke <mka@chromium.org>
Cc: "Artur Świgoń" <a.swigon@partner.samsung.com>,
"Abel Vesa" <abel.vesa@nxp.com>,
"Saravana Kannan" <saravanak@google.com>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
"Viresh Kumar" <viresh.kumar@linaro.org>,
dl-linux-imx <linux-imx@nxp.com>,
"Krzysztof Kozlowski" <krzk@kernel.org>,
"Lukasz Luba" <l.luba@partner.samsung.com>,
"Alexandre Bailon" <abailon@baylibre.com>,
"Martin Kepplinger" <martink@posteo.de>,
"Georgi Djakov" <georgi.djakov@linaro.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"Jacky Bai" <ping.bai@nxp.com>
Subject: Re: [PATCH v7 4/6] PM / devfreq: Introduce devfreq_get_freq_range
Date: Tue, 24 Sep 2019 08:54:45 +0000 [thread overview]
Message-ID: <VI1PR04MB70236E264427C0CFF8F38E3AEE840@VI1PR04MB7023.eurprd04.prod.outlook.com> (raw)
In-Reply-To: e8d1d04a-1e8e-0765-179f-2146271787bb@samsung.com
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,
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-09-24 8:55 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-23 21:10 [PATCH v7 0/6] PM / devfreq: Add dev_pm_qos support Leonard Crestez
2019-09-23 21:10 ` [PATCH v7 1/6] PM / devfreq: Don't fail devfreq_dev_release if not in list Leonard Crestez
2019-09-24 2:45 ` Chanwoo Choi
2019-09-23 21:10 ` [PATCH v7 2/6] PM / devfreq: Move more initialization before registration Leonard Crestez
2019-09-24 2:57 ` Chanwoo Choi
2019-09-24 8:00 ` Leonard Crestez
2019-09-23 21:10 ` [PATCH v7 3/6] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
2019-09-24 3:13 ` Chanwoo Choi
2019-09-23 21:10 ` [PATCH v7 4/6] PM / devfreq: Introduce devfreq_get_freq_range Leonard Crestez
2019-09-24 5:13 ` Chanwoo Choi
2019-09-24 8:54 ` Leonard Crestez [this message]
2019-09-23 21:10 ` [PATCH v7 5/6] PM / devfreq: Add PM QoS support Leonard Crestez
2019-09-23 21:42 ` Matthias Kaehlcke
2019-09-24 6:52 ` Chanwoo Choi
2019-09-24 9:27 ` Leonard Crestez
2019-09-23 21:10 ` [PATCH v7 6/6] PM / devfreq: Use PM QoS for sysfs min/max_freq Leonard Crestez
2019-09-24 7:26 ` Chanwoo Choi
2019-09-24 9:48 ` Leonard Crestez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=VI1PR04MB70236E264427C0CFF8F38E3AEE840@VI1PR04MB7023.eurprd04.prod.outlook.com \
--to=leonard.crestez@nxp.com \
--cc=a.swigon@partner.samsung.com \
--cc=abailon@baylibre.com \
--cc=abel.vesa@nxp.com \
--cc=cw00.choi@samsung.com \
--cc=georgi.djakov@linaro.org \
--cc=krzk@kernel.org \
--cc=kyungmin.park@samsung.com \
--cc=l.luba@partner.samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-pm@vger.kernel.org \
--cc=martink@posteo.de \
--cc=mka@chromium.org \
--cc=myungjoo.ham@samsung.com \
--cc=ping.bai@nxp.com \
--cc=saravanak@google.com \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).