[PATCH] devfreq: do not ignore errors during store min, max frequency

[PATCH] devfreq: do not ignore errors during store min, max frequency

[Lukasz Luba]

This patch adds functionality to make sure when a call to set a new
max or min frequency is possible.
If storing the new value was not possible, restore previous one
and forward the error value.

Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
---
It is based on v4.11-rc8.

 drivers/devfreq/devfreq.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index dea0487..8586035 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1059,6 +1059,7 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 	unsigned long value;
 	int ret;
 	unsigned long max;
+	unsigned long old_min;
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
@@ -1071,9 +1072,22 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 		goto unlock;
 	}
 
+	ret = devfreq_get_freq_level(df, value);
+	if (ret < 0) {
+		dev_warn(dev, "Storing min freq failed with (%d) error\n", ret);
+		goto unlock;
+	}
+
+	old_min = df->min_freq;
 	df->min_freq = value;
-	update_devfreq(df);
-	ret = count;
+	ret = update_devfreq(df);
+	if (ret) {
+		df->min_freq = old_min;
+		dev_warn(dev, "Storing min freq failed with (%d) error\n", ret);
+	} else {
+		ret = count;
+	}
+
 unlock:
 	mutex_unlock(&df->lock);
 	return ret;
@@ -1086,6 +1100,7 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 	unsigned long value;
 	int ret;
 	unsigned long min;
+	unsigned long old_max;
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
@@ -1098,9 +1113,22 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 		goto unlock;
 	}
 
+	ret = devfreq_get_freq_level(df, value);
+	if (ret < 0) {
+		dev_warn(dev, "Storing max freq failed with (%d) error\n", ret);
+		goto unlock;
+	}
+
+	old_max = df->max_freq;
 	df->max_freq = value;
-	update_devfreq(df);
-	ret = count;
+	ret = update_devfreq(df);
+	if (ret) {
+		df->max_freq = old_max;
+		dev_warn(dev, "Storing max freq failed with (%d) error\n", ret);
+	} else {
+		ret = count;
+	}
+
 unlock:
 	mutex_unlock(&df->lock);
 	return ret;
-- 
1.9.1

RE: [PATCH] devfreq: do not ignore errors during store min, max frequency

[MyungJoo Ham]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

> This patch adds functionality to make sure when a call to set a new
> max or min frequency is possible.
> If storing the new value was not possible, restore previous one
> and forward the error value.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
> It is based on v4.11-rc8.
> 
>  drivers/devfreq/devfreq.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)

...

> +	ret = devfreq_get_freq_level(df, value);
> +	if (ret < 0) {
> +		dev_warn(dev, "Storing min freq failed with (%d) error\n", ret);
> +		goto unlock;
> +	}
> +

This is not good.

For a device that supports [ 100, 400, 800, 1000 ] MHz,
saying "min = 200 Mhz" or "max = 600 MHz" shouldn't be prohibited.

Those functions are to express lower bound and upper bound, not to
designate the exact operating frequencies.


Cheers,
MyungJoo

Re: [PATCH] devfreq: do not ignore errors during store min, max frequency

[Lukasz Luba]

Hi MyungJoo,

On 27/04/17 09:30, MyungJoo Ham wrote:
>> This patch adds functionality to make sure when a call to set a new
>> max or min frequency is possible.
>> If storing the new value was not possible, restore previous one
>> and forward the error value.
>>
>> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
>> ---
>> It is based on v4.11-rc8.
>>
>>  drivers/devfreq/devfreq.c | 36 ++++++++++++++++++++++++++++++++----
>>  1 file changed, 32 insertions(+), 4 deletions(-)
>
> ...
>
>> +	ret = devfreq_get_freq_level(df, value);
>> +	if (ret < 0) {
>> +		dev_warn(dev, "Storing min freq failed with (%d) error\n", ret);
>> +		goto unlock;
>> +	}
>> +
>
> This is not good.
>
> For a device that supports [ 100, 400, 800, 1000 ] MHz,
> saying "min = 200 Mhz" or "max = 600 MHz" shouldn't be prohibited.
>
> Those functions are to express lower bound and upper bound, not to
> designate the exact operating frequencies.

