Re: [PATCH] pps: add epoll support

[Rodolfo Giometti <giometti@enneenne.com>]

On 20/02/25 17:45, Denis OSTERLAND-HEIM wrote:
> Hi,
> 
> Thanks for the fast answer.
> 
> -----Original Message-----
> From: Rodolfo Giometti <giometti@enneenne.com>
> Sent: Thursday, February 20, 2025 9:51 AM
> To: Denis OSTERLAND-HEIM <denis.osterland@diehl.com>
> Cc: linux-kernel@vger.kernel.org
> 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@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming