| Message ID | 20180525203043.249193-3-mka@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
> 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.
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 { ...
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.
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. > > >
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.
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.
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;
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(-)