Message-ID: <0653b036d2b44d57914f8cb3e405aa0d@diehl.com>
Date: Mon, 24 Feb 2025 12:38:50 +0100 (CET)
From: Denis OSTERLAND-HEIM <denis.osterland@...hl.com>
To: Rodolfo Giometti <giometti@...eenne.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: Re: [PATCH] pps: add epoll support

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@...eenne.com> 
Sent: Friday, February 21, 2025 12:40 PM
To: Denis OSTERLAND-HEIM <denis.osterland@...hl.com>
Cc: linux-kernel@...r.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@...eenne.com
Linux Device Driver                          giometti@...ux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (6038 bytes)

Powered by blists - more mailing lists