[PATCH 0/2] Modify the watchdog selftest for execution with kselftest runner

[PATCH 0/2] Modify the watchdog selftest for execution with kselftest runner

[Laura Nao]

The watchdog selftest script supports various parameters for testing
different IOCTLs. The watchdog ping functionality is validated by starting
a loop where the watchdog device is periodically pet, which can only be
stopped by the user interrupting the script.

This results in a timeout when running this test using the kselftest runner
with no non-oneshot parameters (or no parameters at all):

 TAP version 13
 1..1
 # timeout set to 45
 # selftests: watchdog: watchdog-test
 # Watchdog Ticking Away!
 # .............................................#
 not ok 1 selftests: watchdog: watchdog-test # TIMEOUT 45 seconds

To address this issue, the first patch in this series limits the loop to 5
iterations by default and adds support for a new '-c' option to customize
the number of pings as required.

The second patch conforms the test output to the KTAP format.

Laura Nao (2):
  selftests/watchdog: limit ping loop and allow configuring the number
    of pings
  selftests/watchdog: convert the test output to KTAP format

 .../selftests/watchdog/watchdog-test.c        | 166 +++++++++++-------
 1 file changed, 101 insertions(+), 65 deletions(-)

-- 
2.30.2

[PATCH 1/2] selftests/watchdog: limit ping loop and allow configuring the number of pings

[Laura Nao]

In order to run the watchdog selftest with the kselftest runner, the
loop responsible for pinging the watchdog should be finite. This
change limits the loop to 5 iterations by default and introduces a new
'-c' option to adjust the number of pings as needed.

Signed-off-by: Laura Nao <laura.nao@collabora.com>
---
 tools/testing/selftests/watchdog/watchdog-test.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
index bc71cbca0dde..786cc5a26206 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -24,16 +24,18 @@
 #include <linux/watchdog.h>
 
 #define DEFAULT_PING_RATE	1
+#define DEFAULT_PING_COUNT	5
 
 int fd;
 const char v = 'V';
