Re: [PATCH for-next 1/3] selftests/watchdog: add count parameter for watchdog-test

[Zhijian Li (Fujitsu)]

On 28/10/2024 11:29, Shuah Khan wrote:
> On 10/27/24 18:50, Zhijian Li (Fujitsu) wrote:
>>
>>
>> On 27/10/2024 08:28, Shuah Khan wrote:
>>> On 10/24/24 19:39, Li Zhijian wrote:
>>>> Currently, watchdog-test keep running until it gets a SIGINT. However,
>>>> when watchdog-test is executed from the kselftests framework, where it
>>>> launches test via timeout which will send SIGTERM in time up. This could
>>>> lead to
>>>> 1. watchdog haven't stop, a watchdog reset is triggered to reboot the OS
>>>>      in silent.
>>>> 2. kselftests gets an timeout exit code, and judge watchdog-test as
>>>>     'not ok'
>>>>
>>> This test isn't really supposed to be run from kselftest framework.
>>> This is the reason why it isn't included in the default run.
>>
>> May I know what's the default run, is it different from `make run_tests` ?
> 
> No it isn't. "make kselftest" runs only the targets mentioned in the
> selftests Makefile. That is considered the kselftest default run.

Hey, Shuah,


Thanks for your explanation.
If that is the case, I do not have an urgent need for the current patch, expect
I'd like to avoid the reboot issue after an accidentally `make run_tests`

Some changes are make as below, please take a look. I will send it out we reach a consensus.


commit 2296f9d88fde4921758a45bf160a7f1b9d4678a0 (HEAD)
Author: Li Zhijian <lizhijian@xxxxxxxxxxx>
Date:   Mon Oct 28 11:54:03 2024 +0800

     selftests/watchdog-test: Fix system accidentally reset after watchdog-test
     
     After `make run_tests` to run watchdog-test, a system reboot would
     happen due to watchdog not stop.
     ```
     [ 1367.185172] watchdog: watchdog0: watchdog did not stop!
     ```
     
     Fix it by registering a timeout signal in watchdog-test, where its
     signal handler will stop the watchdog.
     
     After that
      # timeout 1 ./watchdog-test
      Watchdog Ticking Away!
      .
      Stopping watchdog ticks...
     
     Signed-off-by: Li Zhijian <lizhijian@xxxxxxxxxxx>

diff --git a/tools/testing/selftests/watchdog/watchdog-test.c b/tools/testing/selftests/watchdog/watchdog-test.c
index bc71cbca0dde..97acb90f8b30 100644
--- a/tools/testing/selftests/watchdog/watchdog-test.c
+++ b/tools/testing/selftests/watchdog/watchdog-test.c
@@ -335,6 +335,10 @@ int main(int argc, char *argv[])
         printf("Watchdog Ticking Away!\n");
  
         signal(SIGINT, term);
+       /*
+        * Register the timeout signal
+        */
+       signal(SIGTERM, term);
  
         while (1) {
                 keep_alive();




> 
> There is a reason why watchdog isn't included in the default run.
> It isn't intended to be run by users by default as this is test is
> just for testing watchdog api
> 
>>
>>
>>>
>>>> This patch is prepare to fix above 2 issues
>>>
>>> This series needs a separate cover letter explaining how this problem is
>>> being fixed.
>>
>> Cover letter is in this patch, see below:
>> In addition, we can get the 'How' by reading the simple change in each change.
> 
> That isn't enough to understand why this change is needed.
> Send patch series with a cover letter explaining what you are
> doing.
> 
>>
>>
>>>
>>>>
>>>> Signed-off-by: Li Zhijian <lizhijian@xxxxxxxxxxx>
>>>> ---
>>>> Hey,
>>>> Cover letter is here.
>>>>
>>>> It's notice that a OS reboot was triggerred after ran the watchdog-test
>>>> in kselftests framwork 'make run_tests', that's because watchdog-test
>>>> didn't stop feeding the watchdog after enable it.
>>>>
>>>> In addition, current watchdog-test didn't adapt to the kselftests
>>>> framework which launchs the test with /usr/bin/timeout and no timeout
>>>> is expected.
>>>> ---
> 
> thanks,
> -- Shuah