Re: [PATCH 02/11] PM / devfreq: Fix handling of min/max_freq == 0

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

Hi,

On 2018년 05월 31일 06:13, Matthias Kaehlcke wrote:
> On Wed, May 30, 2018 at 05:04:14PM +0900, Chanwoo Choi wrote:
>> Hi,
>>
>> On 2018년 05월 30일 03:57, Matthias Kaehlcke wrote:
>>> On Mon, May 28, 2018 at 03:37:47PM +0900, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> On 2018년 05월 26일 05:30, Matthias Kaehlcke wrote:
>>>>> Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding the
>>>>> devfreq device") initializes df->min/max_freq with the min/max OPP when
>>>>> the device is added. Later commit f1d981eaecf8 ("PM / devfreq: Use the
>>>>> available min/max frequency") adds df->scaling_min/max_freq and the
>>>>> following to the frequency adjustment code:
>>>>>
>>>>>   max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq);
>>>>>
>>>>> With the current handling of min/max_freq this is incorrect:
>>>>>
>>>>> Even though df->max_freq is now initialized to a value != 0 user space
>>>>> can still set it to 0, in this case max_freq would be 0 instead of
>>>>> df->scaling_max_freq as intended. In consequence the frequency adjustment
>>>>> is not performed:
>>>>>
>>>>>   if (max_freq && freq > max_freq) {
>>>>> 	freq = max_freq;
>>>>>
>>>>> To fix this set df->min/max freq to the min/max OPP in max/max_freq_store,
>>>>> when the user passes a value of 0. This also prevents df->max_freq from
>>>>> being set below the min OPP when df->min_freq is 0, and similar for
>>>>> min_freq. Since it is now guaranteed that df->min/max_freq can't be 0 the
>>>>> checks for this case can be removed.
>>>>>
>>>>> Fixes: f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency")
>>>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>>>> ---
>>>>>  drivers/devfreq/devfreq.c | 30 ++++++++++++++++++------------
>>>>>  1 file changed, 18 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index 0057ef5b0a98..67da4e7b486b 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -283,11 +283,11 @@ int update_devfreq(struct devfreq *devfreq)
>>>>>  	max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq);
>>>>>  	min_freq = MAX(devfreq->scaling_min_freq, devfreq->min_freq);
>>>>>  
>>>>> -	if (min_freq && freq < min_freq) {
>>>>> +	if (freq < min_freq) {
>>>>>  		freq = min_freq;
>>>>>  		flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
>>>>>  	}
>>>>> -	if (max_freq && freq > max_freq) {
>>>>> +	if (freq > max_freq) {
>>>>>  		freq = max_freq;
>>>>>  		flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
>>>>>  	}
>>>>> @@ -1123,17 +1123,20 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
>>>>>  	struct devfreq *df = to_devfreq(dev);
>>>>>  	unsigned long value;
>>>>>  	int ret;
>>>>> -	unsigned long max;
>>>>>  
>>>>>  	ret = sscanf(buf, "%lu", &value);
>>>>>  	if (ret != 1)
>>>>>  		return -EINVAL;
>>>>>  
>>>>>  	mutex_lock(&df->lock);
>>>>> -	max = df->max_freq;
>>>>> -	if (value && max && value > max) {
>>>>> -		ret = -EINVAL;
>>>>> -		goto unlock;
>>>>> +
>>>>> +	if (value) {
>>>>> +		if (value > df->max_freq) {
>>>>> +			ret = -EINVAL;
>>>>> +			goto unlock;
>>>>> +		}
>>>>> +	} else {
>>>>> +		value = df->profile->freq_table[df->profile->max_state - 1];
>>>>>  	}
>>>>
>>>> If you want to prevent that df->min_freq is zero, 
>>>> you should reinitialize 'value' as following.
>>>> Because freq_table must be in ascending order.
>>>> 	value = df->profile->freq_table[0];
>>>
>>> Thanks for pointing this out!
>>>
>>> The devfreq device I tested with (a Mali GPU) uses descending order
>>> for some reason, and I assumed that's the usual order.
>>>
>>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c#208
>>>
>>> It seems the ordering doesn't have any impact beyond this patch. If
>>> the order isn't mandatory for drivers that set up their own freq_table
>>> we should probably support both cases to be safe.
>>
>> Prior to that 'freq_table' is optional. So, patch[1] initialize the 'freq_table'
>> by using OPP interface if 'freq_table' is NULL.
>> [1] commit 0ec09ac2cebe ("PM / devfreq: Set the freq_table of devfreq device")
>>
>> Current devfreq recommend the ascending order for 'freq_table'.
>> But, as you know, it might be not enough to support them.
>>
>> I agree that we should support the both cases (ascending or descending order).
>>
>> Maybe, it might be not proper to access the freq_table[] directly
>> because we don't know the ordering style of 'freq_table'
>> if 'freq_table' is made by devfreq user instead of devfreq core.
> 
> If we can assume that it is either ascending or descending, but not
> random order than a simple check if freq_table[0] <
> freq_table[max_state - 1] would be sufficient.

Also, we should consider the order way of freq_table on available_frequency
because available_frequency have to show the frequency as the ascending order
even if freq_table uses the descending order.

> 
> Otherwise we could also determine the min/max after initialization and
> save the result, though that would leave us with yet another frequency
> pair, which might be confusing, especially if we don't come up with
> good names to distinguish between them.

IMO, it might make the confusion if devfreq device has the two frequency
table.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics