Re: [PATCH 2/2] selftests: timers: Remove local NSEC_PER_SEC and USEC_PER_SEC defines

[Shuah Khan <skhan@linuxfoundation.org>]

On 9/24/24 17:59, John Stultz wrote:
> On Tue, Sep 24, 2024 at 8:57 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> Remove local NSEC_PER_SEC and USEC_PER_SEC defines. Pick them up from
>> include/vdso/time64.h. This requires -I $(top_srcdir) to the timers
>> Makefile to include the include/vdso/time64.h.
>>
>> posix_timers test names the defines NSECS_PER_SEC and USECS_PER_SEC.
>> Include the include/vdso/time64.h and change NSECS_PER_SEC and
>> USECS_PER_SEC references to NSEC_PER_SEC and USEC_PER_SEC respectively.
> 
> Nit: You got the last bit switched there. This patch changes local
> NSEC_PER_SEC to the upstream defined NSECS_PER_SEC.

I think what IO have is correct. posix_timers defines them as NSECS_PER_SEC
and USECS_PER_SEC and the header file doesn't have the extra S. It could
use rephrasing thought to make it clear.

Is it okay to fix this when I apply the patch or would you like me to send v2?

> 
> Overall no objection from me. I've always pushed to have the tests be
> mostly self-contained so they can be built outside of the kernel
> source, but at this point the current kselftest.h dependencies means
> it already takes some work, so this isn't introducing an undue
> hardship.
> 

Yes. At this point it would be hard to build it outside. DO you think
these defines can be part of uapi?
  
> Other then the nit,
>    Acked-by: John Stultz <jstultz@google.com>
> 
>> diff --git a/tools/testing/selftests/timers/adjtick.c b/tools/testing/selftests/timers/adjtick.c
>> index 205b76a4abb4..cb9a30f54662 100644
>> --- a/tools/testing/selftests/timers/adjtick.c
>> +++ b/tools/testing/selftests/timers/adjtick.c
>> @@ -22,14 +22,12 @@
>>   #include <sys/time.h>
>>   #include <sys/timex.h>
>>   #include <time.h>
>> +#include <include/vdso/time64.h>
>>
>>   #include "../kselftest.h"
>>
>>   #define CLOCK_MONOTONIC_RAW    4
> 
> I suspect CLOCK_MONOTONIC_RAW (and the other clockid definitions)
> could be similarly removed here as well in a future patch?

Yes. It could be cleaned up. I will send a patch in for this.

thanks,
-- Shuah