[PATCH 0/4] ntsync: some small fixes for doc and selftests

[PATCH 0/4] ntsync: some small fixes for doc and selftests

[Su Hui]

There are four small fixes for ntsync test and doc. I divided these into
four different patches due to different types of errors. If one patch is
better, I can do it too.

Su Hui (4):
  selftests: ntsync: fix the wrong condition in wake_all
  selftests: ntsync: avoid possible overflow in 32-bit machine
  selftests: ntsync: update config
  docs: ntsync: update NTSYNC_IOC_*

 Documentation/userspace-api/ntsync.rst         | 18 +++++++++---------
 tools/testing/selftests/drivers/ntsync/config  |  2 +-
 .../testing/selftests/drivers/ntsync/ntsync.c  |  6 +++---
 3 files changed, 13 insertions(+), 13 deletions(-)

-- 
2.30.2

[PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all

[Su Hui]

When  'manual=false' and  'signaled=true', then expected value when using
NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 3aad311574c4..bfb6fad653d0 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -968,7 +968,7 @@ TEST(wake_all)
 	auto_event_args.manual = false;
 	auto_event_args.signaled = true;
 	objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
-	EXPECT_EQ(0, objs[3]);
+	EXPECT_LE(0, objs[3]);
 
 	wait_args.timeout = get_abs_timeout(1000);
 	wait_args.objs = (uintptr_t)objs;
-- 
2.30.2

[PATCH 2/4] selftests: ntsync: avoid possible overflow in 32-bit machine

[Su Hui]

When using '-m32' flag  to compile this test with gcc, there are some
errors when running test:

 ntsync.c:785:wake_any:Expected ETIMEDOUT (110) == ret (0)
 ntsync.c:823:wake_any:Expected (1) (1) == __count (0)
 ...
 FAILED: 7 / 11 tests passed.
 Totals: pass:7 fail:4 xfail:0 xpass:0 skip:0 error:0

There is an overflow about 'timeout'. 'timespec->tv_sec' is 4 bytes in
32-bit machine. And 'timeout.tv_sec * 1000000000' causing the overflow
problem, adding a cast to avoid this problem.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 tools/testing/selftests/drivers/ntsync/ntsync.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index bfb6fad653d0..ded83dc58e6b 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -103,7 +103,7 @@ static int wait_objs(int fd, unsigned long request, __u32 count,
 
 	clock_gettime(CLOCK_MONOTONIC, &timeout);
 
-	args.timeout = timeout.tv_sec * 1000000000 + timeout.tv_nsec;
+	args.timeout = (__u64)timeout.tv_sec * 1000000000 + timeout.tv_nsec;
 	args.count = count;
 	args.objs = (uintptr_t)objs;
 	args.owner = owner;
@@ -729,7 +729,7 @@ static __u64 get_abs_timeout(unsigned int ms)
 {
 	struct timespec timeout;
 	clock_gettime(CLOCK_MONOTONIC, &timeout);
-	return (timeout.tv_sec * 1000000000) + timeout.tv_nsec + (ms * 1000000);
+	return ((__u64)timeout.tv_sec * 1000000000) + timeout.tv_nsec + (ms * 1000000);
 }
 
 static int wait_for_thread(pthread_t thread, unsigned int ms)
-- 
2.30.2

[PATCH 3/4] selftests: ntsync: update config

[Su Hui]

ntsync should be tested when CONFIG_NTSYNC is setting rather than
CONFIG_WINESYNC, correct this.

Signed-off-by: Su Hui <suhui@nfschina.com>
---
 tools/testing/selftests/drivers/ntsync/config | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/ntsync/config b/tools/testing/selftests/drivers/ntsync/config
index 60539c826d06..0aa68de147af 100644
--- a/tools/testing/selftests/drivers/ntsync/config
+++ b/tools/testing/selftests/drivers/ntsync/config
@@ -1 +1 @@
-CONFIG_WINESYNC=y
+CONFIG_NTSYNC=y
-- 
2.30.2

Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all

[Dan Carpenter]

On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote:
> When  'manual=false' and  'signaled=true', then expected value when using
> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error.
> 
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
> index 3aad311574c4..bfb6fad653d0 100644
> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c
> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
> @@ -968,7 +968,7 @@ TEST(wake_all)
>  	auto_event_args.manual = false;
>  	auto_event_args.signaled = true;
>  	objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
> -	EXPECT_EQ(0, objs[3]);
> +	EXPECT_LE(0, objs[3]);

It's kind of weird how these macros put the constant on the left.
It returns an "fd" on success.  So this look reasonable.  It probably
won't return the zero fd so we could probably check EXPECT_LT()?

regards,
dan carpenter

Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all

[Su Hui]

On 2025/3/14 18:14, Su Hui wrote:
> On 2025/3/14 17:21, Dan Carpenter wrote:
>> On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote:
>>> When  'manual=false' and  'signaled=true', then expected value when using
>>> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error.
>>>
>>> Signed-off-by: Su Hui<suhui@nfschina.com>
>>> ---
>>>   tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
>>> index 3aad311574c4..bfb6fad653d0 100644
>>> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c
>>> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
>>> @@ -968,7 +968,7 @@ TEST(wake_all)
>>>   	auto_event_args.manual = false;
>>>   	auto_event_args.signaled = true;
>>>   	objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
>>> -	EXPECT_EQ(0, objs[3]);
>>> +	EXPECT_LE(0, objs[3]);
>> It's kind of weird how these macros put the constant on the left.
>> It returns an "fd" on success.  So this look reasonable.  It probably
>> won't return the zero fd so we could probably check EXPECT_LT()?
> Agreed, there are about 29 items that can be changed to EXPECT_LT().
> I can send a v2 patchset with this change if there is no more other
> suggestions.

Sorry for the wrong style of email:(.

Su Hui

Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all

[Elizabeth Figura]

On Friday, 14 March 2025 02:14:51 CDT Su Hui wrote:
> When  'manual=false' and  'signaled=true', then expected value when using
> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error.
> 
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
> index 3aad311574c4..bfb6fad653d0 100644
> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c
> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
> @@ -968,7 +968,7 @@ TEST(wake_all)
>  	auto_event_args.manual = false;
>  	auto_event_args.signaled = true;
>  	objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
> -	EXPECT_EQ(0, objs[3]);
> +	EXPECT_LE(0, objs[3]);
>  
>  	wait_args.timeout = get_abs_timeout(1000);
>  	wait_args.objs = (uintptr_t)objs;
> 

Reviewed-by: Elizabeth Figura <zfigura@codeweavers.com>

Re: [PATCH 2/4] selftests: ntsync: avoid possible overflow in 32-bit machine

[Elizabeth Figura]

On Friday, 14 March 2025 02:14:52 CDT Su Hui wrote:
> When using '-m32' flag  to compile this test with gcc, there are some
> errors when running test:
> 
>  ntsync.c:785:wake_any:Expected ETIMEDOUT (110) == ret (0)
>  ntsync.c:823:wake_any:Expected (1) (1) == __count (0)
>  ...
>  FAILED: 7 / 11 tests passed.
>  Totals: pass:7 fail:4 xfail:0 xpass:0 skip:0 error:0
> 
> There is an overflow about 'timeout'. 'timespec->tv_sec' is 4 bytes in
> 32-bit machine. And 'timeout.tv_sec * 1000000000' causing the overflow
> problem, adding a cast to avoid this problem.
> 
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  tools/testing/selftests/drivers/ntsync/ntsync.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
> index bfb6fad653d0..ded83dc58e6b 100644
> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c
> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
> @@ -103,7 +103,7 @@ static int wait_objs(int fd, unsigned long request, __u32 count,
>  
>  	clock_gettime(CLOCK_MONOTONIC, &timeout);
>  
> -	args.timeout = timeout.tv_sec * 1000000000 + timeout.tv_nsec;
> +	args.timeout = (__u64)timeout.tv_sec * 1000000000 + timeout.tv_nsec;
>  	args.count = count;
>  	args.objs = (uintptr_t)objs;
>  	args.owner = owner;
> @@ -729,7 +729,7 @@ static __u64 get_abs_timeout(unsigned int ms)
>  {
>  	struct timespec timeout;
>  	clock_gettime(CLOCK_MONOTONIC, &timeout);
> -	return (timeout.tv_sec * 1000000000) + timeout.tv_nsec + (ms * 1000000);
> +	return ((__u64)timeout.tv_sec * 1000000000) + timeout.tv_nsec + (ms * 1000000);
>  }
>  
>  static int wait_for_thread(pthread_t thread, unsigned int ms)
> 

Reviewed-by: Elizabeth Figura <zfigura@codeweavers.com>

Re: [PATCH 3/4] selftests: ntsync: update config

[Elizabeth Figura]

On Friday, 14 March 2025 02:14:53 CDT Su Hui wrote:
> ntsync should be tested when CONFIG_NTSYNC is setting rather than
> CONFIG_WINESYNC, correct this.
> 
> Signed-off-by: Su Hui <suhui@nfschina.com>
> ---
>  tools/testing/selftests/drivers/ntsync/config | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/drivers/ntsync/config b/tools/testing/selftests/drivers/ntsync/config
> index 60539c826d06..0aa68de147af 100644
> --- a/tools/testing/selftests/drivers/ntsync/config
> +++ b/tools/testing/selftests/drivers/ntsync/config
> @@ -1 +1 @@
> -CONFIG_WINESYNC=y
> +CONFIG_NTSYNC=y
> 

Reviewed-by: Elizabeth Figura <zfigura@codeweavers.com>

Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all

[Elizabeth Figura]

On Friday, 14 March 2025 05:14:30 CDT Su Hui wrote:
> On 2025/3/14 17:21, Dan Carpenter wrote:
> > On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote:
> >> When  'manual=false' and  'signaled=true', then expected value when using
> >> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error.
> >>
> >> Signed-off-by: Su Hui<suhui@nfschina.com>
> >> ---
> >>   tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
> >> index 3aad311574c4..bfb6fad653d0 100644
> >> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c
> >> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
> >> @@ -968,7 +968,7 @@ TEST(wake_all)
> >>   	auto_event_args.manual = false;
> >>   	auto_event_args.signaled = true;
> >>   	objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
> >> -	EXPECT_EQ(0, objs[3]);
> >> +	EXPECT_LE(0, objs[3]);
> > It's kind of weird how these macros put the constant on the left.
> > It returns an "fd" on success.  So this look reasonable.  It probably
> > won't return the zero fd so we could probably check EXPECT_LT()?
> Agreed, there are about 29 items that can be changed to EXPECT_LT().
> I can send a v2 patchset with this change if there is no more other
> suggestions.

I personally think it looks wrong to use EXPECT_LT(), but I'll certainly defer to a higher maintainer on this point.

Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all

[Dan Carpenter]

On Fri, Mar 14, 2025 at 05:13:50PM -0500, Elizabeth Figura wrote:
> On Friday, 14 March 2025 05:14:30 CDT Su Hui wrote:
> > On 2025/3/14 17:21, Dan Carpenter wrote:
> > > On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote:
> > >> When  'manual=false' and  'signaled=true', then expected value when using
> > >> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error.
> > >>
> > >> Signed-off-by: Su Hui<suhui@nfschina.com>
> > >> ---
> > >>   tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +-
> > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
> > >> index 3aad311574c4..bfb6fad653d0 100644
> > >> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c
> > >> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
> > >> @@ -968,7 +968,7 @@ TEST(wake_all)
> > >>   	auto_event_args.manual = false;
> > >>   	auto_event_args.signaled = true;
> > >>   	objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
> > >> -	EXPECT_EQ(0, objs[3]);
> > >> +	EXPECT_LE(0, objs[3]);
> > > It's kind of weird how these macros put the constant on the left.
> > > It returns an "fd" on success.  So this look reasonable.  It probably
> > > won't return the zero fd so we could probably check EXPECT_LT()?
> > Agreed, there are about 29 items that can be changed to EXPECT_LT().
> > I can send a v2 patchset with this change if there is no more other
> > suggestions.
> 
> I personally think it looks wrong to use EXPECT_LT(), but I'll certainly
> defer to a higher maintainer on this point.

I'm not sure I understand what you are saying.  Are you saying that we
should allow zero as an expected file descriptor here?  I don't have
strong feelings about that either way.

Putting variables on the right, Yoda speak is.  Unnatural is.

I did a git grep and the KUNIT_EXPECT_LT() just calls the parameters
left and right instead of "expected" and "seen".  Expected is wrong
for LT because we expect it to be != to the expected value.  It's
the opposite.  We're expecting the unexpected!  It would be better
to just call them left and right.

regards,
dan carpenter

Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all

[Elizabeth Figura]

On Saturday, 15 March 2025 04:39:46 CDT Dan Carpenter wrote:
> On Fri, Mar 14, 2025 at 05:13:50PM -0500, Elizabeth Figura wrote:
> > On Friday, 14 March 2025 05:14:30 CDT Su Hui wrote:
> > > On 2025/3/14 17:21, Dan Carpenter wrote:
> > > > On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote:
> > > >> When  'manual=false' and  'signaled=true', then expected value when using
> > > >> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error.
> > > >>
> > > >> Signed-off-by: Su Hui<suhui@nfschina.com>
> > > >> ---
> > > >>   tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +-
> > > >>   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
> > > >> index 3aad311574c4..bfb6fad653d0 100644
> > > >> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c
> > > >> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
> > > >> @@ -968,7 +968,7 @@ TEST(wake_all)
> > > >>   	auto_event_args.manual = false;
> > > >>   	auto_event_args.signaled = true;
> > > >>   	objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
> > > >> -	EXPECT_EQ(0, objs[3]);
> > > >> +	EXPECT_LE(0, objs[3]);
> > > > It's kind of weird how these macros put the constant on the left.
> > > > It returns an "fd" on success.  So this look reasonable.  It probably
> > > > won't return the zero fd so we could probably check EXPECT_LT()?
> > > Agreed, there are about 29 items that can be changed to EXPECT_LT().
> > > I can send a v2 patchset with this change if there is no more other
> > > suggestions.
> > 
> > I personally think it looks wrong to use EXPECT_LT(), but I'll certainly
> > defer to a higher maintainer on this point.
> 
> I'm not sure I understand what you are saying.  Are you saying that we
> should allow zero as an expected file descriptor here?  I don't have
> strong feelings about that either way.

Yes, my apologies for the ambiguous wording. That is, EXPECT_LE looks more correct to me than EXPECT_LT per se.

> Putting variables on the right, Yoda speak is.  Unnatural is.

Yes, I certainly agree with this. I wrote it this way in the first place because I was following some other example, I forget which.

> I did a git grep and the KUNIT_EXPECT_LT() just calls the parameters
> left and right instead of "expected" and "seen".  Expected is wrong
> for LT because we expect it to be != to the expected value.  It's
> the opposite.  We're expecting the unexpected!  It would be better
> to just call them left and right.
> 
> regards,
> dan carpenter
> 

Re: [PATCH 1/4] selftests: ntsync: fix the wrong condition in wake_all

[Su Hui]

On 2025/3/16 04:29, Elizabeth Figura wrote:
> On Saturday, 15 March 2025 04:39:46 CDT Dan Carpenter wrote:
>> On Fri, Mar 14, 2025 at 05:13:50PM -0500, Elizabeth Figura wrote:
>>> On Friday, 14 March 2025 05:14:30 CDT Su Hui wrote:
>>>> On 2025/3/14 17:21, Dan Carpenter wrote:
>>>>> On Fri, Mar 14, 2025 at 03:14:51PM +0800, Su Hui wrote:
>>>>>> When  'manual=false' and  'signaled=true', then expected value when using
>>>>>> NTSYNC_IOC_CREATE_EVENT should be greater than zero. Fix this typo error.
>>>>>>
>>>>>> Signed-off-by: Su Hui<suhui@nfschina.com>
>>>>>> ---
>>>>>>    tools/testing/selftests/drivers/ntsync/ntsync.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
>>>>>> index 3aad311574c4..bfb6fad653d0 100644
>>>>>> --- a/tools/testing/selftests/drivers/ntsync/ntsync.c
>>>>>> +++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
>>>>>> @@ -968,7 +968,7 @@ TEST(wake_all)
>>>>>>    	auto_event_args.manual = false;
>>>>>>    	auto_event_args.signaled = true;
>>>>>>    	objs[3] = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
>>>>>> -	EXPECT_EQ(0, objs[3]);
>>>>>> +	EXPECT_LE(0, objs[3]);
>>>>> It's kind of weird how these macros put the constant on the left.
>>>>> It returns an "fd" on success.  So this look reasonable.  It probably
>>>>> won't return the zero fd so we could probably check EXPECT_LT()?
>>>> Agreed, there are about 29 items that can be changed to EXPECT_LT().
>>>> I can send a v2 patchset with this change if there is no more other
>>>> suggestions.
>>> I personally think it looks wrong to use EXPECT_LT(), but I'll certainly
>>> defer to a higher maintainer on this point.
>> I'm not sure I understand what you are saying.  Are you saying that we
>> should allow zero as an expected file descriptor here?  I don't have
>> strong feelings about that either way.
> Yes, my apologies for the ambiguous wording. That is, EXPECT_LE looks more correct to me than EXPECT_LT per se.

Got it, I think there is no need for v2 patch that using EXPECT_LT(). 
Thanks for your feedback.

Su Hui