* RE: [EXT] Re: [PATCH] pps: add epoll support
@ 2025-02-21 10:49 Denis OSTERLAND-HEIM
2025-02-21 11:39 ` Rodolfo Giometti
0 siblings, 1 reply; 3+ messages in thread
From: Denis OSTERLAND-HEIM @ 2025-02-21 10:49 UTC (permalink / raw)
To: Rodolfo Giometti; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 9404 bytes --]
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.
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.
```c
#include <stdio.h>
#include <string.h>
#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.
With my patch, this looks like:
```
$ cat /sys/class/pps/pps0/assert; ./test /dev/pps0
1520599016.682113965#122
assert: 123
time: 1520599017.682133871
assert: 124
time: 1520599018.682155815
assert: 125
time: 1520599019.682161204
```
I am currently rebasing and testing it with recent changes to pps.c for v2.
Regards, Denis
-----Original Message-----
From: Rodolfo Giometti <giometti@enneenne.com>
Sent: Friday, February 21, 2025 10:18 AM
To: Denis OSTERLAND-HEIM <denis.osterland@diehl.com>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] pps: add epoll support
On 20/02/25 17:45, Denis OSTERLAND-HEIM wrote:
> 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.
Why are you saying that? If you use blocking I/O with a timeout in the poll() it
should work.
> 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) {
fdata is not initialized here... is it set to all zero?
> 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*/)) {
Here you are using poll()...
> 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) {
Again, fdata is not initialized here...
> 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;
> }
> ```
I think you should try current LinuxPPS implementation but with proper fdata
initialization.
>> 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.
Yes, but the char device used to implement the PPS API should work with
select()/poll()!
> 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 agree.
> 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?
As far as I know, epoll() uses the kernel select/poll mechanism and this support
should work correctly at the moment. If no, we have to fix it.
Try your code with the current LinuxPPS implementation replacing the ioctl(fd,
PPS_FETCH &fdata) with:
time_pps_fetch(instances[i].fd, PPS_TSFMT_TSPEC, &info, NULL);
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 --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [EXT] Re: [PATCH] pps: add epoll support
2025-02-21 10:49 [EXT] Re: [PATCH] pps: add epoll support Denis OSTERLAND-HEIM
@ 2025-02-21 11:39 ` Rodolfo Giometti
2025-02-21 11:54 ` Denis OSTERLAND-HEIM
0 siblings, 1 reply; 3+ messages in thread
From: Rodolfo Giometti @ 2025-02-21 11:39 UTC (permalink / raw)
To: Denis OSTERLAND-HEIM; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: Re: [PATCH] pps: add epoll support
2025-02-21 11:39 ` Rodolfo Giometti
@ 2025-02-21 11:54 ` Denis OSTERLAND-HEIM
0 siblings, 0 replies; 3+ messages in thread
From: Denis OSTERLAND-HEIM @ 2025-02-21 11:54 UTC (permalink / raw)
To: Rodolfo Giometti; +Cc: linux-kernel
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/>.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-02-21 11:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-21 10:49 [EXT] Re: [PATCH] pps: add epoll support Denis OSTERLAND-HEIM
2025-02-21 11:39 ` Rodolfo Giometti
2025-02-21 11:54 ` Denis OSTERLAND-HEIM
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®