Message-ID: <9d9f2a9a-ab90-4fbf-bc0e-d4c8b83d7082@enneenne.com>
Date: Fri, 21 Feb 2025 12:39:45 +0100
From: Rodolfo Giometti <giometti@...eenne.com>
To: Denis OSTERLAND-HEIM <denis.osterland@...hl.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [EXT] 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

Powered by blists - more mailing lists