| Message ID | 20180703234705.227473-3-mka@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Matthias, On 2018년 07월 04일 08:46, 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> > Reviewed-by: Brian Norris <briannorris@chromium.org> > --- > Changes in v5: > - none > > Changes in v4: > - added 'Reviewed-by: Brian Norris <briannorris@chromium.org>' tag > > Changes in v3: > - none > > Changes in v2: > - handle freq tables sorted in ascending and descending order in > min/max_freq_store() > - use same order for conditional statements in min/max_freq_store() > --- > drivers/devfreq/devfreq.c | 42 ++++++++++++++++++++++++++++----------- > 1 file changed, 30 insertions(+), 12 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 0057ef5b0a98..6f604f8b2b81 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 */ > } > @@ -1122,18 +1122,27 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, > { > struct devfreq *df = to_devfreq(dev); > unsigned long value; > + unsigned long *freq_table; You can move 'freq_table' under 'else' statement. > 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 { > + freq_table = df->profile->freq_table; > + /* typical order is ascending, some drivers use descending */ You better to explain what is doing of following code. How about modifying it as following? /* Get minimum frequency according to sorting way */ > + if (freq_table[0] < freq_table[df->profile->max_state - 1]) > + value = freq_table[0]; > + else > + value = freq_table[df->profile->max_state - 1]; > } > > df->min_freq = value; > @@ -1157,18 +1166,27 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > { > struct devfreq *df = to_devfreq(dev); > unsigned long value; > + unsigned long *freq_table; ditto. You can move 'freq_table' under 'else' statement. > 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) { > + if (value < df->min_freq) { > + ret = -EINVAL; > + goto unlock; > + } > + } else { > + freq_table = df->profile->freq_table; > + /* typical order is ascending, some drivers use descending */ ditto. /* Get maximum frequency according to sorting way */ > + if (freq_table[0] < freq_table[df->profile->max_state - 1]) > + value = freq_table[df->profile->max_state - 1]; > + else > + value = freq_table[0]; > } > > df->max_freq = value; > If you agree my comment and modify this patch according to my comment, feel free to add my review tag. - Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Hi Chanwoo, On Wed, Jul 04, 2018 at 11:20:31AM +0900, Chanwoo Choi wrote: > Hi Matthias, > > On 2018년 07월 04일 08:46, 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> > > Reviewed-by: Brian Norris <briannorris@chromium.org> > > --- > > Changes in v5: > > - none > > > > Changes in v4: > > - added 'Reviewed-by: Brian Norris <briannorris@chromium.org>' tag > > > > Changes in v3: > > - none > > > > Changes in v2: > > - handle freq tables sorted in ascending and descending order in > > min/max_freq_store() > > - use same order for conditional statements in min/max_freq_store() > > --- > > drivers/devfreq/devfreq.c | 42 ++++++++++++++++++++++++++++----------- > > 1 file changed, 30 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index 0057ef5b0a98..6f604f8b2b81 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 */ > > } > > @@ -1122,18 +1122,27 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, > > { > > struct devfreq *df = to_devfreq(dev); > > unsigned long value; > > + unsigned long *freq_table; > > You can move 'freq_table' under 'else' statement. Will do > > 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 { > > + freq_table = df->profile->freq_table; > > + /* typical order is ascending, some drivers use descending */ > > You better to explain what is doing of following code. > How about modifying it as following? > > /* Get minimum frequency according to sorting way */ Ok, will slightly modify it to 'sorting order' if you don't mind. > > + if (freq_table[0] < freq_table[df->profile->max_state - 1]) > > + value = freq_table[0]; > > + else > > + value = freq_table[df->profile->max_state - 1]; > > } > > > > df->min_freq = value; > > @@ -1157,18 +1166,27 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, > > { > > struct devfreq *df = to_devfreq(dev); > > unsigned long value; > > + unsigned long *freq_table; > > ditto. You can move 'freq_table' under 'else' statement. Will do > > 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) { > > + if (value < df->min_freq) { > > + ret = -EINVAL; > > + goto unlock; > > + } > > + } else { > > + freq_table = df->profile->freq_table; > > + /* typical order is ascending, some drivers use descending */ > > ditto. > /* Get maximum frequency according to sorting way */ Ok > > + if (freq_table[0] < freq_table[df->profile->max_state - 1]) > > + value = freq_table[df->profile->max_state - 1]; > > + else > > + value = freq_table[0]; > > } > > > > df->max_freq = value; > > > > If you agree my comment and modify this patch according to my comment, > feel free to add my review tag. > - Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> Thanks for the review! Matthias
Hi Matthias, On 2018년 07월 07일 01:36, Matthias Kaehlcke wrote: > Hi Chanwoo, > > On Wed, Jul 04, 2018 at 11:20:31AM +0900, Chanwoo Choi wrote: >> Hi Matthias, >> >> On 2018년 07월 04일 08:46, 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> >>> Reviewed-by: Brian Norris <briannorris@chromium.org> >>> --- >>> Changes in v5: >>> - none >>> >>> Changes in v4: >>> - added 'Reviewed-by: Brian Norris <briannorris@chromium.org>' tag >>> >>> Changes in v3: >>> - none >>> >>> Changes in v2: >>> - handle freq tables sorted in ascending and descending order in >>> min/max_freq_store() >>> - use same order for conditional statements in min/max_freq_store() >>> --- >>> drivers/devfreq/devfreq.c | 42 ++++++++++++++++++++++++++++----------- >>> 1 file changed, 30 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 0057ef5b0a98..6f604f8b2b81 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 */ >>> } >>> @@ -1122,18 +1122,27 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, >>> { >>> struct devfreq *df = to_devfreq(dev); >>> unsigned long value; >>> + unsigned long *freq_table; >> >> You can move 'freq_table' under 'else' statement. > > Will do > >>> 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 { >>> + freq_table = df->profile->freq_table; >>> + /* typical order is ascending, some drivers use descending */ >> >> You better to explain what is doing of following code. >> How about modifying it as following? >> >> /* Get minimum frequency according to sorting way */ > > Ok, will slightly modify it to 'sorting order' if you don't mind. I don't mind of 'soring order'. Thanks. (snip)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 0057ef5b0a98..6f604f8b2b81 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 */ } @@ -1122,18 +1122,27 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr, { struct devfreq *df = to_devfreq(dev); unsigned long value; + unsigned long *freq_table; 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 { + freq_table = df->profile->freq_table; + /* typical order is ascending, some drivers use descending */ + if (freq_table[0] < freq_table[df->profile->max_state - 1]) + value = freq_table[0]; + else + value = freq_table[df->profile->max_state - 1]; } df->min_freq = value; @@ -1157,18 +1166,27 @@ static ssize_t max_freq_store(struct device *dev, struct device_attribute *attr, { struct devfreq *df = to_devfreq(dev); unsigned long value; + unsigned long *freq_table; 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) { + if (value < df->min_freq) { + ret = -EINVAL; + goto unlock; + } + } else { + freq_table = df->profile->freq_table; + /* typical order is ascending, some drivers use descending */ + if (freq_table[0] < freq_table[df->profile->max_state - 1]) + value = freq_table[df->profile->max_state - 1]; + else + value = freq_table[0]; } 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> Reviewed-by: Brian Norris <briannorris@chromium.org> --- Changes in v5: - none Changes in v4: - added 'Reviewed-by: Brian Norris <briannorris@chromium.org>' tag Changes in v3: - none Changes in v2: - handle freq tables sorted in ascending and descending order in min/max_freq_store() - use same order for conditional statements in min/max_freq_store() --- drivers/devfreq/devfreq.c | 42 ++++++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 12 deletions(-)