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

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

[Denis OSTERLAND-HEIM]

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.
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) {
                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*/)) {
            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) {
                    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;
}
```

> 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.

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 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?

> Also, if we decide to add this support, we should also consider adding the
> PPS_CANPOLL mode bit definition (see
> https://www.rfc-editor.org/rfc/rfc2783.html#section-3.3), and proving proper
> code in order to show how we can use or not this new functionality.

> > @@ -300,7 +312,13 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
> >   {
> >   struct pps_device *pps = container_of(inode->i_cdev,
> >   struct pps_device, cdev);
> > -file->private_data = pps;
> > +struct pps_context *ctx = kzalloc(sizeof(struct pps_context), GFP_KERNEL);
> > +
> > +if (unlikely(ZERO_OR_NULL_PTR(ctx)))
> > +return -ENOMEM;
> > +file->private_data = ctx;
> > +ctx->pps = pps;
> > +ctx->ev = pps->last_ev;
>
> Doing so, we always miss the first event... maybe can we drop this setting and
> leaving ctx->ev set to zero?
Well, I see two use cases:
1. open(), ioctl() to get current value, epoll() to wait for next event, ioctl() to get the next value
2. open(), epoll() wait for next event or time-out to see if pps is "alive"
I do not see the case that we completely miss one.

Regards, Denis

> Ciao,
>
> Rodolfo
>
> --
> GNU/Linux Solutions                  e-mail: giometti@enneenne.com
> Linux Device Driver                          giometti@linux.it
> Embedded Systems                     phone:  +39 349 2432127
> UNIX programming
Diehl Metering GmbH, Donaustrasse 120, 90451 Nuernberg
Sitz der Gesellschaft: Ansbach, Registergericht: Ansbach HRB 69
Geschaeftsfuehrer: Dr. Christof Bosbach (Sprecher), Dipl.-Dolm. Annette Geuther, Dipl.-Kfm. Reiner Edel, Jean-Claude Luttringer

Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail drucken. Diese E-Mail kann vertrauliche Informationen enthalten. Sollten die in dieser E-Mail enthaltenen Informationen nicht für Sie bestimmt sein, informieren Sie bitte unverzueglich den Absender per E-Mail und loeschen Sie diese E-Mail in Ihrem System. Jede unberechtigte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt. Informationen zum Datenschutz finden Sie auf unserer Homepage<https://www.diehl.com/metering/de/impressum-und-rechtliche-hinweise/>.

Before printing, think about environmental responsibility.This message may contain confidential information. If you are not authorized to receive this information please advise the sender immediately by reply e-mail and delete this message without making any copies. Any form of unauthorized use, publication, reproduction, copying or disclosure of the e-mail is not permitted. Information about data protection can be found on our homepage<https://www.diehl.com/metering/en/data-protection/>.

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

[Denis OSTERLAND-HEIM]

Hi,

Thanks!
I will test your suggentions and report next week ;-)
Have a nice weekend.

Regards, Denis

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


On 21/02/25 11:49, Denis OSTERLAND-HEIM wrote:
> 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.

I think you are right!

Looking at the code I think the correct patch should be:

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 6a02245ea35f..7a52bb9f835b 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -41,7 +41,7 @@ static __poll_t pps_cdev_poll(struct file *file, poll_table *wait)

         poll_wait(file, &pps->queue, wait);

-       return EPOLLIN | EPOLLRDNORM;
+       return pps->info.mode & PPS_CANWAIT ? 0 : EPOLLIN | EPOLLRDNORM;
  }

  static int pps_cdev_fasync(int fd, struct file *file, int on)

> 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.

OK

> ```c
> #include <stdio.h>
> #include <string.h>///home/giometti/Projects/ailux/imx9/linux/linux-imx
> #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.

Please, try again with the above patch.

However, before doing the test, you should consider to add this patch too:

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 6a02245ea35f..7a52bb9f835b 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -56,10 +56,13 @@ static int pps_cdev_pps_fetch(struct pps_device *pps, struct
pps_fdata *fdata)
         int err = 0;

         /* Manage the timeout */
