drivers/pps/pps.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)
Gentle ping
-----Original Message-----
From: Denis OSTERLAND-HEIM
Sent: Monday, January 20, 2025 2:11 PM
To: 'Rodolfo Giometti' <giometti@enneenne.com>
Cc: linux-kernel@vger.kernel.org
Subject: [PATCH] pps: add epoll support
This patch adds pps_context to store the per file read counter.
Signed-off-by: Denis Osterland-Heim <denis.osterland@diehl.com>
---
drivers/pps/pps.c | 33 ++++++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 25d47907db17..b5834c592e2a 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -21,6 +21,12 @@
#include "kc.h"
+struct pps_context {
+ struct pps_device *pps;
+ unsigned int ev;
+};
+
+
/*
* Local variables
*/
@@ -37,17 +43,19 @@ static DEFINE_IDR(pps_idr);
static __poll_t pps_cdev_poll(struct file *file, poll_table *wait)
{
- struct pps_device *pps = file->private_data;
+ struct pps_context *ctx = file->private_data;
+ struct pps_device *pps = ctx->pps;
poll_wait(file, &pps->queue, wait);
- return EPOLLIN | EPOLLRDNORM;
+ return (ctx->ev != pps->last_ev) ? (EPOLLIN | EPOLLRDNORM) : 0;
}
static int pps_cdev_fasync(int fd, struct file *file, int on)
{
- struct pps_device *pps = file->private_data;
- return fasync_helper(fd, file, on, &pps->async_queue);
+ struct pps_context *ctx = file->private_data;
+
+ return fasync_helper(fd, file, on, &ctx->pps->async_queue);
}
static int pps_cdev_pps_fetch(struct pps_device *pps, struct pps_fdata *fdata)
@@ -90,7 +98,8 @@ static int pps_cdev_pps_fetch(struct pps_device *pps, struct pps_fdata *fdata)
static long pps_cdev_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
{
- struct pps_device *pps = file->private_data;
+ struct pps_context *ctx = file->private_data;
+ struct pps_device *pps = ctx->pps;
struct pps_kparams params;
void __user *uarg = (void __user *) arg;
int __user *iuarg = (int __user *) arg;
@@ -189,6 +198,7 @@ static long pps_cdev_ioctl(struct file *file,
/* Return the fetched timestamp */
spin_lock_irq(&pps->lock);
+ ctx->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;
@@ -249,7 +259,8 @@ static long pps_cdev_ioctl(struct file *file,
static long pps_cdev_compat_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
{
- struct pps_device *pps = file->private_data;
+ struct pps_context *ctx = file->private_data;
+ struct pps_device *pps = ctx->pps;
void __user *uarg = (void __user *) arg;
cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
@@ -275,6 +286,7 @@ static long pps_cdev_compat_ioctl(struct file *file,
/* Return the fetched timestamp */
spin_lock_irq(&pps->lock);
+ ctx->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;
@@ -300,7 +312,13 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
{
struct pps_device *pps = container_of(inode->i_cdev,
struct pps_device, cdev);
- file->private_data = pps;
+ struct pps_context *ctx = kzalloc(sizeof(struct pps_context), GFP_KERNEL);
+
+ if (unlikely(ZERO_OR_NULL_PTR(ctx)))
+ return -ENOMEM;
+ file->private_data = ctx;
+ ctx->pps = pps;
+ ctx->ev = pps->last_ev;
kobject_get(&pps->dev->kobj);
return 0;
}
@@ -309,6 +327,7 @@ static int pps_cdev_release(struct inode *inode, struct file *file)
{
struct pps_device *pps = container_of(inode->i_cdev,
struct pps_device, cdev);
+ kfree(file->private_data);
kobject_put(&pps->dev->kobj);
return 0;
}
--
2.45.2
On 19/02/25 13:21, Denis OSTERLAND-HEIM wrote:
> Gentle ping
>
> -----Original Message-----
> From: Denis OSTERLAND-HEIM
> Sent: Monday, January 20, 2025 2:11 PM
> To: 'Rodolfo Giometti' <giometti@enneenne.com>
> Cc: linux-kernel@vger.kernel.org
> Subject: [PATCH] pps: add epoll support
>
> This patch adds pps_context to store the per file read counter.
Can you explain it a bit better?
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?
Also, if we decide to add this support, we should also consider adding the
PPS_CANPOLL mode bit definition (see
https://www.rfc-editor.org/rfc/rfc2783.html#section-3.3), and proving proper
code in order to show how we can use or not this new functionality.
> Signed-off-by: Denis Osterland-Heim <denis.osterland@diehl.com>
> ---
> drivers/pps/pps.c | 33 ++++++++++++++++++++++++++-------
> 1 file changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 25d47907db17..b5834c592e2a 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -21,6 +21,12 @@
>
> #include "kc.h"
>
> +struct pps_context {
> + struct pps_device *pps;
> + unsigned int ev;
> +};
> +
> +
> /*
> * Local variables
> */
> @@ -37,17 +43,19 @@ static DEFINE_IDR(pps_idr);
>
> static __poll_t pps_cdev_poll(struct file *file, poll_table *wait)
> {
> - struct pps_device *pps = file->private_data;
> + struct pps_context *ctx = file->private_data;
> + struct pps_device *pps = ctx->pps;
>
> poll_wait(file, &pps->queue, wait);
>
> - return EPOLLIN | EPOLLRDNORM;
> + return (ctx->ev != pps->last_ev) ? (EPOLLIN | EPOLLRDNORM) : 0;
> }
>
> static int pps_cdev_fasync(int fd, struct file *file, int on)
> {
> - struct pps_device *pps = file->private_data;
> - return fasync_helper(fd, file, on, &pps->async_queue);
> + struct pps_context *ctx = file->private_data;
> +
> + return fasync_helper(fd, file, on, &ctx->pps->async_queue);
> }
>
> static int pps_cdev_pps_fetch(struct pps_device *pps, struct pps_fdata *fdata)
> @@ -90,7 +98,8 @@ static int pps_cdev_pps_fetch(struct pps_device *pps, struct pps_fdata *fdata)
> static long pps_cdev_ioctl(struct file *file,
> unsigned int cmd, unsigned long arg)
> {
> - struct pps_device *pps = file->private_data;
> + struct pps_context *ctx = file->private_data;
> + struct pps_device *pps = ctx->pps;
> struct pps_kparams params;
> void __user *uarg = (void __user *) arg;
> int __user *iuarg = (int __user *) arg;
> @@ -189,6 +198,7 @@ static long pps_cdev_ioctl(struct file *file,
> /* Return the fetched timestamp */
> spin_lock_irq(&pps->lock);
>
> + ctx->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;
> @@ -249,7 +259,8 @@ static long pps_cdev_ioctl(struct file *file,
> static long pps_cdev_compat_ioctl(struct file *file,
> unsigned int cmd, unsigned long arg)
> {
> - struct pps_device *pps = file->private_data;
> + struct pps_context *ctx = file->private_data;
> + struct pps_device *pps = ctx->pps;
> void __user *uarg = (void __user *) arg;
>
> cmd = _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(void *));
> @@ -275,6 +286,7 @@ static long pps_cdev_compat_ioctl(struct file *file,
> /* Return the fetched timestamp */
> spin_lock_irq(&pps->lock);
>
> + ctx->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;
> @@ -300,7 +312,13 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
> {
> struct pps_device *pps = container_of(inode->i_cdev,
> struct pps_device, cdev);
> - file->private_data = pps;
> + struct pps_context *ctx = kzalloc(sizeof(struct pps_context), GFP_KERNEL);
> +
> + if (unlikely(ZERO_OR_NULL_PTR(ctx)))
> + return -ENOMEM;
> + file->private_data = ctx;
> + ctx->pps = pps;
> + ctx->ev = pps->last_ev;
Doing so, we always miss the first event... maybe can we drop this setting and
leaving ctx->ev set to zero?
> kobject_get(&pps->dev->kobj);
> return 0;
> }
> @@ -309,6 +327,7 @@ static int pps_cdev_release(struct inode *inode, struct file *file)
> {
> struct pps_device *pps = container_of(inode->i_cdev,
> struct pps_device, cdev);
> + kfree(file->private_data);
> kobject_put(&pps->dev->kobj);
> return 0;
> }
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,
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.
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) {
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*/)) {
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) {
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;
}
```
> 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.
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 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?
> Also, if we decide to add this support, we should also consider adding the
> PPS_CANPOLL mode bit definition (see
> https://www.rfc-editor.org/rfc/rfc2783.html#section-3.3), and proving proper
> code in order to show how we can use or not this new functionality.
> > @@ -300,7 +312,13 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
> > {
> > struct pps_device *pps = container_of(inode->i_cdev,
> > struct pps_device, cdev);
> > -file->private_data = pps;
> > +struct pps_context *ctx = kzalloc(sizeof(struct pps_context), GFP_KERNEL);
> > +
> > +if (unlikely(ZERO_OR_NULL_PTR(ctx)))
> > +return -ENOMEM;
> > +file->private_data = ctx;
> > +ctx->pps = pps;
> > +ctx->ev = pps->last_ev;
>
> Doing so, we always miss the first event... maybe can we drop this setting and
> leaving ctx->ev set to zero?
Well, I see two use cases:
1. open(), ioctl() to get current value, epoll() to wait for next event, ioctl() to get the next value
2. open(), epoll() wait for next event or time-out to see if pps is "alive"
I do not see the case that we completely miss one.
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/>.
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
© 2016 - 2025 Red Hat, Inc.