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

[Rodolfo Giometti]

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@xxxxxxxxxxxx
Linux Device Driver                          giometti@xxxxxxxx
Embedded Systems                     phone:  +39 349 2432127
UNIX programming