Would it be possible to convince you to store a
'posible/valid frequency' (from OPP) in that value?
Governors (like simpleondemand) and drivers use it with the flags.
It would be useful for thermal subsystem. It could have the max/min for 
the devfreq device.

I could prepare a patch which sets the freq from OPP respecting
rounding up/down based on 'devfreq_recommended_opp()'.

Regards,
Lukasz

RE: Re: [PATCH] devfreq: do not ignore errors during store min, max frequency

[MyungJoo Ham]

> >
> > ...
> >
> >> +	ret = devfreq_get_freq_level(df, value);
> >> +	if (ret < 0) {
> >> +		dev_warn(dev, "Storing min freq failed with (%d) error\n", ret);
> >> +		goto unlock;
> >> +	}
> >> +
> >
> > This is not good.
> >
> > For a device that supports [ 100, 400, 800, 1000 ] MHz,
> > saying "min = 200 Mhz" or "max = 600 MHz" shouldn't be prohibited.
> >
> > Those functions are to express lower bound and upper bound, not to
> > designate the exact operating frequencies.
> 
> Would it be possible to convince you to store a
> 'posible/valid frequency' (from OPP) in that value?
> Governors (like simpleondemand) and drivers use it with the flags.
> It would be useful for thermal subsystem. It could have the max/min for 
> the devfreq device.
> 
> I could prepare a patch which sets the freq from OPP respecting
> rounding up/down based on 'devfreq_recommended_opp()'.
> 
> Regards,
> Lukasz

Before talking about implementation detail,
what's the benefit of what you are suggesting?

What's the scenario that you cannot do with the current system
while you can do with what you suggest?

Why do you need rounded (OPP-enabled) max/min values?
Why unrounded (lower/upper limits) max/min values cannot do what you want?


Cheers,
MyungJoo


ps. I'll be away from the network for about 10 days after few hours.

Re: [PATCH] devfreq: do not ignore errors during store min, max frequency

[Lukasz Luba]

On 28/04/17 00:41, MyungJoo Ham wrote:
>>>
>>> ...
>>>
>>>> +	ret = devfreq_get_freq_level(df, value);
>>>> +	if (ret < 0) {
>>>> +		dev_warn(dev, "Storing min freq failed with (%d) error\n", ret);
>>>> +		goto unlock;
>>>> +	}
>>>> +
>>>
>>> This is not good.
>>>
>>> For a device that supports [ 100, 400, 800, 1000 ] MHz,
>>> saying "min = 200 Mhz" or "max = 600 MHz" shouldn't be prohibited.
>>>
>>> Those functions are to express lower bound and upper bound, not to
>>> designate the exact operating frequencies.
>>
>> Would it be possible to convince you to store a
>> 'posible/valid frequency' (from OPP) in that value?
>> Governors (like simpleondemand) and drivers use it with the flags.
>> It would be useful for thermal subsystem. It could have the max/min for
>> the devfreq device.
>>
>> I could prepare a patch which sets the freq from OPP respecting
>> rounding up/down based on 'devfreq_recommended_opp()'.
>>
>> Regards,
>> Lukasz
>
> Before talking about implementation detail,
> what's the benefit of what you are suggesting?
>
> What's the scenario that you cannot do with the current system
> while you can do with what you suggest?
>
> Why do you need rounded (OPP-enabled) max/min values?
> Why unrounded (lower/upper limits) max/min values cannot do what you want?
>
>
> Cheers,
> MyungJoo
>
>
> ps. I'll be away from the network for about 10 days after few hours.
>
i.e. when you look at the function:
devfreq_cooling_get_max_state() in drivers/thermal/devfreq_cooling
you will see that it is not aware of any changes
(there are some other issues as well).
I am going to modify devfreq_cooling a bit.

I thought it would be good to reuse the df->max_freq
as much as possible.

Regards,
Lukasz Luba