Re: [PATCH v2 2/2] PM / devfreq: Use PM QoS for sysfs min/max_freq

[Chanwoo Choi <cw00.choi@samsung.com>]

On 12/6/19 3:02 AM, Matthias Kaehlcke wrote:
> On Thu, Dec 05, 2019 at 12:05:07PM +0200, Leonard Crestez wrote:
>> Switch the handling of min_freq and max_freq from sysfs to use the
>> dev_pm_qos_request interface.
>>
>> Since 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 compatibility by rounding min values down and rounding
>> max values up.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>>  drivers/devfreq/devfreq.c | 76 ++++++++++++++++++++++++++++++---------
>>  include/linux/devfreq.h   |  9 ++---
>>  2 files changed, 64 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index e8b943fc4259..bcb286509547 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -139,14 +139,10 @@ static void get_freq_range(struct devfreq *devfreq,
>>  	*min_freq = max(*min_freq, (unsigned long)HZ_PER_KHZ * qos_min_freq);
>>  	if (qos_max_freq != PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE)
>>  		*max_freq = min(*max_freq,
>>  				(unsigned long)HZ_PER_KHZ * qos_max_freq);
>>  
>> -	/* 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);
>>  	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
>>  
>>  	if (*min_freq > *max_freq)
>> @@ -703,10 +699,23 @@ static void devfreq_dev_release(struct device *dev)
>>  					 DEV_PM_QOS_MIN_FREQUENCY);
>>  	if (err && err != -ENOENT)
>>  		dev_warn(dev->parent,
>>  			"Failed to remove min_freq notifier: %d\n", err);
>>  
>> +	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req)) {
>> +		err = dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
>> +		if (err)
>> +			dev_warn(dev->parent,
>> +				"Failed to remove max_freq request: %d\n", err);
>> +	}
>> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req)) {
>> +		err = dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>> +		if (err)
>> +			dev_warn(dev->parent,
>> +				"Failed to remove min_freq request: %d\n", err);
>> +	}
>> +
>>  	if (devfreq->profile->exit)
>>  		devfreq->profile->exit(devfreq->dev.parent);
>>  
>>  	mutex_destroy(&devfreq->lock);
>>  	kfree(devfreq);
>> @@ -776,19 +785,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",
>> @@ -825,10 +832,20 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>  
>>  	srcu_init_notifier_head(&devfreq->transition_notifier_list);
>>  
>>  	mutex_unlock(&devfreq->lock);
>>  
>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
>> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>> +	if (err < 0)
>> +		goto err_devfreq;
>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
>> +				     DEV_PM_QOS_MAX_FREQUENCY,
>> +				     PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
>> +	if (err < 0)
>> +		goto err_devfreq;
>> +
>>  	devfreq->nb_min.notifier_call = qos_min_notifier_call;
>>  	err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
>>  				      DEV_PM_QOS_MIN_FREQUENCY);
>>  	if (err)
>>  		goto err_devfreq;
>> @@ -1418,18 +1435,26 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>  {
>>  	struct devfreq *df = to_devfreq(dev);
>>  	unsigned long value;
>>  	int ret;
>>  
>> +	/*
>> +	 * Protect against theoretical sysfs writes between
>> +	 * device_add and dev_pm_qos_add_request
>> +	 */
>> +	if (!dev_pm_qos_request_active(&df->user_min_freq_req))
>> +		return -EINVAL;
> 
> The error code -EINVAL is a bit misleading. I guess it's not super
> important, especially since this is a very rare case. In case you
> re-spin you could consider returning -EAGAIN ('Resource temporarily
> unavailable') in this case from min/max_freq_store/show()

I agree. -EAGAIN is better than -EINVAL. I'll change it by myself.

> 
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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