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

[Matthias Kaehlcke]

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@xxxxxxxxxxxx>
> >>> ---
> >>>  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.

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.