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

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

On 2019-09-24 10:22 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 24. 오전 6:10, 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 | 49 +++++++++++++++++++++++++--------------
>>   include/linux/devfreq.h   |  9 +++----
>>   2 files changed, 36 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 9887408f23bb..a00737e34d36 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -132,14 +132,10 @@ static void devfreq_get_freq_range(struct devfreq *devfreq,
>>   	*min_freq = max(*min_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>>   				devfreq->dev.parent, DEV_PM_QOS_MIN_FREQUENCY));
>>   	*max_freq = min(*max_freq, HZ_PER_KHZ * (unsigned long)dev_pm_qos_read_value(
>>   				devfreq->dev.parent, DEV_PM_QOS_MAX_FREQUENCY));
>>   
>> -	/* 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);
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>>   	mutex_destroy(&devfreq->lock);
>>   	kfree(devfreq);
>>   }
>> @@ -747,18 +745,26 @@ 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;
>> +
>> +	/* PM QoS requests for min/max freq from sysfs */
> 
> The comment is important. But the devfreq_add_device() has usually
> not added the comment for each step. I'm not sure to add the some
> comments for only this. How about removing it?

OK.

>> +	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 +849,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);
>> +	if (dev_pm_qos_request_active(&devfreq->user_min_freq_req))
>> +		dev_pm_qos_remove_request(&devfreq->user_min_freq_req);
>>   	kfree(devfreq->time_in_state);
>>   	kfree(devfreq->trans_table);
>>   	kfree(devfreq);
>>   err_out:
>>   	return ERR_PTR(err);
>> @@ -1387,14 +1397,17 @@ 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 dev_pm_qos */
> 
> Please remove it.
> 
>> +	if (value)
>> +		value = value / HZ_PER_KHZ;
> 
> It doesn't be necessary.
> 
>> +
>> +	ret = dev_pm_qos_update_request(&df->user_min_freq_req, value);
> 
> Change it as following:
> 	ret = dev_pm_qos_update_request(&df->user_min_freq_req, (value / HZ_PER_KHZ));

OK

>> +	if (ret < 0)
>> +		return ret;
>>   
>>   	return count;
>>   }
>>   
>>   static ssize_t min_freq_show(struct device *dev, struct device_attribute *attr,
>> @@ -1419,19 +1432,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 dev_pm_qos and interpret zero as "don't care" */
> 
> Please remove it.
> 
>> +	if (value)
>> +		value = DIV_ROUND_UP(value, HZ_PER_KHZ);
> 
> Why do you use 'DIV_ROUND_UP(value, HZ_PER_KHZ)'
> instead of 'value / HZ_PER_KHZ' in min_freq_store()?

If an user request of max=666666666 Hz is converted to max=666666 kHz 
and then back to 666666000 Hz then the OPP with freq=2/3 gHz might be 
cut out. I deliberately rounded down for min and up for max to make the 
constraint interval "inclusive".

Does this make sense? It seems like the path least likely to cause issues.
>> +	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 8b92ccbd1962..3162eb9b0954 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)
>> + * @user_max_freq_req:	PM QoS max frequency request from user (via sysfs)
> 
> I think that 'user' prefix is not needed. You better to change it as following

Was requested here:
     https://patchwork.kernel.org/patch/11149505/#22891887

It makes sense to mention ths request is from userspace via sysfs. Other 
drivers can also make PM QoS frequency requests for their own reasons.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel