PPS_ECHOASSERT |
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@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
On 24/02/25 12:38, Denis OSTERLAND-HEIM wrote:
> Hi,
>
> I tested it today and that patch creates not the expected behavior.
[snip]
> 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.
I agree, but are you sure that your solution will save you in case of fork()?
So, why don't simply add a new variable as below?
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 6a02245ea35f..dded1452c3a8 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -40,8 +40,11 @@ static __poll_t pps_cdev_poll(struct file *file, poll_table
*wait)
struct pps_device *pps = file->private_data;
poll_wait(file, &pps->queue, wait);
+ if (pps->info.mode & PPS_CANWAIT)
+ if (pps->fetched_ev != pps->last_ev)
+ return EPOLLIN | EPOLLRDNORM;
- return EPOLLIN | EPOLLRDNORM;
+ return 0;
}
static int pps_cdev_fasync(int fd, struct file *file, int on)
@@ -186,9 +195,11 @@ static long pps_cdev_ioctl(struct file *file,
if (err)
return err;
- /* Return the fetched timestamp */
+ /* Return the fetched timestamp and save last fetched event */
spin_lock_irq(&pps->lock);
+ pps->last_fetched_ev = pps->last_ev;
+
fdata.info.assert_sequence = pps->assert_sequence;
fdata.info.clear_sequence = pps->clear_sequence;
fdata.info.assert_tu = pps->assert_tu;
@@ -272,9 +283,11 @@ static long pps_cdev_compat_ioctl(struct file *file,
if (err)
return err;
- /* Return the fetched timestamp */
+ /* Return the fetched timestamp and save last fetched event */
spin_lock_irq(&pps->lock);
+ pps->last_fetched_ev = pps->last_ev;
+
compat.info.assert_sequence = pps->assert_sequence;
compat.info.clear_sequence = pps->clear_sequence;
compat.info.current_mode = pps->current_mode;
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index c7abce28ed29..aab0aebb529e 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -52,6 +52,7 @@ struct pps_device {
int current_mode; /* PPS mode at event time */
unsigned int last_ev; /* last PPS event id */
+ unsigned int last_fetched_ev; /* last fetched PPS event id */
wait_queue_head_t queue; /* PPS event queue */
unsigned int id; /* PPS source unique ID */
> 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.
All clients specify their PPS information within the struct pps_source_info, and
if for some reason one source doesn't add the PPS_CANWAIT flag, we should
properly support this setting.
However, this is related to the poll() support, and we can defer it for the moment.
Thank you so much for your suggestions and tests! :)
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming
> Hi,
>
> -----Original Message-----
> From: Rodolfo Giometti <giometti@enneenne.com>
> Sent: Monday, February 24, 2025 6:39 PM
> To: Denis OSTERLAND-HEIM <denis.osterland@diehl.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] pps: add epoll support
>
> > 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.
>
> I agree, but are you sure that your solution will save you in case of fork()?
Well, I have not tested it.
I would expect that regular files behave the same and a read in the forked process influences the f_pos in the origin process as well.
So this would be not supprising that the fork may "steals" the poll condition.
> So, why don't simply add a new variable as below?
If more than one process opens the same pps, they refer all to the same pps_device structure.
They have all their own file struct, but that refer to the same pps_device.
I guess this leads to the situation that only one process sees the `last_fetched_ev != last_ev` condition, but I will test.
I will give your patch a try and report.
>
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 6a02245ea35f..dded1452c3a8 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -40,8 +40,11 @@ static __poll_t pps_cdev_poll(struct file *file, poll_table
> *wait)
> struct pps_device *pps = file->private_data;
>
> poll_wait(file, &pps->queue, wait);
> + if (pps->info.mode & PPS_CANWAIT)
> + if (pps->fetched_ev != pps->last_ev)
> + return EPOLLIN | EPOLLRDNORM;
maybe leave poll direct with error condition, to signal that this is not support instead of normal timeout behavior
+if ((pps->info.mode & PPS_CANWAIT) == 0)
+return EPOLLERR;
+if (pps->fetched_ev != pps->last_ev)
+return EPOLLIN | EPOLLRDNORM;
>
> - return EPOLLIN | EPOLLRDNORM;
> + return 0;
> }
>
> static int pps_cdev_fasync(int fd, struct file *file, int on)
> @@ -186,9 +195,11 @@ static long pps_cdev_ioctl(struct file *file,
> if (err)
> return err;
>
> - /* Return the fetched timestamp */
> + /* Return the fetched timestamp and save last fetched event */
> spin_lock_irq(&pps->lock);
>
> + pps->last_fetched_ev = pps->last_ev;
> +
> fdata.info.assert_sequence = pps->assert_sequence;
> fdata.info.clear_sequence = pps->clear_sequence;
> fdata.info.assert_tu = pps->assert_tu;
> @@ -272,9 +283,11 @@ static long pps_cdev_compat_ioctl(struct file *file,
> if (err)
> return err;
>
> - /* Return the fetched timestamp */
> + /* Return the fetched timestamp and save last fetched event */
> spin_lock_irq(&pps->lock);
>
> + pps->last_fetched_ev = pps->last_ev;
> +
> compat.info.assert_sequence = pps->assert_sequence;
> compat.info.clear_sequence = pps->clear_sequence;
> compat.info.current_mode = pps->current_mode;
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index c7abce28ed29..aab0aebb529e 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -52,6 +52,7 @@ struct pps_device {
> int current_mode; /* PPS mode at event time */
>
> unsigned int last_ev; /* last PPS event id */
> + unsigned int last_fetched_ev; /* last fetched PPS event id */
> wait_queue_head_t queue; /* PPS event queue */
>
> unsigned int id; /* PPS source unique ID */
>
> > 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.
>
> All clients specify their PPS information within the struct pps_source_info, and
> if for some reason one source doesn't add the PPS_CANWAIT flag, we should
> properly support this setting.
>
> However, this is related to the poll() support, and we can defer it for the moment.
I see. Maybe it is easier to reason about the details when a real need araises.
>
> Thank you so much for your suggestions and tests! :)
You´re welcome.
Regards, Denis
>
> 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/>.
© 2016 - 2025 Red Hat, Inc.