Re: [PATCH v3 2/2] PM / devfreq: Use dev_pm_qos for sysfs min/max_freq

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

On 19. 8. 22. 오후 9:24, Leonard Crestez wrote:
> On 22.08.2019 14:06, Chanwoo Choi wrote:
>> On 19. 8. 22. 오후 7:58, Leonard Crestez wrote:
>>> On 8/22/2019 1:03 PM, Chanwoo Choi wrote:
>>>> This patch doesn't check the range of input value
>>>> with the supported frequencies of devfreq device.
>>>>
>>>> For example,
>>>> The devfreq0 device has the following available frequencies
>>>> 100000000 200000000 300000000
>>>>
>>>> and then user enters the 500000000 as following:
>>>> echo 500000000 > /sys/class/devfreq/devfreq0/min_freq
>>>>
>>>> In result, get_effective_min_freq() will return the 500000000.
>>>> It is wrong value. it show the unsupported frequency through
>>>> min_freq sysfs path.
>>>
>>> Through dev_pm_qos devices can also ask for a freq higher than the
>>> maximum OPP and unlike sysfs there is no way to reject such requests,
>>> instead PM qos claims it's based on "best effort".
>>>
>>> There are many requests in the kernel for "PM_QOS_CPU_DMA_LATENCY 0" and
>>> I think that DEV_PM_QOS_MIN_FREQUENCY, MAX_S32 is a reasonable way for a
>>> device to request "max performance" from devfreq.
>>>
>>> Rejecting min > max is complicated (what happens to rejected requests
>>> when max goes back up?) and I think it's better to just stick with a
>>> "max overrides min" policy since it can already deal with conflicts.
>>>
>>> Do you have a usecase for rejecting out-of-range min_freq from
>>> userspace? cpufreq also accepts arbitrary min/max values and handles them.
>>
>> I don't mean the rejecting when user enter the out-of-range frequency.
>> As I commented, user can enter the value they want. But, we should
>> check the range of input because devfreq have to show the correct supported
>> frequency through sysfs.
> 
> We can avoid showing an out-of-range value in min_freq by printing 
> min(min_freq, max_freq).

You can check the range of frequency in the get_min_freq()/get_max_freq().
I want to return the meaningful frequency from get_min_freq()/get_max_freq().

Everyone expects get_min_freq()/get_max_freq() functions will retunrn
the supported min/max frequency. If get_min_freq()/get_max_freq()
return the our-of-range frequency, it is not nice and cause the confusion
why these functions return the out-of-range frequency..

Also, if we don't check the return value of dev_pm_qos_read_value(),
the out-of-range frequency of dev_pm_qos_read_value() would be used
on multiple point of devfreq.c. I think that it is not good.

> 
> The max_freq value from pm_qos can still be between OPPs so maybe print 
> devfreq_recommended_opp(max_freq, DEVFREQ_FLAG_LEAST_UPPER_BOUND).
> 
>>> In theory pm_qos could be extended to differentiate between "soft"
>>> requests (current behavior) and "hard" requests which return errors if
>>> they can't be guaranteed but this is far out of scope.
>>
>> I think that you agreed the limitation of dev_pm_qos when entering
>> or requesting the unsupported frequency.
> 
> Yes, "best effort qos" is acceptable for now.
> 

And please don't remove the my previous comment.
Just reply your opinion without any removal.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics