Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [RFC][PATCH 1/3] PM /devfreq: add user frequency limits into
 devfreq struct
To:     Lukasz Luba <lukasz.luba@arm.com>, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org
Cc:     vireshk@kernel.org, rafael@kernel.org, daniel.lezcano@linaro.org,
        Dietmar.Eggemann@arm.com, amitk@kernel.org, rui.zhang@intel.com,
        myungjoo.ham@samsung.com, kyungmin.park@samsung.com
From:   Chanwoo Choi <cw00.choi@samsung.com>
Organization: Samsung Electronics
Message-ID: <ea409e2f-f3ca-437f-d787-7ba793a2c226@samsung.com>
Date:   Wed, 3 Feb 2021 19:11:19 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101
        Thunderbird/59.0
MIME-Version: 1.0
In-Reply-To: <20210126104001.20361-2-lukasz.luba@arm.com>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20210126104001.20361-1-lukasz.luba@arm.com>
        <CGME20210126104217epcas1p349c717ccf0ea4f964153040b48c72352@epcas1p3.samsung.com>
        <20210126104001.20361-2-lukasz.luba@arm.com>
Precedence: bulk

Hi Lukasz,

When accessing the max_freq and min_freq at devfreq-cooling.c,
even if can access 'user_max_freq' and 'lock' by using the 'devfreq' instance,
I think that the direct access of variables (lock/user_max_freq/user_min_freq)
of struct devfreq are not good.

Instead, how about using the 'DEVFREQ_TRANSITION_NOTIFIER'
notification with following changes of 'struct devfreq_freq'?
Also, need to add codes into devfreq_set_target() for initializing
'new_max_freq' and 'new_min_freq' before sending the DEVFREQ_POSTCHANGE
notification.

diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 147a229056d2..d5726592d362 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -207,6 +207,8 @@ struct devfreq {
 struct devfreq_freqs {
        unsigned long old;
        unsigned long new;
+       unsigned long new_max_freq;
+       unsigned long new_min_freq;
 };


And I think that new 'user_min_freq'/'user_max_freq' are not necessary.
You can get the current max_freq/min_freq by using the following steps:

	get_freq_range(devfreq, &min_freq, &max_freq);
	dev_pm_opp_find_freq_floor(pdev, &min_freq);
	dev_pm_opp_find_freq_floor(pdev, &max_freq);

So that you can get the 'max_freq/min_freq' and then
initialize the 'freqs.new_max_freq and freqs.new_min_freq'
with them as following:

in devfreq_set_target()
	get_freq_range(devfreq, &min_freq, &max_freq);
	dev_pm_opp_find_freq_floor(pdev, &min_freq);
	dev_pm_opp_find_freq_floor(pdev, &max_freq);
	freqs.new_max_freq = min_freq;
	freqs.new_max_freq = max_freq;
	devfreq_notify_transition(devfreq, &freqs, DEVFREQ_POSTCHANGE);


On 1/26/21 7:39 PM, Lukasz Luba wrote:
> The new fields inside devfreq struct allow to check the frequency limits
> set by the user via sysfs. These limits are important for thermal governor
> Intelligent Power Allocation (IPA) which needs to know the maximum allowed
> power consumption of the device.
> 
> Signed-off-by: Lukasz Luba <lukasz.luba@arm.com>
> ---
>  drivers/devfreq/devfreq.c | 41 +++++++++++++++++++++++++++++++++++----
>  include/linux/devfreq.h   |  4 ++++
>  2 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 94cc25fd68da..e985a76e5ff3 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -843,6 +843,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  		goto err_dev;
>  	}
>  
> +	devfreq->user_min_freq = devfreq->scaling_min_freq;
> +	devfreq->user_max_freq = devfreq->scaling_max_freq;
> +
>  	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>  	atomic_set(&devfreq->suspend_count, 0);
>  
> @@ -1513,6 +1516,8 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
>  	struct devfreq *df = to_devfreq(dev);
> +	struct device *pdev = df->dev.parent;
> +	struct dev_pm_opp *opp;
>  	unsigned long value;
>  	int ret;
>  
> @@ -1533,6 +1538,19 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (!value)
> +		value = df->scaling_min_freq;
> +
> +	opp = dev_pm_opp_find_freq_ceil(pdev, &value);
> +	if (IS_ERR(opp))
> +		return count;
> +
> +	dev_pm_opp_put(opp);
> +
> +	mutex_lock(&df->lock);
> +	df->user_min_freq = value;
> +	mutex_unlock(&df->lock);
> +
>  	return count;
>  }
>  
> @@ -1554,7 +1572,9 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
>  	struct devfreq *df = to_devfreq(dev);
> -	unsigned long value;
> +	struct device *pdev = df->dev.parent;
> +	unsigned long value, value_khz;
> +	struct dev_pm_opp *opp;
>  	int ret;
>  
>  	/*
> @@ -1579,14 +1599,27 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  	 * A value of zero means "no limit".
>  	 */
>  	if (value)
> -		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> +		value_khz = DIV_ROUND_UP(value, HZ_PER_KHZ);
>  	else
> -		value = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
> +		value_khz = PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE;
>  
> -	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
> +	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value_khz);
>  	if (ret < 0)
>  		return ret;
>  
> +	if (!value)
> +		value = df->scaling_max_freq;
> +
> +	opp = dev_pm_opp_find_freq_floor(pdev, &value);
> +	if (IS_ERR(opp))
> +		return count;
> +
> +	dev_pm_opp_put(opp);
> +
> +	mutex_lock(&df->lock);
> +	df->user_max_freq = value;
> +	mutex_unlock(&df->lock);
> +
>  	return count;
>  }
>  
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index b6d3bae1c74d..147a229056d2 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -147,6 +147,8 @@ struct devfreq_stats {
>   *		touch this.
>   * @user_min_freq_req:	PM QoS minimum frequency request from user (via sysfs)
>   * @user_max_freq_req:	PM QoS maximum frequency request from user (via sysfs)
> + * @user_min_freq:	User's minimum frequency
> + * @user_max_freq:	User's maximum frequency
>   * @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.
> @@ -185,6 +187,8 @@ struct devfreq {
>  	struct dev_pm_qos_request user_max_freq_req;
>  	unsigned long scaling_min_freq;
>  	unsigned long scaling_max_freq;
> +	unsigned long user_min_freq;
> +	unsigned long user_max_freq;
>  	bool stop_polling;
>  
>  	unsigned long suspend_freq;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics