[PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor

[PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor

[Lukasz Luba]

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);
+}
+
+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");
+}
+
+static void devfreq_simple_ondemand_sysfs_remove(struct devfreq *df)
+{
+	sysfs_remove_file(&df->dev.kobj, &dev_attr_upthreshold.attr);
+	sysfs_remove_file(&df->dev.kobj, &dev_attr_downdifferential.attr);
+}
+
 static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
 				unsigned int event, void *data)
 {
 	switch (event) {
 	case DEVFREQ_GOV_START:
 		devfreq_monitor_start(devfreq);
+		devfreq_simple_ondemand_sysfs_setup(devfreq);
 		break;
 
 	case DEVFREQ_GOV_STOP:
+		devfreq_simple_ondemand_sysfs_remove(devfreq);
 		devfreq_monitor_stop(devfreq);
 		break;
 
-- 
2.17.1

Re: [PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor

[Chanwoo Choi]

Hi Lukasz,

I has been developed[1] the sysfs attributes for upthreshold and downdifferential.
But, I has not yet posted it[1]. I'll post my approach with some changes.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=e7c59dfb4afebe0c96de54516e9be23d76d96492

On 1/16/21 2:05 AM, 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);
> +}
> +
> +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");
> +}
> +
> +static void devfreq_simple_ondemand_sysfs_remove(struct devfreq *df)
> +{
> +	sysfs_remove_file(&df->dev.kobj, &dev_attr_upthreshold.attr);
> +	sysfs_remove_file(&df->dev.kobj, &dev_attr_downdifferential.attr);
> +}
> +
>  static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
>  				unsigned int event, void *data)
>  {
>  	switch (event) {
>  	case DEVFREQ_GOV_START:
>  		devfreq_monitor_start(devfreq);
> +		devfreq_simple_ondemand_sysfs_setup(devfreq);
>  		break;
>  
>  	case DEVFREQ_GOV_STOP:
> +		devfreq_simple_ondemand_sysfs_remove(devfreq);
>  		devfreq_monitor_stop(devfreq);
>  		break;
>  
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor

[Lukasz Luba]

Hi Chanwoo,

On 1/18/21 6:49 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> I has been developed[1] the sysfs attributes for upthreshold and downdifferential.
> But, I has not yet posted it[1]. I'll post my approach with some changes.

Fair enough. Please add me on CC when you post it. I can help you with
reviewing it.

> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/commit/?h=devfreq-testing&id=e7c59dfb4afebe0c96de54516e9be23d76d96492

Thank you for the link. The code looks pretty good, just use
kstrtouint() instead of sscanf() - checkpatch would not complain.

Regards,
Lukasz

Re: [PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor

[Greg KH]

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 :(


> +}
> +
> +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.

thanks,

greg k-h

Re: [PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor

[Lukasz Luba]

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
> 

Re: [PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor

[Greg KH]

On Mon, Jan 18, 2021 at 05:56:06PM +0000, Lukasz Luba wrote:
> 
> 
> 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.

No, never use "raw" kobjects in the sys/devices/ tree, that is a sure
way to ensure that userspace will never be notified or see your
attributes.

thanks,

greg k-h

Re: [PATCH] PM / devfreq: Add sysfs attributes to simple_ondemand governor

[Lukasz Luba]

On 1/18/21 6:14 PM, Greg KH wrote:
> On Mon, Jan 18, 2021 at 05:56:06PM +0000, Lukasz Luba wrote:
>>
>>
>> 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.
> 
> No, never use "raw" kobjects in the sys/devices/ tree, that is a sure
> way to ensure that userspace will never be notified or see your
> attributes.

Interesting, I've seen it in the schedutil governor, but might in a
different context.

Regards,
Lukasz