[v7,4/6] PM / devfreq: Introduce devfreq_get_freq_range
diff mbox series

Message ID 15e599188c5b50edad3efc37787dc2b70d6efead.1569272883.git.leonard.crestez@nxp.com
State Superseded
Headers show
Series
  • PM / devfreq: Add dev_pm_qos support
Related show

Commit Message

Leonard Crestez Sept. 23, 2019, 9:10 p.m. UTC
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 | 111 +++++++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 48 deletions(-)

Comments

Chanwoo Choi Sept. 24, 2019, 5:13 a.m. UTC | #1
Hi,

On 19. 9. 24. 오전 6:10, 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 | 111 +++++++++++++++++++++-----------------
>  1 file changed, 63 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 1cec816d3d00..7f152a582e78 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -24,10 +24,12 @@
>  #include <linux/printk.h>
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
>  #include "governor.h"
>  
> +#define HZ_PER_KHZ 1000

It is not used on this patch. If it is used on later patch,
move it to other patch.

> +
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/devfreq.h>
>  
>  static struct class *devfreq_class;
>  
> @@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  		dev_pm_opp_put(opp);
>  
>  	return max_freq;
>  }
>  
> +/**
> + * devfreq_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 devfreq_get_freq_range(struct devfreq *devfreq,

This function is used in the only devfreq core.
I think that the internal function doesn't need to add 'devfreq' prefix.
Also, when I developed the patches, I didn't use 'devfreq' prefix
for internal function.

> +				   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 */

I think that the comments in the devfreq_get_freq_range(),
it is not necessary. But if you think that it is necessary,
please add more detailed description like as following:

	/*
	 * The devfreq recommend that freq_table must be generated
	 * in ascending order but, some devfreq driver used the descending order
	 * for freq_table. In order to support all cases, check the order
	 * of freq_table and then initialize the min/max frequency from freq_table.
	 */

> +	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 */

ditto. Need to add more detailed comment by keeping the comment style of devfreq.


> +	*min_freq = max(*min_freq, devfreq->min_freq);
> +	*max_freq = min(*max_freq, devfreq->max_freq);
> +
> +	/* constraints from OPP interface */

ditto.

> +	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
> +	/* scaling_max_freq can be zero on error */
> +	if (devfreq->scaling_max_freq)
> +		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
> +
> +	/* max_freq takes precedence over min_freq */

It is not necessary. We can know that min_freq have to be
under max_freq without any 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,21 +391,13 @@ int update_devfreq(struct devfreq *devfreq)
>  
>  	/* Reevaluate the proper frequency */
>  	err = devfreq->governor->get_target_freq(devfreq, &freq);
>  	if (err)
>  		return err;
> +	devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
>  
> -	/*
> -	 * 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);
> -
> +	/* max freq takes priority over min freq */

Please remove it. 

>  	if (freq < min_freq) {
>  		freq = min_freq;
>  		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>  	}
>  	if (freq > max_freq) {
> @@ -1278,40 +1312,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;
>  
> -	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
> +	mutex_lock(&df->lock);
> +	devfreq_get_freq_range(df, &min_freq, &max_freq);
> +	mutex_unlock(&df->lock);
> +
> +	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)
>  {
> @@ -1323,40 +1345,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 this comment.

> +	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);
> +	devfreq_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,
>
Leonard Crestez Sept. 24, 2019, 8:54 a.m. UTC | #2
On 2019-09-24 8:09 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 24. 오전 6:10, 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 | 111 +++++++++++++++++++++-----------------
>>   1 file changed, 63 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 1cec816d3d00..7f152a582e78 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -24,10 +24,12 @@
>>   #include <linux/printk.h>
>>   #include <linux/hrtimer.h>
>>   #include <linux/of.h>
>>   #include "governor.h"
>>   
>> +#define HZ_PER_KHZ 1000
> 
> It is not used on this patch. If it is used on later patch,
> move it to other patch.

Good catch.

>>   #define CREATE_TRACE_POINTS
>>   #include <trace/events/devfreq.h>
>>   
>>   static struct class *devfreq_class;
>>   
>> @@ -96,10 +98,50 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>   		dev_pm_opp_put(opp);
>>   
>>   	return max_freq;
>>   }
>>   
>> +/**
>> + * devfreq_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 devfreq_get_freq_range(struct devfreq *devfreq,
> 
> This function is used in the only devfreq core.
> I think that the internal function doesn't need to add 'devfreq' prefix.
> Also, when I developed the patches, I didn't use 'devfreq' prefix
> for internal function.

This does not appear to a clear convention, there are other internal 
functions with a devfreq prefix. I usually keep prefixes always but it 
does result in extra verbosity.
>> +				   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 */
> 
> I think that the comments in the devfreq_get_freq_range(),
> it is not necessary. But if you think that it is necessary,
> please add more detailed description like as following:
> 
> 	/*
> 	 * The devfreq recommend that freq_table must be generated
> 	 * in ascending order but, some devfreq driver used the descending order
> 	 * for freq_table. In order to support all cases, check the order
> 	 * of freq_table and then initialize the min/max frequency from freq_table.
> 	 */

OK. Not strictly related to this series: wouldn't it make sense to just 
fix the drivers? In linux kernel API compatibility hacks are usually 
avoided.

>> +	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 */
> 
> ditto. Need to add more detailed comment by keeping the comment style of devfreq.

What comment style, capitalization and punctuation? This is similar to 
the rest of the file.

>> +	*min_freq = max(*min_freq, devfreq->min_freq);
>> +	*max_freq = min(*max_freq, devfreq->max_freq);
>> +
>> +	/* constraints from OPP interface */
> 
> ditto.
> 
>> +	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
>> +	/* scaling_max_freq can be zero on error */
>> +	if (devfreq->scaling_max_freq)
>> +		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
>> +
>> +	/* max_freq takes precedence over min_freq */
> 
> It is not necessary. We can know that min_freq have to be
> under max_freq without any comment.
> 
>> +	if (*min_freq > *max_freq)
>> +		*min_freq = *max_freq;

This is a deliberate change introduced by this patch: instead of 
ensuring min_freq <= max_freq at sysfs write time we make min_freq take 
precedence at evaluation time.

With PM QoS there is no way to ensure that the value coming through 
DEV_PM_QOS_MIN_FREQ is less than through DEV_PM_QOS_MAX_FREQ.

>> +}
>> +
>>   /**
>>    * devfreq_get_freq_level() - Lookup freq_table for the frequency
>>    * @devfreq:	the devfreq instance
>>    * @freq:	the target frequency
>>    */
>> @@ -349,21 +391,13 @@ int update_devfreq(struct devfreq *devfreq)
>>   
>>   	/* Reevaluate the proper frequency */
>>   	err = devfreq->governor->get_target_freq(devfreq, &freq);
>>   	if (err)
>>   		return err;
>> +	devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
>>   
>> -	/*
>> -	 * 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);
>> -
>> +	/* max freq takes priority over min freq */
> 
> Please remove it.

OK

>>   	if (freq < min_freq) {
>>   		freq = min_freq;
>>   		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>>   	}
>>   	if (freq > max_freq) {
>> @@ -1278,40 +1312,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;
>>   
>> -	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
>> +	mutex_lock(&df->lock);
>> +	devfreq_get_freq_range(df, &min_freq, &max_freq);
>> +	mutex_unlock(&df->lock);
>> +
>> +	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)
>>   {
>> @@ -1323,40 +1345,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 this comment.

This is documented in Documentation/ABI/testing/sysfs-class-devfreq but 
not immediately obvious. Setting max to "zero" could have meant "min 
possible" instead of "don't care".

>> +	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);
>> +	devfreq_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,

Patch
diff mbox series

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 1cec816d3d00..7f152a582e78 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -24,10 +24,12 @@ 
 #include <linux/printk.h>
 #include <linux/hrtimer.h>
 #include <linux/of.h>
 #include "governor.h"
 
+#define HZ_PER_KHZ 1000
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/devfreq.h>
 
 static struct class *devfreq_class;
 
@@ -96,10 +98,50 @@  static unsigned long find_available_max_freq(struct devfreq *devfreq)
 		dev_pm_opp_put(opp);
 
 	return max_freq;
 }
 
+/**
+ * devfreq_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 devfreq_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 */
+	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 */
+	*min_freq = max(*min_freq, devfreq->min_freq);
+	*max_freq = min(*max_freq, devfreq->max_freq);
+
+	/* constraints from OPP interface */
+	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
+	/* scaling_max_freq can be zero on error */
+	if (devfreq->scaling_max_freq)
+		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
+
+	/* max_freq takes precedence over min_freq */
+	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,21 +391,13 @@  int update_devfreq(struct devfreq *devfreq)
 
 	/* Reevaluate the proper frequency */
 	err = devfreq->governor->get_target_freq(devfreq, &freq);
 	if (err)
 		return err;
+	devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
 
-	/*
-	 * 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);
-
+	/* max freq takes priority over min freq */
 	if (freq < min_freq) {
 		freq = min_freq;
 		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
 	}
 	if (freq > max_freq) {
@@ -1278,40 +1312,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;
 
-	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
+	mutex_lock(&df->lock);
+	devfreq_get_freq_range(df, &min_freq, &max_freq);
+	mutex_unlock(&df->lock);
+
+	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)
 {
@@ -1323,40 +1345,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" */
+	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);
+	devfreq_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,