RE: [EXT] Re: [PATCH] pps: add epoll support

[Denis OSTERLAND-HEIM <denis.osterland@diehl.com>]

[-- Attachment #1: Type: text/plain, Size: 9404 bytes --]

Hi,

Okay, if poll is expected to work, than we have a bug.
Actually a pretty old one.

pps_cdev_poll() uncoditionally returns (EPOLLIN | EPOLLRDNORM), which results in poll() will return immediately with data available 
(EPOLLIN | EPOLLRDNORM).
To avoid this, you need conditionally return 0.
My patch adds a context per open file to store the last_ev value when ioctl(PPS_FETCH) is invoked and uses this last_ev in poll as 
condition.

Sorry, for the missing memset(&fdata, 0, sizeof(fdata)).
Intention was set to 0, yes.

```c
#include <stdio.h>
#include <string.h>
#include <poll.h>
#include <fcntl.h>
#include "timepps.h"

int main(int argc, const char* argv[]) {
    struct pollfd instance = { .fd = open((argc > 1) ? argv[1] : "/dev/pps0", O_RDONLY), .events = POLLIN|POLLERR , .revents = 0 };
    pps_handle_t pps_handle;
    static const struct timespec timeout = { 0, 0 };
    if (time_pps_create(instance.fd, &pps_handle)) {
        perror("failed to create pps handle");
        return 1;
    }
    for (int loops = 4; --loops; ) {
        pps_info_t pps_info;
        memset(&pps_info, 0, sizeof(pps_info));
        if (!poll(&instance, 1, 2000/*ms*/)) {
            printf("timeout");
            continue;
        }
        if ((instance.revents & POLLIN) != POLLIN) {
            printf("nothing to read?");
            continue;
        }
        if (time_pps_fetch(pps_handle, PPS_TSFMT_TSPEC, &pps_info, &timeout)) {
            perror("failed to fetch");
            return 1;
        }

        printf(
            "assert: %lu\ntime: %ld.%09ld\n",
            pps_info.assert_sequence,
            pps_info.assert_tu.tspec.tv_sec,
            pps_info.assert_tu.tspec.tv_nsec
        );
    }
    return 0;
}
```

Currently output looks like:
```
$ cat /sys/class/pps/pps0/assert; ./test /dev/pps0
1520598954.468882076#60
assert: 60
time: 1520598954.468882076
assert: 60
time: 1520598954.468882076
assert: 60
time: 1520598954.468882076
```

You see no waits between the loops.

With my patch, this looks like:
```
$ cat /sys/class/pps/pps0/assert; ./test /dev/pps0
1520599016.682113965#122
assert: 123
time: 1520599017.682133871
assert: 124
time: 1520599018.682155815
assert: 125
time: 1520599019.682161204
```

I am currently rebasing and testing it with recent changes to pps.c for v2.

Regards, Denis

-----Original Message-----
From: Rodolfo Giometti <giometti@enneenne.com>
Sent: Friday, February 21, 2025 10:18 AM
To: Denis OSTERLAND-HEIM <denis.osterland@diehl.com>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] pps: add epoll support


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

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6038 bytes --]