Re: [PATCH v8 6/6] PM / devfreq: Use PM QoS for sysfs min/max_freq

[Leonard Crestez <leonard.crestez@nxp.com>]

On 25.09.2019 05:36, Chanwoo Choi wrote:
> On 19. 9. 24. 오후 7:11, 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>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>   drivers/devfreq/devfreq.c | 46 ++++++++++++++++++++++++---------------
>>   include/linux/devfreq.h   |  9 ++++----
>>   2 files changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 784f3e40536a..8bb7efd821ab 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -137,14 +137,10 @@ static void get_freq_range(struct devfreq *devfreq,
>>   	qos_max_freq = dev_pm_qos_read_value(devfreq->dev.parent,
>>   					     DEV_PM_QOS_MIN_FREQUENCY);
>>   	*min_freq = max(*min_freq, HZ_PER_KHZ * qos_min_freq);
>>   	*max_freq = min(*max_freq, HZ_PER_KHZ * qos_max_freq);
>>   
>> -	/* 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);
>> @@ -679,10 +675,12 @@ static void devfreq_dev_release(struct device *dev)
>>   			DEV_PM_QOS_MIN_FREQUENCY);
>>   
>>   	if (devfreq->profile->exit)
>>   		devfreq->profile->exit(devfreq->dev.parent);
>>   
>> +	dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
>> +	dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
> 
> Please check the return value if error happen, just print the err with dev_err()
> without stopping the release steps.

OK, will print errors

>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>>   	mutex_destroy(&devfreq->lock);
>>   	kfree(devfreq);
>>   }
>> @@ -747,18 +745,25 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   	devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>>   	if (!devfreq->scaling_min_freq) {
>>   		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) {
>>   		err = -EINVAL;
>>   		goto err_dev;
>>   	}
>> -	devfreq->max_freq = devfreq->scaling_max_freq;
>> +
>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
>> +				     DEV_PM_QOS_MIN_FREQUENCY, 0);
>> +	if (err < 0)
>> +		goto err_dev;
>> +	err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
>> +				     DEV_PM_QOS_MAX_FREQUENCY, S32_MAX);
>> +	if (err < 0)
>> +		goto err_dev;
>>   
>>   	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>>   	atomic_set(&devfreq->suspend_count, 0);
>>   
>>   	devfreq->trans_table = kzalloc(
>> @@ -843,10 +848,14 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   err_dev:
>>   	/*
>>   	 * Cleanup path for errors that happen before registration.
>>   	 * Otherwise we rely on devfreq_dev_release.
>>   	 */
>> +	if (dev_pm_qos_request_active(&devfreq->user_max_freq_req))
>> +		dev_pm_qos_remove_request(&devfreq->user_max_freq_req);
> 
> Please check the return value if error happen, just print the err with dev_err()
> without stopping the release steps.

OK, will print errors

> 
> 	dev_err(... "failed to remove request of DEV_PM_QOS_MAX_FREQUENCY\n");
> 
>> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
>> +		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
> 	
> 	dev_err(... "failed to remove request of DEV_PM_QOS_MIN_FREQUENCY\n");
> 
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>>   	kfree(devfreq);
>>   err_out:
>>   	return ERR_PTR(err);
>> @@ -1407,14 +1416,15 @@ 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);
>> -	df->min_freq = value;
>> -	update_devfreq(df);
>> -	mutex_unlock(&df->lock);
>> +	/* round down to kHz for PM QoS */
> 
> I prefer more detailed description as following:
> 
> 	/*
> 	 * Round down to KHz to decide the proper minimum frequency
> 	 * which is closed to user request.
>   	 */
> 
> 
>> +	ret = dev_pm_qos_update_request(&df->user_min_freq_req,
>> +					value / HZ_PER_KHZ);
>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	return count;
>>   }
>>   
>>   static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>> @@ -1439,19 +1449,19 @@ 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);
>> -
>> -	/* Interpret zero as "don't care" */
>> -	if (!value)
>> -		value = ULONG_MAX;
>> +	/* round up to kHz for PM QoS and interpret zero as "don't care" */
> 
> I think that "don't care" comment style is not good.
> 
> I referred to the Documentation/ABI/testing/sysfs-class-devfreq file.
> I prefer more detailed description as following:
> 	/*
> 	 * Round up to KHz to decide the proper maximum frequency
> 	 * which is closed to user request. If value is zero,
> 	 * the user does not care.
>   	 */

OK, will update this comment

>> +	if (value)
>> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
>> +	else
>> +		value = S32_MAX;
>>   
>> -	df->max_freq = value;
>> -	update_devfreq(df);
>> -	mutex_unlock(&df->lock);
>> +	ret = dev_pm_qos_update_request(&df->user_max_freq_req, value);
>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	return count;
>>   }
>>   static DEVICE_ATTR_RW(min_freq);
>>   
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index dac0dffeabb4..7849fe4c666d 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)
>> + * @user_min_freq_req:	PM QoS min frequency request from user (via sysfs)
> 
> min -> minimum and then remove parenthesis as following:
> 	PM QoS minimum frequency request by user via sysfs
> 
>> + * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
> 
> ditto. max -> maximum
> 	PM QoS maximum frequency request by user via sysfs

OK

>>    * @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 user_min_freq_req;
>> +	struct dev_pm_qos_request user_max_freq_req;
>>   	unsigned long scaling_min_freq;
>>   	unsigned long scaling_max_freq;
>>   	bool stop_polling;
>>   
>>   	unsigned long suspend_freq;
>>
> 
> 

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