On 31.10.2019 04:37, Chanwoo Choi wrote:

On 19. 10. 3. 오전 4:25, Leonard Crestez wrote:

quoted

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 <redacted>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
  drivers/devfreq/devfreq.c | 110 +++++++++++++++++++++-----------------
  1 file changed, 62 insertions(+), 48 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 87eff789ce24..2d63692903ff 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c

@@ -96,10 +96,53 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
  		dev_pm_opp_put(opp);
  
  	return max_freq;
  }
  
+/**
+ * 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 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);
+
+	/*
+	 * Initialize minimum/maximum frequency from freq table.
+	 * The devfreq drivers can initialize this in either ascending or
+	 * descending order and devfreq core supports both.
+	 */
+	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];
+	}
+
+	/* Apply constraints from sysfs */
+	*min_freq = max(*min_freq, devfreq->min_freq);
+	*max_freq = min(*max_freq, devfreq->max_freq);
+
+	/* Apply 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)

nitpick:
This if statement is necessary?

The patch3 in this series initializes the 'devfreq->scaling_max_freq'
is ULONG_MAX if failed to get the available frequency from find_available_max_freq.

Ok, seem I forgot to drop the if statement.

quoted

+		*max_freq = min(*max_freq, devfreq->scaling_max_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
   */

@@ -348,20 +391,11 @@ int update_devfreq(struct devfreq *devfreq)
  
  	/* Reevaluate the proper frequency */
  	err = devfreq->governor->get_target_freq(devfreq, &freq);
  	if (err)
  		return err;
-
-	/*
-	 * 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);
+	get_freq_range(devfreq, &min_freq, &max_freq);
  
  	if (freq < min_freq) {
  		freq = min_freq;
  		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
  	}

@@ -1281,40 +1315,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);
+	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)
  {

@@ -1326,40 +1348,32 @@ 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];
-	}
+	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);
+	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,

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel