Re: [PATCH] pps: add epoll support

[Rodolfo Giometti]

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@xxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxx>
> ---
>   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@xxxxxxxxxxxx
Linux Device Driver                          giometti@xxxxxxxx
Embedded Systems                     phone:  +39 349 2432127
UNIX programming