Re: [PATCH v8 4/6] PM / devfreq: Introduce get_freq_range helper

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

On 2019-09-26 4:02 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 9. 26. 오전 5:55, Leonard Crestez wrote:
>> On 25.09.2019 04:32, Chanwoo Choi wrote:
>>> On 19. 9. 24. 오후 7:11, Leonard Crestez wrote:
>>>> Moving handling of min/max freq to a single function and call it from
>>>> update_devfreq and for printing min/max freq values in sysfs.
>>>>
>>>> This changes the behavior of out-of-range min_freq/max_freq: clamping
>>>> is now done at evaluation time. This means that if an out-of-range
>>>> constraint is imposed by sysfs and it later becomes valid then it will
>>>> be enforced.
>>>>
>>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>>>> ---
>>>>    drivers/devfreq/devfreq.c | 112 ++++++++++++++++++++++----------------
>>>>    1 file changed, 64 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index 4a878baa809e..eee403e70c84 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -96,10 +96,54 @@ static unsigned long find_available_max_freq(struct devfreq *devfreq)
>>>>    		dev_pm_opp_put(opp);
>>>>    
>>>>    	return max_freq;
>>>>    }
>>>>    
>>>> +/**
>>>> + * get_freq_range() - Get the current freq range
>>>> + * @devfreq:	the devfreq instance
>>>> + * @min_freq:	the min frequency
>>>> + * @max_freq:	the max frequency
>>>> + *
>>>> + * This takes into consideration all constraints.
>>>> + */
>>>> +static void get_freq_range(struct devfreq *devfreq,
>>>> +			   unsigned long *min_freq,
>>>> +			   unsigned long *max_freq)
>>>> +{
>>>> +	unsigned long *freq_table = devfreq->profile->freq_table;
>>>> +
>>>> +	lockdep_assert_held(&devfreq->lock);
>>>> +
>>>> +	/*
>>>> +	 * Init min/max frequency from freq table.
>>>
>>> Init -> Initialize
>>> min/max -> minimum/maximum
>>>
>>>> +	 * Drivers can initialize this in either ascending or descending order
>>>
>>> Drivers -> devfreq drivers
>>>
>>>> +	 * and devfreq core supports both.
>>>> +	 */
>>>
>>>
>>> In result, I prefer to change the comments as following:
>>> 	/*
>>> 	 * Initialize the minimum/maximum frequency from freq_table.
>>>    	 * The devfreq drivers can initialize freq_table in either
>>> 	 * ascending or descending order and devfreq core supports both.
>>> 	 */
>>
>> OK
>>
>>>> +	if (freq_table[0] < freq_table[devfreq->profile->max_state - 1]) {
>>>> +		*min_freq = freq_table[0];
>>>> +		*max_freq = freq_table[devfreq->profile->max_state - 1];
>>>> +	} else {
>>>> +		*min_freq = freq_table[devfreq->profile->max_state - 1];
>>>> +		*max_freq = freq_table[0];
>>>> +	}
>>>> +
>>>> +	/* constraints from sysfs */
>>>
>>> 'constraints' -> Constraint because first verb have to be used
>>> as the sigular verbs. Also, I think that have to enhance the comments
>>> I prefer to use following comments:
>>>
>>> 	 /* Constraint minimum/maximum frequency from user input via sysfs */
>>>
>>>
>>>
>>>> +	*min_freq = max(*min_freq, devfreq->min_freq);
>>>> +	*max_freq = min(*max_freq, devfreq->max_freq);
>>>> +
>>>> +	/* constraints from OPP interface */
>>>
>>> ditto. I prefer to use following comments:
>>>
>>> 	/* Constraint minimum/maximum frequency from OPP interface */
>>>
>>>
>>>> +	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
>>>> +	/* scaling_max_freq can be zero on error */
>>>
>>> Please remove it.
>>>
>>>> +	if (devfreq->scaling_max_freq)
>>>
>>> As I knew, devfreq->scaling_max_freq is never zero.
>>> So, it is always true. This if statement is needed.
>>
>> It can happen if find_available_max_freq encounters an error when called
>> from devfreq_notifier_call.
> 
> If you are wondering this case, I think that have to fix
> the possible issue on there instead of this point.
> 
>>
>> Maybe that should be separately fixed to set scaling_max_freq to a
>> neutral value such as "ULONG_MAX" instead?
> 
> OK.
> 
>>
>> BTW: the devfreq_notifier_call function returns -EINVAL on error but it
>> should return one of the NOTIFY_OK/DONE/STOP values instead. The OPP
>> framework ignores notifier results but (-EINVAL & NOTIFY_STOP) evaluates
>> as true so other notifiers will be skipped unintentionally.
> 
> I agree. It is needed to fix the return value type.

Will include as separate patches in v9.

>>>> +		*max_freq = min(*max_freq, devfreq->scaling_max_freq);
>>>> +
>>>> +	/* max_freq takes precedence over min_freq */
>>>
>>> As I said, almost people know that min_freq have be under than max_freq.
>>> Please remove it. And until finishing the discussion on mailing list,
>>> please don't send the next version. If you just replied from my comment
>>> and then wait my next comment, we can save the time without replying
>>> the repetitive and same comment for same point.
>>
>> This series makes it possible to set a min_freq higher than max_freq
>> (for example via PM QoS from various devices).
>>
>> It is not obvious that min_freq takes precedence over max_freq but the
>> code is self-evident so I will remove the comment.
>>
>>>> +	if (*min_freq > *max_freq)
>>>> +		*min_freq = *max_freq;
>>>> +}
>>>> +
>>>>    /**
>>>>     * devfreq_get_freq_level() - Lookup freq_table for the frequency
>>>>     * @devfreq:	the devfreq instance
>>>>     * @freq:	the target frequency
>>>>     */
>>>> @@ -349,20 +393,11 @@ int update_devfreq(struct devfreq *devfreq)
>>>>    
>>>>    	/* Reevaluate the proper frequency */
>>>>    	err = devfreq->governor->get_target_freq(devfreq, &freq);
>>>>    	if (err)
>>>>    		return err;
>>>> -
>>>> -	/*
>>>> -	 * Adjust the frequency with user freq, QoS and available freq.
>>>> -	 *
>>>> -	 * 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);
>>>> +	get_freq_range(devfreq, &min_freq, &max_freq);
>>>>    
>>>>    	if (freq < min_freq) {
>>>>    		freq = min_freq;
>>>>    		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>>>>    	}
>>>> @@ -1298,40 +1333,28 @@ 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;
>>>> -
>>>> -		/* 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];
>>>> -	}
>>>> -
>>>>    	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)
>>>>    {
>>>>    	struct devfreq *df = to_devfreq(dev);
>>>> +	unsigned long min_freq, max_freq;
>>>> +
>>>> +	mutex_lock(&df->lock);
>>>> +	get_freq_range(df, &min_freq, &max_freq);
>>>> +	mutex_unlock(&df->lock);
>>>>    
>>>> -	return sprintf(buf, "%lu\n", max(df->scaling_min_freq, df->min_freq));
>>>> +	return sprintf(buf, "%lu\n", min_freq);
>>>>    }
>>>>    
>>>>    static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>>    			      const char *buf, size_t count)
>>>>    {
>>>> @@ -1343,40 +1366,33 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>>    	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;
>>>> -
>>>> -		/* 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];
>>>> -	}
>>>> +	/* Interpret zero as "don't care" */
>>>
>>> Please remove it. Also, the detailed comment for user have to add
>>> the documentation file.
>>
>> OK
>>
>>>
>>>> +	if (!value)
>>>> +		value = ULONG_MAX;
>>>>    
>>>>    	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)
>>>>    {
>>>>    	struct devfreq *df = to_devfreq(dev);
>>>> +	unsigned long min_freq, max_freq;
>>>> +
>>>> +	mutex_lock(&df->lock);
>>>> +	get_freq_range(df, &min_freq, &max_freq);
>>>> +	mutex_unlock(&df->lock);
>>>>    
>>>> -	return sprintf(buf, "%lu\n", min(df->scaling_max_freq, df->max_freq));
>>>> +	return sprintf(buf, "%lu\n", max_freq);
>>>>    }
>>>>    static DEVICE_ATTR_RW(max_freq);
>>>>    
>>>>    static ssize_t available_frequencies_show(struct device *d,
>>>>    					  struct device_attribute *attr,
> 
>