Re: [PATCH] pps: add epoll support

From: Rodolfo Giometti
Date: Fri Feb 21 2025 - 04:22:43 EST

On 20/02/25 17:45, Denis OSTERLAND-HEIM wrote:
Hi,

Thanks for the fast answer.

-----Original Message-----
From: Rodolfo Giometti <giometti@xxxxxxxxxxxx>
Sent: Thursday, February 20, 2025 9:51 AM
To: Denis OSTERLAND-HEIM <denis.osterland@xxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx
Subject: [EXT] Re: [PATCH] pps: add epoll support

Can you explain it a bit better?
I will do my best.

In an application, that has more to do than just dealing with one PPS device,
to use PPS_FETCH with a timeout until next event, you need a thread which can sleep.

Why are you saying that? If you use blocking I/O with a timeout in the poll() it should work.

I would really like to avoid threads and the resulting synchronization complexity.

Alternative is to fetch the current assert value in at least twice the expected fequency.
This would definetly work, but epoll is the more efficent way to do.

Without epoll in one thread:
```c
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <linux/pps.h>

struct per_pps {
int dev_fd;
struct pps_fdata fdata;
unsigned int last_assert;
};

int main(int argc, const char* argv[]) {
int ret = 0;
struct per_pps instances[] = {
{ .dev_fd = open((argc > 1) ? argv[1] : "/dev/pps0", O_RDONLY) },
{ .dev_fd = open((argc > 2) ? argv[2] : "/dev/pps1", O_RDONLY) }
};
if (instances[0].dev_fd < 0 || instances[1].dev_fd < 0) {
perror("failed to open dev");
ret = 1;
goto out;
}

for (int loops = 10; --loops;) {
for (int i = 0; i < 2; ++i) {
if (ioctl(instances[i].dev_fd, PPS_FETCH, &instances[i].fdata) < 0) {

fdata is not initialized here... is it set to all zero?

perror("failed to fetch data");
ret = 1;
goto out;
}

if (instances[i].last_assert != instances[i].fdata.info.assert_sequence) {
instances[i].last_assert = instances[i].fdata.info.assert_sequence;
printf(
"assert: %u\ntime: %lld.%09d\n",
instances[i].fdata.info.assert_sequence,
instances[i].fdata.info.assert_tu.sec,
instances[i].fdata.info.assert_tu.nsec
);
}

}
usleep(300000);
}

out:
if (instances[0].dev_fd >= 0)
close(instances[0].dev_fd);
if (instances[1].dev_fd >= 0)
close(instances[1].dev_fd);
return ret;
}
```

Syscalls are pretty expensive and epoll allows use to reduce them.

```c
#include <stdio.h>
#include <string.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <linux/pps.h>
#include <poll.h>

int main(int argc, const char* argv[]) {
int ret = 0;
struct pollfd instances[] = {
{ .fd = open((argc > 1) ? argv[1] : "/dev/pps0", O_RDONLY), .events = POLLIN|POLLERR , .revents = 0 },
{ .fd = open((argc > 2) ? argv[2] : "/dev/pps1", O_RDONLY), .events = POLLIN|POLLERR , .revents = 0 }
};
if (instances[0].fd < 0 || instances[1].fd < 0) {
perror("failed to open dev");
ret = 1;
goto out;
}

for (int loops = 4; --loops;) {
if(poll(instances, 2, 2000/*ms*/)) {

Here you are using poll()...

struct pps_fdata fdata;
for (int i = 0; i < 2; ++i) {
if ((instances[i].revents & POLLIN) != POLLIN)
continue;

if (ioctl(instances[i].fd, PPS_FETCH, &fdata) < 0) {

Again, fdata is not initialized here...

perror("failed to fetch data");
ret = 1;
goto out;
}

printf(
"assert: %u\ntime: %lld.%09d\n",
fdata.info.assert_sequence,
fdata.info.assert_tu.sec,
fdata.info.assert_tu.nsec
);
}
} else {
printf("time-out\n");
}
}

out:
if (instances[0].fd >= 0)
close(instances[0].fd);
if (instances[1].fd >= 0)
close(instances[1].fd);
return ret;
}
```

I think you should try current LinuxPPS implementation but with proper fdata initialization.

RFC2783 states that to access to PPS timestamps we should use the
time_pps_fetch() function, where we may read:

3.4.3 New functions: access to PPS timestamps

The API includes one function that gives applications access to PPS
timestamps. As an implementation option, the application may request
the API to block until the next timestamp is captured. (The API does
not directly support the use of the select() or poll() system calls
to wait for PPS events.)

How do you think to use this new select()/poll() support without breaking the
RFC2783 compliance?
To me RFC reads like the spcification of pps-tools/timepps.h and not the one for the char device.

Yes, but the char device used to implement the PPS API should work with select()/poll()!

3.4.1 New functions: obtaining PPS sources
...
The definition of what special files are appropriate for use with the
PPS API is outside the scope of this specification, and may vary
based on both operating system implementation, and local system
configuration.

To me "The API does not directly support the use of the select() or poll() system calls" simply means:
there is no wrapper function that calls select() or poll() for you

I agree.

I do not see why an additional function of the underlying character device would break the API.
You may just do not use it and everything works like before.
But I see your point.
If the char dev interface is ment to be the RFC interface only, there is no need to support epoll.
Maybe it would be better to add epoll support to sysfs assert/clear?

As far as I know, epoll() uses the kernel select/poll mechanism and this support should work correctly at the moment. If no, we have to fix it.

Try your code with the current LinuxPPS implementation replacing the ioctl(fd, PPS_FETCH &fdata) with:

time_pps_fetch(instances[i].fd, PPS_TSFMT_TSPEC, &info, NULL);

Ciao,

Rodolfo

--
GNU/Linux Solutions e-mail: giometti@xxxxxxxxxxxx
Linux Device Driver giometti@xxxxxxxx
Embedded Systems phone: +39 349 2432127
UNIX programming