| Message ID | 20240702101259.1251377-2-usama.anjum@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] selftests/thermel/intel: conform the test to TAP output | expand |
On Tue, 2024-07-02 at 15:12 +0500, Muhammad Usama Anjum wrote: > Conform the layout, informational and status messages to TAP. No > functional change is intended other than the layout of output > messages. > Not true. You did functional change by adding a break in the loop. The purpose here to wait for these message continuously till ctrl-c or similar. > The test has infinite loop to read the value of status_str. Break the > loop after getting the value once and finish the test. > No, that is not the purpose. > Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > --- > Changes since v1: > - Use ksft_exit_fail_perror if read() returns error > - Break the infinite loop after printing status_str > --- > .../intel/power_floor/power_floor_test.c | 70 ++++++++--------- > -- > 1 file changed, 30 insertions(+), 40 deletions(-) > > diff --git > a/tools/testing/selftests/thermal/intel/power_floor/power_floor_test. > c > b/tools/testing/selftests/thermal/intel/power_floor/power_floor_test. > c > index 0326b39a11b91..c06b275acd36b 100644 > --- > a/tools/testing/selftests/thermal/intel/power_floor/power_floor_test. > c > +++ > b/tools/testing/selftests/thermal/intel/power_floor/power_floor_test. > c > @@ -9,6 +9,7 @@ > #include <fcntl.h> > #include <poll.h> > #include <signal.h> > +#include "../../../kselftest.h" > > #define POWER_FLOOR_ENABLE_ATTRIBUTE > "/sys/bus/pci/devices/0000:00:04.0/power_limits/power_floor_enable" > #define POWER_FLOOR_STATUS_ATTRIBUTE > "/sys/bus/pci/devices/0000:00:04.0/power_limits/power_floor_status" > @@ -20,17 +21,13 @@ void power_floor_exit(int signum) > /* Disable feature via sysfs knob */ > > fd = open(POWER_FLOOR_ENABLE_ATTRIBUTE, O_RDWR); > - if (fd < 0) { > - perror("Unable to open power floor enable file\n"); > - exit(1); > - } > + if (fd < 0) > + ksft_exit_fail_perror("Unable to open power floor > enable file"); > > - if (write(fd, "0\n", 2) < 0) { > - perror("Can' disable power floor notifications\n"); > - exit(1); > - } > + if (write(fd, "0\n", 2) < 0) > + ksft_exit_fail_perror("Can' disable power floor > notifications"); > > - printf("Disabled power floor notifications\n"); > + ksft_print_msg("Disabled power floor notifications\n"); > > close(fd); > } > @@ -41,6 +38,9 @@ int main(int argc, char **argv) > char status_str[3]; > int fd, ret; > > + ksft_print_header(); > + ksft_set_plan(1); > + > if (signal(SIGINT, power_floor_exit) == SIG_IGN) > signal(SIGINT, SIG_IGN); > if (signal(SIGHUP, power_floor_exit) == SIG_IGN) > @@ -50,59 +50,49 @@ int main(int argc, char **argv) > > /* Enable feature via sysfs knob */ > fd = open(POWER_FLOOR_ENABLE_ATTRIBUTE, O_RDWR); > - if (fd < 0) { > - perror("Unable to open power floor enable file\n"); > - exit(1); > - } > + if (fd < 0) > + ksft_exit_fail_perror("Unable to open power floor > enable file"); > > - if (write(fd, "1\n", 2) < 0) { > - perror("Can' enable power floor notifications\n"); > - exit(1); > - } > + if (write(fd, "1\n", 2) < 0) > + ksft_exit_fail_perror("Can' enable power floor > notifications"); > > close(fd); > > - printf("Enabled power floor notifications\n"); > + ksft_print_msg("Enabled power floor notifications\n"); > > while (1) { > fd = open(POWER_FLOOR_STATUS_ATTRIBUTE, O_RDONLY); > - if (fd < 0) { > - perror("Unable to power floor status > file\n"); > - exit(1); > - } > + if (fd < 0) > + ksft_exit_fail_perror("Unable to power floor > status file"); > > - if ((lseek(fd, 0L, SEEK_SET)) < 0) { > - fprintf(stderr, "Failed to set pointer to > beginning\n"); > - exit(1); > - } > + if ((lseek(fd, 0L, SEEK_SET)) < 0) > + ksft_exit_fail_perror("Failed to set pointer > to beginning\n"); > > - if (read(fd, status_str, sizeof(status_str)) < 0) { > - fprintf(stderr, "Failed to read from:%s\n", > - POWER_FLOOR_STATUS_ATTRIBUTE); > - exit(1); > - } > + if (read(fd, status_str, sizeof(status_str)) < 0) > + ksft_exit_fail_perror("Failed to read from: > power_floor_status"); > > ufd.fd = fd; > ufd.events = POLLPRI; > > ret = poll(&ufd, 1, -1); > if (ret < 0) { > - perror("poll error"); > - exit(1); > + ksft_exit_fail_msg("Poll error\n"); > } else if (ret == 0) { > - printf("Poll Timeout\n"); > + ksft_print_msg("Poll Timeout\n"); > } else { > - if ((lseek(fd, 0L, SEEK_SET)) < 0) { > - fprintf(stderr, "Failed to set > pointer to beginning\n"); > - exit(1); > - } > + if ((lseek(fd, 0L, SEEK_SET)) < 0) > + ksft_exit_fail_msg("Failed to set > pointer to beginning\n"); > > if (read(fd, status_str, sizeof(status_str)) > < 0) > - exit(0); > + ksft_exit_fail_perror("Failed to > read"); > > - printf("power floor status: %s\n", > status_str); > + ksft_print_msg("power floor status: %s\n", > status_str); > + break; > } > > close(fd); > } > + > + ksft_test_result_pass("Successfully read\n"); > + ksft_finished(); > }
On 7/3/24 9:40 AM, srinivas pandruvada wrote: > On Tue, 2024-07-02 at 15:12 +0500, Muhammad Usama Anjum wrote: >> Conform the layout, informational and status messages to TAP. No >> functional change is intended other than the layout of output >> messages. >> > Not true. You did functional change by adding a break in the loop. > The purpose here to wait for these message continuously till ctrl-c or > similar. I'll correct the description. > >> The test has infinite loop to read the value of status_str. Break the >> loop after getting the value once and finish the test. >> > No, that is not the purpose. We want to finish the test after some iterations as in CIs we cannot abort the test by sending signals. Would 10 or 100 iterations be enough for this test? Or it is the kind of test which wouldn't get events until some other process is using the same driver files? > >> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> >> --- >> Changes since v1: >> - Use ksft_exit_fail_perror if read() returns error >> - Break the infinite loop after printing status_str >> --- >> .../intel/power_floor/power_floor_test.c | 70 ++++++++--------- >> -- >> 1 file changed, 30 insertions(+), 40 deletions(-) >> >> diff --git >> a/tools/testing/selftests/thermal/intel/power_floor/power_floor_test. >> c >> b/tools/testing/selftests/thermal/intel/power_floor/power_floor_test. >> c >> index 0326b39a11b91..c06b275acd36b 100644 >> --- >> a/tools/testing/selftests/thermal/intel/power_floor/power_floor_test. >> c >> +++ >> b/tools/testing/selftests/thermal/intel/power_floor/power_floor_test. >> c >> @@ -9,6 +9,7 @@ >> #include <fcntl.h> >> #include <poll.h> >> #include <signal.h> >> +#include "../../../kselftest.h" >> >> #define POWER_FLOOR_ENABLE_ATTRIBUTE >> "/sys/bus/pci/devices/0000:00:04.0/power_limits/power_floor_enable" >> #define POWER_FLOOR_STATUS_ATTRIBUTE >> "/sys/bus/pci/devices/0000:00:04.0/power_limits/power_floor_status" >> @@ -20,17 +21,13 @@ void power_floor_exit(int signum) >> /* Disable feature via sysfs knob */ >> >> fd = open(POWER_FLOOR_ENABLE_ATTRIBUTE, O_RDWR); >> - if (fd < 0) { >> - perror("Unable to open power floor enable file\n"); >> - exit(1); >> - } >> + if (fd < 0) >> + ksft_exit_fail_perror("Unable to open power floor >> enable file"); >> >> - if (write(fd, "0\n", 2) < 0) { >> - perror("Can' disable power floor notifications\n"); >> - exit(1); >> - } >> + if (write(fd, "0\n", 2) < 0) >> + ksft_exit_fail_perror("Can' disable power floor >> notifications"); >> >> - printf("Disabled power floor notifications\n"); >> + ksft_print_msg("Disabled power floor notifications\n"); >> >> close(fd); >> } >> @@ -41,6 +38,9 @@ int main(int argc, char **argv) >> char status_str[3]; >> int fd, ret; >> >> + ksft_print_header(); >> + ksft_set_plan(1); >> + >> if (signal(SIGINT, power_floor_exit) == SIG_IGN) >> signal(SIGINT, SIG_IGN); >> if (signal(SIGHUP, power_floor_exit) == SIG_IGN) >> @@ -50,59 +50,49 @@ int main(int argc, char **argv) >> >> /* Enable feature via sysfs knob */ >> fd = open(POWER_FLOOR_ENABLE_ATTRIBUTE, O_RDWR); >> - if (fd < 0) { >> - perror("Unable to open power floor enable file\n"); >> - exit(1); >> - } >> + if (fd < 0) >> + ksft_exit_fail_perror("Unable to open power floor >> enable file"); >> >> - if (write(fd, "1\n", 2) < 0) { >> - perror("Can' enable power floor notifications\n"); >> - exit(1); >> - } >> + if (write(fd, "1\n", 2) < 0) >> + ksft_exit_fail_perror("Can' enable power floor >> notifications"); >> >> close(fd); >> >> - printf("Enabled power floor notifications\n"); >> + ksft_print_msg("Enabled power floor notifications\n"); >> >> while (1) { >> fd = open(POWER_FLOOR_STATUS_ATTRIBUTE, O_RDONLY); >> - if (fd < 0) { >> - perror("Unable to power floor status >> file\n"); >> - exit(1); >> - } >> + if (fd < 0) >> + ksft_exit_fail_perror("Unable to power floor >> status file"); >> >> - if ((lseek(fd, 0L, SEEK_SET)) < 0) { >> - fprintf(stderr, "Failed to set pointer to >> beginning\n"); >> - exit(1); >> - } >> + if ((lseek(fd, 0L, SEEK_SET)) < 0) >> + ksft_exit_fail_perror("Failed to set pointer >> to beginning\n"); >> >> - if (read(fd, status_str, sizeof(status_str)) < 0) { >> - fprintf(stderr, "Failed to read from:%s\n", >> - POWER_FLOOR_STATUS_ATTRIBUTE); >> - exit(1); >> - } >> + if (read(fd, status_str, sizeof(status_str)) < 0) >> + ksft_exit_fail_perror("Failed to read from: >> power_floor_status"); >> >> ufd.fd = fd; >> ufd.events = POLLPRI; >> >> ret = poll(&ufd, 1, -1); >> if (ret < 0) { >> - perror("poll error"); >> - exit(1); >> + ksft_exit_fail_msg("Poll error\n"); >> } else if (ret == 0) { >> - printf("Poll Timeout\n"); >> + ksft_print_msg("Poll Timeout\n"); >> } else { >> - if ((lseek(fd, 0L, SEEK_SET)) < 0) { >> - fprintf(stderr, "Failed to set >> pointer to beginning\n"); >> - exit(1); >> - } >> + if ((lseek(fd, 0L, SEEK_SET)) < 0) >> + ksft_exit_fail_msg("Failed to set >> pointer to beginning\n"); >> >> if (read(fd, status_str, sizeof(status_str)) >> < 0) >> - exit(0); >> + ksft_exit_fail_perror("Failed to >> read"); >> >> - printf("power floor status: %s\n", >> status_str); >> + ksft_print_msg("power floor status: %s\n", >> status_str); >> + break; >> } >> >> close(fd); >> } >> + >> + ksft_test_result_pass("Successfully read\n"); >> + ksft_finished(); >> } > >
On Wed, 2024-07-03 at 11:19 +0500, Muhammad Usama Anjum wrote: > On 7/3/24 9:40 AM, srinivas pandruvada wrote: > > On Tue, 2024-07-02 at 15:12 +0500, Muhammad Usama Anjum wrote: > > > Conform the layout, informational and status messages to TAP. No > > > functional change is intended other than the layout of output > > > messages. > > > > > Not true. You did functional change by adding a break in the loop. > > The purpose here to wait for these message continuously till ctrl-c > > or > > similar. > I'll correct the description. > > > > > > The test has infinite loop to read the value of status_str. Break > > > the > > > loop after getting the value once and finish the test. > > > > > No, that is not the purpose. > We want to finish the test after some iterations as in CIs we cannot > abort > the test by sending signals. Would 10 or 100 iterations be enough for > this > test? Or it is the kind of test which wouldn't get events until some > other > process is using the same driver files? > You generate events, you need to do workload and reduce power limit to trigger. Otherwise these are rare events. Thanks, Srinivas > > > > > Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> > > > --- > > > Changes since v1: > > > - Use ksft_exit_fail_perror if read() returns error > > > - Break the infinite loop after printing status_str > > > --- > > > .../intel/power_floor/power_floor_test.c | 70 ++++++++----- > > > ---- > > > -- > > > 1 file changed, 30 insertions(+), 40 deletions(-) > > > > > > diff --git > > > a/tools/testing/selftests/thermal/intel/power_floor/power_floor_t > > > est. > > > c > > > b/tools/testing/selftests/thermal/intel/power_floor/power_floor_t > > > est. > > > c > > > index 0326b39a11b91..c06b275acd36b 100644 > > > --- > > > a/tools/testing/selftests/thermal/intel/power_floor/power_floor_t > > > est. > > > c > > > +++ > > > b/tools/testing/selftests/thermal/intel/power_floor/power_floor_t > > > est. > > > c > > > @@ -9,6 +9,7 @@ > > > #include <fcntl.h> > > > #include <poll.h> > > > #include <signal.h> > > > +#include "../../../kselftest.h" > > > > > > #define POWER_FLOOR_ENABLE_ATTRIBUTE > > > "/sys/bus/pci/devices/0000:00:04.0/power_limits/power_floor_enabl > > > e" > > > #define POWER_FLOOR_STATUS_ATTRIBUTE > > > "/sys/bus/pci/devices/0000:00:04.0/power_limits/power_floor_statu > > > s" > > > @@ -20,17 +21,13 @@ void power_floor_exit(int signum) > > > /* Disable feature via sysfs knob */ > > > > > > fd = open(POWER_FLOOR_ENABLE_ATTRIBUTE, O_RDWR); > > > - if (fd < 0) { > > > - perror("Unable to open power floor enable > > > file\n"); > > > - exit(1); > > > - } > > > + if (fd < 0) > > > + ksft_exit_fail_perror("Unable to open power > > > floor > > > enable file"); > > > > > > - if (write(fd, "0\n", 2) < 0) { > > > - perror("Can' disable power floor > > > notifications\n"); > > > - exit(1); > > > - } > > > + if (write(fd, "0\n", 2) < 0) > > > + ksft_exit_fail_perror("Can' disable power floor > > > notifications"); > > > > > > - printf("Disabled power floor notifications\n"); > > > + ksft_print_msg("Disabled power floor notifications\n"); > > > > > > close(fd); > > > } > > > @@ -41,6 +38,9 @@ int main(int argc, char **argv) > > > char status_str[3]; > > > int fd, ret; > > > > > > + ksft_print_header(); > > > + ksft_set_plan(1); > > > + > > > if (signal(SIGINT, power_floor_exit) == SIG_IGN) > > > signal(SIGINT, SIG_IGN); > > > if (signal(SIGHUP, power_floor_exit) == SIG_IGN) > > > @@ -50,59 +50,49 @@ int main(int argc, char **argv) > > > > > > /* Enable feature via sysfs knob */ > > > fd = open(POWER_FLOOR_ENABLE_ATTRIBUTE, O_RDWR); > > > - if (fd < 0) { > > > - perror("Unable to open power floor enable > > > file\n"); > > > - exit(1); > > > - } > > > + if (fd < 0) > > > + ksft_exit_fail_perror("Unable to open power > > > floor > > > enable file"); > > > > > > - if (write(fd, "1\n", 2) < 0) { > > > - perror("Can' enable power floor > > > notifications\n"); > > > - exit(1); > > > - } > > > + if (write(fd, "1\n", 2) < 0) > > > + ksft_exit_fail_perror("Can' enable power floor > > > notifications"); > > > > > > close(fd); > > > > > > - printf("Enabled power floor notifications\n"); > > > + ksft_print_msg("Enabled power floor notifications\n"); > > > > > > while (1) { > > > fd = open(POWER_FLOOR_STATUS_ATTRIBUTE, > > > O_RDONLY); > > > - if (fd < 0) { > > > - perror("Unable to power floor status > > > file\n"); > > > - exit(1); > > > - } > > > + if (fd < 0) > > > + ksft_exit_fail_perror("Unable to power > > > floor > > > status file"); > > > > > > - if ((lseek(fd, 0L, SEEK_SET)) < 0) { > > > - fprintf(stderr, "Failed to set pointer > > > to > > > beginning\n"); > > > - exit(1); > > > - } > > > + if ((lseek(fd, 0L, SEEK_SET)) < 0) > > > + ksft_exit_fail_perror("Failed to set > > > pointer > > > to beginning\n"); > > > > > > - if (read(fd, status_str, sizeof(status_str)) < > > > 0) { > > > - fprintf(stderr, "Failed to read > > > from:%s\n", > > > - POWER_FLOOR_STATUS_ATTRIBUTE); > > > - exit(1); > > > - } > > > + if (read(fd, status_str, sizeof(status_str)) < > > > 0) > > > + ksft_exit_fail_perror("Failed to read > > > from: > > > power_floor_status"); > > > > > > ufd.fd = fd; > > > ufd.events = POLLPRI; > > > > > > ret = poll(&ufd, 1, -1); > > > if (ret < 0) { > > > - perror("poll error"); > > > - exit(1); > > > + ksft_exit_fail_msg("Poll error\n"); > > > } else if (ret == 0) { > > > - printf("Poll Timeout\n"); > > > + ksft_print_msg("Poll Timeout\n"); > > > } else { > > > - if ((lseek(fd, 0L, SEEK_SET)) < 0) { > > > - fprintf(stderr, "Failed to set > > > pointer to beginning\n"); > > > - exit(1); > > > - } > > > + if ((lseek(fd, 0L, SEEK_SET)) < 0) > > > + ksft_exit_fail_msg("Failed to > > > set > > > pointer to beginning\n"); > > > > > > if (read(fd, status_str, > > > sizeof(status_str)) > > > < 0) > > > - exit(0); > > > + ksft_exit_fail_perror("Failed to > > > read"); > > > > > > - printf("power floor status: %s\n", > > > status_str); > > > + ksft_print_msg("power floor status: > > > %s\n", > > > status_str); > > > + break; > > > } > > > > > > close(fd); > > > } > > > + > > > + ksft_test_result_pass("Successfully read\n"); > > > + ksft_finished(); > > > } > > > > >
diff --git a/tools/testing/selftests/thermal/intel/power_floor/power_floor_test.c b/tools/testing/selftests/thermal/intel/power_floor/power_floor_test.c index 0326b39a11b91..c06b275acd36b 100644 --- a/tools/testing/selftests/thermal/intel/power_floor/power_floor_test.c +++ b/tools/testing/selftests/thermal/intel/power_floor/power_floor_test.c @@ -9,6 +9,7 @@ #include <fcntl.h> #include <poll.h> #include <signal.h> +#include "../../../kselftest.h" #define POWER_FLOOR_ENABLE_ATTRIBUTE "/sys/bus/pci/devices/0000:00:04.0/power_limits/power_floor_enable" #define POWER_FLOOR_STATUS_ATTRIBUTE "/sys/bus/pci/devices/0000:00:04.0/power_limits/power_floor_status" @@ -20,17 +21,13 @@ void power_floor_exit(int signum) /* Disable feature via sysfs knob */ fd = open(POWER_FLOOR_ENABLE_ATTRIBUTE, O_RDWR); - if (fd < 0) { - perror("Unable to open power floor enable file\n"); - exit(1); - } + if (fd < 0) + ksft_exit_fail_perror("Unable to open power floor enable file"); - if (write(fd, "0\n", 2) < 0) { - perror("Can' disable power floor notifications\n"); - exit(1); - } + if (write(fd, "0\n", 2) < 0) + ksft_exit_fail_perror("Can' disable power floor notifications"); - printf("Disabled power floor notifications\n"); + ksft_print_msg("Disabled power floor notifications\n"); close(fd); } @@ -41,6 +38,9 @@ int main(int argc, char **argv) char status_str[3]; int fd, ret; + ksft_print_header(); + ksft_set_plan(1); + if (signal(SIGINT, power_floor_exit) == SIG_IGN) signal(SIGINT, SIG_IGN); if (signal(SIGHUP, power_floor_exit) == SIG_IGN) @@ -50,59 +50,49 @@ int main(int argc, char **argv) /* Enable feature via sysfs knob */ fd = open(POWER_FLOOR_ENABLE_ATTRIBUTE, O_RDWR); - if (fd < 0) { - perror("Unable to open power floor enable file\n"); - exit(1); - } + if (fd < 0) + ksft_exit_fail_perror("Unable to open power floor enable file"); - if (write(fd, "1\n", 2) < 0) { - perror("Can' enable power floor notifications\n"); - exit(1); - } + if (write(fd, "1\n", 2) < 0) + ksft_exit_fail_perror("Can' enable power floor notifications"); close(fd); - printf("Enabled power floor notifications\n"); + ksft_print_msg("Enabled power floor notifications\n"); while (1) { fd = open(POWER_FLOOR_STATUS_ATTRIBUTE, O_RDONLY); - if (fd < 0) { - perror("Unable to power floor status file\n"); - exit(1); - } + if (fd < 0) + ksft_exit_fail_perror("Unable to power floor status file"); - if ((lseek(fd, 0L, SEEK_SET)) < 0) { - fprintf(stderr, "Failed to set pointer to beginning\n"); - exit(1); - } + if ((lseek(fd, 0L, SEEK_SET)) < 0) + ksft_exit_fail_perror("Failed to set pointer to beginning\n"); - if (read(fd, status_str, sizeof(status_str)) < 0) { - fprintf(stderr, "Failed to read from:%s\n", - POWER_FLOOR_STATUS_ATTRIBUTE); - exit(1); - } + if (read(fd, status_str, sizeof(status_str)) < 0) + ksft_exit_fail_perror("Failed to read from: power_floor_status"); ufd.fd = fd; ufd.events = POLLPRI; ret = poll(&ufd, 1, -1); if (ret < 0) { - perror("poll error"); - exit(1); + ksft_exit_fail_msg("Poll error\n"); } else if (ret == 0) { - printf("Poll Timeout\n"); + ksft_print_msg("Poll Timeout\n"); } else { - if ((lseek(fd, 0L, SEEK_SET)) < 0) { - fprintf(stderr, "Failed to set pointer to beginning\n"); - exit(1); - } + if ((lseek(fd, 0L, SEEK_SET)) < 0) + ksft_exit_fail_msg("Failed to set pointer to beginning\n"); if (read(fd, status_str, sizeof(status_str)) < 0) - exit(0); + ksft_exit_fail_perror("Failed to read"); - printf("power floor status: %s\n", status_str); + ksft_print_msg("power floor status: %s\n", status_str); + break; } close(fd); } + + ksft_test_result_pass("Successfully read\n"); + ksft_finished(); }
Conform the layout, informational and status messages to TAP. No functional change is intended other than the layout of output messages. The test has infinite loop to read the value of status_str. Break the loop after getting the value once and finish the test. Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com> --- Changes since v1: - Use ksft_exit_fail_perror if read() returns error - Break the infinite loop after printing status_str --- .../intel/power_floor/power_floor_test.c | 70 ++++++++----------- 1 file changed, 30 insertions(+), 40 deletions(-)