| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9a7ddaae-83ff-440d-8423-58178dfb76cd@enneenne.com>
Date: Fri, 21 Feb 2025 10:17:55 +0100
From: Rodolfo Giometti <giometti@...eenne.com>
To: Denis OSTERLAND-HEIM <denis.osterland@...hl.com>
Cc: "linux-kernel@...r.kernel.org" <linux-kernel@...r.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@...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.
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@...eenne.com
Linux Device Driver giometti@...ux.it
Embedded Systems phone: +39 349 2432127
UNIX programming
Powered by blists - more mailing lists