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 Sent: Friday, February 21, 2025 12:40 PM To: Denis OSTERLAND-HEIM 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 > #include ///home/giometti/Projects/ailux/imx9/linux/linux-imx > #include > #include > #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