-       if (fdata->timeout.flags & PPS_TIME_INVALID)
-               err = wait_event_interruptible(pps->queue,
+       if (fdata->timeout.flags & PPS_TIME_INVALID) {
+               if (pps->info.mode & PPS_CANWAIT)
+                       err = wait_event_interruptible(pps->queue,
                                 ev != pps->last_ev);
-       else {
+               else
+                       return -EOPNOTSUPP;
+       } else {
                 unsigned long ticks;

                 dev_dbg(&pps->dev, "timeout %lld.%09d\n",
@@ -69,12 +72,15 @@ static int pps_cdev_pps_fetch(struct pps_device *pps, struct
pps_fdata *fdata)
                 ticks += fdata->timeout.nsec / (NSEC_PER_SEC / HZ);

                 if (ticks != 0) {
-                       err = wait_event_interruptible_timeout(
+                       if (pps->info.mode & PPS_CANWAIT) {
+                               err = wait_event_interruptible_timeout(
                                         pps->queue,
                                         ev != pps->last_ev,
                                         ticks);
-                       if (err == 0)
-                               return -ETIMEDOUT;
+                               if (err == 0)
+                                       return -ETIMEDOUT;
+                       } else
+                               return -EOPNOTSUPP;
                 }
         }

In fact RFC2783 states:

3.4.3 New functions: access to PPS timestamps

...
    Support for blocking behavior is an implementation option.  If the
    PPS_CANWAIT mode bit is clear, and the timeout parameter is either
    NULL or points to a non-zero value, the function returns an
    EOPNOTSUPP error.  An application can discover whether the feature is
    implemented by using time_pps_getcap() to see if the PPS_CANWAIT mode
    bit is set.
...

Ciao,

Rodolfo

--
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming
Diehl Metering GmbH, Donaustrasse 120, 90451 Nuernberg
Sitz der Gesellschaft: Ansbach, Registergericht: Ansbach HRB 69
Geschaeftsfuehrer: Dr. Christof Bosbach (Sprecher), Dipl.-Dolm. Annette Geuther, Dipl.-Kfm. Reiner Edel, Jean-Claude Luttringer

Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail drucken. Diese E-Mail kann vertrauliche Informationen enthalten. Sollten die in dieser E-Mail enthaltenen Informationen nicht für Sie bestimmt sein, informieren Sie bitte unverzueglich den Absender per E-Mail und loeschen Sie diese E-Mail in Ihrem System. Jede unberechtigte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt. Informationen zum Datenschutz finden Sie auf unserer Homepage<https://www.diehl.com/metering/de/impressum-und-rechtliche-hinweise/>.

Before printing, think about environmental responsibility.This message may contain confidential information. If you are not authorized to receive this information please advise the sender immediately by reply e-mail and delete this message without making any copies. Any form of unauthorized use, publication, reproduction, copying or disclosure of the e-mail is not permitted. Information about data protection can be found on our homepage<https://www.diehl.com/metering/en/data-protection/>.

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

[Denis OSTERLAND-HEIM]

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

Hi,

I tested it today and that patch creates not the expected behavior.

With your patch:
```
# cat /sys/class/pps/pps1/assert; PpsPollTest /dev/pps1; cat /sys/class/pps/pps1/assert
1520599141.381117584#77
timeout
timeout
timeout
1520599147.529114368#83
```

And pps-ktimer without POLL flag restores current behavor:

diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
index d33106bd7..07a804c35 100644
--- a/drivers/pps/clients/pps-ktimer.c
+++ b/drivers/pps/clients/pps-ktimer.c
@@ -46,7 +46,7 @@ static struct pps_source_info pps_ktimer_info = {
        .path           = "",
        .mode           = PPS_CAPTUREASSERT | PPS_OFFSETASSERT |
                          PPS_ECHOASSERT |
-                         PPS_CANWAIT | PPS_TSFMT_TSPEC,
+                         PPS_TSFMT_TSPEC,
        .owner          = THIS_MODULE,
 };

```
# cat /sys/class/pps/pps1/mode
1051
# cat /sys/class/pps/pps1/assert; PpsPollTest /dev/pps1; cat /sys/class/pps/pps1/assert
1520598959.622011600#45
assert: 45
time: 1520598959.622011600
assert: 45
time: 1520598959.622011600
assert: 45
time: 1520598959.622011600
1520598959.622011600#45
```

The idea is to use poll to wait for the next data become available.
The should poll work like `wait_event_interruptible(pps->queue, ev != pps->last_ev)`,
where `poll_wait(file, &pps->queue, wait)` already does the first part,
but the condition `ev != pps->last_ev` is missing.

Poll shall return 0 if ev == ps->last_ev
and shall return EPOLLIN if ev != ps->last_ev; EPOLLRDNORM is equivalent[^1] so better return both

In case of ioctl(PPS_FETCH) ev is stored on the stack.
If two applications access one pps device, they both get the right result, because the wait_event uses the ev from their stack.
To do so with poll() ev needs to be available via file, because every applications opens a sepate file and the file is passed to poll.
That is what my patch does.
It adds a per file storage so that poll has the ev value of the last fetch to compare.

If you want to avoid this extra alloc and derefers, we may use file position (file.f_pos) to store last fetched ev value.
The pps does not provide read/write, so f_pos is unused anyway.

I am a little bit puzzeled about your second diff.
Every pps client that uses pps_event() supports WAITEVENT, because this is in the generic part.
To me not using pps_event() but reimplement parts of pps_event() seems to be a bad idea.
That’s why I thing that this diff is not needed.

Regards, Denis

[^1]: https://man7.org/linux/man-pages/man2/poll.2.html


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

On 21/02/25 11:49, Denis OSTERLAND-HEIM wrote:
> 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.

I think you are right!

Looking at the code I think the correct patch should be:

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 6a02245ea35f..7a52bb9f835b 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -41,7 +41,7 @@ static __poll_t pps_cdev_poll(struct file *file, poll_table *wait)

         poll_wait(file, &pps->queue, wait);

-       return EPOLLIN | EPOLLRDNORM;
+       return pps->info.mode & PPS_CANWAIT ? 0 : EPOLLIN | EPOLLRDNORM;
  }

  static int pps_cdev_fasync(int fd, struct file *file, int on)

> 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.

OK

> ```c
> #include <stdio.h>
> #include <string.h>///home/giometti/Projects/ailux/imx9/linux/linux-imx
> #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.

Please, try again with the above patch.

However, before doing the test, you should consider to add this patch too:

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 6a02245ea35f..7a52bb9f835b 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -56,10 +56,13 @@ static int pps_cdev_pps_fetch(struct pps_device *pps, struct 
pps_fdata *fdata)
         int err = 0;

         /* Manage the timeout */
-       if (fdata->timeout.flags & PPS_TIME_INVALID)
-               err = wait_event_interruptible(pps->queue,
+       if (fdata->timeout.flags & PPS_TIME_INVALID) {
+               if (pps->info.mode & PPS_CANWAIT)
+                       err = wait_event_interruptible(pps->queue,
                                 ev != pps->last_ev);
-       else {
+               else
+                       return -EOPNOTSUPP;
+       } else {
                 unsigned long ticks;

                 dev_dbg(&pps->dev, "timeout %lld.%09d\n",
@@ -69,12 +72,15 @@ static int pps_cdev_pps_fetch(struct pps_device *pps, struct 
pps_fdata *fdata)
                 ticks += fdata->timeout.nsec / (NSEC_PER_SEC / HZ);

                 if (ticks != 0) {
-                       err = wait_event_interruptible_timeout(
+                       if (pps->info.mode & PPS_CANWAIT) {
+                               err = wait_event_interruptible_timeout(
                                         pps->queue,
                                         ev != pps->last_ev,
                                         ticks);
-                       if (err == 0)
-                               return -ETIMEDOUT;
+                               if (err == 0)
+                                       return -ETIMEDOUT;
+                       } else
+                               return -EOPNOTSUPP;
                 }
         }

In fact RFC2783 states:

3.4.3 New functions: access to PPS timestamps

...
    Support for blocking behavior is an implementation option.  If the
    PPS_CANWAIT mode bit is clear, and the timeout parameter is either
    NULL or points to a non-zero value, the function returns an
    EOPNOTSUPP error.  An application can discover whether the feature is
    implemented by using time_pps_getcap() to see if the PPS_CANWAIT mode
    bit is set.
...

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 --]

Re: [PATCH] pps: add epoll support

[Rodolfo Giometti]

On 24/02/25 12:38, Denis OSTERLAND-HEIM wrote:
> Hi,
> 
> I tested it today and that patch creates not the expected behavior.

[snip]

> The idea is to use poll to wait for the next data become available.
> The should poll work like `wait_event_interruptible(pps->queue, ev != pps->last_ev)`,
> where `poll_wait(file, &pps->queue, wait)` already does the first part,
> but the condition `ev != pps->last_ev` is missing.
> 
> Poll shall return 0 if ev == ps->last_ev
> and shall return EPOLLIN if ev != ps->last_ev; EPOLLRDNORM is equivalent[^1] so better return both
> 
> In case of ioctl(PPS_FETCH) ev is stored on the stack.
> If two applications access one pps device, they both get the right result, because the wait_event uses the ev from their stack.
> To do so with poll() ev needs to be available via file, because every applications opens a sepate file and the file is passed to poll.
> That is what my patch does.
> It adds a per file storage so that poll has the ev value of the last fetch to compare.

I agree, but are you sure that your solution will save you in case of fork()? 
So, why don't simply add a new variable as below?

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 6a02245ea35f..dded1452c3a8 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -40,8 +40,11 @@ static __poll_t pps_cdev_poll(struct file *file, poll_table 
*wait)
         struct pps_device *pps = file->private_data;

         poll_wait(file, &pps->queue, wait);
+       if (pps->info.mode & PPS_CANWAIT)
+               if (pps->fetched_ev != pps->last_ev)
+                       return EPOLLIN | EPOLLRDNORM;

-       return EPOLLIN | EPOLLRDNORM;
+       return 0;
  }

  static int pps_cdev_fasync(int fd, struct file *file, int on)
@@ -186,9 +195,11 @@ static long pps_cdev_ioctl(struct file *file,
                 if (err)
                         return err;

-               /* Return the fetched timestamp */
+               /* Return the fetched timestamp and save last fetched event  */
                 spin_lock_irq(&pps->lock);

+               pps->last_fetched_ev = pps->last_ev;
+
                 fdata.info.assert_sequence = pps->assert_sequence;
                 fdata.info.clear_sequence = pps->clear_sequence;
                 fdata.info.assert_tu = pps->assert_tu;
@@ -272,9 +283,11 @@ static long pps_cdev_compat_ioctl(struct file *file,
                 if (err)
                         return err;

-               /* Return the fetched timestamp */
+               /* Return the fetched timestamp and save last fetched event  */
                 spin_lock_irq(&pps->lock);

+               pps->last_fetched_ev = pps->last_ev;
+
                 compat.info.assert_sequence = pps->assert_sequence;
                 compat.info.clear_sequence = pps->clear_sequence;
                 compat.info.current_mode = pps->current_mode;
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index c7abce28ed29..aab0aebb529e 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -52,6 +52,7 @@ struct pps_device {
         int current_mode;                       /* PPS mode at event time */

         unsigned int last_ev;                   /* last PPS event id */
+       unsigned int last_fetched_ev;           /* last fetched PPS event id */
         wait_queue_head_t queue;                /* PPS event queue */

         unsigned int id;                        /* PPS source unique ID */

> If you want to avoid this extra alloc and derefers, we may use file position (file.f_pos) to store last fetched ev value.
> The pps does not provide read/write, so f_pos is unused anyway.
> 
> I am a little bit puzzeled about your second diff.
> Every pps client that uses pps_event() supports WAITEVENT, because this is in the generic part.
> To me not using pps_event() but reimplement parts of pps_event() seems to be a bad idea.
> That’s why I thing that this diff is not needed.

All clients specify their PPS information within the struct pps_source_info, and 
if for some reason one source doesn't add the PPS_CANWAIT flag, we should 
properly support this setting.

However, this is related to the poll() support, and we can defer it for the moment.

Thank you so much for your suggestions and tests! :)

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

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

[Denis OSTERLAND-HEIM]

> Hi,
>
> -----Original Message-----
> From: Rodolfo Giometti <giometti@enneenne.com>
> Sent: Monday, February 24, 2025 6:39 PM
> To: Denis OSTERLAND-HEIM <denis.osterland@diehl.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] pps: add epoll support
>
> > The idea is to use poll to wait for the next data become available.
> > The should poll work like `wait_event_interruptible(pps->queue, ev != pps->last_ev)`,
> > where `poll_wait(file, &pps->queue, wait)` already does the first part,
> > but the condition `ev != pps->last_ev` is missing.
> >
> > Poll shall return 0 if ev == ps->last_ev
> > and shall return EPOLLIN if ev != ps->last_ev; EPOLLRDNORM is equivalent[^1] so better return both
> >
> > In case of ioctl(PPS_FETCH) ev is stored on the stack.
> > If two applications access one pps device, they both get the right result, because the wait_event uses the ev from their stack.
> > To do so with poll() ev needs to be available via file, because every applications opens a sepate file and the file is passed to poll.
> > That is what my patch does.
> > It adds a per file storage so that poll has the ev value of the last fetch to compare.
>
> I agree, but are you sure that your solution will save you in case of fork()?
Well, I have not tested it.
I would expect that regular files behave the same and a read in the forked process influences the f_pos in the origin process as well.
So this would be not supprising that the fork may "steals" the poll condition.

> So, why don't simply add a new variable as below?
If more than one process opens the same pps, they refer all to the same pps_device structure.
They have all their own file struct, but that refer to the same pps_device.
I guess this leads to the situation that only one process sees the `last_fetched_ev != last_ev` condition, but I will test.

I will give your patch a try and report.

>
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 6a02245ea35f..dded1452c3a8 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -40,8 +40,11 @@ static __poll_t pps_cdev_poll(struct file *file, poll_table
> *wait)
>          struct pps_device *pps = file->private_data;
>
>          poll_wait(file, &pps->queue, wait);
> +       if (pps->info.mode & PPS_CANWAIT)
> +               if (pps->fetched_ev != pps->last_ev)
> +                       return EPOLLIN | EPOLLRDNORM;
maybe leave poll direct with error condition, to signal that this is not support instead of normal timeout behavior

+if ((pps->info.mode & PPS_CANWAIT) == 0)
+return EPOLLERR;
+if (pps->fetched_ev != pps->last_ev)
+return EPOLLIN | EPOLLRDNORM;


>
> -       return EPOLLIN | EPOLLRDNORM;
> +       return 0;
>   }
>
>   static int pps_cdev_fasync(int fd, struct file *file, int on)
> @@ -186,9 +195,11 @@ static long pps_cdev_ioctl(struct file *file,
>                  if (err)
>                          return err;
>
> -               /* Return the fetched timestamp */
> +               /* Return the fetched timestamp and save last fetched event  */
>                  spin_lock_irq(&pps->lock);
>
> +               pps->last_fetched_ev = pps->last_ev;
> +
>                  fdata.info.assert_sequence = pps->assert_sequence;
>                  fdata.info.clear_sequence = pps->clear_sequence;
>                  fdata.info.assert_tu = pps->assert_tu;
> @@ -272,9 +283,11 @@ static long pps_cdev_compat_ioctl(struct file *file,
>                  if (err)
>                          return err;
>
> -               /* Return the fetched timestamp */
> +               /* Return the fetched timestamp and save last fetched event  */
>                  spin_lock_irq(&pps->lock);
>
> +               pps->last_fetched_ev = pps->last_ev;
> +
>                  compat.info.assert_sequence = pps->assert_sequence;
>                  compat.info.clear_sequence = pps->clear_sequence;
>                  compat.info.current_mode = pps->current_mode;
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index c7abce28ed29..aab0aebb529e 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -52,6 +52,7 @@ struct pps_device {
>          int current_mode;                       /* PPS mode at event time */
>
>          unsigned int last_ev;                   /* last PPS event id */
> +       unsigned int last_fetched_ev;           /* last fetched PPS event id */
>          wait_queue_head_t queue;                /* PPS event queue */
>
>          unsigned int id;                        /* PPS source unique ID */
>
> > If you want to avoid this extra alloc and derefers, we may use file position (file.f_pos) to store last fetched ev value.
> > The pps does not provide read/write, so f_pos is unused anyway.
> >
> > I am a little bit puzzeled about your second diff.
> > Every pps client that uses pps_event() supports WAITEVENT, because this is in the generic part.
> > To me not using pps_event() but reimplement parts of pps_event() seems to be a bad idea.
> > That’s why I thing that this diff is not needed.
>
> All clients specify their PPS information within the struct pps_source_info, and
> if for some reason one source doesn't add the PPS_CANWAIT flag, we should
> properly support this setting.
>
> However, this is related to the poll() support, and we can defer it for the moment.
I see. Maybe it is easier to reason about the details when a real need araises.

>
> Thank you so much for your suggestions and tests! :)
You´re welcome.

Regards, Denis
>
> Ciao,
>
> Rodolfo
>
> --
> GNU/Linux Solutions                  e-mail: giometti@enneenne.com
> Linux Device Driver                          giometti@linux.it
> Embedded Systems                     phone:  +39 349 2432127
> UNIX programming
>
Diehl Metering GmbH, Donaustrasse 120, 90451 Nuernberg
Sitz der Gesellschaft: Ansbach, Registergericht: Ansbach HRB 69
Geschaeftsfuehrer: Dr. Christof Bosbach (Sprecher), Dipl.-Dolm. Annette Geuther, Dipl.-Kfm. Reiner Edel, Jean-Claude Luttringer

Bitte denken Sie an die Umwelt, bevor Sie diese E-Mail drucken. Diese E-Mail kann vertrauliche Informationen enthalten. Sollten die in dieser E-Mail enthaltenen Informationen nicht für Sie bestimmt sein, informieren Sie bitte unverzueglich den Absender per E-Mail und loeschen Sie diese E-Mail in Ihrem System. Jede unberechtigte Form der Reproduktion, Bekanntgabe, Aenderung, Verteilung und/oder Publikation dieser E-Mail ist strengstens untersagt. Informationen zum Datenschutz finden Sie auf unserer Homepage<https://www.diehl.com/metering/de/impressum-und-rechtliche-hinweise/>.

Before printing, think about environmental responsibility.This message may contain confidential information. If you are not authorized to receive this information please advise the sender immediately by reply e-mail and delete this message without making any copies. Any form of unauthorized use, publication, reproduction, copying or disclosure of the e-mail is not permitted. Information about data protection can be found on our homepage<https://www.diehl.com/metering/en/data-protection/>.

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

[Denis OSTERLAND-HEIM]

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

Hi,

I fixed-up s/pps->fetched_ev/pps->last_fetched_ev/ and tested it.

With one process it works well.

```
# cat /sys/class/pps/pps1/assert; PpsPollTest /dev/pps1; cat /sys/class/pps/pps1/assert
1520599383.268749168#29
assert: 29
time: 1520599383.268749168
assert: 30
time: 1520599384.292728352
assert: 31
time: 1520599385.316788416
1520599385.316788416#31
```

If I start multiple user space programs data races are visible.

```
# for i in 0 1 2 3 4 5 6; do PpsPollTest /dev/pps1 > log$i & done
# sleep 6
# tail log*
==> log0 <==
timeout
assert: 196
time: 1520599554.276752928
assert: 197
time: 1520599555.300692704

==> log1 <==
assert: 193
time: 1520599551.204723248
timeout
assert: 196
time: 1520599554.276752928

==> log2 <==
assert: 195
time: 1520599553.252784688
timeout
assert: 198
time: 1520599556.324909152

==> log3 <==
assert: 195
time: 1520599553.252784688
timeout
assert: 198
time: 1520599556.324909152

==> log4 <==
assert: 194
time: 1520599552.228723584
assert: 195
time: 1520599553.252784688
timeout

==> log5 <==
assert: 194
time: 1520599552.228723584
assert: 195
time: 1520599553.252784688
timeout

==> log6 <==
assert: 194
time: 1520599552.228723584
assert: 195
time: 1520599553.252784688
```

From my point of view it would be great to fix this bug without such an limitation.

Regards, Denis

-----Original Message-----
From: Denis OSTERLAND-HEIM 
Sent: Tuesday, February 25, 2025 1:47 PM
To: 'Rodolfo Giometti' <giometti@enneenne.com>
Cc: linux-kernel@vger.kernel.org
Subject: RE: Re: [PATCH] pps: add epoll support

> Hi,
> 
> -----Original Message-----
> From: Rodolfo Giometti <giometti@enneenne.com> 
> Sent: Monday, February 24, 2025 6:39 PM
> To: Denis OSTERLAND-HEIM <denis.osterland@diehl.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] pps: add epoll support
> 
> > The idea is to use poll to wait for the next data become available.
> > The should poll work like `wait_event_interruptible(pps->queue, ev != pps->last_ev)`,
> > where `poll_wait(file, &pps->queue, wait)` already does the first part,
> > but the condition `ev != pps->last_ev` is missing.
> > 
> > Poll shall return 0 if ev == ps->last_ev
> > and shall return EPOLLIN if ev != ps->last_ev; EPOLLRDNORM is equivalent[^1] so better return both
> > 
> > In case of ioctl(PPS_FETCH) ev is stored on the stack.
> > If two applications access one pps device, they both get the right result, because the wait_event uses the ev from their stack.
> > To do so with poll() ev needs to be available via file, because every applications opens a sepate file and the file is passed to poll.
> > That is what my patch does.
> > It adds a per file storage so that poll has the ev value of the last fetch to compare.
> 
> I agree, but are you sure that your solution will save you in case of fork()? 
Well, I have not tested it.
I would expect that regular files behave the same and a read in the forked process influences the f_pos in the origin process as well.
So this would be not supprising that the fork may "steals" the poll condition.

> So, why don't simply add a new variable as below?
If more than one process opens the same pps, they refer all to the same pps_device structure.
They have all their own file struct, but that refer to the same pps_device.
I guess this leads to the situation that only one process sees the `last_fetched_ev != last_ev` condition, but I will test.

I will give your patch a try and report.

> 
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 6a02245ea35f..dded1452c3a8 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -40,8 +40,11 @@ static __poll_t pps_cdev_poll(struct file *file, poll_table 
> *wait)
>          struct pps_device *pps = file->private_data;
> 
>          poll_wait(file, &pps->queue, wait);
> +       if (pps->info.mode & PPS_CANWAIT)
> +               if (pps->fetched_ev != pps->last_ev)
> +                       return EPOLLIN | EPOLLRDNORM;
maybe leave poll direct with error condition, to signal that this is not support instead of normal timeout behavior

+	if ((pps->info.mode & PPS_CANWAIT) == 0)
+		return EPOLLERR;
+	if (pps->fetched_ev != pps->last_ev)
+		return EPOLLIN | EPOLLRDNORM;


> 
> -       return EPOLLIN | EPOLLRDNORM;
> +       return 0;
>   }
> 
>   static int pps_cdev_fasync(int fd, struct file *file, int on)
> @@ -186,9 +195,11 @@ static long pps_cdev_ioctl(struct file *file,
>                  if (err)
>                          return err;
> 
> -               /* Return the fetched timestamp */
> +               /* Return the fetched timestamp and save last fetched event  */
>                  spin_lock_irq(&pps->lock);
> 
> +               pps->last_fetched_ev = pps->last_ev;
> +
>                  fdata.info.assert_sequence = pps->assert_sequence;
>                  fdata.info.clear_sequence = pps->clear_sequence;
>                  fdata.info.assert_tu = pps->assert_tu;
> @@ -272,9 +283,11 @@ static long pps_cdev_compat_ioctl(struct file *file,
>                  if (err)
>                          return err;
> 
> -               /* Return the fetched timestamp */
> +               /* Return the fetched timestamp and save last fetched event  */
>                  spin_lock_irq(&pps->lock);
> 
> +               pps->last_fetched_ev = pps->last_ev;
> +
>                  compat.info.assert_sequence = pps->assert_sequence;
>                  compat.info.clear_sequence = pps->clear_sequence;
>                  compat.info.current_mode = pps->current_mode;
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index c7abce28ed29..aab0aebb529e 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -52,6 +52,7 @@ struct pps_device {
>          int current_mode;                       /* PPS mode at event time */
> 
>          unsigned int last_ev;                   /* last PPS event id */
> +       unsigned int last_fetched_ev;           /* last fetched PPS event id */
>          wait_queue_head_t queue;                /* PPS event queue */
> 
>          unsigned int id;                        /* PPS source unique ID */
> 
> > If you want to avoid this extra alloc and derefers, we may use file position (file.f_pos) to store last fetched ev value.
> > The pps does not provide read/write, so f_pos is unused anyway.
> > 
> > I am a little bit puzzeled about your second diff.
> > Every pps client that uses pps_event() supports WAITEVENT, because this is in the generic part.
> > To me not using pps_event() but reimplement parts of pps_event() seems to be a bad idea.
> > That’s why I thing that this diff is not needed.
> 
> All clients specify their PPS information within the struct pps_source_info, and 
> if for some reason one source doesn't add the PPS_CANWAIT flag, we should 
> properly support this setting.
> 
> However, this is related to the poll() support, and we can defer it for the moment.
I see. Maybe it is easier to reason about the details when a real need araises.

> 
> Thank you so much for your suggestions and tests! :)
You´re welcome.

Regards, Denis
> 
> 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 --]

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

[Denis OSTERLAND-HEIM]

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

Hi,

> -----Original Message-----
> From: Rodolfo Giometti <giometti@enneenne.com> 
> Sent: Tuesday, February 25, 2025 3:24 PM
> To: Denis OSTERLAND-HEIM <denis.osterland@diehl.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] pps: add epoll support
>  
[snip]
> 
> > If I start multiple user space programs data races are visible.
> > 
> > ```
> > # for i in 0 1 2 3 4 5 6; do PpsPollTest /dev/pps1 > log$i & done
> > # sleep 6
> > # tail log*
> > ==> log0 <==
> > timeout
> > assert: 196
> > time: 1520599554.276752928
> > assert: 197
> > time: 1520599555.300692704
> 
> This is the same behavior we have when working with a serial port: if more than 
> one process gets access to it, data is stolen.
Okay, then lets choose your suggestion.

> 
> >>From my point of view it would be great to fix this bug without such an limitation.
> 
> I disagree, it is not a limitation! It is like a normal char device work.
> 
> What we have to test now is if your initial goal has been addressed! That is, in 
> an application that has more to do than just dealing with one PPS device, we can 
> use poll()/select() in order to avoid using threads.
I will do the final test.
I expect that it works with your patch.

Shall I prepare the patch?
Shall I add you as Co-author?
Or do you want to send your patch with me in reported-by and tested-by?

Fixes: eae9d2ba0cfc ("LinuxPPS: core support")?

CC stable?

Regards, Denis

> 
> 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 --]