| Message ID | 1405926154-27214-1-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lukasz, I haven't replied yet as I wanted to see what the general feed of Rafael is going to be :) As this is something new and wasn't sure if we really want this.. On 21 July 2014 12:32, Lukasz Majewski <l.majewski@samsung.com> wrote: > This commit adds first regression test "cpufreq_freq_test.sh" for the > cpufreq subsystem. That's not enough, Tell us why we should continue reading this mail.. > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > --- > Changes for v2: > - Replace *_PATCH with *_PATH for variables names > - Corrected mistakes in the README file > - Providing detailed explanation of the patch in the README file > --- > drivers/cpufreq/tests/README | 33 +++++++ > drivers/cpufreq/tests/cpufreq_freq_test.sh | 149 +++++++++++++++++++++++++++++ Probably a better place would be tools/power/cpufreq/ @Rafael? > 2 files changed, 182 insertions(+) > create mode 100644 drivers/cpufreq/tests/README > create mode 100755 drivers/cpufreq/tests/cpufreq_freq_test.sh > > diff --git a/drivers/cpufreq/tests/README b/drivers/cpufreq/tests/README > new file mode 100644 > index 0000000..3e9cd80 > --- /dev/null > +++ b/drivers/cpufreq/tests/README > @@ -0,0 +1,33 @@ > +This file contains list of cpufreq's available regression tests with a short > +usage description. > + > +1. cpufreq_freq_test.sh > + > +Description: > +------------ > +This script is supposed to test if cpufreq attributes exported by sysfs are > +exposing correct values. > + > +To achieve this goal it saves the current governor and changes it to > +"performance". Afterwards, it reads the "scaling_available_frequencies" > +property. With the list of supported frequencies it is able to enforce each of > +them by writing to "scaling_max_freq" attribute. To make the test more reliable > +a superfluous load with gzip is created to be sure that we are running with > +highest possible frequency. This high load is regulated with the 'sleep' > +duration. After this time the "cpufreq_cur_freq" is read and compared with the > +original value. As the last step the original governor is restored. I couldn't make out the purpose of this test and why we need it. How do we ensure that "cpufreq attributes exported by sysfs are exposing correct values"? And actually what do we mean by this statement even? What kind of errors can be there in exposing these values. I want to understand the purpose of this script very clearly first and then only will look at the details.
Hi Viresh, > Hi Lukasz, > > I haven't replied yet as I wanted to see what the general feed of > Rafael is going to be :) > > As this is something new and wasn't sure if we really want this.. Do you want to say that we have enough tests and we don't need more ? I always thought that we shall have as much regression tests as possible. > > On 21 July 2014 12:32, Lukasz Majewski <l.majewski@samsung.com> wrote: > > This commit adds first regression test "cpufreq_freq_test.sh" for > > the cpufreq subsystem. > > That's not enough, Tell us why we should continue reading this mail.. Hmm... If "regression" and "test" don't catch the attention of a diligent maintainer, then I cannot do much more to encourage him to read the whole e-mail :-) I can imagine that maintainers are very busy, therefore I've prepared README file with detailed description of the script operation. > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > --- > > Changes for v2: > > - Replace *_PATCH with *_PATH for variables names > > - Corrected mistakes in the README file > > - Providing detailed explanation of the patch in the README file > > --- > > drivers/cpufreq/tests/README | 33 +++++++ > > drivers/cpufreq/tests/cpufreq_freq_test.sh | 149 > > +++++++++++++++++++++++++++++ > > Probably a better place would be tools/power/cpufreq/ > > @Rafael? Rafael, I'm open for suggestions. > > > 2 files changed, 182 insertions(+) > > create mode 100644 drivers/cpufreq/tests/README > > create mode 100755 drivers/cpufreq/tests/cpufreq_freq_test.sh > > > > diff --git a/drivers/cpufreq/tests/README > > b/drivers/cpufreq/tests/README new file mode 100644 > > index 0000000..3e9cd80 > > --- /dev/null > > +++ b/drivers/cpufreq/tests/README > > @@ -0,0 +1,33 @@ > > +This file contains list of cpufreq's available regression tests > > with a short +usage description. > > + > > +1. cpufreq_freq_test.sh > > + > > +Description: > > +------------ > > +This script is supposed to test if cpufreq attributes exported by > > sysfs are +exposing correct values. > > + > > +To achieve this goal it saves the current governor and changes it > > to +"performance". Afterwards, it reads the > > "scaling_available_frequencies" +property. With the list of > > supported frequencies it is able to enforce each of +them by > > writing to "scaling_max_freq" attribute. To make the test more > > reliable +a superfluous load with gzip is created to be sure that > > we are running with +highest possible frequency. This high load is > > regulated with the 'sleep' +duration. After this time the > > "cpufreq_cur_freq" is read and compared with the +original value. > > As the last step the original governor is restored. > > I couldn't make out the purpose of this test and why we need it. How > do we ensure that "cpufreq attributes exported by sysfs are exposing > correct values"? First of all the cpufreq attributes are part of the subsystem API. There are systems which actually depend on them, so we would be better off to test if they work as intended. Secondly, the test takes those values and then with use of other attribute enforce the value, which is then read via cat'ing cpufreq_cur_freq. If any of the attributes is wrong then we will spot the error immediately. > > And actually what do we mean by this statement even? What kind of > errors can be there in exposing these values. Errors with cpufreq and CCF cooperation - especially when some parts of cpufreq code uses direct write to MUX, DIV or PLL SoC registers. Also, one can check if permutations of changing all available frequencies are working properly. This script allowed me to find at least two bugs at Exynos cpufreq subsystem in the past. And those fixes are already applied to mainline. > > I want to understand the purpose of this script very clearly first > and then only will look at the details. There is a clean shell script code to go through it, the README file with detailed description of the script purpose and operation. What else shall I write? > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On 23 July 2014 13:08, Lukasz Majewski <l.majewski@samsung.com> wrote: > Do you want to say that we have enough tests and we don't need more ? No. We don't have any tests at all :) > I always thought that we shall have as much regression tests as > possible. Yeah, tests are welcomed but the question is where should they get added. Don't know if its common to add tests directly to kernel. And also if the test is really good, not discouraging your work. >> On 21 July 2014 12:32, Lukasz Majewski <l.majewski@samsung.com> wrote: >> > This commit adds first regression test "cpufreq_freq_test.sh" for >> > the cpufreq subsystem. >> >> That's not enough, Tell us why we should continue reading this mail.. > > Hmm... If "regression" and "test" don't catch the attention of a > diligent maintainer, then I cannot do much more to encourage him to > read the whole e-mail :-) What I meant to say was, your subject and body must be good enough to answer most of the things. You don't have to tell much about the implementation but other things should be pretty clear from logs. Your current logs are quite short for something that's not a normal practice. > I can imagine that maintainers are very busy, therefore I've prepared > README file with detailed description of the script operation. Yeah, a README is welcomed and would be useful for users as well.. >> I couldn't make out the purpose of this test and why we need it. How >> do we ensure that "cpufreq attributes exported by sysfs are exposing >> correct values"? > > First of all the cpufreq attributes are part of the subsystem API. > There are systems which actually depend on them, so we would be better > off to test if they work as intended. > > Secondly, the test takes those values and then with use of other > attribute enforce the value, which is then read via cat'ing > cpufreq_cur_freq. If any of the attributes is wrong then we will spot > the error immediately. Shouldn't you use userspace governor then instead of performance? And then we don't need the gzip stuff at all. We can just set it to the right freq and get current freq to see if it matches? And now that we are starting to get tests added into the kernel (will still wait to see what Rafael has to advice), we better think of the way these are going to get added. Probably a single script with parameters like what to test? >> And actually what do we mean by this statement even? What kind of >> errors can be there in exposing these values. > > Errors with cpufreq and CCF cooperation - especially when some parts of > cpufreq code uses direct write to MUX, DIV or PLL SoC registers. > > Also, one can check if permutations of changing all available > frequencies are working properly. Yeah, that would be fine. Probably need to think more about scripts name.
Hi Viresh, > On 23 July 2014 13:08, Lukasz Majewski <l.majewski@samsung.com> wrote: > > Do you want to say that we have enough tests and we don't need > > more ? > > No. We don't have any tests at all :) Then we should encourage as many developers as possible to share their private tests with us. > > > I always thought that we shall have as much regression tests as > > possible. > > Yeah, tests are welcomed but the question is where should they get > added. Don't know if its common to add tests directly to kernel. There was a similar discussion with device tree and finally it was included in the mainline repository. > > And also if the test is really good, not discouraging your work. > > >> On 21 July 2014 12:32, Lukasz Majewski <l.majewski@samsung.com> > >> wrote: > >> > This commit adds first regression test "cpufreq_freq_test.sh" for > >> > the cpufreq subsystem. > >> > >> That's not enough, Tell us why we should continue reading this > >> mail.. > > > > Hmm... If "regression" and "test" don't catch the attention of a > > diligent maintainer, then I cannot do much more to encourage him to > > read the whole e-mail :-) > > What I meant to say was, your subject and body must be good enough > to answer most of the things. You don't have to tell much about the > implementation but other things should be pretty clear from logs. > > Your current logs are quite short for something that's not a normal > practice. It is hard for me to agree on this issue. > > > I can imagine that maintainers are very busy, therefore I've > > prepared README file with detailed description of the script > > operation. > > Yeah, a README is welcomed and would be useful for users as well.. > > >> I couldn't make out the purpose of this test and why we need it. > >> How do we ensure that "cpufreq attributes exported by sysfs are > >> exposing correct values"? > > > > First of all the cpufreq attributes are part of the subsystem API. > > There are systems which actually depend on them, so we would be > > better off to test if they work as intended. > > > > Secondly, the test takes those values and then with use of other > > attribute enforce the value, which is then read via cat'ing > > cpufreq_cur_freq. If any of the attributes is wrong then we will > > spot the error immediately. > > Shouldn't you use userspace governor then instead of performance? Performance assures that we will have the right frequency set. However, there can be a similar patch to use userspace governor and various load to fail if ondemand's frequency flipping is detected. > And then we don't need the gzip stuff at all. We can just set it to > the right freq and get current freq to see if it matches? Sometimes "interresting" things show up when you have 100% CPU load and you try to switch frequency. In my opinion usage of gzip makes the test more difficult to pass. > > And now that we are starting to get tests added into the kernel (will > still wait to see what Rafael has to advice), Ok. Lets wait for Rafael's opinion. > we better think of the > way these are going to get added. Probably a single script with > parameters like what to test? It is one possible solution, where another one is to run the all scripts in the directory. I'm curious about Rafael's opinion. > > >> And actually what do we mean by this statement even? What kind of > >> errors can be there in exposing these values. > > > > Errors with cpufreq and CCF cooperation - especially when some > > parts of cpufreq code uses direct write to MUX, DIV or PLL SoC > > registers. > > > > Also, one can check if permutations of changing all available > > frequencies are working properly. > > Yeah, that would be fine. Probably need to think more about scripts > name.
On 23 July 2014 15:40, Lukasz Majewski <l.majewski@samsung.com> wrote: >> Shouldn't you use userspace governor then instead of performance? > > Performance assures that we will have the right frequency set. Why wouldn't userspace assure that? > However, there can be a similar patch to use userspace governor and > various load to fail if ondemand's frequency flipping is detected. That's why I want to get to the motive behind this patch. AFAIU, we are checking if its fine to switch to available frequencies or not and if yes, do we actually switch to those. Right? For, this testcase we just need a single test and I still don't see why performance is better than userspace? >> And then we don't need the gzip stuff at all. We can just set it to >> the right freq and get current freq to see if it matches? > > Sometimes "interresting" things show up when you have 100% CPU load and > you try to switch frequency. That's a different test then. And that's how it should be presented. So, probably another option to the script, which isn't forced on people.
On Wednesday, July 23, 2014 02:19:54 PM Viresh Kumar wrote: > On 23 July 2014 13:08, Lukasz Majewski <l.majewski@samsung.com> wrote: > > Do you want to say that we have enough tests and we don't need more ? > > No. We don't have any tests at all :) > > > I always thought that we shall have as much regression tests as > > possible. > > Yeah, tests are welcomed but the question is where should they get added. > Don't know if its common to add tests directly to kernel. Yes, it is. > And also if the test is really good, not discouraging your work. > > >> On 21 July 2014 12:32, Lukasz Majewski <l.majewski@samsung.com> wrote: > >> > This commit adds first regression test "cpufreq_freq_test.sh" for > >> > the cpufreq subsystem. > >> > >> That's not enough, Tell us why we should continue reading this mail.. > > > > Hmm... If "regression" and "test" don't catch the attention of a > > diligent maintainer, then I cannot do much more to encourage him to > > read the whole e-mail :-) > > What I meant to say was, your subject and body must be good enough > to answer most of the things. You don't have to tell much about the > implementation but other things should be pretty clear from logs. > > Your current logs are quite short for something that's not a normal practice. > > > I can imagine that maintainers are very busy, therefore I've prepared > > README file with detailed description of the script operation. > > Yeah, a README is welcomed and would be useful for users as well.. > > >> I couldn't make out the purpose of this test and why we need it. How > >> do we ensure that "cpufreq attributes exported by sysfs are exposing > >> correct values"? > > > > First of all the cpufreq attributes are part of the subsystem API. > > There are systems which actually depend on them, so we would be better > > off to test if they work as intended. > > > > Secondly, the test takes those values and then with use of other > > attribute enforce the value, which is then read via cat'ing > > cpufreq_cur_freq. If any of the attributes is wrong then we will spot > > the error immediately. > > Shouldn't you use userspace governor then instead of performance? > And then we don't need the gzip stuff at all. We can just set it to the > right freq and get current freq to see if it matches? > > And now that we are starting to get tests added into the kernel (will still > wait to see what Rafael has to advice), we better think of the way these > are going to get added. Probably a single script with parameters like > what to test? I've had a look at the Lukasz' patch in the first iteration and I'm going to look at it again shortly. At this point I can only say that it should be clear to the user of the script what is tested, as well as what "success" and what "failure" mean.
On Wed, Jul 23, 2014 at 02:19:54PM +0530, Viresh Kumar wrote: > On 23 July 2014 13:08, Lukasz Majewski <l.majewski@samsung.com> wrote: > > Do you want to say that we have enough tests and we don't need more ? > > No. We don't have any tests at all :) Not really true. I've found bugs triggering opps using cpufreq-bench. http://marc.info/?l=linux-pm&m=138165517321579&w=2 and i hope you learned from that experience and run this tool when making changes to the core. There is an old writeup of cpufreq-bench here: https://lwn.net/Articles/339862/ and the code itself is in the mainline tree, tools/power/cpupower/bench Andrew
On Monday, July 21, 2014 09:02:34 AM Lukasz Majewski wrote: > This commit adds first regression test "cpufreq_freq_test.sh" for the > cpufreq subsystem. First of all, I'm not seeing any explanation why this script should be shipped with the kernel. What regressions it tests against in particular and how it does that. Please write that down in the changelog. It doesn't need to be very detailed. Second, I'm not sure this is the first such test (someone already mentioned cpupower). > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > --- > Changes for v2: > - Replace *_PATCH with *_PATH for variables names > - Corrected mistakes in the README file > - Providing detailed explanation of the patch in the README file > --- > drivers/cpufreq/tests/README | 33 +++++++ > drivers/cpufreq/tests/cpufreq_freq_test.sh | 149 +++++++++++++++++++++++++++++ > 2 files changed, 182 insertions(+) > create mode 100644 drivers/cpufreq/tests/README > create mode 100755 drivers/cpufreq/tests/cpufreq_freq_test.sh > > diff --git a/drivers/cpufreq/tests/README b/drivers/cpufreq/tests/README drivers/cpufreq/ is not a place for scripts. We have scripts/ for that and you can add a "power" subdirectory in there and put your script into it. Alternatively, you can use the existing tools/power/ directory for that (but then please add a subdirectory for your script). > new file mode 100644 > index 0000000..3e9cd80 > --- /dev/null > +++ b/drivers/cpufreq/tests/README > @@ -0,0 +1,33 @@ > +This file contains list of cpufreq's available regression tests with a short > +usage description. > + > +1. cpufreq_freq_test.sh > + > +Description: > +------------ > +This script is supposed to test if cpufreq attributes exported by sysfs are > +exposing correct values. > + > +To achieve this goal it saves the current governor and changes it to > +"performance". Afterwards, it reads the "scaling_available_frequencies" > +property. With the list of supported frequencies it is able to enforce each of > +them by writing to "scaling_max_freq" attribute. To make the test more reliable > +a superfluous load with gzip is created to be sure that we are running with > +highest possible frequency. This high load is regulated with the 'sleep' > +duration. After this time the "cpufreq_cur_freq" is read and compared with the > +original value. As the last step the original governor is restored. > + > +This script can work with or without BOOST enabled and helps in spotting errors > +related to cpufreq and common clock framework. > + > +Used attributes: > +---------------- > +- "scaling_available_frequencies" > +- "cpuinfo_cur_freq" > +- "scaling_governor" > +- "scaling_max_freq" > + > +Target devices: > +--------------- > + > +All devices which exports mentioned above sysfs attributes. > \ No newline at end of file > diff --git a/drivers/cpufreq/tests/cpufreq_freq_test.sh b/drivers/cpufreq/tests/cpufreq_freq_test.sh > new file mode 100755 > index 0000000..c25f05c > --- /dev/null > +++ b/drivers/cpufreq/tests/cpufreq_freq_test.sh > @@ -0,0 +1,149 @@ > +#!/bin/bash > +# > +# This file provides a simple mean to test if all declared freqs at > +# "scaling_available_frequencies" can be set and if "cpuinfo_cur_freq" > +# returns this value. > +# > +# Usage: ./cpufreq_freq_test.sh > +# Requisite: Compiled in "performance" governor > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, you can access it online at > +# http://www.gnu.org/licenses/gpl-2.0.html. > +# > +# Copyright (C) Samsung Electronics, 2014 > +# > +# Author: Lukasz Majewski <l.majewski@samsung.com> > + > +set +x > + > +COLOUR_RED="\33[31m" > +COLOUR_BLUE="\33[34m" > +COLOUR_GREEN="\33[32m" > +COLOUR_DEFAULT="\33[0m" > + > +T_PATH=/sys/devices/system/cpu/cpu0/cpufreq > +BOOST_PATH=/sys/devices/system/cpu/cpufreq > + > +if [ ! -d "$T_PATH" ]; then > + printf " $COLOUR_RED No path to CPUFREQ $COLOUR_DEFAULT\n" > + exit 1 > +fi > + > +ERRORS=0 > + > +OLD_GOV=`cat $T_PATH/scaling_governor` > +echo "CURRENT GOVERNOR: $OLD_GOV" > +echo "SET GOVERNOR: performance" > +echo "performance" > $T_PATH/scaling_governor > + > +function test_freqs1 { > + FREQS=`cat $1` > + for I in $FREQS; do > + cpufreq_set_freq $I > + if [ "$2" ]; then > + printf "$COLOUR_BLUE BOOST $COLOUR_DEFAULT" $I > + fi > + cpufreq_test_freq $I > + done > +} > + > +function test_freqs2 { > + FREQ=`cat $1` > + FREQS_ARRAY=($FREQ) > + > + for freq in ${FREQS_ARRAY[@]} > + do > + echo "REFERENCE FREQ: $freq" > + for f in ${FREQS_ARRAY[@]} > + do > + cpufreq_set_freq $freq > + echo -n "----> " > + cpufreq_set_freq $f > + cpufreq_test_freq $f > + done > + done > +} > + > +function restore { > + if [ -f $BOOST_PATH/boost ]; then > + cpufreq_boost_state $BOOST_STATE > + fi > + > + echo "SET GOVERNOR: $OLD_GOV" > + echo $OLD_GOV > $T_PATH/scaling_governor > +} > + > +function die { > + printf " $COLOUR_RED FAILED $COLOUR_DEFAULT\n" > + restore_gov > + exit 1 > +} > + > +function cpufreq_test_freq { > + gzip < /dev/urandom > /dev/null & > + pid=$! > + sleep 0.1 > + CURR_FREQ=`cat $T_PATH/cpuinfo_cur_freq` > + if [ $1 -eq $CURR_FREQ ]; then > + printf "\t$COLOUR_GREEN OK $COLOUR_DEFAULT\n" > + else > + printf "$COLOUR_RED CURRENT $CURR_FREQ $COLOUR_DEFAULT\n" > + ERRORS=`expr $ERRORS + 1` > + #die > + fi > + kill -9 $pid > + wait $! 2>/dev/null > +} > + > +function cpufreq_set_freq { > + echo $1 > $T_PATH/scaling_max_freq || die $? > + printf "FREQ:$COLOUR_GREEN %s $COLOUR_DEFAULT" $1 > +} > + > +function cpufreq_boost_state { > + echo $1 > $BOOST_PATH/boost > +} > + > +function cpufreq_boost_status { > + cat $BOOST_PATH/boost > +} > + > +if [ -f $BOOST_PATH/boost ]; then > + echo "######################################" > + echo "TEST BOOST OPERATION" > + echo "######################################" > + > + BOOST_STATE=$(cpufreq_boost_status) > + if [ $BOOST_STATE -eq 0 ]; then > + cpufreq_boost_state 1 > + fi > + test_freqs1 $T_PATH/scaling_boost_frequencies 1 > +fi > + > +echo "######################################" > +echo "TEST AVAILABLE FREQS" > +echo "######################################" > +test_freqs1 $T_PATH/scaling_available_frequencies > + > +echo "######################################" > +echo "TEST FREQS SWITCHING" > +echo "######################################" > +test_freqs2 $T_PATH/scaling_available_frequencies > + > +echo "######################################" > +echo "ERRORS: $ERRORS" > +echo "######################################" > + > +restore > +exit 0 >
Hi Rafael, > On Monday, July 21, 2014 09:02:34 AM Lukasz Majewski wrote: > > This commit adds first regression test "cpufreq_freq_test.sh" for > > the cpufreq subsystem. > > First of all, I'm not seeing any explanation why this script should be > shipped with the kernel. OK. > > What regressions it tests against in particular and Do you require SHA's/commit messages of commits which were developed to fix issues spotted with this test script? > how it does that. Is this information required in the commit message or can it stay in the README file created in the same commit? > > Please write that down in the changelog. It doesn't need to be very > detailed. > > Second, I'm not sure this is the first such test (someone already > mentioned cpupower). It's no problem for me to add such scripts to other work. > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > --- > > Changes for v2: > > - Replace *_PATCH with *_PATH for variables names > > - Corrected mistakes in the README file > > - Providing detailed explanation of the patch in the README file > > --- > > drivers/cpufreq/tests/README | 33 +++++++ > > drivers/cpufreq/tests/cpufreq_freq_test.sh | 149 > > +++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+) > > create mode 100644 drivers/cpufreq/tests/README > > create mode 100755 drivers/cpufreq/tests/cpufreq_freq_test.sh > > > > diff --git a/drivers/cpufreq/tests/README > > b/drivers/cpufreq/tests/README > > drivers/cpufreq/ is not a place for scripts. > > We have scripts/ for that and you can add a "power" subdirectory in > there and put your script into it. > > Alternatively, you can use the existing tools/power/ directory for > that (but then please add a subdirectory for your script). Ok. > > > new file mode 100644 > > index 0000000..3e9cd80 > > --- /dev/null > > +++ b/drivers/cpufreq/tests/README > > @@ -0,0 +1,33 @@ > > +This file contains list of cpufreq's available regression tests > > with a short +usage description. > > + > > +1. cpufreq_freq_test.sh > > + > > +Description: > > +------------ > > +This script is supposed to test if cpufreq attributes exported by > > sysfs are +exposing correct values. > > + > > +To achieve this goal it saves the current governor and changes it > > to +"performance". Afterwards, it reads the > > "scaling_available_frequencies" +property. With the list of > > supported frequencies it is able to enforce each of +them by > > writing to "scaling_max_freq" attribute. To make the test more > > reliable +a superfluous load with gzip is created to be sure that > > we are running with +highest possible frequency. This high load is > > regulated with the 'sleep' +duration. After this time the > > "cpufreq_cur_freq" is read and compared with the +original value. > > As the last step the original governor is restored. + +This script > > can work with or without BOOST enabled and helps in spotting errors > > +related to cpufreq and common clock framework. + > > +Used attributes: > > +---------------- > > +- "scaling_available_frequencies" > > +- "cpuinfo_cur_freq" > > +- "scaling_governor" > > +- "scaling_max_freq" > > + > > +Target devices: > > +--------------- > > + > > +All devices which exports mentioned above sysfs attributes. > > \ No newline at end of file > > diff --git a/drivers/cpufreq/tests/cpufreq_freq_test.sh > > b/drivers/cpufreq/tests/cpufreq_freq_test.sh new file mode 100755 > > index 0000000..c25f05c > > --- /dev/null > > +++ b/drivers/cpufreq/tests/cpufreq_freq_test.sh > > @@ -0,0 +1,149 @@ > > +#!/bin/bash > > +# > > +# This file provides a simple mean to test if all declared freqs at > > +# "scaling_available_frequencies" can be set and if > > "cpuinfo_cur_freq" +# returns this value. > > +# > > +# Usage: ./cpufreq_freq_test.sh > > +# Requisite: Compiled in "performance" governor > > +# > > +# This program is free software; you can redistribute it and/or > > modify +# it under the terms of the GNU General Public License as > > published by +# the Free Software Foundation; either version 2 of > > the License, or +# (at your option) any later version. > > +# > > +# This program is distributed in the hope that it will be useful, > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +# GNU General Public License for more details. > > +# > > +# You should have received a copy of the GNU General Public License > > +# along with this program; if not, you can access it online at > > +# http://www.gnu.org/licenses/gpl-2.0.html. > > +# > > +# Copyright (C) Samsung Electronics, 2014 > > +# > > +# Author: Lukasz Majewski <l.majewski@samsung.com> > > + > > +set +x > > + > > +COLOUR_RED="\33[31m" > > +COLOUR_BLUE="\33[34m" > > +COLOUR_GREEN="\33[32m" > > +COLOUR_DEFAULT="\33[0m" > > + > > +T_PATH=/sys/devices/system/cpu/cpu0/cpufreq > > +BOOST_PATH=/sys/devices/system/cpu/cpufreq > > + > > +if [ ! -d "$T_PATH" ]; then > > + printf " $COLOUR_RED No path to CPUFREQ $COLOUR_DEFAULT\n" > > + exit 1 > > +fi > > + > > +ERRORS=0 > > + > > +OLD_GOV=`cat $T_PATH/scaling_governor` > > +echo "CURRENT GOVERNOR: $OLD_GOV" > > +echo "SET GOVERNOR: performance" > > +echo "performance" > $T_PATH/scaling_governor > > + > > +function test_freqs1 { > > + FREQS=`cat $1` > > + for I in $FREQS; do > > + cpufreq_set_freq $I > > + if [ "$2" ]; then > > + printf "$COLOUR_BLUE BOOST $COLOUR_DEFAULT" $I > > + fi > > + cpufreq_test_freq $I > > + done > > +} > > + > > +function test_freqs2 { > > + FREQ=`cat $1` > > + FREQS_ARRAY=($FREQ) > > + > > + for freq in ${FREQS_ARRAY[@]} > > + do > > + echo "REFERENCE FREQ: $freq" > > + for f in ${FREQS_ARRAY[@]} > > + do > > + cpufreq_set_freq $freq > > + echo -n "----> " > > + cpufreq_set_freq $f > > + cpufreq_test_freq $f > > + done > > + done > > +} > > + > > +function restore { > > + if [ -f $BOOST_PATH/boost ]; then > > + cpufreq_boost_state $BOOST_STATE > > + fi > > + > > + echo "SET GOVERNOR: $OLD_GOV" > > + echo $OLD_GOV > $T_PATH/scaling_governor > > +} > > + > > +function die { > > + printf " $COLOUR_RED FAILED $COLOUR_DEFAULT\n" > > + restore_gov > > + exit 1 > > +} > > + > > +function cpufreq_test_freq { > > + gzip < /dev/urandom > /dev/null & > > + pid=$! > > + sleep 0.1 > > + CURR_FREQ=`cat $T_PATH/cpuinfo_cur_freq` > > + if [ $1 -eq $CURR_FREQ ]; then > > + printf "\t$COLOUR_GREEN OK $COLOUR_DEFAULT\n" > > + else > > + printf "$COLOUR_RED CURRENT $CURR_FREQ $COLOUR_DEFAULT\n" > > + ERRORS=`expr $ERRORS + 1` > > + #die > > + fi > > + kill -9 $pid > > + wait $! 2>/dev/null > > +} > > + > > +function cpufreq_set_freq { > > + echo $1 > $T_PATH/scaling_max_freq || die $? > > + printf "FREQ:$COLOUR_GREEN %s $COLOUR_DEFAULT" $1 > > +} > > + > > +function cpufreq_boost_state { > > + echo $1 > $BOOST_PATH/boost > > +} > > + > > +function cpufreq_boost_status { > > + cat $BOOST_PATH/boost > > +} > > + > > +if [ -f $BOOST_PATH/boost ]; then > > + echo "######################################" > > + echo "TEST BOOST OPERATION" > > + echo "######################################" > > + > > + BOOST_STATE=$(cpufreq_boost_status) > > + if [ $BOOST_STATE -eq 0 ]; then > > + cpufreq_boost_state 1 > > + fi > > + test_freqs1 $T_PATH/scaling_boost_frequencies 1 > > +fi > > + > > +echo "######################################" > > +echo "TEST AVAILABLE FREQS" > > +echo "######################################" > > +test_freqs1 $T_PATH/scaling_available_frequencies > > + > > +echo "######################################" > > +echo "TEST FREQS SWITCHING" > > +echo "######################################" > > +test_freqs2 $T_PATH/scaling_available_frequencies > > + > > +echo "######################################" > > +echo "ERRORS: $ERRORS" > > +echo "######################################" > > + > > +restore > > +exit 0 > > >
On Wed, Jul 23, 2014 at 11:10:42AM +0100, Lukasz Majewski wrote: > Hi Viresh, > > > On 23 July 2014 13:08, Lukasz Majewski <l.majewski@samsung.com> wrote: > > > Do you want to say that we have enough tests and we don't need > > > more ? > > > > No. We don't have any tests at all :) > > Then we should encourage as many developers as possible to share their > private tests with us. Linaro publishes their regression tests here: https://git.linaro.org/tools/pm-qa.git Description: https://wiki.linaro.org/WorkingGroups/PowerManagement/Resources/TestSuite/PmQaSpecification#Cpufreq I have only used the thermal ones, I can't comment on the cpufreq tests. Cheers, Javi
On Thursday, July 24, 2014 09:04:02 AM Lukasz Majewski wrote: > Hi Rafael, > > > On Monday, July 21, 2014 09:02:34 AM Lukasz Majewski wrote: > > > This commit adds first regression test "cpufreq_freq_test.sh" for > > > the cpufreq subsystem. > > > > First of all, I'm not seeing any explanation why this script should be > > shipped with the kernel. > > OK. > > > > > What regressions it tests against in particular and > > Do you require SHA's/commit messages of commits which were developed to > fix issues spotted with this test script? No. I want information about what kind of bugs can be catched with the help of this script. > > how it does that. > > Is this information required in the commit message or can it stay in > the README file created in the same commit? There should be *some* information in the changelog too.
diff --git a/drivers/cpufreq/tests/README b/drivers/cpufreq/tests/README new file mode 100644 index 0000000..3e9cd80 --- /dev/null +++ b/drivers/cpufreq/tests/README @@ -0,0 +1,33 @@ +This file contains list of cpufreq's available regression tests with a short +usage description. + +1. cpufreq_freq_test.sh + +Description: +------------ +This script is supposed to test if cpufreq attributes exported by sysfs are +exposing correct values. + +To achieve this goal it saves the current governor and changes it to +"performance". Afterwards, it reads the "scaling_available_frequencies" +property. With the list of supported frequencies it is able to enforce each of +them by writing to "scaling_max_freq" attribute. To make the test more reliable +a superfluous load with gzip is created to be sure that we are running with +highest possible frequency. This high load is regulated with the 'sleep' +duration. After this time the "cpufreq_cur_freq" is read and compared with the +original value. As the last step the original governor is restored. + +This script can work with or without BOOST enabled and helps in spotting errors +related to cpufreq and common clock framework. + +Used attributes: +---------------- +- "scaling_available_frequencies" +- "cpuinfo_cur_freq" +- "scaling_governor" +- "scaling_max_freq" + +Target devices: +--------------- + +All devices which exports mentioned above sysfs attributes. \ No newline at end of file diff --git a/drivers/cpufreq/tests/cpufreq_freq_test.sh b/drivers/cpufreq/tests/cpufreq_freq_test.sh new file mode 100755 index 0000000..c25f05c --- /dev/null +++ b/drivers/cpufreq/tests/cpufreq_freq_test.sh @@ -0,0 +1,149 @@ +#!/bin/bash +# +# This file provides a simple mean to test if all declared freqs at +# "scaling_available_frequencies" can be set and if "cpuinfo_cur_freq" +# returns this value. +# +# Usage: ./cpufreq_freq_test.sh +# Requisite: Compiled in "performance" governor +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, you can access it online at +# http://www.gnu.org/licenses/gpl-2.0.html. +# +# Copyright (C) Samsung Electronics, 2014 +# +# Author: Lukasz Majewski <l.majewski@samsung.com> + +set +x + +COLOUR_RED="\33[31m" +COLOUR_BLUE="\33[34m" +COLOUR_GREEN="\33[32m" +COLOUR_DEFAULT="\33[0m" + +T_PATH=/sys/devices/system/cpu/cpu0/cpufreq +BOOST_PATH=/sys/devices/system/cpu/cpufreq + +if [ ! -d "$T_PATH" ]; then + printf " $COLOUR_RED No path to CPUFREQ $COLOUR_DEFAULT\n" + exit 1 +fi + +ERRORS=0 + +OLD_GOV=`cat $T_PATH/scaling_governor` +echo "CURRENT GOVERNOR: $OLD_GOV" +echo "SET GOVERNOR: performance" +echo "performance" > $T_PATH/scaling_governor + +function test_freqs1 { + FREQS=`cat $1` + for I in $FREQS; do + cpufreq_set_freq $I + if [ "$2" ]; then + printf "$COLOUR_BLUE BOOST $COLOUR_DEFAULT" $I + fi + cpufreq_test_freq $I + done +} + +function test_freqs2 { + FREQ=`cat $1` + FREQS_ARRAY=($FREQ) + + for freq in ${FREQS_ARRAY[@]} + do + echo "REFERENCE FREQ: $freq" + for f in ${FREQS_ARRAY[@]} + do + cpufreq_set_freq $freq + echo -n "----> " + cpufreq_set_freq $f + cpufreq_test_freq $f + done + done +} + +function restore { + if [ -f $BOOST_PATH/boost ]; then + cpufreq_boost_state $BOOST_STATE + fi + + echo "SET GOVERNOR: $OLD_GOV" + echo $OLD_GOV > $T_PATH/scaling_governor +} + +function die { + printf " $COLOUR_RED FAILED $COLOUR_DEFAULT\n" + restore_gov + exit 1 +} + +function cpufreq_test_freq { + gzip < /dev/urandom > /dev/null & + pid=$! + sleep 0.1 + CURR_FREQ=`cat $T_PATH/cpuinfo_cur_freq` + if [ $1 -eq $CURR_FREQ ]; then + printf "\t$COLOUR_GREEN OK $COLOUR_DEFAULT\n" + else + printf "$COLOUR_RED CURRENT $CURR_FREQ $COLOUR_DEFAULT\n" + ERRORS=`expr $ERRORS + 1` + #die + fi + kill -9 $pid + wait $! 2>/dev/null +} + +function cpufreq_set_freq { + echo $1 > $T_PATH/scaling_max_freq || die $? + printf "FREQ:$COLOUR_GREEN %s $COLOUR_DEFAULT" $1 +} + +function cpufreq_boost_state { + echo $1 > $BOOST_PATH/boost +} + +function cpufreq_boost_status { + cat $BOOST_PATH/boost +} + +if [ -f $BOOST_PATH/boost ]; then + echo "######################################" + echo "TEST BOOST OPERATION" + echo "######################################" + + BOOST_STATE=$(cpufreq_boost_status) + if [ $BOOST_STATE -eq 0 ]; then + cpufreq_boost_state 1 + fi + test_freqs1 $T_PATH/scaling_boost_frequencies 1 +fi + +echo "######################################" +echo "TEST AVAILABLE FREQS" +echo "######################################" +test_freqs1 $T_PATH/scaling_available_frequencies + +echo "######################################" +echo "TEST FREQS SWITCHING" +echo "######################################" +test_freqs2 $T_PATH/scaling_available_frequencies + +echo "######################################" +echo "ERRORS: $ERRORS" +echo "######################################" + +restore +exit 0
This commit adds first regression test "cpufreq_freq_test.sh" for the cpufreq subsystem. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> --- Changes for v2: - Replace *_PATCH with *_PATH for variables names - Corrected mistakes in the README file - Providing detailed explanation of the patch in the README file --- drivers/cpufreq/tests/README | 33 +++++++ drivers/cpufreq/tests/cpufreq_freq_test.sh | 149 +++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+) create mode 100644 drivers/cpufreq/tests/README create mode 100755 drivers/cpufreq/tests/cpufreq_freq_test.sh