-static const char sopts[] = "bdehp:st:Tn:NLf:i";
+static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
 static const struct option lopts[] = {
 	{"bootstatus",          no_argument, NULL, 'b'},
 	{"disable",             no_argument, NULL, 'd'},
 	{"enable",              no_argument, NULL, 'e'},
 	{"help",                no_argument, NULL, 'h'},
 	{"pingrate",      required_argument, NULL, 'p'},
+	{"pingcount",     required_argument, NULL, 'c'},
 	{"status",              no_argument, NULL, 's'},
 	{"timeout",       required_argument, NULL, 't'},
 	{"gettimeout",          no_argument, NULL, 'T'},
@@ -90,6 +92,8 @@ static void usage(char *progname)
 	printf(" -h, --help\t\tPrint the help message\n");
 	printf(" -p, --pingrate=P\tSet ping rate to P seconds (default %d)\n",
 	       DEFAULT_PING_RATE);
+	printf(" -c, --pingcount=C\tSet number of pings to C (default %d)\n",
+	       DEFAULT_PING_COUNT);
 	printf(" -t, --timeout=T\tSet timeout to T seconds\n");
 	printf(" -T, --gettimeout\tGet the timeout\n");
 	printf(" -n, --pretimeout=T\tSet the pretimeout to T seconds\n");
@@ -172,6 +176,7 @@ int main(int argc, char *argv[])
 {
 	int flags;
 	unsigned int ping_rate = DEFAULT_PING_RATE;
+	unsigned int ping_count = DEFAULT_PING_COUNT;
 	int ret;
 	int c;
 	int oneshot = 0;
@@ -248,6 +253,12 @@ int main(int argc, char *argv[])
 				ping_rate = DEFAULT_PING_RATE;
 			printf("Watchdog ping rate set to %u seconds.\n", ping_rate);
 			break;
+		case 'c':
+			ping_count = strtoul(optarg, NULL, 0);
+			if (!ping_count)
+				ping_count = DEFAULT_PING_COUNT;
+			printf("Number of pings set to %u.\n", ping_count);
+			break;
 		case 's':
 			flags = 0;
 			oneshot = 1;
@@ -336,9 +347,10 @@ int main(int argc, char *argv[])
 
 	signal(SIGINT, term);
 
-	while (1) {
+	while (ping_count > 0) {
 		keep_alive();
 		sleep(ping_rate);
+		ping_count--;
 	}
 end:
 	/*
-- 
2.30.2

[PATCH 2/2] selftests/watchdog: convert the test output to KTAP format

[Laura Nao]

Modify the script output to conform to the KTAP format standard. The
number of tests executed is determined by the script arguments, and
options such as -c, -f, -h, -i, and -p do not impact the total test
count.

No functional change is intended.

Signed-off-by: Laura Nao <laura.nao@collabora.com>
---
 .../selftests/watchdog/watchdog-test.c        | 154 ++++++++++--------
 1 file changed, 89 insertions(+), 65 deletions(-)

diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
index 786cc5a26206..90f32de9e194 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -22,6 +22,7 @@
 #include <sys/ioctl.h>
 #include <linux/types.h>
 #include <linux/watchdog.h>
+#include "../kselftest.h"
 
 #define DEFAULT_PING_RATE	1
 #define DEFAULT_PING_COUNT	5
@@ -29,6 +30,7 @@
 int fd;
 const char v = 'V';
 static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
+static const char topts[] = "bdeLn:Nst:T";
 static const struct option lopts[] = {
 	{"bootstatus",          no_argument, NULL, 'b'},
 	{"disable",             no_argument, NULL, 'd'},
@@ -52,7 +54,7 @@ static const struct option lopts[] = {
  * the PC Watchdog card to reset its internal timer so it doesn't trigger
  * a computer reset.
  */
-static void keep_alive(void)
+static int keep_alive(void)
 {
 	int dummy;
 	int ret;
@@ -60,6 +62,8 @@ static void keep_alive(void)
 	ret = ioctl(fd, WDIOC_KEEPALIVE, &dummy);
 	if (!ret)
 		printf(".");
+
+	return ret;
 }
 
 /*
@@ -73,36 +77,36 @@ static void term(int sig)
 
 	close(fd);
 	if (ret < 0)
-		printf("\nStopping watchdog ticks failed (%d)...\n", errno);
+		ksft_print_msg("\nStopping watchdog ticks failed (%d)...\n", errno);
 	else
-		printf("\nStopping watchdog ticks...\n");
+		ksft_print_msg("\nStopping watchdog ticks...\n");
 	exit(0);
 }
 
 static void usage(char *progname)
 {
-	printf("Usage: %s [options]\n", progname);
-	printf(" -f, --file\t\tOpen watchdog device file\n");
-	printf("\t\t\tDefault is /dev/watchdog\n");
-	printf(" -i, --info\t\tShow watchdog_info\n");
-	printf(" -s, --status\t\tGet status & supported features\n");
-	printf(" -b, --bootstatus\tGet last boot status (Watchdog/POR)\n");
-	printf(" -d, --disable\t\tTurn off the watchdog timer\n");
-	printf(" -e, --enable\t\tTurn on the watchdog timer\n");
-	printf(" -h, --help\t\tPrint the help message\n");
-	printf(" -p, --pingrate=P\tSet ping rate to P seconds (default %d)\n",
+	ksft_print_msg("Usage: %s [options]\n", progname);
+	ksft_print_msg(" -f, --file\t\tOpen watchdog device file\n");
+	ksft_print_msg("\t\t\tDefault is /dev/watchdog\n");
+	ksft_print_msg(" -i, --info\t\tShow watchdog_info\n");
+	ksft_print_msg(" -s, --status\t\tGet status & supported features\n");
+	ksft_print_msg(" -b, --bootstatus\tGet last boot status (Watchdog/POR)\n");
+	ksft_print_msg(" -d, --disable\t\tTurn off the watchdog timer\n");
+	ksft_print_msg(" -e, --enable\t\tTurn on the watchdog timer\n");
+	ksft_print_msg(" -h, --help\t\tPrint the help message\n");
+	ksft_print_msg(" -p, --pingrate=P\tSet ping rate to P seconds (default %d)\n",
 	       DEFAULT_PING_RATE);
-	printf(" -c, --pingcount=C\tSet number of pings to C (default %d)\n",
+	ksft_print_msg(" -c, --pingcount=C\tSet number of pings to C (default %d)\n",
 	       DEFAULT_PING_COUNT);
-	printf(" -t, --timeout=T\tSet timeout to T seconds\n");
-	printf(" -T, --gettimeout\tGet the timeout\n");
-	printf(" -n, --pretimeout=T\tSet the pretimeout to T seconds\n");
-	printf(" -N, --getpretimeout\tGet the pretimeout\n");
-	printf(" -L, --gettimeleft\tGet the time left until timer expires\n");
-	printf("\n");
-	printf("Parameters are parsed left-to-right in real-time.\n");
-	printf("Example: %s -d -t 10 -p 5 -e\n", progname);
-	printf("Example: %s -t 12 -T -n 7 -N\n", progname);
+	ksft_print_msg(" -t, --timeout=T\tSet timeout to T seconds\n");
+	ksft_print_msg(" -T, --gettimeout\tGet the timeout\n");
+	ksft_print_msg(" -n, --pretimeout=T\tSet the pretimeout to T seconds\n");
+	ksft_print_msg(" -N, --getpretimeout\tGet the pretimeout\n");
+	ksft_print_msg(" -L, --gettimeleft\tGet the time left until timer expires\n");
+	ksft_print_msg("\n");
+	ksft_print_msg("Parameters are parsed left-to-right in real-time.\n");
+	ksft_print_msg("Example: %s -d -t 10 -p 5 -e\n", progname);
+	ksft_print_msg("Example: %s -t 12 -T -n 7 -N\n", progname);
 }
 
 struct wdiof_status {
@@ -128,13 +132,13 @@ static void print_status(int flags)
 	int wdiof = 0;
 
 	if (flags == WDIOS_UNKNOWN) {
-		printf("Unknown status error from WDIOC_GETSTATUS\n");
+		ksft_print_msg("Unknown status error from WDIOC_GETSTATUS\n");
 		return;
 	}
 
 	for (wdiof = 0; wdiof < WDIOF_NUM_STATUS; wdiof++) {
 		if (flags & wdiof_status[wdiof].flag)
-			printf("Support/Status: %s\n",
+			ksft_print_msg("Support/Status: %s\n",
 				wdiof_status[wdiof].status_str);
 	}
 }
@@ -156,18 +160,18 @@ static void print_boot_status(int flags)
 	int wdiof = 0;
 
 	if (flags == WDIOF_UNKNOWN) {
-		printf("Unknown flag error from WDIOC_GETBOOTSTATUS\n");
+		ksft_print_msg("Unknown flag error from WDIOC_GETBOOTSTATUS\n");
 		return;
 	}
 
 	if (flags == 0) {
-		printf("Last boot is caused by: Power-On-Reset\n");
+		ksft_print_msg("Last boot is caused by: Power-On-Reset\n");
 		return;
 	}
 
 	for (wdiof = 0; wdiof < WDIOF_NUM_BOOTSTATUS; wdiof++) {
 		if (flags & wdiof_bootstatus[wdiof].flag)
-			printf("Last boot is caused by: %s\n",
+			ksft_print_msg("Last boot is caused by: %s\n",
 				wdiof_bootstatus[wdiof].status_str);
 	}
 }
@@ -183,25 +187,28 @@ int main(int argc, char *argv[])
 	char *file = "/dev/watchdog";
 	struct watchdog_info info;
 	int temperature;
+	/* run WDIOC_KEEPALIVE test by default */
+	int test_num = 1;
 
 	setbuf(stdout, NULL);
 
 	while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) {
 		if (c == 'f')
 			file = optarg;
+
+		if (strchr(topts, c))
+			test_num++;
 	}
 
 	fd = open(file, O_WRONLY);
 
 	if (fd == -1) {
 		if (errno == ENOENT)
-			printf("Watchdog device (%s) not found.\n", file);
+			ksft_exit_skip("Watchdog device (%s) not found.\n", file);
 		else if (errno == EACCES)
-			printf("Run watchdog as root.\n");
+			ksft_exit_skip("Run watchdog as root.\n");
 		else
-			printf("Watchdog device open failed %s\n",
-				strerror(errno));
-		exit(-1);
+			ksft_exit_skip("Watchdog device open failed %s\n", strerror(errno));
 	}
 
 	/*
@@ -209,13 +216,15 @@ int main(int argc, char *argv[])
 	 */
 	ret = ioctl(fd, WDIOC_GETSUPPORT, &info);
 	if (ret) {
-		printf("WDIOC_GETSUPPORT error '%s'\n", strerror(errno));
 		close(fd);
-		exit(ret);
+		ksft_exit_skip("WDIOC_GETSUPPORT error '%s'\n", strerror(errno));
 	}
 
 	optind = 0;
 
+	ksft_print_header();
+	ksft_set_plan(test_num);
+
 	while ((c = getopt_long(argc, argv, sopts, lopts, NULL)) != -1) {
 		switch (c) {
 		case 'b':
@@ -225,39 +234,42 @@ int main(int argc, char *argv[])
 			if (!ret)
 				print_boot_status(flags);
 			else
-				printf("WDIOC_GETBOOTSTATUS error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETBOOTSTATUS error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETBOOTSTATUS\n");
 			break;
 		case 'd':
 			flags = WDIOS_DISABLECARD;
 			ret = ioctl(fd, WDIOC_SETOPTIONS, &flags);
 			if (!ret)
-				printf("Watchdog card disabled.\n");
+				ksft_print_msg("Watchdog card disabled.\n");
 			else {
-				printf("WDIOS_DISABLECARD error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOS_DISABLECARD error '%s'\n", strerror(errno));
 				oneshot = 1;
 			}
+			ksft_test_result(!ret, "WDIOC_SETOPTIONS_WDIOS_DISABLECARD\n");
 			break;
 		case 'e':
 			flags = WDIOS_ENABLECARD;
 			ret = ioctl(fd, WDIOC_SETOPTIONS, &flags);
 			if (!ret)
-				printf("Watchdog card enabled.\n");
+				ksft_print_msg("Watchdog card enabled.\n");
 			else {
-				printf("WDIOS_ENABLECARD error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOS_ENABLECARD error '%s'\n", strerror(errno));
 				oneshot = 1;
 			}
+			ksft_test_result(!ret, "WDIOC_SETOPTIONS_WDIOS_ENABLECARD\n");
 			break;
 		case 'p':
 			ping_rate = strtoul(optarg, NULL, 0);
 			if (!ping_rate)
 				ping_rate = DEFAULT_PING_RATE;
-			printf("Watchdog ping rate set to %u seconds.\n", ping_rate);
+			ksft_print_msg("Watchdog ping rate set to %u seconds.\n", ping_rate);
 			break;
 		case 'c':
 			ping_count = strtoul(optarg, NULL, 0);
 			if (!ping_count)
 				ping_count = DEFAULT_PING_COUNT;
-			printf("Number of pings set to %u.\n", ping_count);
+			ksft_print_msg("Number of pings set to %u.\n", ping_count);
 			break;
 		case 's':
 			flags = 0;
@@ -266,57 +278,62 @@ int main(int argc, char *argv[])
 			if (!ret)
 				print_status(flags);
 			else
-				printf("WDIOC_GETSTATUS error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETSTATUS error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETSTATUS\n");
 			ret = ioctl(fd, WDIOC_GETTEMP, &temperature);
 			if (ret)
-				printf("WDIOC_GETTEMP: '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETTEMP: '%s'\n", strerror(errno));
 			else
-				printf("Temperature %d\n", temperature);
-
+				ksft_print_msg("Temperature %d\n", temperature);
 			break;
 		case 't':
 			flags = strtoul(optarg, NULL, 0);
 			ret = ioctl(fd, WDIOC_SETTIMEOUT, &flags);
 			if (!ret)
-				printf("Watchdog timeout set to %u seconds.\n", flags);
+				ksft_print_msg("Watchdog timeout set to %u seconds.\n", flags);
 			else {
-				printf("WDIOC_SETTIMEOUT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_SETTIMEOUT error '%s'\n", strerror(errno));
 				oneshot = 1;
 			}
+			ksft_test_result(!ret, "WDIOC_SETTIMEOUT\n");
 			break;
 		case 'T':
 			oneshot = 1;
 			ret = ioctl(fd, WDIOC_GETTIMEOUT, &flags);
 			if (!ret)
-				printf("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
+				ksft_print_msg("WDIOC_GETTIMEOUT returns %u seconds.\n", flags);
 			else
-				printf("WDIOC_GETTIMEOUT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETTIMEOUT error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETTIMEOUT\n");
 			break;
 		case 'n':
 			flags = strtoul(optarg, NULL, 0);
 			ret = ioctl(fd, WDIOC_SETPRETIMEOUT, &flags);
 			if (!ret)
-				printf("Watchdog pretimeout set to %u seconds.\n", flags);
+				ksft_print_msg("Watchdog pretimeout set to %u seconds.\n", flags);
 			else {
-				printf("WDIOC_SETPRETIMEOUT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_SETPRETIMEOUT error '%s'\n", strerror(errno));
 				oneshot = 1;
 			}
+			ksft_test_result(!ret, "WDIOC_SETPRETIMEOUT\n");
 			break;
 		case 'N':
 			oneshot = 1;
 			ret = ioctl(fd, WDIOC_GETPRETIMEOUT, &flags);
 			if (!ret)
-				printf("WDIOC_GETPRETIMEOUT returns %u seconds.\n", flags);
+				ksft_print_msg("WDIOC_GETPRETIMEOUT returns %u seconds.\n", flags);
 			else
-				printf("WDIOC_GETPRETIMEOUT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETPRETIMEOUT error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETPRETIMEOUT\n");
 			break;
 		case 'L':
 			oneshot = 1;
 			ret = ioctl(fd, WDIOC_GETTIMELEFT, &flags);
 			if (!ret)
-				printf("WDIOC_GETTIMELEFT returns %u seconds.\n", flags);
+				ksft_print_msg("WDIOC_GETTIMELEFT returns %u seconds.\n", flags);
 			else
-				printf("WDIOC_GETTIMELEFT error '%s'\n", strerror(errno));
+				ksft_print_msg("WDIOC_GETTIMELEFT error '%s'\n", strerror(errno));
+			ksft_test_result(!ret, "WDIOC_GETTIMELEFT\n");
 			break;
 		case 'f':
 			/* Handled above */
@@ -327,31 +344,38 @@ int main(int argc, char *argv[])
 			 * validation. So we just show it here.
 			 */
 			oneshot = 1;
-			printf("watchdog_info:\n");
-			printf(" identity:\t\t%s\n", info.identity);
-			printf(" firmware_version:\t%u\n",
-			       info.firmware_version);
+			ksft_print_msg("watchdog_info:\n");
+			ksft_print_msg(" identity:\t\t%s\n", info.identity);
+			ksft_print_msg(" firmware_version:\t%u\n", info.firmware_version);
 			print_status(info.options);
 			break;
 
 		default:
 			usage(argv[0]);
+			ksft_test_result_skip("WDIOC_KEEPALIVE\n");
 			goto end;
 		}
 	}
 
-	if (oneshot)
+	if (oneshot) {
+		ksft_test_result_skip("WDIOC_KEEPALIVE\n");
 		goto end;
+	}
 
-	printf("Watchdog Ticking Away!\n");
+	ksft_print_msg("Watchdog Ticking Away!\n");
+	ksft_print_msg("");
 
 	signal(SIGINT, term);
 
 	while (ping_count > 0) {
-		keep_alive();
+		ret = keep_alive();
+		if (ret)
+			break;
 		sleep(ping_rate);
 		ping_count--;
 	}
+	printf("\n");
+	ksft_test_result(!ret, "WDIOC_KEEPALIVE\n");
 end:
 	/*
 	 * Send specific magic character 'V' just in case Magic Close is
@@ -359,7 +383,7 @@ int main(int argc, char *argv[])
 	 */
 	ret = write(fd, &v, 1);
 	if (ret < 0)
-		printf("Stopping watchdog ticks failed (%d)...\n", errno);
+		ksft_print_msg("Stopping watchdog ticks failed (%d)...\n", errno);
 	close(fd);
-	return 0;
+	ksft_finished();
 }
-- 
2.30.2

Re: [PATCH 0/2] Modify the watchdog selftest for execution with

[Laura Nao]

Hi Shuah,

On 5/6/24 13:13, Laura Nao wrote:
> The watchdog selftest script supports various parameters for testing
> different IOCTLs. The watchdog ping functionality is validated by
> starting
> a loop where the watchdog device is periodically pet, which can only
> be
> stopped by the user interrupting the script.
> 
> This results in a timeout when running this test using the kselftest
> runner
> with no non-oneshot parameters (or no parameters at all):
> 
>   TAP version 13
>   1..1
>   # timeout set to 45
>   # selftests: watchdog: watchdog-test
>   # Watchdog Ticking Away!
>   # .............................................#
>   not ok 1 selftests: watchdog: watchdog-test # TIMEOUT 45 seconds
> 
> To address this issue, the first patch in this series limits the loop
> to 5
> iterations by default and adds support for a new '-c' option to
> customize
> the number of pings as required.
> 
> The second patch conforms the test output to the KTAP format.
> 

Gentle ping - any thoughts on this series? It would simplify running the
watchdog kselftest in CI environments by leveraging the runner.

Thanks!

Laura

Re: [PATCH 0/2] Modify the watchdog selftest for execution with

[Shuah Khan]

On 6/6/24 03:57, Laura Nao wrote:
> Hi Shuah,
> 
> On 5/6/24 13:13, Laura Nao wrote:
>> The watchdog selftest script supports various parameters for testing
>> different IOCTLs. The watchdog ping functionality is validated by
>> starting
>> a loop where the watchdog device is periodically pet, which can only
>> be
>> stopped by the user interrupting the script.
>>
>> This results in a timeout when running this test using the kselftest
>> runner
>> with no non-oneshot parameters (or no parameters at all):


Sorry for the delay on this.

This test isn't include in the default kselftest run? How are you
running this?

>>
>>    TAP version 13
>>    1..1
>>    # timeout set to 45
>>    # selftests: watchdog: watchdog-test
>>    # Watchdog Ticking Away!
>>    # .............................................#
>>    not ok 1 selftests: watchdog: watchdog-test # TIMEOUT 45 seconds
>>
>> To address this issue, the first patch in this series limits the loop
>> to 5
>> iterations by default and adds support for a new '-c' option to
>> customize
>> the number of pings as required.
>>
>> The second patch conforms the test output to the KTAP format.
>>
> 
> Gentle ping - any thoughts on this series? It would simplify running the
> watchdog kselftest in CI environments by leveraging the runner.
> 

This test isn't intended to be included in the default run. It requires
loading a watchdog driver first. Do you load the driver from the runner?

thanks,
-- Shuah

Re: [PATCH 0/2] Modify the watchdog selftest for execution with

[Laura Nao]

Hi Shuah,

On 6/7/24 01:03, Shuah Khan wrote:
> On 6/6/24 03:57, Laura Nao wrote:
>> Hi Shuah,
>>
>> On 5/6/24 13:13, Laura Nao wrote:
>>> The watchdog selftest script supports various parameters for testing
>>> different IOCTLs. The watchdog ping functionality is validated by
>>> starting
>>> a loop where the watchdog device is periodically pet, which can only
>>> be
>>> stopped by the user interrupting the script.
>>>
>>> This results in a timeout when running this test using the kselftest
>>> runner
>>> with no non-oneshot parameters (or no parameters at all):
> 
> 
> Sorry for the delay on this.
> 
> This test isn't include in the default kselftest run? How are you
> running this?
>

The goal of this series is to enable the test to be run using the
kselftest runner individually, not as part of the default run. So for
example w/out args:

make -C tools/testing/selftests TARGETS=watchdog run_tests

or with args:

KSELFTEST_WATCHDOG_TEST_ARGS='-b -d -e -s -t 12 -T 3 -n 7 -N -L' make -C
tools/testing/selftests TARGETS=watchdog run_tests
 
>>>
>>>    TAP version 13
>>>    1..1
>>>    # timeout set to 45
>>>    # selftests: watchdog: watchdog-test
>>>    # Watchdog Ticking Away!
>>>    # .............................................#
>>>    not ok 1 selftests: watchdog: watchdog-test # TIMEOUT 45 seconds
>>>
>>> To address this issue, the first patch in this series limits the
>>> loop
>>> to 5
>>> iterations by default and adds support for a new '-c' option to
>>> customize
>>> the number of pings as required.
>>>
>>> The second patch conforms the test output to the KTAP format.
>>>
>>
>> Gentle ping - any thoughts on this series? It would simplify running
>> the
>> watchdog kselftest in CI environments by leveraging the runner.
>>
> 
> This test isn't intended to be included in the default run. It
> requires
> loading a watchdog driver first. Do you load the driver from the
> runner?
>

I get this test requires watchdog drivers to be loaded (which in this
case can't be added to a config fragment that goes with the selftest, as
they are platform-specific) and therefore cannot be included in the
default run. However, having ktap output support and limiting the ping
loop would allow the test to be run individually in the same way as
other selftests (so through the kselftest runner). 

Naturally, driver dependencies must be met for the test to run and
produce valid results. From my understanding the runner itself cannot
ensure this, so in this case it would be up to the user or CI to
enable/load the appropriate drivers before running the test.
If these dependencies are not satisfied, the test could just exit
with a skip code.

Does this make sense to you? or is the kselftest runner intended to be
used to run exclusively a subset of tests in the selftests directory
(i.e. the ones that don't have platform-specific driver requirements)?

Thanks,

Laura

Re: [PATCH 0/2] Modify the watchdog selftest for execution with

[Shuah Khan]

On 6/7/24 03:53, Laura Nao wrote:
> Hi Shuah,
> 
> On 6/7/24 01:03, Shuah Khan wrote:
>> On 6/6/24 03:57, Laura Nao wrote:
>>> Hi Shuah,
>>>
>>> On 5/6/24 13:13, Laura Nao wrote:
>>>> The watchdog selftest script supports various parameters for testing
>>>> different IOCTLs. The watchdog ping functionality is validated by
>>>> starting
>>>> a loop where the watchdog device is periodically pet, which can only
>>>> be
>>>> stopped by the user interrupting the script.
>>>>
>>>> This results in a timeout when running this test using the kselftest
>>>> runner
>>>> with no non-oneshot parameters (or no parameters at all):
>>
>>
>> Sorry for the delay on this.
>>
>> This test isn't include in the default kselftest run? How are you
>> running this?
>>
> 
> The goal of this series is to enable the test to be run using the
> kselftest runner individually, not as part of the default run. So for
> example w/out args:
> 
> make -C tools/testing/selftests TARGETS=watchdog run_tests
> 
> or with args:
> 
> KSELFTEST_WATCHDOG_TEST_ARGS='-b -d -e -s -t 12 -T 3 -n 7 -N -L' make -C
> tools/testing/selftests TARGETS=watchdog run_tests
>   
>>>>
>>>>     TAP version 13
>>>>     1..1
>>>>     # timeout set to 45
>>>>     # selftests: watchdog: watchdog-test
>>>>     # Watchdog Ticking Away!
>>>>     # .............................................#
>>>>     not ok 1 selftests: watchdog: watchdog-test # TIMEOUT 45 seconds
>>>>
>>>> To address this issue, the first patch in this series limits the
>>>> loop
>>>> to 5
>>>> iterations by default and adds support for a new '-c' option to
>>>> customize
>>>> the number of pings as required.
>>>>
>>>> The second patch conforms the test output to the KTAP format.
>>>>
>>>
>>> Gentle ping - any thoughts on this series? It would simplify running
>>> the
>>> watchdog kselftest in CI environments by leveraging the runner.
>>>
>>
>> This test isn't intended to be included in the default run. It
>> requires
>> loading a watchdog driver first. Do you load the driver from the
>> runner?
>>
> 
> I get this test requires watchdog drivers to be loaded (which in this
> case can't be added to a config fragment that goes with the selftest, as
> they are platform-specific) and therefore cannot be included in the
> default run. However, having ktap output support and limiting the ping
> loop would allow the test to be run individually in the same way as
> other selftests (so through the kselftest runner).
> 
> Naturally, driver dependencies must be met for the test to run and
> produce valid results. From my understanding the runner itself cannot
> ensure this, so in this case it would be up to the user or CI to
> enable/load the appropriate drivers before running the test.
> If these dependencies are not satisfied, the test could just exit
> with a skip code.
> 
> Does this make sense to you? or is the kselftest runner intended to be
> used to run exclusively a subset of tests in the selftests directory
> (i.e. the ones that don't have platform-specific driver requirements)?
> 

There are several tests that aren't included in the default run because
they have dependencies and potentially damaging to the running system.
These tests are not included for a reason.

The first step would to be ensure writing shell script to load and
unload the watchdog module and then pass in existing parameters such
as the oneshot to run the test.

There is no need to add a new parameter yet. Also there is no need to
convert this to ktap yet.

thanks,
-- Shuah

Re: [PATCH 0/2] Modify the watchdog selftest for execution with

[Laura Nao]

Hi Shuah,

On 6/7/24 23:07, Shuah Khan wrote:
> On 6/7/24 03:53, Laura Nao wrote:
>> Hi Shuah,
>>
>> On 6/7/24 01:03, Shuah Khan wrote:
>>> On 6/6/24 03:57, Laura Nao wrote:
>>>> Hi Shuah,
>>>>
>>>> On 5/6/24 13:13, Laura Nao wrote:
>>>>> The watchdog selftest script supports various parameters for testing
>>>>> different IOCTLs. The watchdog ping functionality is validated by
>>>>> starting
>>>>> a loop where the watchdog device is periodically pet, which can only
>>>>> be
>>>>> stopped by the user interrupting the script.
>>>>>
>>>>> This results in a timeout when running this test using the kselftest
>>>>> runner
>>>>> with no non-oneshot parameters (or no parameters at all):
>>>
>>>
>>> Sorry for the delay on this.
>>>
>>> This test isn't include in the default kselftest run? How are you
>>> running this?
>>>
>>
>> The goal of this series is to enable the test to be run using the
>> kselftest runner individually, not as part of the default run. So for
>> example w/out args:
>>
>> make -C tools/testing/selftests TARGETS=watchdog run_tests
>>
>> or with args:
>>
>> KSELFTEST_WATCHDOG_TEST_ARGS='-b -d -e -s -t 12 -T 3 -n 7 -N -L' make -C
>> tools/testing/selftests TARGETS=watchdog run_tests
>>>>>
>>>>>     TAP version 13
>>>>>     1..1
>>>>>     # timeout set to 45
>>>>>     # selftests: watchdog: watchdog-test
>>>>>     # Watchdog Ticking Away!
>>>>>     # .............................................#
>>>>>     not ok 1 selftests: watchdog: watchdog-test # TIMEOUT 45 seconds
>>>>>
>>>>> To address this issue, the first patch in this series limits the
>>>>> loop
>>>>> to 5
>>>>> iterations by default and adds support for a new '-c' option to
>>>>> customize
>>>>> the number of pings as required.
>>>>>
>>>>> The second patch conforms the test output to the KTAP format.
>>>>>
>>>>
>>>> Gentle ping - any thoughts on this series? It would simplify running
>>>> the
>>>> watchdog kselftest in CI environments by leveraging the runner.
>>>>
>>>
>>> This test isn't intended to be included in the default run. It
>>> requires
>>> loading a watchdog driver first. Do you load the driver from the
>>> runner?
>>>
>>
>> I get this test requires watchdog drivers to be loaded (which in this
>> case can't be added to a config fragment that goes with the selftest, as
>> they are platform-specific) and therefore cannot be included in the
>> default run. However, having ktap output support and limiting the ping
>> loop would allow the test to be run individually in the same way as
>> other selftests (so through the kselftest runner).
>>
>> Naturally, driver dependencies must be met for the test to run and
>> produce valid results. From my understanding the runner itself cannot
>> ensure this, so in this case it would be up to the user or CI to
>> enable/load the appropriate drivers before running the test.
>> If these dependencies are not satisfied, the test could just exit
>> with a skip code.
>>
>> Does this make sense to you? or is the kselftest runner intended to be
>> used to run exclusively a subset of tests in the selftests directory
>> (i.e. the ones that don't have platform-specific driver requirements)?
>>
> 
> There are several tests that aren't included in the default run because
> they have dependencies and potentially damaging to the running system.
> These tests are not included for a reason.
> 
> The first step would to be ensure writing shell script to load and
> unload the watchdog module and then pass in existing parameters such
> as the oneshot to run the test.
> 
> There is no need to add a new parameter yet. Also there is no need to
> convert this to ktap yet.
> 

To clarify, I understand that this test is not suitable for the default 
run, and I do not intend to change that. The patch series is meant to 
make the test usable in a non-interactive environment, such as a CI,
when explicitly enabled and with the required modules already loaded.

Thanks,

Laura

Re: [PATCH 0/2] Modify the watchdog selftest for execution with

[Shuah Khan]

On 6/18/24 07:40, Laura Nao wrote:
> Hi Shuah,
> 
> On 6/7/24 23:07, Shuah Khan wrote:
>> On 6/7/24 03:53, Laura Nao wrote:
>>> Hi Shuah,
>>>
>>> On 6/7/24 01:03, Shuah Khan wrote:
>>>> On 6/6/24 03:57, Laura Nao wrote:
>>>>> Hi Shuah,
>>>>>
>>>>> On 5/6/24 13:13, Laura Nao wrote:
>>>>>> The watchdog selftest script supports various parameters for testing
>>>>>> different IOCTLs. The watchdog ping functionality is validated by
>>>>>> starting
>>>>>> a loop where the watchdog device is periodically pet, which can only
>>>>>> be
>>>>>> stopped by the user interrupting the script.
>>>>>>
>>>>>> This results in a timeout when running this test using the kselftest
>>>>>> runner
>>>>>> with no non-oneshot parameters (or no parameters at all):
>>>>
>>>>
>>>> Sorry for the delay on this.
>>>>
>>>> This test isn't include in the default kselftest run? How are you
>>>> running this?
>>>>
>>>
>>> The goal of this series is to enable the test to be run using the
>>> kselftest runner individually, not as part of the default run. So for
>>> example w/out args:
>>>
>>> make -C tools/testing/selftests TARGETS=watchdog run_tests
>>>
>>> or with args:
>>>
>>> KSELFTEST_WATCHDOG_TEST_ARGS='-b -d -e -s -t 12 -T 3 -n 7 -N -L' make -C
>>> tools/testing/selftests TARGETS=watchdog run_tests
>>>>>>
>>>>>>      TAP version 13
>>>>>>      1..1
>>>>>>      # timeout set to 45
>>>>>>      # selftests: watchdog: watchdog-test
>>>>>>      # Watchdog Ticking Away!
>>>>>>      # .............................................#
>>>>>>      not ok 1 selftests: watchdog: watchdog-test # TIMEOUT 45 seconds
>>>>>>
>>>>>> To address this issue, the first patch in this series limits the
>>>>>> loop
>>>>>> to 5
>>>>>> iterations by default and adds support for a new '-c' option to
>>>>>> customize
>>>>>> the number of pings as required.
>>>>>>
>>>>>> The second patch conforms the test output to the KTAP format.
>>>>>>
>>>>>
>>>>> Gentle ping - any thoughts on this series? It would simplify running
>>>>> the
>>>>> watchdog kselftest in CI environments by leveraging the runner.
>>>>>
>>>>
>>>> This test isn't intended to be included in the default run. It
>>>> requires
>>>> loading a watchdog driver first. Do you load the driver from the
>>>> runner?
>>>>
>>>
>>> I get this test requires watchdog drivers to be loaded (which in this
>>> case can't be added to a config fragment that goes with the selftest, as
>>> they are platform-specific) and therefore cannot be included in the
>>> default run. However, having ktap output support and limiting the ping
>>> loop would allow the test to be run individually in the same way as
>>> other selftests (so through the kselftest runner).
>>>
>>> Naturally, driver dependencies must be met for the test to run and
>>> produce valid results. From my understanding the runner itself cannot
>>> ensure this, so in this case it would be up to the user or CI to
>>> enable/load the appropriate drivers before running the test.
>>> If these dependencies are not satisfied, the test could just exit
>>> with a skip code.
>>>
>>> Does this make sense to you? or is the kselftest runner intended to be
>>> used to run exclusively a subset of tests in the selftests directory
>>> (i.e. the ones that don't have platform-specific driver requirements)?
>>>
>>
>> There are several tests that aren't included in the default run because
>> they have dependencies and potentially damaging to the running system.
>> These tests are not included for a reason.
>>
>> The first step would to be ensure writing shell script to load and
>> unload the watchdog module and then pass in existing parameters such
>> as the oneshot to run the test.
>>
>> There is no need to add a new parameter yet. Also there is no need to
>> convert this to ktap yet.
>>
> 
> To clarify, I understand that this test is not suitable for the default
> run, and I do not intend to change that. The patch series is meant to
> make the test usable in a non-interactive environment, such as a CI,
> when explicitly enabled and with the required modules already loaded.
> 

The test can be with one shot timer - how is this test currently not
usable and how are your changes making it usable?

thanks,
-- Shuah

Re: [PATCH 0/2] Modify the watchdog selftest for execution with

[Laura Nao]

On 6/21/24 23:08, Shuah Khan wrote:
> On 6/18/24 07:40, Laura Nao wrote:
>> Hi Shuah,
>>
>> On 6/7/24 23:07, Shuah Khan wrote:
>>> On 6/7/24 03:53, Laura Nao wrote:
>>>> Hi Shuah,
>>>>
>>>> On 6/7/24 01:03, Shuah Khan wrote:
>>>>> On 6/6/24 03:57, Laura Nao wrote:
>>>>>> Hi Shuah,
>>>>>>
>>>>>> On 5/6/24 13:13, Laura Nao wrote:
>>>>>>> The watchdog selftest script supports various parameters for
>>>>>>> testing
>>>>>>> different IOCTLs. The watchdog ping functionality is validated
>>>>>>> by
>>>>>>> starting
>>>>>>> a loop where the watchdog device is periodically pet, which can
>>>>>>> only
>>>>>>> be
>>>>>>> stopped by the user interrupting the script.
>>>>>>>
>>>>>>> This results in a timeout when running this test using the
>>>>>>> kselftest
>>>>>>> runner
>>>>>>> with no non-oneshot parameters (or no parameters at all):
>>>>>
>>>>>
>>>>> Sorry for the delay on this.
>>>>>
>>>>> This test isn't include in the default kselftest run? How are you
>>>>> running this?
>>>>>
>>>>
>>>> The goal of this series is to enable the test to be run using the
>>>> kselftest runner individually, not as part of the default run. So
>>>> for
>>>> example w/out args:
>>>>
>>>> make -C tools/testing/selftests TARGETS=watchdog run_tests
>>>>
>>>> or with args:
>>>>
>>>> KSELFTEST_WATCHDOG_TEST_ARGS='-b -d -e -s -t 12 -T 3 -n 7 -N -L' 
>>>> make -C
>>>> tools/testing/selftests TARGETS=watchdog run_tests
>>>>>>>
>>>>>>>      TAP version 13
>>>>>>>      1..1
>>>>>>>      # timeout set to 45
>>>>>>>      # selftests: watchdog: watchdog-test
>>>>>>>      # Watchdog Ticking Away!
>>>>>>>      # .............................................#
>>>>>>>      not ok 1 selftests: watchdog: watchdog-test # TIMEOUT 45 
>>>>>>> seconds
>>>>>>>
>>>>>>> To address this issue, the first patch in this series limits the
>>>>>>> loop
>>>>>>> to 5
>>>>>>> iterations by default and adds support for a new '-c' option to
>>>>>>> customize
>>>>>>> the number of pings as required.
>>>>>>>
>>>>>>> The second patch conforms the test output to the KTAP format.
>>>>>>>
>>>>>>
>>>>>> Gentle ping - any thoughts on this series? It would simplify
>>>>>> running
>>>>>> the
>>>>>> watchdog kselftest in CI environments by leveraging the runner.
>>>>>>
>>>>>
>>>>> This test isn't intended to be included in the default run. It
>>>>> requires
>>>>> loading a watchdog driver first. Do you load the driver from the
>>>>> runner?
>>>>>
>>>>
>>>> I get this test requires watchdog drivers to be loaded (which in
>>>> this
>>>> case can't be added to a config fragment that goes with the 
>>>> selftest, as
>>>> they are platform-specific) and therefore cannot be included in the
>>>> default run. However, having ktap output support and limiting the
>>>> ping
>>>> loop would allow the test to be run individually in the same way as
>>>> other selftests (so through the kselftest runner).
>>>>
>>>> Naturally, driver dependencies must be met for the test to run and
>>>> produce valid results. From my understanding the runner itself
>>>> cannot
>>>> ensure this, so in this case it would be up to the user or CI to
>>>> enable/load the appropriate drivers before running the test.
>>>> If these dependencies are not satisfied, the test could just exit
>>>> with a skip code.
>>>>
>>>> Does this make sense to you? or is the kselftest runner intended to
>>>> be
>>>> used to run exclusively a subset of tests in the selftests
>>>> directory
>>>> (i.e. the ones that don't have platform-specific driver
>>>> requirements)?
>>>>
>>>
>>> There are several tests that aren't included in the default run
>>> because
>>> they have dependencies and potentially damaging to the running
>>> system.
>>> These tests are not included for a reason.
>>>
>>> The first step would to be ensure writing shell script to load and
>>> unload the watchdog module and then pass in existing parameters such
>>> as the oneshot to run the test.
>>>
>>> There is no need to add a new parameter yet. Also there is no need
>>> to
>>> convert this to ktap yet.
>>>
>>
>> To clarify, I understand that this test is not suitable for the
>> default
>> run, and I do not intend to change that. The patch series is meant to
>> make the test usable in a non-interactive environment, such as a CI,
>> when explicitly enabled and with the required modules already loaded.
>>
> 
> The test can be with one shot timer - how is this test currently not
> usable and how are your changes making it usable?
> 

In a non-interactive environment, the current test can only be run
through the oneshot parameters, making it impossible to test the
WDIOC_KEEPALIVE ioctl (since the ping loop is only interrupted by
SIGINT). So the first patch enables testing of the WDIOC_KEEPALIVE ioctl
in such an environment, by making the ping loop finite. 
The second patch allows the reuse of the ktap result parser used for
other kselftests, eliminating the need for a custom parser for this
test.

Thanks,

Laura

Re: [PATCH 2/2] selftests/watchdog: convert the test output to KTAP format

[Shuah Khan]

On 5/6/24 05:13, Laura Nao wrote:
> Modify the script output to conform to the KTAP format standard. The

What is script here?

> number of tests executed is determined by the script arguments, and
> options such as -c, -f, -h, -i, and -p do not impact the total test
> count.
> 
> No functional change is intended.

There are functional changes - keep_alive() coupled with changes
tailored by a script that isn't in the kernel code which isn't
ideal.

Why not inlcude the script in this patch series to make it part
of the kernel?

> 
> Signed-off-by: Laura Nao <laura.nao@collabora.com>
> ---
>   .../selftests/watchdog/watchdog-test.c        | 154 ++++++++++--------
>   1 file changed, 89 insertions(+), 65 deletions(-)
> 
> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
> index 786cc5a26206..90f32de9e194 100644
> --- a/tools/testing/selftests/watchdog/watchdog-test.c
> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
> @@ -22,6 +22,7 @@
>   #include <sys/ioctl.h>
>   #include <linux/types.h>
>   #include <linux/watchdog.h>
> +#include "../kselftest.h"
>   
>   #define DEFAULT_PING_RATE	1
>   #define DEFAULT_PING_COUNT	5
> @@ -29,6 +30,7 @@
>   int fd;
>   const char v = 'V';
>   static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
> +static const char topts[] = "bdeLn:Nst:T";
>   static const struct option lopts[] = {
>   	{"bootstatus",          no_argument, NULL, 'b'},
>   	{"disable",             no_argument, NULL, 'd'},
> @@ -52,7 +54,7 @@ static const struct option lopts[] = {
>    * the PC Watchdog card to reset its internal timer so it doesn't trigger
>    * a computer reset.
>    */
> -static void keep_alive(void)
> +static int keep_alive(void)
>   {
>   	int dummy;
>   	int ret;
> @@ -60,6 +62,8 @@ static void keep_alive(void)
>   	ret = ioctl(fd, WDIOC_KEEPALIVE, &dummy);
>   	if (!ret)
>   		printf(".");
> +
> +	return ret;
>   }

Are these changes driven by the script that isn't in the kernel code?
I don't want to see changes to keep_alive() bevator.

thanks,
-- Shuah

Re: [PATCH 1/2] selftests/watchdog: limit ping loop and allow configuring the number of pings

[Shuah Khan]

On 5/6/24 05:13, Laura Nao wrote:
> In order to run the watchdog selftest with the kselftest runner, the
> loop responsible for pinging the watchdog should be finite. This
> change limits the loop to 5 iterations by default and introduces a new
> '-c' option to adjust the number of pings as needed.

This patch makes the test run finite in all cases changing the bevavior
to run it forever?
> 
> Signed-off-by: Laura Nao <laura.nao@collabora.com>
> ---
>   tools/testing/selftests/watchdog/watchdog-test.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
> index bc71cbca0dde..786cc5a26206 100644
> --- a/tools/testing/selftests/watchdog/watchdog-test.c
> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
> @@ -24,16 +24,18 @@
>   #include <linux/watchdog.h>
>   
>   #define DEFAULT_PING_RATE	1
> +#define DEFAULT_PING_COUNT	5
>   
>   int fd;
>   const char v = 'V';
> -static const char sopts[] = "bdehp:st:Tn:NLf:i";
> +static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
>   static const struct option lopts[] = {
>   	{"bootstatus",          no_argument, NULL, 'b'},
>   	{"disable",             no_argument, NULL, 'd'},
>   	{"enable",              no_argument, NULL, 'e'},
>   	{"help",                no_argument, NULL, 'h'},
>   	{"pingrate",      required_argument, NULL, 'p'},
> +	{"pingcount",     required_argument, NULL, 'c'},
>   	{"status",              no_argument, NULL, 's'},
>   	{"timeout",       required_argument, NULL, 't'},
>   	{"gettimeout",          no_argument, NULL, 'T'},
> @@ -90,6 +92,8 @@ static void usage(char *progname)
>   	printf(" -h, --help\t\tPrint the help message\n");
>   	printf(" -p, --pingrate=P\tSet ping rate to P seconds (default %d)\n",
>   	       DEFAULT_PING_RATE);
> +	printf(" -c, --pingcount=C\tSet number of pings to C (default %d)\n",
> +	       DEFAULT_PING_COUNT);
>   	printf(" -t, --timeout=T\tSet timeout to T seconds\n");
>   	printf(" -T, --gettimeout\tGet the timeout\n");
>   	printf(" -n, --pretimeout=T\tSet the pretimeout to T seconds\n");
> @@ -172,6 +176,7 @@ int main(int argc, char *argv[])
>   {
>   	int flags;
>   	unsigned int ping_rate = DEFAULT_PING_RATE;
> +	unsigned int ping_count = DEFAULT_PING_COUNT;
>   	int ret;
>   	int c;
>   	int oneshot = 0;
> @@ -248,6 +253,12 @@ int main(int argc, char *argv[])
>   				ping_rate = DEFAULT_PING_RATE;
>   			printf("Watchdog ping rate set to %u seconds.\n", ping_rate);
>   			break;
> +		case 'c':
> +			ping_count = strtoul(optarg, NULL, 0);
> +			if (!ping_count)
> +				ping_count = DEFAULT_PING_COUNT;
> +			printf("Number of pings set to %u.\n", ping_count);
> +			break;
>   		case 's':
>   			flags = 0;
>   			oneshot = 1;
> @@ -336,9 +347,10 @@ int main(int argc, char *argv[])
>   
>   	signal(SIGINT, term);
>   
> -	while (1) {
> +	while (ping_count > 0) {
>   		keep_alive();
>   		sleep(ping_rate);
> +		ping_count--;

So this test no longer runs forever?

thanks,
-- Shuah

Re: [PATCH 1/2] selftests/watchdog: limit ping loop and allow configuring the number of pings

[Laura Nao]

On 6/27/24 20:48, Shuah Khan wrote:
> On 5/6/24 05:13, Laura Nao wrote:
>> In order to run the watchdog selftest with the kselftest runner, the
>> loop responsible for pinging the watchdog should be finite. This
>> change limits the loop to 5 iterations by default and introduces a new
>> '-c' option to adjust the number of pings as needed.
> 
> This patch makes the test run finite in all cases changing the bevavior
> to run it forever?

Correct.

>>
>> Signed-off-by: Laura Nao <laura.nao@collabora.com>
>> ---
>>   tools/testing/selftests/watchdog/watchdog-test.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c 
>> b/tools/testing/selftests/watchdog/watchdog-test.c
>> index bc71cbca0dde..786cc5a26206 100644
>> --- a/tools/testing/selftests/watchdog/watchdog-test.c
>> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
>> @@ -24,16 +24,18 @@
>>   #include <linux/watchdog.h>
>>   #define DEFAULT_PING_RATE    1
>> +#define DEFAULT_PING_COUNT    5
>>   int fd;
>>   const char v = 'V';
>> -static const char sopts[] = "bdehp:st:Tn:NLf:i";
>> +static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
>>   static const struct option lopts[] = {
>>       {"bootstatus",          no_argument, NULL, 'b'},
>>       {"disable",             no_argument, NULL, 'd'},
>>       {"enable",              no_argument, NULL, 'e'},
>>       {"help",                no_argument, NULL, 'h'},
>>       {"pingrate",      required_argument, NULL, 'p'},
>> +    {"pingcount",     required_argument, NULL, 'c'},
>>       {"status",              no_argument, NULL, 's'},
>>       {"timeout",       required_argument, NULL, 't'},
>>       {"gettimeout",          no_argument, NULL, 'T'},
>> @@ -90,6 +92,8 @@ static void usage(char *progname)
>>       printf(" -h, --help\t\tPrint the help message\n");
>>       printf(" -p, --pingrate=P\tSet ping rate to P seconds (default 
>> %d)\n",
>>              DEFAULT_PING_RATE);
>> +    printf(" -c, --pingcount=C\tSet number of pings to C (default 
>> %d)\n",
>> +           DEFAULT_PING_COUNT);
>>       printf(" -t, --timeout=T\tSet timeout to T seconds\n");
>>       printf(" -T, --gettimeout\tGet the timeout\n");
>>       printf(" -n, --pretimeout=T\tSet the pretimeout to T seconds\n");
>> @@ -172,6 +176,7 @@ int main(int argc, char *argv[])
>>   {
>>       int flags;
>>       unsigned int ping_rate = DEFAULT_PING_RATE;
>> +    unsigned int ping_count = DEFAULT_PING_COUNT;
>>       int ret;
>>       int c;
>>       int oneshot = 0;
>> @@ -248,6 +253,12 @@ int main(int argc, char *argv[])
>>                   ping_rate = DEFAULT_PING_RATE;
>>               printf("Watchdog ping rate set to %u seconds.\n", 
>> ping_rate);
>>               break;
>> +        case 'c':
>> +            ping_count = strtoul(optarg, NULL, 0);
>> +            if (!ping_count)
>> +                ping_count = DEFAULT_PING_COUNT;
>> +            printf("Number of pings set to %u.\n", ping_count);
>> +            break;
>>           case 's':
>>               flags = 0;
>>               oneshot = 1;
>> @@ -336,9 +347,10 @@ int main(int argc, char *argv[])
>>       signal(SIGINT, term);
>> -    while (1) {
>> +    while (ping_count > 0) {
>>           keep_alive();
>>           sleep(ping_rate);
>> +        ping_count--;
> 
> So this test no longer runs forever?
>

That's correct, with this patch applied the test no longer runs forever. 
I understand you prefer the current behavior - how about keeping the
keep_alive() loop infinite by default and only making it finite when the 
-c argument is passed? Would that be reasonable?

Thanks for your feedback!

Laura

Re: [PATCH 2/2] selftests/watchdog: convert the test output to KTAP format

[Laura Nao]

On 6/27/24 20:41, Shuah Khan wrote:
> On 5/6/24 05:13, Laura Nao wrote:
>> Modify the script output to conform to the KTAP format standard. The
> 
> What is script here?
> 

I was referring to the watchdog-test.c file addressed in this patch. I 
understand this could be confusing, I will rephrase the commit message 
to avoid ambiguity.

>> number of tests executed is determined by the script arguments, and
>> options such as -c, -f, -h, -i, and -p do not impact the total test
>> count.
>>
>> No functional change is intended.
> 
> There are functional changes - keep_alive() coupled with changes
> tailored by a script that isn't in the kernel code which isn't
> ideal.
> 
> Why not inlcude the script in this patch series to make it part
> of the kernel?
> 

Right, I'll remove the 'no functional change is intended' sentence from 
the commit message.

Apart from the patches already in this series, no other script is 
required to run the test in a CI environment.

>>
>> Signed-off-by: Laura Nao <laura.nao@collabora.com>
>> ---
>>   .../selftests/watchdog/watchdog-test.c        | 154 ++++++++++--------
>>   1 file changed, 89 insertions(+), 65 deletions(-)
>>
>> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c 
>> b/tools/testing/selftests/watchdog/watchdog-test.c
>> index 786cc5a26206..90f32de9e194 100644
>> --- a/tools/testing/selftests/watchdog/watchdog-test.c
>> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
>> @@ -22,6 +22,7 @@
>>   #include <sys/ioctl.h>
>>   #include <linux/types.h>
>>   #include <linux/watchdog.h>
>> +#include "../kselftest.h"
>>   #define DEFAULT_PING_RATE    1
>>   #define DEFAULT_PING_COUNT    5
>> @@ -29,6 +30,7 @@
>>   int fd;
>>   const char v = 'V';
>>   static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
>> +static const char topts[] = "bdeLn:Nst:T";
>>   static const struct option lopts[] = {
>>       {"bootstatus",          no_argument, NULL, 'b'},
>>       {"disable",             no_argument, NULL, 'd'},
>> @@ -52,7 +54,7 @@ static const struct option lopts[] = {
>>    * the PC Watchdog card to reset its internal timer so it doesn't 
>> trigger
>>    * a computer reset.
>>    */
>> -static void keep_alive(void)
>> +static int keep_alive(void)
>>   {
>>       int dummy;
>>       int ret;
>> @@ -60,6 +62,8 @@ static void keep_alive(void)
>>       ret = ioctl(fd, WDIOC_KEEPALIVE, &dummy);
>>       if (!ret)
>>           printf(".");
>> +
>> +    return ret;
>>   }
> 
> Are these changes driven by the script that isn't in the kernel code?
> I don't want to see changes to keep_alive() bevator.
> 

These changes are not driven by any external script; the aim of this
patch is just to conform the output to KTAP for easier parsing of the
results in CI environments.

Returning ret from keep_alive() allows to track the result for
the last WDIOC_KEEPALIVE ioctl and report it to the user through 
ksft_test_result, analogously to other ioctls tested in this
same file.

Thanks,

Laura

Re: [PATCH 1/2] selftests/watchdog: limit ping loop and allow configuring the number of pings

[Shuah Khan]

On 7/3/24 08:48, Laura Nao wrote:
> On 6/27/24 20:48, Shuah Khan wrote:
>> On 5/6/24 05:13, Laura Nao wrote:
>>> In order to run the watchdog selftest with the kselftest runner, the
>>> loop responsible for pinging the watchdog should be finite. This
>>> change limits the loop to 5 iterations by default and introduces a new
>>> '-c' option to adjust the number of pings as needed.
>>
>> This patch makes the test run finite in all cases changing the bevavior
>> to run it forever?
> 
> Correct.
> 
>>>
>>> Signed-off-by: Laura Nao <laura.nao@collabora.com>
>>> ---
>>>    tools/testing/selftests/watchdog/watchdog-test.c | 16 ++++++++++++++--
>>>    1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/watchdog/watchdog-test.c
>>> b/tools/testing/selftests/watchdog/watchdog-test.c
>>> index bc71cbca0dde..786cc5a26206 100644
>>> --- a/tools/testing/selftests/watchdog/watchdog-test.c
>>> +++ b/tools/testing/selftests/watchdog/watchdog-test.c
>>> @@ -24,16 +24,18 @@
>>>    #include <linux/watchdog.h>
>>>    #define DEFAULT_PING_RATE    1
>>> +#define DEFAULT_PING_COUNT    5
>>>    int fd;
>>>    const char v = 'V';
>>> -static const char sopts[] = "bdehp:st:Tn:NLf:i";
>>> +static const char sopts[] = "bdehp:c:st:Tn:NLf:i";
>>>    static const struct option lopts[] = {
>>>        {"bootstatus",          no_argument, NULL, 'b'},
>>>        {"disable",             no_argument, NULL, 'd'},
>>>        {"enable",              no_argument, NULL, 'e'},
>>>        {"help",                no_argument, NULL, 'h'},
>>>        {"pingrate",      required_argument, NULL, 'p'},
>>> +    {"pingcount",     required_argument, NULL, 'c'},
>>>        {"status",              no_argument, NULL, 's'},
>>>        {"timeout",       required_argument, NULL, 't'},
>>>        {"gettimeout",          no_argument, NULL, 'T'},
>>> @@ -90,6 +92,8 @@ static void usage(char *progname)
>>>        printf(" -h, --help\t\tPrint the help message\n");
>>>        printf(" -p, --pingrate=P\tSet ping rate to P seconds (default
>>> %d)\n",
>>>               DEFAULT_PING_RATE);
>>> +    printf(" -c, --pingcount=C\tSet number of pings to C (default
>>> %d)\n",
>>> +           DEFAULT_PING_COUNT);
>>>        printf(" -t, --timeout=T\tSet timeout to T seconds\n");
>>>        printf(" -T, --gettimeout\tGet the timeout\n");
>>>        printf(" -n, --pretimeout=T\tSet the pretimeout to T seconds\n");
>>> @@ -172,6 +176,7 @@ int main(int argc, char *argv[])
>>>    {
>>>        int flags;
>>>        unsigned int ping_rate = DEFAULT_PING_RATE;
>>> +    unsigned int ping_count = DEFAULT_PING_COUNT;
>>>        int ret;
>>>        int c;
>>>        int oneshot = 0;
>>> @@ -248,6 +253,12 @@ int main(int argc, char *argv[])
>>>                    ping_rate = DEFAULT_PING_RATE;
>>>                printf("Watchdog ping rate set to %u seconds.\n",
>>> ping_rate);
>>>                break;
>>> +        case 'c':
>>> +            ping_count = strtoul(optarg, NULL, 0);
>>> +            if (!ping_count)
>>> +                ping_count = DEFAULT_PING_COUNT;
>>> +            printf("Number of pings set to %u.\n", ping_count);
>>> +            break;
>>>            case 's':
>>>                flags = 0;
>>>                oneshot = 1;
>>> @@ -336,9 +347,10 @@ int main(int argc, char *argv[])
>>>        signal(SIGINT, term);
>>> -    while (1) {
>>> +    while (ping_count > 0) {
>>>            keep_alive();
>>>            sleep(ping_rate);
>>> +        ping_count--;
>>
>> So this test no longer runs forever?
>>
> 
> That's correct, with this patch applied the test no longer runs forever.
> I understand you prefer the current behavior - how about keeping the
> keep_alive() loop infinite by default and only making it finite when the
> -c argument is passed? Would that be reasonable?
> 

Yes. I am open to taking the patch if the default behavior doesn't change.

thanks,
-- Shuah