RE: Re: [PATCH] pps: add epoll support

[Denis OSTERLAND-HEIM <denis.osterland@diehl.com>]

[-- Attachment #1: Type: text/plain, Size: 7545 bytes --]

Hi,

I fixed-up s/pps->fetched_ev/pps->last_fetched_ev/ and tested it.

With one process it works well.

```
# cat /sys/class/pps/pps1/assert; PpsPollTest /dev/pps1; cat /sys/class/pps/pps1/assert
1520599383.268749168#29
assert: 29
time: 1520599383.268749168
assert: 30
time: 1520599384.292728352
assert: 31
time: 1520599385.316788416
1520599385.316788416#31
```

If I start multiple user space programs data races are visible.

```
# for i in 0 1 2 3 4 5 6; do PpsPollTest /dev/pps1 > log$i & done
# sleep 6
# tail log*
==> log0 <==
timeout
assert: 196
time: 1520599554.276752928
assert: 197
time: 1520599555.300692704

==> log1 <==
assert: 193
time: 1520599551.204723248
timeout
assert: 196
time: 1520599554.276752928

==> log2 <==
assert: 195
time: 1520599553.252784688
timeout
assert: 198
time: 1520599556.324909152

==> log3 <==
assert: 195
time: 1520599553.252784688
timeout
assert: 198
time: 1520599556.324909152

==> log4 <==
assert: 194
time: 1520599552.228723584
assert: 195
time: 1520599553.252784688
timeout

==> log5 <==
assert: 194
time: 1520599552.228723584
assert: 195
time: 1520599553.252784688
timeout

==> log6 <==
assert: 194
time: 1520599552.228723584
assert: 195
time: 1520599553.252784688
```

From my point of view it would be great to fix this bug without such an limitation.

Regards, Denis

-----Original Message-----
From: Denis OSTERLAND-HEIM 
Sent: Tuesday, February 25, 2025 1:47 PM
To: 'Rodolfo Giometti' <giometti@enneenne.com>
Cc: linux-kernel@vger.kernel.org
Subject: RE: Re: [PATCH] pps: add epoll support

> 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
>

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6038 bytes --]