Re: [PATCH v6.11 v5.15 v5.4 v4.19 1/1] selftests: breakpoints: use time passed to check if suspend succeed

[Shuah Khan <skhan@linuxfoundation.org>]

On 9/17/24 16:48, Yifei Liu wrote:
> We recently notice that the step_after_suspend_test would
> fail on our plenty devices.  The test believesit failed to

What are "plenty devices"
recently noticed?

Typo - believesit?

> enter suspend state with
> 
> $ sudo ./step_after_suspend_test
> TAP version 13
> Bail out! Failed to enter Suspend state
> 
> However, in the kernel message, I indeed see the system get
> suspended and then wake up later.
> 
> [611172.033108] PM: suspend entry (s2idle)
> [611172.044940] Filesystems sync: 0.006 seconds
> [611172.052254] Freezing user space processes
> [611172.059319] Freezing user space processes completed (elapsed 0.001 seconds)
> [611172.067920] OOM killer disabled.
> [611172.072465] Freezing remaining freezable tasks
> [611172.080332] Freezing remaining freezable tasks completed (elapsed 0.001 seconds)
> [611172.089724] printk: Suspending console(s) (use no_console_suspend to debug)
> [611172.117126] serial 00:03: disabled
> --- some other hardware get reconnected ---
> [611203.136277] OOM killer enabled.
> [611203.140637] Restarting tasks ...
> [611203.141135] usb 1-8.1: USB disconnect, device number 7
> [611203.141755] done.
> [611203.155268] random: crng reseeded on system resumption
> [611203.162059] PM: suspend exit
> 
> After investigation, I notice that for the code block
> if (write(power_state_fd, "mem", strlen("mem")) != strlen("mem"))
> 	ksft_exit_fail_msg("Failed to enter Suspend state\n");
> 
> The write will return -1 and errno is set to 16 (device busy).
> It should be caused by the write function is not successfully returned
> before the system suspend and the return value get messed when waking up.
> As a result, It may be better to check the time passed of those few instructions
> to determine whether the suspend is executed correctly for it is pretty hard to
> execute those few lines for 4 seconds, or even more if it is not long enough.

I don't think this is the right fix. Can you change this to do echo instead.
It does the same thing, but it goes through sysfs interface instead of direct
write:

ret = system("echo mem > /sys/power/state");

> 
> Fixes: bfd092b8c2728 ("selftests: breakpoint: add step_after_suspend_test")
> Reported-by: Sinadin Shan <sinadin.shan@oracle.com>
> Signed-off-by: Yifei Liu <yifei.l.liu@oracle.com>
> ---
>   .../selftests/breakpoints/step_after_suspend_test.c      | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/breakpoints/step_after_suspend_test.c b/tools/testing/selftests/breakpoints/step_after_suspend_test.c
> index dfec31fb9b30d..d615f091e5bae 100644
> --- a/tools/testing/selftests/breakpoints/step_after_suspend_test.c
> +++ b/tools/testing/selftests/breakpoints/step_after_suspend_test.c
> @@ -18,6 +18,7 @@
>   #include <sys/timerfd.h>
>   #include <sys/types.h>
>   #include <sys/wait.h>
> +#include <time.h>
>   
>   #include "../kselftest.h"
>   
> @@ -133,6 +134,7 @@ void suspend(void)
>   	int timerfd;
>   	int err;
>   	struct itimerspec spec = {};
> +	clock_t t;
>   
>   	if (getuid() != 0)
>   		ksft_exit_skip("Please run the test as root - Exiting.\n");
> @@ -152,8 +154,11 @@ void suspend(void)
>   	if (err < 0)
>   		ksft_exit_fail_msg("timerfd_settime() failed\n");

I don't think you will need to add clock() code. timerfd_settime()
sets the time for 5 seconds and you can simply extend the alarm
time.

There needs to be some logic to check timer elapse and poll the
timer_fd
  
>   
> -	if (write(power_state_fd, "mem", strlen("mem")) != strlen("mem"))
> -		ksft_exit_fail_msg("Failed to enter Suspend state\n");
> +	t = clock();
> +	write(power_state_fd, "mem", strlen("mem"));
> +	t = clock()-t;
> +	if ((int)(t) < 4)
> +			ksft_exit_fail_msg("Failed to enter Suspend state %d\n",errno);
>   
>   	close(timerfd);
>   	close(power_state_fd);

thanks,
-- Shuah