| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <43729797-2d3c-be12-ce72-bfe5bca54fa0@arm.com>
Date: Mon, 18 Jan 2021 17:56:06 +0000
From: Lukasz Luba <lukasz.luba@....com>
To: Greg KH <greg@...ah.com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
cw00.choi@...sung.com, myungjoo.ham@...sung.com,
kyungmin.park@...sung.com
Subject: Re: [PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand
governor
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@....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
>
Powered by blists - more mailing lists