Re: [PATCH v2 2/2] selftests/thermel/intel: conform the test to TAP output

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

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();
> > >  }
> > 
> > 
>