Message-ID: <456070491e3642e9a6017ff7d3bb982b@diehl.com>
Date: Thu, 20 Feb 2025 16:45:49 +0000
From: Denis OSTERLAND-HEIM <denis.osterland@...hl.com>
To: Rodolfo Giometti <giometti@...eenne.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: Re: [PATCH] pps: add epoll support

Hi,

Thanks for the fast answer.

-----Original Message-----
From: Rodolfo Giometti <giometti@...eenne.com>
Sent: Thursday, February 20, 2025 9:51 AM
To: Denis OSTERLAND-HEIM <denis.osterland@...hl.com>
Cc: linux-kernel@...r.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@...eenne.com
> Linux Device Driver                          giometti@...ux.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/>.

Powered by blists - more mailing lists