[02/11] PM / devfreq: Fix handling of min/max_freq == 0
diff mbox

Message ID 20180525203043.249193-3-mka@chromium.org
State Not Applicable
Headers show

Commit Message

Matthias Kaehlcke May 25, 2018, 8:30 p.m. UTC
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(-)

Comments

MyungJoo Ham May 28, 2018, 3:59 a.m. UTC | #1
> 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(-)

Thanks a lot! Nice Catch.

Acked-by: MyungJoo Ham <myunngjoo.ham@samsung.com>

Cheers,
MyungJoo.
Chanwoo Choi May 28, 2018, 6:37 a.m. UTC | #2
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];


>  
>  	df->min_freq = value;
> @@ -1158,17 +1161,20 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>  	struct devfreq *df = to_devfreq(dev);
>  	unsigned long value;
>  	int ret;
> -	unsigned long min;
>  
>  	ret = sscanf(buf, "%lu", &value);
>  	if (ret != 1)
>  		return -EINVAL;
>  
>  	mutex_lock(&df->lock);
> -	min = df->min_freq;
> -	if (value && min && value < min) {
> -		ret = -EINVAL;
> -		goto unlock;
> +
> +	if (!value) {
> +		value = df->profile->freq_table[0];

ditto.
	value = df->profile->freq_table[df->profile->max_state - 1];

> +	} else {
> +		if (value < df->min_freq) {
> +			ret = -EINVAL;
> +			goto unlock;
> +		}
>  	}
>  
>  	df->max_freq = value;
> 

Actually, min_freq_store() and max_freq_store() are very similar.
But, this patch changed the order of conditional statement as following:
If there is no special reason, you better to keep the same format
for the readability.


min_freq_store()
	if (value) {
		...
	} else {
		value = df->profile->freq_table[df->profile->max_state - 1];
	}


max_freq_store()
	if (!value) {
		value = df->profile->freq_table[0];
	} else {
		...
Matthias Kaehlcke May 29, 2018, 6:57 p.m. UTC | #3
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.

> > @@ -1158,17 +1161,20 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
> >  	struct devfreq *df = to_devfreq(dev);
> >  	unsigned long value;
> >  	int ret;
> > -	unsigned long min;
> >  
> >  	ret = sscanf(buf, "%lu", &value);
> >  	if (ret != 1)
> >  		return -EINVAL;
> >  
> >  	mutex_lock(&df->lock);
> > -	min = df->min_freq;
> > -	if (value && min && value < min) {
> > -		ret = -EINVAL;
> > -		goto unlock;
> > +
> > +	if (!value) {
> > +		value = df->profile->freq_table[0];
> 
> ditto.
> 	value = df->profile->freq_table[df->profile->max_state - 1];
> 
> > +	} else {
> > +		if (value < df->min_freq) {
> > +			ret = -EINVAL;
> > +			goto unlock;
> > +		}
> >  	}
> >  
> >  	df->max_freq = value;
> > 
> 
> Actually, min_freq_store() and max_freq_store() are very similar.
> But, this patch changed the order of conditional statement as following:
> If there is no special reason, you better to keep the same format
> for the readability.
> 
> 
> min_freq_store()
> 	if (value) {
> 		...
> 	} else {
> 		value = df->profile->freq_table[df->profile->max_state - 1];
> 	}
> 
> 
> max_freq_store()
> 	if (!value) {
> 		value = df->profile->freq_table[0];
> 	} else {
> 		...
> 

Agreed, better use the same format, I'll update it in the next revision.
Chanwoo Choi May 30, 2018, 8:04 a.m. UTC | #4
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.


> 
>>> @@ -1158,17 +1161,20 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
>>>  	struct devfreq *df = to_devfreq(dev);
>>>  	unsigned long value;
>>>  	int ret;
>>> -	unsigned long min;
>>>  
>>>  	ret = sscanf(buf, "%lu", &value);
>>>  	if (ret != 1)
>>>  		return -EINVAL;
>>>  
>>>  	mutex_lock(&df->lock);
>>> -	min = df->min_freq;
>>> -	if (value && min && value < min) {
>>> -		ret = -EINVAL;
>>> -		goto unlock;
>>> +
>>> +	if (!value) {
>>> +		value = df->profile->freq_table[0];
>>
>> ditto.
>> 	value = df->profile->freq_table[df->profile->max_state - 1];
>>
>>> +	} else {
>>> +		if (value < df->min_freq) {
>>> +			ret = -EINVAL;
>>> +			goto unlock;
>>> +		}
>>>  	}
>>>  
>>>  	df->max_freq = value;
>>>
>>
>> Actually, min_freq_store() and max_freq_store() are very similar.
>> But, this patch changed the order of conditional statement as following:
>> If there is no special reason, you better to keep the same format
>> for the readability.
>>
>>
>> min_freq_store()
>> 	if (value) {
>> 		...
>> 	} else {
>> 		value = df->profile->freq_table[df->profile->max_state - 1];
>> 	}
>>
>>
>> max_freq_store()
>> 	if (!value) {
>> 		value = df->profile->freq_table[0];
>> 	} else {
>> 		...
>>
> 
> Agreed, better use the same format, I'll update it in the next revision.
> 
> 
>
Matthias Kaehlcke May 30, 2018, 9:13 p.m. UTC | #5
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.

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.
Chanwoo Choi June 5, 2018, 9:40 a.m. UTC | #6
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.

Patch
diff mbox

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];
 	}
 
 	df->min_freq = value;
@@ -1158,17 +1161,20 @@  static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr,
 	struct devfreq *df = to_devfreq(dev);
 	unsigned long value;
 	int ret;
-	unsigned long min;
 
 	ret = sscanf(buf, "%lu", &value);
 	if (ret != 1)
 		return -EINVAL;
 
 	mutex_lock(&df->lock);
-	min = df->min_freq;
-	if (value && min && value < min) {
-		ret = -EINVAL;
-		goto unlock;
+
+	if (!value) {
+		value = df->profile->freq_table[0];
+	} else {
+		if (value < df->min_freq) {
+			ret = -EINVAL;
+			goto unlock;
+		}
 	}
 
 	df->max_freq = value;