Message-ID: <af668744b0e54ce2b324900c07fdef16@diehl.com>
Date: Tue, 25 Feb 2025 12:47:11 +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,
>
> -----Original Message-----
> From: Rodolfo Giometti <giometti@...eenne.com>
> Sent: Monday, February 24, 2025 6:39 PM
> To: Denis OSTERLAND-HEIM <denis.osterland@...hl.com>
> Cc: linux-kernel@...r.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@...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