[PATCH v3 0/2] PM / devfreq: Add dev_pm_qos support

[PATCH v3 0/2] PM / devfreq: Add dev_pm_qos support

[Leonard Crestez]

Add dev_pm_qos notifies to devfreq core in order to support frequency
limits via the dev_pm_qos_add_request.

Unlike the rest of devfreq the dev_pm_qos frequency is measured in Khz,
this is consistent with current dev_pm_qos usage for cpufreq and
allows frequencies above 2Ghz (pm_qos expresses limits as s32).

Like with cpufreq the handling of min_freq/max_freq is moved to the dev_pm_qos
mechanism and this decreases the precision of the sysfs entries.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---

Changes since v2:
 * Handle sysfs via dev_pm_qos (in separate patch)
 * Add locking to {min,max}_freq_show
 * Fix checkpatch issues (long lines etc)
Link to v2: https://patchwork.kernel.org/patch/11084279/

Changes since v1:
 * Add doxygen comments for min_nb/max_nb
 * Remove notifiers on error/cleanup paths. Keep gotos simple by relying on
dev_pm_qos_remove_notifier ignoring notifiers which were not added.
Link to v1: https://patchwork.kernel.org/patch/11078475/

Leonard Crestez (2):
  PM / devfreq: Add dev_pm_qos support
  PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

 drivers/devfreq/devfreq.c | 170 +++++++++++++++++++++++++++-----------
 include/linux/devfreq.h   |  14 +++-
 2 files changed, 131 insertions(+), 53 deletions(-)

-- 
2.17.1

[PATCH v3 1/2] PM / devfreq: Add dev_pm_qos support

[Leonard Crestez]

Add dev_pm_qos notifies to devfreq core in order to support frequency
limits via the dev_pm_qos_add_request.

Unlike the rest of devfreq the dev_pm_qos frequency is measured in Khz,
this is consistent with current dev_pm_qos usage for cpufreq and
allows frequencies above 2Ghz (pm_qos expresses limits as s32).

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 95 ++++++++++++++++++++++++++++++++++++---
 include/linux/devfreq.h   |  5 +++
 2 files changed, 95 insertions(+), 5 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 784c08e4f931..58deffa52a37 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -22,10 +22,11 @@
 #include <linux/platform_device.h>
 #include <linux/list.h>
 #include <linux/printk.h>
 #include <linux/hrtimer.h>
 #include <linux/of.h>
+#include <linux/pm_qos.h>
 #include "governor.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/devfreq.h>
 
@@ -96,10 +97,30 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
 		dev_pm_opp_put(opp);
 
 	return max_freq;
 }
 
+static unsigned long get_effective_min_freq(struct devfreq *devfreq)
+{
+	lockdep_assert_held(&devfreq->lock);
+
+	return max3(devfreq->scaling_min_freq, devfreq->min_freq,
+		1000 * (unsigned long)dev_pm_qos_read_value(
+				devfreq->dev.parent,
+				DEV_PM_QOS_MIN_FREQUENCY));
+}
+
+static unsigned long get_effective_max_freq(struct devfreq *devfreq)
+{
+	lockdep_assert_held(&devfreq->lock);
+
+	return min3(devfreq->scaling_max_freq, devfreq->max_freq,
+		1000 * (unsigned long)dev_pm_qos_read_value(
+				devfreq->dev.parent,
+				DEV_PM_QOS_MAX_FREQUENCY));
+}
+
 /**
  * devfreq_get_freq_level() - Lookup freq_table for the frequency
  * @devfreq:	the devfreq instance
  * @freq:	the target frequency
  */
@@ -356,12 +377,12 @@ int update_devfreq(struct devfreq *devfreq)
 	 *
 	 * 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 = get_effective_max_freq(devfreq);
+	min_freq = get_effective_min_freq(devfreq);
 
 	if (freq < min_freq) {
 		freq = min_freq;
 		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
 	}
@@ -570,10 +591,37 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
 	mutex_unlock(&devfreq->lock);
 
 	return ret;
 }
 
+static int devfreq_qos_notifier_call(struct devfreq *devfreq)
+{
+	int ret;
+
+	mutex_lock(&devfreq->lock);
+	ret = update_devfreq(devfreq);
+	mutex_unlock(&devfreq->lock);
+
+	return ret;
+}
+
+static int devfreq_qos_min_notifier_call(struct notifier_block *nb,
+					 unsigned long val, void *ptr)
+{
+	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
+
+	return devfreq_qos_notifier_call(devfreq);
+}
+
+static int devfreq_qos_max_notifier_call(struct notifier_block *nb,
+					 unsigned long val, void *ptr)
+{
+	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max);
+
+	return devfreq_qos_notifier_call(devfreq);
+}
+
 /**
  * devfreq_dev_release() - Callback for struct device to release the device.
  * @dev:	the devfreq device
  *
  * Remove devfreq from the list and release its resources.
@@ -592,10 +640,14 @@ static void devfreq_dev_release(struct device *dev)
 	mutex_unlock(&devfreq_list_lock);
 
 	if (devfreq->profile->exit)
 		devfreq->profile->exit(devfreq->dev.parent);
 
+	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
+			DEV_PM_QOS_MAX_FREQUENCY);
+	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
+			DEV_PM_QOS_MIN_FREQUENCY);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
 
 /**
@@ -636,21 +688,40 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		err = -ENOMEM;
 		goto err_out;
 	}
 
 	mutex_init(&devfreq->lock);
-	mutex_lock(&devfreq->lock);
 	devfreq->dev.parent = dev;
 	devfreq->dev.class = devfreq_class;
 	devfreq->dev.release = devfreq_dev_release;
 	devfreq->profile = profile;
 	strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
 	devfreq->previous_freq = profile->initial_freq;
 	devfreq->last_status.current_frequency = profile->initial_freq;
 	devfreq->data = data;
 	devfreq->nb.notifier_call = devfreq_notifier_call;
 
+	/*
+	 * notifier from pm_qos
+	 *
+	 * initialized outside of devfreq->lock to avoid circular warning
+	 * between devfreq->lock and dev_pm_qos_mtx
+	 */
+	devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call;
+	devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call;
+
+	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
+				      DEV_PM_QOS_MIN_FREQUENCY);
+	if (err)
+		goto err_dev;
+
+	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
+				      DEV_PM_QOS_MAX_FREQUENCY);
+	if (err)
+		goto err_dev;
+
+	mutex_lock(&devfreq->lock);
 	if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
 		mutex_unlock(&devfreq->lock);
 		err = set_freq_table(devfreq);
 		if (err < 0)
 			goto err_dev;
@@ -741,10 +812,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	mutex_unlock(&devfreq_list_lock);
 err_devfreq:
 	devfreq_remove_device(devfreq);
 	devfreq = NULL;
 err_dev:
+	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
+				   DEV_PM_QOS_MAX_FREQUENCY);
+	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
+				   DEV_PM_QOS_MIN_FREQUENCY);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL(devfreq_add_device);
@@ -1312,12 +1387,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
 
 static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
 	struct devfreq *df = to_devfreq(dev);
+	ssize_t ret;
+
+	mutex_lock(&df->lock);
+	ret = sprintf(buf, "%lu\n", get_effective_min_freq(df));
+	mutex_unlock(&df->lock);
 
-	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
+	return ret;
 }
 
 static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -1357,12 +1437,17 @@ 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);
+	ssize_t ret;
 
