From: Lukasz Luba <lukasz.luba@arm.com>
To: Greg KH <greg@kroah.com>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
cw00.choi@samsung.com, myungjoo.ham@samsung.com,
kyungmin.park@samsung.com
Subject: Re: [PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor
Date: Mon, 18 Jan 2021 17:56:06 +0000 [thread overview]
Message-ID: <43729797-2d3c-be12-ce72-bfe5bca54fa0@arm.com> (raw)
In-Reply-To: <YAXCnMsUy3n+asfm@kroah.com>
On 1/18/21 5:17 PM, Greg KH wrote:
> On Fri, Jan 15, 2021 at 05:05:30PM +0000, Lukasz Luba wrote:
>> The simple_ondemand devfreq governor is used by quite a few devices, like
>> GPUs, DSPs, memory controllers, etc. It implements algorithm which tries
>> to predict the device frequency based on past statistics. There are two
>> tunables for the algorithm: 'upthreshold' and 'downdifferential'. These
>> tunables change the behavior of the decision, e.g. how fast to increase
>> the frequency or how rapidly limit the frequency. These values might be
>> different based on the application which is currently running, e.g.
>> different behavior is needed for a game than for web browsing or clean
>> desktop. The patch exports these two tunables so they can be adjusted
>> based on current need. There is also a check with the allowed ranges
>> to make sure the values are correct and safe.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>> drivers/devfreq/governor_simpleondemand.c | 135 ++++++++++++++++++++++
>> 1 file changed, 135 insertions(+)
>>
>> diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
>> index d57b82a2b570..4b3c182e0a49 100644
>> --- a/drivers/devfreq/governor_simpleondemand.c
>> +++ b/drivers/devfreq/governor_simpleondemand.c
>> @@ -15,6 +15,7 @@
>> /* Default constants for DevFreq-Simple-Ondemand (DFSO) */
>> #define DFSO_UPTHRESHOLD (90)
>> #define DFSO_DOWNDIFFERENCTIAL (5)
>> +#define DFSO_MAX_VALUE (100)
>> static int devfreq_simple_ondemand_func(struct devfreq *df,
>> unsigned long *freq)
>> {
>> @@ -84,15 +85,149 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
>> return 0;
>> }
>>
>> +static ssize_t upthreshold_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq_simple_ondemand_data *data;
>> + struct devfreq *df = to_devfreq(dev);
>> +
>> + if (!df->data)
>> + return -EINVAL;
>> +
>> + data = df->data;
>> +
>> + return sprintf(buf, "%d\n", data->upthreshold);
>
> sysfs_emit()?
>
> Also, you forgot the Documentation/ABI/ updates for new sysfs files :(
True, I will remember next time.
>
>
>> +}
>> +
>> +static ssize_t upthreshold_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct devfreq_simple_ondemand_data *data;
>> + struct devfreq *df = to_devfreq(dev);
>> + unsigned int value;
>> + int ret;
>> +
>> + if (!df->data)
>> + return -EINVAL;
>> +
>> + data = df->data;
>> +
>> + ret = kstrtouint(buf, 10, &value);
>> + if (ret < 0)
>> + return -EINVAL;
>> +
>> + mutex_lock(&df->lock);
>> +
>> + if (value > DFSO_MAX_VALUE || value <= data->downdifferential) {
>> + mutex_unlock(&df->lock);
>> + return -EINVAL;
>> + }
>> +
>> + data->upthreshold = value;
>> + mutex_unlock(&df->lock);
>> +
>> + return count;
>> +}
>> +static DEVICE_ATTR_RW(upthreshold);
>> +
>> +static ssize_t downdifferential_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct devfreq_simple_ondemand_data *data;
>> + struct devfreq *df = to_devfreq(dev);
>> +
>> + if (!df->data)
>> + return -EINVAL;
>> +
>> + data = df->data;
>> +
>> + return sprintf(buf, "%d\n", data->downdifferential);
>> +}
>> +
>> +static ssize_t downdifferential_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct devfreq_simple_ondemand_data *data;
>> + struct devfreq *df = to_devfreq(dev);
>> + unsigned int value;
>> + int ret;
>> +
>> + if (!df->data)
>> + return -EINVAL;
>> +
>> + data = df->data;
>> +
>> + ret = kstrtouint(buf, 10, &value);
>> + if (ret < 0)
>> + return -EINVAL;
>> +
>> + mutex_lock(&df->lock);
>> +
>> + if (value > DFSO_MAX_VALUE || value >= data->upthreshold) {
>> + mutex_unlock(&df->lock);
>> + return -EINVAL;
>> + }
>> +
>> + data->downdifferential = value;
>> + mutex_unlock(&df->lock);
>> +
>> + return count;
>> +}
>> +static DEVICE_ATTR_RW(downdifferential);
>> +
>> +static void devfreq_simple_ondemand_sysfs_setup(struct devfreq *df)
>> +{
>> + struct devfreq_simple_ondemand_data *data;
>> + int ret;
>> +
>> + if (!df->data) {
>> + /* The memory will be freed automatically */
>> + df->data = devm_kzalloc(&df->dev,
>> + sizeof(struct devfreq_simple_ondemand_data),
>> + GFP_KERNEL);
>> + if (!df->data) {
>> + dev_warn(&df->dev, "Unable to allocate memory");
>> + return;
>> + }
>> + }
>> +
>> + data = df->data;
>> +
>> + /* After new allocation setup default values, since they are used */
>> + if (!data->upthreshold)
>> + data->upthreshold = DFSO_UPTHRESHOLD;
>> +
>> + if (!data->downdifferential)
>> + data->downdifferential = DFSO_DOWNDIFFERENCTIAL;
>> +
>> + ret = sysfs_create_file(&df->dev.kobj, &dev_attr_upthreshold.attr);
>> + if (ret < 0)
>> + dev_warn(&df->dev, "Unable to create 'upthreshold' attr\n");
>> +
>> + ret = sysfs_create_file(&df->dev.kobj, &dev_attr_downdifferential.attr);
>> + if (ret < 0)
>> + dev_warn(&df->dev, "Unable to create 'downdifferential' attr\n");
>
> You just raced with userspace and lost :(
>
> Please use the default sysfs groups so that it all works properly.
>
> Huge hint, when calling sysfs_* from a driver, that usually means you
> are doing something wrong.
I should have used kobject_init_and_add() and the default devfreq
sysfs group as a parent.
Thank you for the comments and hints. I'll make use of them next time.
This patch is abandoned, since Chanwoo is already trying to address
the same issue.
Regards,
Lukasz
>
> thanks,
>
> greg k-h
>
next prev parent reply other threads:[~2021-01-18 17:57 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20210115170541epcas1p1b7bc36fb5a4f1b907da5771ca0c37891@epcas1p1.samsung.com>
2021-01-15 17:05 ` [PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor Lukasz Luba
2021-01-18 6:49 ` Chanwoo Choi
2021-01-18 9:56 ` Lukasz Luba
2021-01-18 17:17 ` Greg KH
2021-01-18 17:56 ` Lukasz Luba [this message]
2021-01-18 18:14 ` Greg KH
2021-01-18 18:26 ` Lukasz Luba
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=43729797-2d3c-be12-ce72-bfe5bca54fa0@arm.com \
--to=lukasz.luba@arm.com \
--cc=cw00.choi@samsung.com \
--cc=greg@kroah.com \
--cc=kyungmin.park@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=myungjoo.ham@samsung.com \
/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).