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>,
Viresh Kumar <viresh.kumar@linaro.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>,
dl-linux-imx <linux-imx@nxp.com>,
"Krzysztof Kozlowski" <krzk@kernel.org>,
"Lukasz Luba" <l.luba@partner.samsung.com>,
"Alexandre Bailon" <abailon@baylibre.com>,
"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 v8 4/6] PM / devfreq: Introduce get_freq_range helper
Date: Thu, 26 Sep 2019 14:03:45 +0000 [thread overview]
Message-ID: <VI1PR04MB7023CAD2919FB5F2782ABFC4EE860@VI1PR04MB7023.eurprd04.prod.outlook.com> (raw)
In-Reply-To: e8ec6c2e-8536-b926-c1fc-468f9e9adca7@samsung.com
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,
>
>
_______________________________________________
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-26 14:03 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20190924101140epcas1p4abeedf42f223e65f58c88a0ddf1e4e56@epcas1p4.samsung.com>
2019-09-24 10:11 ` [PATCH v8 0/6] PM / devfreq: Add dev_pm_qos support Leonard Crestez
2019-09-24 10:11 ` [PATCH v8 1/6] PM / devfreq: Don't fail devfreq_dev_release if not in list Leonard Crestez
2019-09-24 10:11 ` [PATCH v8 2/6] PM / devfreq: Move more initialization before registration Leonard Crestez
2019-09-25 1:46 ` Chanwoo Choi
2019-09-24 10:11 ` [PATCH v8 3/6] PM / devfreq: Don't take lock in devfreq_add_device Leonard Crestez
2019-09-24 10:11 ` [PATCH v8 4/6] PM / devfreq: Introduce get_freq_range helper Leonard Crestez
2019-09-25 1:37 ` Chanwoo Choi
2019-09-25 20:55 ` Leonard Crestez
2019-09-26 1:06 ` Chanwoo Choi
2019-09-26 14:03 ` Leonard Crestez [this message]
2019-09-24 10:11 ` [PATCH v8 5/6] PM / devfreq: Add PM QoS support Leonard Crestez
2019-09-24 19:11 ` Matthias Kaehlcke
2019-09-24 19:22 ` Leonard Crestez
2019-09-25 2:17 ` Chanwoo Choi
2019-09-25 19:40 ` Leonard Crestez
2019-09-26 1:08 ` Chanwoo Choi
2019-09-25 2:15 ` Chanwoo Choi
2019-09-25 16:06 ` Matthias Kaehlcke
2019-09-25 21:18 ` Leonard Crestez
2019-09-26 1:12 ` Chanwoo Choi
2019-09-26 13:43 ` Leonard Crestez
2019-09-27 1:49 ` Chanwoo Choi
2019-09-30 13:16 ` Leonard Crestez
2019-09-30 21:42 ` Chanwoo Choi
2019-10-01 9:39 ` Leonard Crestez
2019-10-01 21:55 ` Chanwoo Choi
2019-09-26 1:19 ` Chanwoo Choi
2019-09-30 12:43 ` Leonard Crestez
2019-09-30 21:21 ` Chanwoo Choi
2019-09-24 10:11 ` [PATCH v8 6/6] PM / devfreq: Use PM QoS for sysfs min/max_freq Leonard Crestez
2019-09-25 2:41 ` Chanwoo Choi
2019-09-25 16:45 ` Matthias Kaehlcke
2019-09-26 1:25 ` Chanwoo Choi
2019-09-26 16:04 ` Matthias Kaehlcke
2019-09-27 1:58 ` Chanwoo Choi
2019-09-25 22:11 ` Leonard Crestez
2019-09-26 1:26 ` Chanwoo Choi
2019-09-25 1:44 ` [PATCH v8 0/6] PM / devfreq: Add dev_pm_qos support Chanwoo Choi
2019-09-25 19:37 ` Leonard Crestez
[not found] ` <CGME20190924101139epcas1p4c6799a5de9bdb4e90abb74de1e881388@epcms1p4>
2019-09-25 1:58 ` [PATCH v8 2/6] PM / devfreq: Move more initialization before registration MyungJoo Ham
2019-09-25 19:33 ` 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=VI1PR04MB7023CAD2919FB5F2782ABFC4EE860@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=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).