-	return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
+	mutex_lock(&df->lock);
+	ret = sprintf(buf, "%lu\n", get_effective_max_freq(df));
+	mutex_unlock(&df->lock);
+
+	return ret;
 }
 static DEVICE_ATTR_RW(max_freq);
 
 static ssize_t available_frequencies_show(struct device *d,
 					  struct device_attribute *attr,
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 2bae9ed3c783..8b92ccbd1962 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -134,10 +134,12 @@ struct devfreq_dev_profile {
  * @total_trans:	Number of devfreq transitions
  * @trans_table:	Statistics of devfreq transitions
  * @time_in_state:	Statistics of devfreq states
  * @last_stat_updated:	The last time stat updated
  * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
+ * @nb_min:		Notifier block for DEV_PM_QOS_MIN_FREQUENCY
+ * @nb_max:		Notifier block for DEV_PM_QOS_MAX_FREQUENCY
  *
  * This structure stores the devfreq information for a give device.
  *
  * Note that when a governor accesses entries in struct devfreq in its
  * functions except for the context of callbacks defined in struct
@@ -176,10 +178,13 @@ struct devfreq {
 	unsigned int *trans_table;
 	unsigned long *time_in_state;
 	unsigned long last_stat_updated;
 
 	struct srcu_notifier_head transition_notifier_list;
+
+	struct notifier_block nb_min;
+	struct notifier_block nb_max;
 };
 
 struct devfreq_freqs {
 	unsigned long old;
 	unsigned long new;
-- 
2.17.1

Re: [PATCH v3 1/2] PM / devfreq: Add dev_pm_qos support

[Chanwoo Choi]

Hi,

On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
> Add dev_pm_qos notifies to devfreq core in order to support frequency
> limits via the dev_pm_qos_add_request.
> 
> Unlike the rest of devfreq the dev_pm_qos frequency is measured in Khz,
> this is consistent with current dev_pm_qos usage for cpufreq and
> allows frequencies above 2Ghz (pm_qos expresses limits as s32).
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 95 ++++++++++++++++++++++++++++++++++++---
>  include/linux/devfreq.h   |  5 +++
>  2 files changed, 95 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 784c08e4f931..58deffa52a37 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -22,10 +22,11 @@
>  #include <linux/platform_device.h>
>  #include <linux/list.h>
>  #include <linux/printk.h>
>  #include <linux/hrtimer.h>
>  #include <linux/of.h>
> +#include <linux/pm_qos.h>
>  #include "governor.h"
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/devfreq.h>
>  
> @@ -96,10 +97,30 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  		dev_pm_opp_put(opp);
>  
>  	return max_freq;
>  }
>  
> +static unsigned long get_effective_min_freq(struct devfreq *devfreq)

I'm not sure that 'effective' expression is correct.
From this function, the devfreq can get the final target frequency.

I think that we need to use the more correct expression
to give the meaning of function as following:

get_min_freq
get_max_freq

or

get_biggest_min_freq
get_biggest_max_freq

or

others if there are more proper function name.


> +{
> +	lockdep_assert_held(&devfreq->lock);
> +
> +	return max3(devfreq->scaling_min_freq, devfreq->min_freq,
> +		1000 * (unsigned long)dev_pm_qos_read_value(

I prefer to use 'KHZ' defintion instead of 1000.
The constant definition is more easy to inform
the correct meaning of constant.

> +				devfreq->dev.parent,
> +				DEV_PM_QOS_MIN_FREQUENCY));
> +}
> +
> +static unsigned long get_effective_max_freq(struct devfreq *devfreq)

ditto.

> +{
> +	lockdep_assert_held(&devfreq->lock);
> +
> +	return min3(devfreq->scaling_max_freq, devfreq->max_freq,
> +		1000 * (unsigned long)dev_pm_qos_read_value(

ditto.

> +				devfreq->dev.parent,
> +				DEV_PM_QOS_MAX_FREQUENCY));
> +}
> +
>  /**
>   * devfreq_get_freq_level() - Lookup freq_table for the frequency
>   * @devfreq:	the devfreq instance
>   * @freq:	the target frequency
>   */
> @@ -356,12 +377,12 @@ int update_devfreq(struct devfreq *devfreq)
>  	 *
>  	 * 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 = get_effective_max_freq(devfreq);
> +	min_freq = get_effective_min_freq(devfreq);
>  
>  	if (freq < min_freq) {
>  		freq = min_freq;
>  		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>  	}
> @@ -570,10 +591,37 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>  	mutex_unlock(&devfreq->lock);
>  
>  	return ret;
>  }
>  
> +static int devfreq_qos_notifier_call(struct devfreq *devfreq)
> +{
> +	int ret;
> +
> +	mutex_lock(&devfreq->lock);
> +	ret = update_devfreq(devfreq);
> +	mutex_unlock(&devfreq->lock);
> +
> +	return ret;
> +}
> +
> +static int devfreq_qos_min_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_min);
> +
> +	return devfreq_qos_notifier_call(devfreq);
> +}
> +
> +static int devfreq_qos_max_notifier_call(struct notifier_block *nb,
> +					 unsigned long val, void *ptr)
> +{
> +	struct devfreq *devfreq = container_of(nb, struct devfreq, nb_max);
> +
> +	return devfreq_qos_notifier_call(devfreq);
> +}

Although the all functions has not the function description in devfreq.c,
You need to add the function description for newly added functions.

> +
>  /**
>   * devfreq_dev_release() - Callback for struct device to release the device.
>   * @dev:	the devfreq device
>   *
>   * Remove devfreq from the list and release its resources.
> @@ -592,10 +640,14 @@ static void devfreq_dev_release(struct device *dev)
>  	mutex_unlock(&devfreq_list_lock);
>  
>  	if (devfreq->profile->exit)
>  		devfreq->profile->exit(devfreq->dev.parent);
>  
> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
> +			DEV_PM_QOS_MAX_FREQUENCY);
> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +			DEV_PM_QOS_MIN_FREQUENCY);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
>  
>  /**
> @@ -636,21 +688,40 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  		err = -ENOMEM;
>  		goto err_out;
>  	}
>  
>  	mutex_init(&devfreq->lock);
> -	mutex_lock(&devfreq->lock);

Basically, I think that it is safe to lock when touch
the variable of the devfreq.

it is not proper way for the dev_pm_qos because 
it breaks the existing locking reason of devfreq's variables. 

>  	devfreq->dev.parent = dev;
>  	devfreq->dev.class = devfreq_class;
>  	devfreq->dev.release = devfreq_dev_release;
>  	devfreq->profile = profile;
>  	strncpy(devfreq->governor_name, governor_name, DEVFREQ_NAME_LEN);
>  	devfreq->previous_freq = profile->initial_freq;
>  	devfreq->last_status.current_frequency = profile->initial_freq;
>  	devfreq->data = data;
>  	devfreq->nb.notifier_call = devfreq_notifier_call;
>  
> +	/*
> +	 * notifier from pm_qos
> +	 *
> +	 * initialized outside of devfreq->lock to avoid circular warning
> +	 * between devfreq->lock and dev_pm_qos_mtx
> +	 */
> +	devfreq->nb_min.notifier_call = devfreq_qos_min_notifier_call;
> +	devfreq->nb_max.notifier_call = devfreq_qos_max_notifier_call;
> +
> +	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +				      DEV_PM_QOS_MIN_FREQUENCY);
> +	if (err)
> +		goto err_dev;
> +
> +	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_max,
> +				      DEV_PM_QOS_MAX_FREQUENCY);
> +	if (err)
> +		goto err_dev;
> +
> +	mutex_lock(&devfreq->lock);
>  	if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
>  		mutex_unlock(&devfreq->lock);
>  		err = set_freq_table(devfreq);
>  		if (err < 0)
>  			goto err_dev;
> @@ -741,10 +812,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	mutex_unlock(&devfreq_list_lock);
>  err_devfreq:
>  	devfreq_remove_device(devfreq);
>  	devfreq = NULL;
>  err_dev:
> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
> +				   DEV_PM_QOS_MAX_FREQUENCY);
> +	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
> +				   DEV_PM_QOS_MIN_FREQUENCY);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL(devfreq_add_device);
> @@ -1312,12 +1387,17 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  
>  static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>  			     char *buf)
>  {
>  	struct devfreq *df = to_devfreq(dev);
> +	ssize_t ret;
> +
> +	mutex_lock(&df->lock);
> +	ret = sprintf(buf, "%lu\n", get_effective_min_freq(df));
> +	mutex_unlock(&df->lock);
>  
> -	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
> +	return ret;
>  }
>  
>  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
> @@ -1357,12 +1437,17 @@ 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);
> +	ssize_t ret;
>  
> -	return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
> +	mutex_lock(&df->lock);
> +	ret = sprintf(buf, "%lu\n", get_effective_max_freq(df));
> +	mutex_unlock(&df->lock);
> +
> +	return ret;
>  }
>  static DEVICE_ATTR_RW(max_freq);
>  
>  static ssize_t available_frequencies_show(struct device *d,
>  					  struct device_attribute *attr,
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 2bae9ed3c783..8b92ccbd1962 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -134,10 +134,12 @@ struct devfreq_dev_profile {
>   * @total_trans:	Number of devfreq transitions
>   * @trans_table:	Statistics of devfreq transitions
>   * @time_in_state:	Statistics of devfreq states
>   * @last_stat_updated:	The last time stat updated
>   * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
> + * @nb_min:		Notifier block for DEV_PM_QOS_MIN_FREQUENCY
> + * @nb_max:		Notifier block for DEV_PM_QOS_MAX_FREQUENCY
>   *
>   * This structure stores the devfreq information for a give device.
>   *
>   * Note that when a governor accesses entries in struct devfreq in its
>   * functions except for the context of callbacks defined in struct
> @@ -176,10 +178,13 @@ struct devfreq {
>  	unsigned int *trans_table;
>  	unsigned long *time_in_state;
>  	unsigned long last_stat_updated;
>  
>  	struct srcu_notifier_head transition_notifier_list;
> +
> +	struct notifier_block nb_min;
> +	struct notifier_block nb_max;
>  };
>  
>  struct devfreq_freqs {
>  	unsigned long old;
>  	unsigned long new;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 1/2] PM / devfreq: Add dev_pm_qos support

[Leonard Crestez]

On 21.08.2019 04:40, Chanwoo Choi wrote:

> On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
>> Add dev_pm_qos notifies to devfreq core in order to support frequency
>> limits via the dev_pm_qos_add_request.
>>
>> +static unsigned long get_effective_min_freq(struct devfreq *devfreq)
> 
> I'm not sure that 'effective' expression is correct.
>  From this function, the devfreq can get the final target frequency.
> 
> I think that we need to use the more correct expression
> to give the meaning of function as following:
> 
> get_min_freq
> get_max_freq

OK, will rename to get_min_freq and get_max_freq

>> @@ -636,21 +688,40 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   		err = -ENOMEM;
>>   		goto err_out;
>>   	}
>>   
>>   	mutex_init(&devfreq->lock);
>> -	mutex_lock(&devfreq->lock);
> 
> Basically, I think that it is safe to lock when touch
> the variable of the devfreq.
> 
> it is not proper way for the dev_pm_qos because
> it breaks the existing locking reason of devfreq's variables.

I don't understand what you mean. I'm initializing some stuff outside 
the lock to avoid circular lock warning between:

(&devfreq->lock){+.+.}, at: devfreq_qos_min_notifier_call+0x24/0x48
(&(n)->rwsem){++++}, at: blocking_notifier_call_chain+0x3c/0x78

In general you don't need to lock an object while initializing except 
after it becomes accessible from the outside so devfreq_add_device 
doesn't need to take the lock so early.

The QOS notifiers are registered on the parent device so in theory it's 
possible for somebody to add QOS requests while devfreq_add_device is 
executing. Maybe notifier registration should be moved at the end after 
unlock?

--
Regards,
Leonard

Re: [PATCH v3 1/2] PM / devfreq: Add dev_pm_qos support

[Chanwoo Choi]

On 19. 8. 21. 오후 10:00, Leonard Crestez wrote:
> On 21.08.2019 04:40, Chanwoo Choi wrote:
> 
>> On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
>>> Add dev_pm_qos notifies to devfreq core in order to support frequency
>>> limits via the dev_pm_qos_add_request.
>>>
>>> +static unsigned long get_effective_min_freq(struct devfreq *devfreq)
>>
>> I'm not sure that 'effective' expression is correct.
>>  From this function, the devfreq can get the final target frequency.
>>
>> I think that we need to use the more correct expression
>> to give the meaning of function as following:
>>
>> get_min_freq
>> get_max_freq
> 
> OK, will rename to get_min_freq and get_max_freq
> 
>>> @@ -636,21 +688,40 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>>   		err = -ENOMEM;
>>>   		goto err_out;
>>>   	}
>>>   
>>>   	mutex_init(&devfreq->lock);
>>> -	mutex_lock(&devfreq->lock);
>>
>> Basically, I think that it is safe to lock when touch
>> the variable of the devfreq.
>>
>> it is not proper way for the dev_pm_qos because
>> it breaks the existing locking reason of devfreq's variables.
> 
> I don't understand what you mean. I'm initializing some stuff outside 
> the lock to avoid circular lock warning between:
> 
> (&devfreq->lock){+.+.}, at: devfreq_qos_min_notifier_call+0x24/0x48
> (&(n)->rwsem){++++}, at: blocking_notifier_call_chain+0x3c/0x78
> 
> In general you don't need to lock an object while initializing except 
> after it becomes accessible from the outside so devfreq_add_device 
> doesn't need to take the lock so early.
> 
> The QOS notifiers are registered on the parent device so in theory it's 
> possible for somebody to add QOS requests while devfreq_add_device is 
> executing. Maybe notifier registration should be moved at the end after 
> unlock?

I think that it is more clear to add notifier
after mutex_unlock(&devfreq->lock) if there are no any issue.

> 
> --
> Regards,
> Leonard
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Leonard Crestez]

Now that devfreq supports dev_pm_qos requests we can use them to handle
the min/max_freq values set by userspace in sysfs, similar to cpufreq.

Since dev_pm_qos handles frequencies as kHz this change reduces the
precision of min_freq and max_freq. This shouldn't introduce problems
because frequencies which are not an integer number of kHz are likely
not an integer number of Hz either.

Try to ensure compatibilitity by rounding min values down and rounding
max values up.

Simplify the {min,max}_freq_store code by setting "null" values of 0 and
MAX_S32 respectively instead of clamping to what freq tables are
actually supported. Values are already automatically clamped on
readback.

Also simplify by droping the limitation that userspace min_freq must be
lower than userspace max_freq, it is already documented that max_freq
takes precedence.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
 drivers/devfreq/devfreq.c | 79 ++++++++++++++++-----------------------
 include/linux/devfreq.h   |  9 +++--
 2 files changed, 38 insertions(+), 50 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 58deffa52a37..687deadd08ed 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -101,21 +101,21 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
 
 static unsigned long get_effective_min_freq(struct devfreq *devfreq)
 {
 	lockdep_assert_held(&devfreq->lock);
 
-	return max3(devfreq->scaling_min_freq, devfreq->min_freq,
+	return max(devfreq->scaling_min_freq,
 		1000 * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent,
 				DEV_PM_QOS_MIN_FREQUENCY));
 }
 
 static unsigned long get_effective_max_freq(struct devfreq *devfreq)
 {
 	lockdep_assert_held(&devfreq->lock);
 
-	return min3(devfreq->scaling_max_freq, devfreq->max_freq,
+	return min(devfreq->scaling_max_freq,
 		1000 * (unsigned long)dev_pm_qos_read_value(
 				devfreq->dev.parent,
 				DEV_PM_QOS_MAX_FREQUENCY));
 }
 
@@ -644,10 +644,12 @@ static void devfreq_dev_release(struct device *dev)
 
 	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
 			DEV_PM_QOS_MAX_FREQUENCY);
 	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
 			DEV_PM_QOS_MIN_FREQUENCY);
+	dev_pm_qos_remove_request(&devfreq->max_freq_req);
+	dev_pm_qos_remove_request(&devfreq->min_freq_req);
 	mutex_destroy(&devfreq->lock);
 	kfree(devfreq);
 }
 
 /**
@@ -698,10 +700,19 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->previous_freq = profile->initial_freq;
 	devfreq->last_status.current_frequency = profile->initial_freq;
 	devfreq->data = data;
 	devfreq->nb.notifier_call = devfreq_notifier_call;
 
+	err = dev_pm_qos_add_request(dev, &devfreq->min_freq_req,
+				     DEV_PM_QOS_MIN_FREQUENCY, 0);
+	if (err < 0)
+		goto err_dev;
+	err = dev_pm_qos_add_request(dev, &devfreq->max_freq_req,
+				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
+	if (err < 0)
+		goto err_dev;
+
 	/*
 	 * notifier from pm_qos
 	 *
 	 * initialized outside of devfreq->lock to avoid circular warning
 	 * between devfreq->lock and dev_pm_qos_mtx
@@ -732,19 +743,17 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	if (!devfreq->scaling_min_freq) {
 		mutex_unlock(&devfreq->lock);
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->min_freq = devfreq->scaling_min_freq;
 
 	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
 	if (!devfreq->scaling_max_freq) {
 		mutex_unlock(&devfreq->lock);
 		err = -EINVAL;
 		goto err_dev;
 	}
-	devfreq->max_freq = devfreq->scaling_max_freq;
 
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
 	atomic_set(&devfreq->suspend_count, 0);
 
 	dev_set_name(&devfreq->dev, "devfreq%d",
@@ -816,10 +825,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
 err_dev:
 	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
 				   DEV_PM_QOS_MAX_FREQUENCY);
 	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
 				   DEV_PM_QOS_MIN_FREQUENCY);
+	if (dev_pm_qos_request_active(&devfreq->max_freq_req))
+		dev_pm_qos_remove_request(&devfreq->max_freq_req);
+	if (dev_pm_qos_request_active(&devfreq->min_freq_req))
+		dev_pm_qos_remove_request(&devfreq->min_freq_req);
 	kfree(devfreq);
 err_out:
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL(devfreq_add_device);
@@ -1358,33 +1371,20 @@ 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;
+	if (value)
+		value = value / 1000;
+	else
+		value = 0;
 
-		/* 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];
-	}
+	ret = dev_pm_qos_update_request(&df->min_freq_req, value);
+	if (ret < 0)
+		return ret;
 
-	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)
 {
@@ -1407,33 +1407,20 @@ static ssize_t max_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->min_freq) {
-			ret = -EINVAL;
-			goto unlock;
-		}
-	} else {
-		unsigned long *freq_table = df->profile->freq_table;
+	if (value)
+		value = DIV_ROUND_UP(value, 1000);
+	else
+		value = S32_MAX;
 
-		/* 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];
-	}
+	ret = dev_pm_qos_update_request(&df->max_freq_req, value);
+	if (ret < 0)
+		return ret;
 
-	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)
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 8b92ccbd1962..d2c5bb7add0a 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -11,10 +11,11 @@
 #define __LINUX_DEVFREQ_H__
 
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/pm_opp.h>
+#include <linux/pm_qos.h>
 
 #define DEVFREQ_NAME_LEN 16
 
 /* DEVFREQ governor name */
 #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
@@ -121,12 +122,12 @@ struct devfreq_dev_profile {
  *		devfreq.nb to the corresponding register notifier call chain.
  * @work:	delayed work for load monitoring.
  * @previous_freq:	previously configured frequency value.
  * @data:	Private data of the governor. The devfreq framework does not
  *		touch this.
- * @min_freq:	Limit minimum frequency requested by user (0: none)
- * @max_freq:	Limit maximum frequency requested by user (0: none)
+ * @min_freq_req:	Limit minimum frequency requested by user (0: none)
+ * @max_freq_req:	Limit maximum frequency requested by user (0: none)
  * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
  * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
  * @stop_polling:	 devfreq polling status of a device.
  * @suspend_freq:	 frequency of a device set during suspend phase.
  * @resume_freq:	 frequency of a device set in resume phase.
@@ -161,12 +162,12 @@ struct devfreq {
 	unsigned long previous_freq;
 	struct devfreq_dev_status last_status;
 
 	void *data; /* private data for governors */
 
-	unsigned long min_freq;
-	unsigned long max_freq;
+	struct dev_pm_qos_request min_freq_req;
+	struct dev_pm_qos_request max_freq_req;
 	unsigned long scaling_min_freq;
 	unsigned long scaling_max_freq;
 	bool stop_polling;
 
 	unsigned long suspend_freq;
-- 
2.17.1

Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Chanwoo Choi]

On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
> Now that devfreq supports dev_pm_qos requests we can use them to handle
> the min/max_freq values set by userspace in sysfs, similar to cpufreq.
> 
> Since dev_pm_qos handles frequencies as kHz this change reduces the
> precision of min_freq and max_freq. This shouldn't introduce problems
> because frequencies which are not an integer number of kHz are likely
> not an integer number of Hz either.
> 
> Try to ensure compatibilitity by rounding min values down and rounding
> max values up.
> 
> Simplify the {min,max}_freq_store code by setting "null" values of 0 and
> MAX_S32 respectively instead of clamping to what freq tables are
> actually supported. Values are already automatically clamped on
> readback.
> 
> Also simplify by droping the limitation that userspace min_freq must be
> lower than userspace max_freq, it is already documented that max_freq
> takes precedence.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
>  drivers/devfreq/devfreq.c | 79 ++++++++++++++++-----------------------
>  include/linux/devfreq.h   |  9 +++--
>  2 files changed, 38 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 58deffa52a37..687deadd08ed 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -101,21 +101,21 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>  
>  static unsigned long get_effective_min_freq(struct devfreq *devfreq)
>  {
>  	lockdep_assert_held(&devfreq->lock);
>  
> -	return max3(devfreq->scaling_min_freq, devfreq->min_freq,
> +	return max(devfreq->scaling_min_freq,
>  		1000 * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent,
>  				DEV_PM_QOS_MIN_FREQUENCY));
>  }
>  
>  static unsigned long get_effective_max_freq(struct devfreq *devfreq)
>  {
>  	lockdep_assert_held(&devfreq->lock);
>  
> -	return min3(devfreq->scaling_max_freq, devfreq->max_freq,
> +	return min(devfreq->scaling_max_freq,
>  		1000 * (unsigned long)dev_pm_qos_read_value(
>  				devfreq->dev.parent,
>  				DEV_PM_QOS_MAX_FREQUENCY));
>  }
>  
> @@ -644,10 +644,12 @@ static void devfreq_dev_release(struct device *dev)
>  
>  	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
>  			DEV_PM_QOS_MAX_FREQUENCY);
>  	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
>  			DEV_PM_QOS_MIN_FREQUENCY);
> +	dev_pm_qos_remove_request(&devfreq->max_freq_req);
> +	dev_pm_qos_remove_request(&devfreq->min_freq_req);
>  	mutex_destroy(&devfreq->lock);
>  	kfree(devfreq);
>  }
>  
>  /**
> @@ -698,10 +700,19 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->previous_freq = profile->initial_freq;
>  	devfreq->last_status.current_frequency = profile->initial_freq;
>  	devfreq->data = data;
>  	devfreq->nb.notifier_call = devfreq_notifier_call;
>  
> +	err = dev_pm_qos_add_request(dev, &devfreq->min_freq_req,
> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
> +	if (err < 0)
> +		goto err_dev;
> +	err = dev_pm_qos_add_request(dev, &devfreq->max_freq_req,
> +				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
> +	if (err < 0)
> +		goto err_dev;
> +

Please move them under dev_pm_qos_add_notifier()
because it seems that it request the qos without qos_notifier.

>  	/*
>  	 * notifier from pm_qos
>  	 *
>  	 * initialized outside of devfreq->lock to avoid circular warning
>  	 * between devfreq->lock and dev_pm_qos_mtx
> @@ -732,19 +743,17 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	if (!devfreq->scaling_min_freq) {
>  		mutex_unlock(&devfreq->lock);
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->min_freq = devfreq->scaling_min_freq;
>  
>  	devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>  	if (!devfreq->scaling_max_freq) {
>  		mutex_unlock(&devfreq->lock);
>  		err = -EINVAL;
>  		goto err_dev;
>  	}
> -	devfreq->max_freq = devfreq->scaling_max_freq;
>  
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
>  
>  	dev_set_name(&devfreq->dev, "devfreq%d",
> @@ -816,10 +825,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  err_dev:
>  	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_max,
>  				   DEV_PM_QOS_MAX_FREQUENCY);
>  	dev_pm_qos_remove_notifier(devfreq->dev.parent, &devfreq->nb_min,
>  				   DEV_PM_QOS_MIN_FREQUENCY);
> +	if (dev_pm_qos_request_active(&devfreq->max_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->max_freq_req);
> +	if (dev_pm_qos_request_active(&devfreq->min_freq_req))
> +		dev_pm_qos_remove_request(&devfreq->min_freq_req);
>  	kfree(devfreq);
>  err_out:
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL(devfreq_add_device);
> @@ -1358,33 +1371,20 @@ 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;
> -		}

Actually, the user can input the value they want.
So, the above code is not necessary because the devfreq->scaling_max_freq
is never overflow from supported maximum frequency. The devfreq->scaling_max_freq
is based on OPP entries from DT.

But, if we replace the existing request way of devfreq-cooling.c
with dev_pm_qos, devfreq->scaling_max_freq doesn't guarantee
the supported maximum frequency.

We need to keep the supported min_freq/max_freq value without dev_pm_qos
requirement because the dev_pm_qos requirement might have the overflow value.
the dev_pm_qos doesn't know the supported minimum and maximum frequency
of devfreq device.


> -	} else {
> -		unsigned long *freq_table = df->profile->freq_table;
> +	if (value)
> +		value = value / 1000;

Better to use KHZ definition instead of 1000 as I commented on patch1.

> +	else
> +		value = 0;
>  
> -		/* 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];
> -	}
> +	ret = dev_pm_qos_update_request(&df->min_freq_req, value);
> +	if (ret < 0)
> +		return ret;
>  
> -	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)
>  {
> @@ -1407,33 +1407,20 @@ static ssize_t max_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->min_freq) {
> -			ret = -EINVAL;
> -			goto unlock;

> -		}
> -	} else {
> -		unsigned long *freq_table = df->profile->freq_table;
> +	if (value)
> +		value = DIV_ROUND_UP(value, 1000);
> +	else
> +		value = S32_MAX;
>  
> -		/* 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];
> -	}
> +	ret = dev_pm_qos_update_request(&df->max_freq_req, value);
> +	if (ret < 0)
> +		return ret;
>  
> -	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)
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 8b92ccbd1962..d2c5bb7add0a 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -11,10 +11,11 @@
>  #define __LINUX_DEVFREQ_H__
>  
>  #include <linux/device.h>
>  #include <linux/notifier.h>
>  #include <linux/pm_opp.h>
> +#include <linux/pm_qos.h>
>  
>  #define DEVFREQ_NAME_LEN 16
>  
>  /* DEVFREQ governor name */
>  #define DEVFREQ_GOV_SIMPLE_ONDEMAND	"simple_ondemand"
> @@ -121,12 +122,12 @@ struct devfreq_dev_profile {
>   *		devfreq.nb to the corresponding register notifier call chain.
>   * @work:	delayed work for load monitoring.
>   * @previous_freq:	previously configured frequency value.
>   * @data:	Private data of the governor. The devfreq framework does not
>   *		touch this.
> - * @min_freq:	Limit minimum frequency requested by user (0: none)
> - * @max_freq:	Limit maximum frequency requested by user (0: none)
> + * @min_freq_req:	Limit minimum frequency requested by user (0: none)
> + * @max_freq_req:	Limit maximum frequency requested by user (0: none)
>   * @scaling_min_freq:	Limit minimum frequency requested by OPP interface
>   * @scaling_max_freq:	Limit maximum frequency requested by OPP interface
>   * @stop_polling:	 devfreq polling status of a device.
>   * @suspend_freq:	 frequency of a device set during suspend phase.
>   * @resume_freq:	 frequency of a device set in resume phase.
> @@ -161,12 +162,12 @@ struct devfreq {
>  	unsigned long previous_freq;
>  	struct devfreq_dev_status last_status;
>  
>  	void *data; /* private data for governors */
>  
> -	unsigned long min_freq;
> -	unsigned long max_freq;
> +	struct dev_pm_qos_request min_freq_req;
> +	struct dev_pm_qos_request max_freq_req;
>  	unsigned long scaling_min_freq;
>  	unsigned long scaling_max_freq;
>  	bool stop_polling;
>  
>  	unsigned long suspend_freq;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Leonard Crestez]

On 21.08.2019 05:02, Chanwoo Choi wrote:
> On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
>> Now that devfreq supports dev_pm_qos requests we can use them to handle
>> the min/max_freq values set by userspace in sysfs, similar to cpufreq.
>>
>> Since dev_pm_qos handles frequencies as kHz this change reduces the
>> precision of min_freq and max_freq. This shouldn't introduce problems
>> because frequencies which are not an integer number of kHz are likely
>> not an integer number of Hz either.
>>
>> Try to ensure compatibilitity by rounding min values down and rounding
>> max values up.
>>
>> Simplify the {min,max}_freq_store code by setting "null" values of 0 and
>> MAX_S32 respectively instead of clamping to what freq tables are
>> actually supported. Values are already automatically clamped on
>> readback.
>>
>> Also simplify by droping the limitation that userspace min_freq must be
>> lower than userspace max_freq, it is already documented that max_freq
>> takes precedence.
>>
>> @@ -1358,33 +1371,20 @@ 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;
>> -		}
> 
> Actually, the user can input the value they want.
> So, the above code is not necessary because the devfreq->scaling_max_freq
> is never overflow from supported maximum frequency. The devfreq->scaling_max_freq
> is based on OPP entries from DT.
> 
> But, if we replace the existing request way of devfreq-cooling.c
> with dev_pm_qos, devfreq->scaling_max_freq doesn't guarantee
> the supported maximum frequency. >
> We need to keep the supported min_freq/max_freq value without dev_pm_qos
> requirement because the dev_pm_qos requirement might have the overflow value.
> the dev_pm_qos doesn't know the supported minimum and maximum frequency
> of devfreq device.

I'm not sure I understand what you mean. My patch allows user to set 
entirely arbitrary min/max rates and this is good because we already 
have a well-defined way to handle this: max overrides min.

The scaling_min_freq and scaling_max_freq variables can just be kept 
around indefinitely no matter what happens to thermal. They're just a 
cache for dev_pm_opp_find_freq_ceil and dev_pm_opp_find_freq_floor.

BTW: I noticed that scaling_min_freq and scaling_max_freq are updated in 
devfreq_notifier_call but devfreq->nb is not registered by default, only 
when a driver requests it explicitly. Is there a reason for this?
Because I'd call dev_pm_opp_register_notifier inside devfreq_add_device 
and remove all the devfreq_register/unregister_notifier APIs.

--
Regards,
Leonard

Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Chanwoo Choi]

On 19. 8. 21. 오후 10:03, Leonard Crestez wrote:
> On 21.08.2019 05:02, Chanwoo Choi wrote:
>> On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
>>> Now that devfreq supports dev_pm_qos requests we can use them to handle
>>> the min/max_freq values set by userspace in sysfs, similar to cpufreq.
>>>
>>> Since dev_pm_qos handles frequencies as kHz this change reduces the
>>> precision of min_freq and max_freq. This shouldn't introduce problems
>>> because frequencies which are not an integer number of kHz are likely
>>> not an integer number of Hz either.
>>>
>>> Try to ensure compatibilitity by rounding min values down and rounding
>>> max values up.
>>>
>>> Simplify the {min,max}_freq_store code by setting "null" values of 0 and
>>> MAX_S32 respectively instead of clamping to what freq tables are
>>> actually supported. Values are already automatically clamped on
>>> readback.
>>>
>>> Also simplify by droping the limitation that userspace min_freq must be
>>> lower than userspace max_freq, it is already documented that max_freq
>>> takes precedence.
>>>
>>> @@ -1358,33 +1371,20 @@ 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;
>>> -		}
>>
>> Actually, the user can input the value they want.
>> So, the above code is not necessary because the devfreq->scaling_max_freq
>> is never overflow from supported maximum frequency. The devfreq->scaling_max_freq
>> is based on OPP entries from DT.
>>
>> But, if we replace the existing request way of devfreq-cooling.c
>> with dev_pm_qos, devfreq->scaling_max_freq doesn't guarantee
>> the supported maximum frequency. >
>> We need to keep the supported min_freq/max_freq value without dev_pm_qos
>> requirement because the dev_pm_qos requirement might have the overflow value.
>> the dev_pm_qos doesn't know the supported minimum and maximum frequency
>> of devfreq device.
> 
> I'm not sure I understand what you mean. My patch allows user to set 
> entirely arbitrary min/max rates and this is good because we already 
> have a well-defined way to handle this: max overrides min.
> 
> The scaling_min_freq and scaling_max_freq variables can just be kept 
> around indefinitely no matter what happens to thermal. They're just a 
> cache for dev_pm_opp_find_freq_ceil and dev_pm_opp_find_freq_floor.

This patch doesn't check the range of input value
with the supported frequencies of devfreq device.

For example,
The devfreq0 device has the following available frequencies
100000000 200000000 300000000

and then user enters the 500000000 as following:
echo 500000000 > /sys/class/devfreq/devfreq0/min_freq

In result, get_effective_min_freq() will return the 500000000.
It is wrong value. it show the unsupported frequency through
min_freq sysfs path.

- devfreq->scaling_min_freq is 100000000,                                   
- 1000 * (unsigned long)dev_pm_qos_read_value(devfreq->dev.parent,
  DEV_PM_QOS_MIN_FREQUENCY)); is 500000000


> 
> BTW: I noticed that scaling_min_freq and scaling_max_freq are updated in 
> devfreq_notifier_call but devfreq->nb is not registered by default, only 
> when a driver requests it explicitly. Is there a reason for this?

Initial version of devfreq has not forced to use the OPP interface 
as the mandatory. So, it just provides the external function
devfreq_register_opp_notifier.

But,
We can call 'devfreq_register_opp_notifier' during devfreq_add_device()
because the OPP interface is mandatory for devfreq.

> Because I'd call dev_pm_opp_register_notifier inside devfreq_add_device 
> and remove all the devfreq_register/unregister_notifier APIs.

OK.

> 
> --
> Regards,
> Leonard
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Leonard Crestez]

On 8/22/2019 1:03 PM, Chanwoo Choi wrote:
> On 19. 8. 21. 오후 10:03, Leonard Crestez wrote:
>> On 21.08.2019 05:02, Chanwoo Choi wrote:
>>> On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
>>>> Now that devfreq supports dev_pm_qos requests we can use them to handle
>>>> the min/max_freq values set by userspace in sysfs, similar to cpufreq.
>>>>
>>>> Since dev_pm_qos handles frequencies as kHz this change reduces the
>>>> precision of min_freq and max_freq. This shouldn't introduce problems
>>>> because frequencies which are not an integer number of kHz are likely
>>>> not an integer number of Hz either.
>>>>
>>>> Try to ensure compatibilitity by rounding min values down and rounding
>>>> max values up.
>>>>
>>>> Simplify the {min,max}_freq_store code by setting "null" values of 0 and
>>>> MAX_S32 respectively instead of clamping to what freq tables are
>>>> actually supported. Values are already automatically clamped on
>>>> readback.
>>>>
>>>> Also simplify by droping the limitation that userspace min_freq must be
>>>> lower than userspace max_freq, it is already documented that max_freq
>>>> takes precedence.
>>>>
>>>> @@ -1358,33 +1371,20 @@ 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;
>>>> -		}
>>>
>>> Actually, the user can input the value they want.
>>> So, the above code is not necessary because the devfreq->scaling_max_freq
>>> is never overflow from supported maximum frequency. The devfreq->scaling_max_freq
>>> is based on OPP entries from DT.
>>>
>>> But, if we replace the existing request way of devfreq-cooling.c
>>> with dev_pm_qos, devfreq->scaling_max_freq doesn't guarantee
>>> the supported maximum frequency. >
>>> We need to keep the supported min_freq/max_freq value without dev_pm_qos
>>> requirement because the dev_pm_qos requirement might have the overflow value.
>>> the dev_pm_qos doesn't know the supported minimum and maximum frequency
>>> of devfreq device.
>>
>> I'm not sure I understand what you mean. My patch allows user to set
>> entirely arbitrary min/max rates and this is good because we already
>> have a well-defined way to handle this: max overrides min.
>>
>> The scaling_min_freq and scaling_max_freq variables can just be kept
>> around indefinitely no matter what happens to thermal. They're just a
>> cache for dev_pm_opp_find_freq_ceil and dev_pm_opp_find_freq_floor.
> 
> This patch doesn't check the range of input value
> with the supported frequencies of devfreq device.
> 
> For example,
> The devfreq0 device has the following available frequencies
> 100000000 200000000 300000000
> 
> and then user enters the 500000000 as following:
> echo 500000000 > /sys/class/devfreq/devfreq0/min_freq
> 
> In result, get_effective_min_freq() will return the 500000000.
> It is wrong value. it show the unsupported frequency through
> min_freq sysfs path.

Through dev_pm_qos devices can also ask for a freq higher than the 
maximum OPP and unlike sysfs there is no way to reject such requests, 
instead PM qos claims it's based on "best effort".

There are many requests in the kernel for "PM_QOS_CPU_DMA_LATENCY 0" and 
I think that DEV_PM_QOS_MIN_FREQUENCY, MAX_S32 is a reasonable way for a 
device to request "max performance" from devfreq.

Rejecting min > max is complicated (what happens to rejected requests 
when max goes back up?) and I think it's better to just stick with a 
"max overrides min" policy since it can already deal with conflicts.

Do you have a usecase for rejecting out-of-range min_freq from 
userspace? cpufreq also accepts arbitrary min/max values and handles them.

In theory pm_qos could be extended to differentiate between "soft" 
requests (current behavior) and "hard" requests which return errors if 
they can't be guaranteed but this is far out of scope.

--
Regards,
Leonard

Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Chanwoo Choi]

On 19. 8. 22. 오후 7:58, Leonard Crestez wrote:
> On 8/22/2019 1:03 PM, Chanwoo Choi wrote:
>> On 19. 8. 21. 오후 10:03, Leonard Crestez wrote:
>>> On 21.08.2019 05:02, Chanwoo Choi wrote:
>>>> On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
>>>>> Now that devfreq supports dev_pm_qos requests we can use them to handle
>>>>> the min/max_freq values set by userspace in sysfs, similar to cpufreq.
>>>>>
>>>>> Since dev_pm_qos handles frequencies as kHz this change reduces the
>>>>> precision of min_freq and max_freq. This shouldn't introduce problems
>>>>> because frequencies which are not an integer number of kHz are likely
>>>>> not an integer number of Hz either.
>>>>>
>>>>> Try to ensure compatibilitity by rounding min values down and rounding
>>>>> max values up.
>>>>>
>>>>> Simplify the {min,max}_freq_store code by setting "null" values of 0 and
>>>>> MAX_S32 respectively instead of clamping to what freq tables are
>>>>> actually supported. Values are already automatically clamped on
>>>>> readback.
>>>>>
>>>>> Also simplify by droping the limitation that userspace min_freq must be
>>>>> lower than userspace max_freq, it is already documented that max_freq
>>>>> takes precedence.
>>>>>
>>>>> @@ -1358,33 +1371,20 @@ 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;
>>>>> -		}
>>>>
>>>> Actually, the user can input the value they want.
>>>> So, the above code is not necessary because the devfreq->scaling_max_freq
>>>> is never overflow from supported maximum frequency. The devfreq->scaling_max_freq
>>>> is based on OPP entries from DT.
>>>>
>>>> But, if we replace the existing request way of devfreq-cooling.c
>>>> with dev_pm_qos, devfreq->scaling_max_freq doesn't guarantee
>>>> the supported maximum frequency. >
>>>> We need to keep the supported min_freq/max_freq value without dev_pm_qos
>>>> requirement because the dev_pm_qos requirement might have the overflow value.
>>>> the dev_pm_qos doesn't know the supported minimum and maximum frequency
>>>> of devfreq device.
>>>
>>> I'm not sure I understand what you mean. My patch allows user to set
>>> entirely arbitrary min/max rates and this is good because we already
>>> have a well-defined way to handle this: max overrides min.
>>>
>>> The scaling_min_freq and scaling_max_freq variables can just be kept
>>> around indefinitely no matter what happens to thermal. They're just a
>>> cache for dev_pm_opp_find_freq_ceil and dev_pm_opp_find_freq_floor.
>>
>> This patch doesn't check the range of input value
>> with the supported frequencies of devfreq device.
>>
>> For example,
>> The devfreq0 device has the following available frequencies
>> 100000000 200000000 300000000
>>
>> and then user enters the 500000000 as following:
>> echo 500000000 > /sys/class/devfreq/devfreq0/min_freq
>>
>> In result, get_effective_min_freq() will return the 500000000.
>> It is wrong value. it show the unsupported frequency through
>> min_freq sysfs path.
> 
> Through dev_pm_qos devices can also ask for a freq higher than the 
> maximum OPP and unlike sysfs there is no way to reject such requests, 
> instead PM qos claims it's based on "best effort".
> 
> There are many requests in the kernel for "PM_QOS_CPU_DMA_LATENCY 0" and 
> I think that DEV_PM_QOS_MIN_FREQUENCY, MAX_S32 is a reasonable way for a 
> device to request "max performance" from devfreq.
> 
> Rejecting min > max is complicated (what happens to rejected requests 
> when max goes back up?) and I think it's better to just stick with a 
> "max overrides min" policy since it can already deal with conflicts.
> 
> Do you have a usecase for rejecting out-of-range min_freq from 
> userspace? cpufreq also accepts arbitrary min/max values and handles them.

I don't mean the rejecting when user enter the out-of-range frequency.
As I commented, user can enter the value they want. But, we should
check the range of input because devfreq have to show the correct supported
frequency through sysfs.

> 
> In theory pm_qos could be extended to differentiate between "soft" 
> requests (current behavior) and "hard" requests which return errors if 
> they can't be guaranteed but this is far out of scope.

I think that you agreed the limitation of dev_pm_qos when entering
or requesting the unsupported frequency.

Until fixing it on dev_pm_qos, it have to be handled on consumer
like devfreq. I think that get_min_freq() and get_max_freq() have
to check the range of return value of dev_pm_qos_read_value().


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Leonard Crestez]

On 22.08.2019 14:06, Chanwoo Choi wrote:
> On 19. 8. 22. 오후 7:58, Leonard Crestez wrote:
>> On 8/22/2019 1:03 PM, Chanwoo Choi wrote:
>>> This patch doesn't check the range of input value
>>> with the supported frequencies of devfreq device.
>>>
>>> For example,
>>> The devfreq0 device has the following available frequencies
>>> 100000000 200000000 300000000
>>>
>>> and then user enters the 500000000 as following:
>>> echo 500000000 > /sys/class/devfreq/devfreq0/min_freq
>>>
>>> In result, get_effective_min_freq() will return the 500000000.
>>> It is wrong value. it show the unsupported frequency through
>>> min_freq sysfs path.
>>
>> Through dev_pm_qos devices can also ask for a freq higher than the
>> maximum OPP and unlike sysfs there is no way to reject such requests,
>> instead PM qos claims it's based on "best effort".
>>
>> There are many requests in the kernel for "PM_QOS_CPU_DMA_LATENCY 0" and
>> I think that DEV_PM_QOS_MIN_FREQUENCY, MAX_S32 is a reasonable way for a
>> device to request "max performance" from devfreq.
>>
>> Rejecting min > max is complicated (what happens to rejected requests
>> when max goes back up?) and I think it's better to just stick with a
>> "max overrides min" policy since it can already deal with conflicts.
>>
>> Do you have a usecase for rejecting out-of-range min_freq from
>> userspace? cpufreq also accepts arbitrary min/max values and handles them.
> 
> I don't mean the rejecting when user enter the out-of-range frequency.
> As I commented, user can enter the value they want. But, we should
> check the range of input because devfreq have to show the correct supported
> frequency through sysfs.

We can avoid showing an out-of-range value in min_freq by printing 
min(min_freq, max_freq).

The max_freq value from pm_qos can still be between OPPs so maybe print 
devfreq_recommended_opp(max_freq, DEVFREQ_FLAG_LEAST_UPPER_BOUND).

>> In theory pm_qos could be extended to differentiate between "soft"
>> requests (current behavior) and "hard" requests which return errors if
>> they can't be guaranteed but this is far out of scope.
> 
> I think that you agreed the limitation of dev_pm_qos when entering
> or requesting the unsupported frequency.

Yes, "best effort qos" is acceptable for now.

Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Chanwoo Choi]

On 19. 8. 22. 오후 9:24, Leonard Crestez wrote:
> On 22.08.2019 14:06, Chanwoo Choi wrote:
>> On 19. 8. 22. 오후 7:58, Leonard Crestez wrote:
>>> On 8/22/2019 1:03 PM, Chanwoo Choi wrote:
>>>> This patch doesn't check the range of input value
>>>> with the supported frequencies of devfreq device.
>>>>
>>>> For example,
>>>> The devfreq0 device has the following available frequencies
>>>> 100000000 200000000 300000000
>>>>
>>>> and then user enters the 500000000 as following:
>>>> echo 500000000 > /sys/class/devfreq/devfreq0/min_freq
>>>>
>>>> In result, get_effective_min_freq() will return the 500000000.
>>>> It is wrong value. it show the unsupported frequency through
>>>> min_freq sysfs path.
>>>
>>> Through dev_pm_qos devices can also ask for a freq higher than the
>>> maximum OPP and unlike sysfs there is no way to reject such requests,
>>> instead PM qos claims it's based on "best effort".
>>>
>>> There are many requests in the kernel for "PM_QOS_CPU_DMA_LATENCY 0" and
>>> I think that DEV_PM_QOS_MIN_FREQUENCY, MAX_S32 is a reasonable way for a
>>> device to request "max performance" from devfreq.
>>>
>>> Rejecting min > max is complicated (what happens to rejected requests
>>> when max goes back up?) and I think it's better to just stick with a
>>> "max overrides min" policy since it can already deal with conflicts.
>>>
>>> Do you have a usecase for rejecting out-of-range min_freq from
>>> userspace? cpufreq also accepts arbitrary min/max values and handles them.
>>
>> I don't mean the rejecting when user enter the out-of-range frequency.
>> As I commented, user can enter the value they want. But, we should
>> check the range of input because devfreq have to show the correct supported
>> frequency through sysfs.
> 
> We can avoid showing an out-of-range value in min_freq by printing 
> min(min_freq, max_freq).

You can check the range of frequency in the get_min_freq()/get_max_freq().
I want to return the meaningful frequency from get_min_freq()/get_max_freq().

Everyone expects get_min_freq()/get_max_freq() functions will retunrn
the supported min/max frequency. If get_min_freq()/get_max_freq()
return the our-of-range frequency, it is not nice and cause the confusion
why these functions return the out-of-range frequency..

Also, if we don't check the return value of dev_pm_qos_read_value(),
the out-of-range frequency of dev_pm_qos_read_value() would be used
on multiple point of devfreq.c. I think that it is not good.

> 
> The max_freq value from pm_qos can still be between OPPs so maybe print 
> devfreq_recommended_opp(max_freq, DEVFREQ_FLAG_LEAST_UPPER_BOUND).
> 
>>> In theory pm_qos could be extended to differentiate between "soft"
>>> requests (current behavior) and "hard" requests which return errors if
>>> they can't be guaranteed but this is far out of scope.
>>
>> I think that you agreed the limitation of dev_pm_qos when entering
>> or requesting the unsupported frequency.
> 
> Yes, "best effort qos" is acceptable for now.
> 

And please don't remove the my previous comment.
Just reply your opinion without any removal.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

[Leonard Crestez]

On 23.08.2019 11:36, Chanwoo Choi wrote:
> On 19. 8. 22. 오후 9:24, Leonard Crestez wrote:
>> On 22.08.2019 14:06, Chanwoo Choi wrote:
>>> On 19. 8. 22. 오후 7:58, Leonard Crestez wrote:
>>>> On 8/22/2019 1:03 PM, Chanwoo Choi wrote:
>>>>> This patch doesn't check the range of input value
>>>>> with the supported frequencies of devfreq device.
>>>>>
>>>>> For example,
>>>>> The devfreq0 device has the following available frequencies
>>>>> 100000000 200000000 300000000
>>>>>
>>>>> and then user enters the 500000000 as following:
>>>>> echo 500000000 > /sys/class/devfreq/devfreq0/min_freq
>>>>>
>>>>> In result, get_effective_min_freq() will return the 500000000.
>>>>> It is wrong value. it show the unsupported frequency through
>>>>> min_freq sysfs path.
>>>>
>>>> Through dev_pm_qos devices can also ask for a freq higher than the
>>>> maximum OPP and unlike sysfs there is no way to reject such requests,
>>>> instead PM qos claims it's based on "best effort".
>>>>
>>>> There are many requests in the kernel for "PM_QOS_CPU_DMA_LATENCY 0" and
>>>> I think that DEV_PM_QOS_MIN_FREQUENCY, MAX_S32 is a reasonable way for a
>>>> device to request "max performance" from devfreq.
>>>>
>>>> Rejecting min > max is complicated (what happens to rejected requests
>>>> when max goes back up?) and I think it's better to just stick with a
>>>> "max overrides min" policy since it can already deal with conflicts.
>>>>
>>>> Do you have a usecase for rejecting out-of-range min_freq from
>>>> userspace? cpufreq also accepts arbitrary min/max values and handles them.
>>>
>>> I don't mean the rejecting when user enter the out-of-range frequency.
>>> As I commented, user can enter the value they want. But, we should
>>> check the range of input because devfreq have to show the correct supported
>>> frequency through sysfs.
>>
>> We can avoid showing an out-of-range value in min_freq by printing
>> min(min_freq, max_freq).
> 
> You can check the range of frequency in the get_min_freq()/get_max_freq().
> I want to return the meaningful frequency from get_min_freq()/get_max_freq().
> 
> Everyone expects get_min_freq()/get_max_freq() functions will retunrn
> the supported min/max frequency. If get_min_freq()/get_max_freq()
> return the our-of-range frequency, it is not nice and cause the confusion
> why these functions return the out-of-range frequency..
> 
> Also, if we don't check the return value of dev_pm_qos_read_value(),
> the out-of-range frequency of dev_pm_qos_read_value() would be used
> on multiple point of devfreq.c. I think that it is not good.

OK, I will try to partially move the min/max conflict logic from 
"update_devfreq" to a "get_freq_range" function and call that from 
show_min_freq/show_max_freq.

>> The max_freq value from pm_qos can still be between OPPs so maybe print
>> devfreq_recommended_opp(max_freq, DEVFREQ_FLAG_LEAST_UPPER_BOUND).
>>
>>>> In theory pm_qos could be extended to differentiate between "soft"
>>>> requests (current behavior) and "hard" requests which return errors if
>>>> they can't be guaranteed but this is far out of scope.
>>>
>>> I think that you agreed the limitation of dev_pm_qos when entering
>>> or requesting the unsupported frequency.
>>
>> Yes, "best effort qos" is acceptable for now.
> 
> And please don't remove the my previous comment.
> Just reply your opinion without any removal.

Sorry, when responding I usually trim sections which are very old or 
where there appears to be broad agreement.

--
Regards,
Leonard