[PATCH] pps: Add support for read operations and return a useful value in poll

[PATCH] pps: Add support for read operations and return a useful value in poll

[Christian Riesch]

The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event
occurs, then returns the time stamp data. While this is fine for
lots of applications, sometimes it would be nice if the poll system
call and a subsequent read could be used to obtain the pps data.

This patch adds support for read operations. Furthermore pps_cdev_poll
is modified to return POLLIN | POLLRDNORM only when new PPS data is
available.

Signed-off-by: Christian Riesch <christian.riesch@omicron.at>
---
 drivers/pps/pps.c          |   32 +++++++++++++++++++++++++++++++-
 include/linux/pps_kernel.h |    1 +
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 2f07cd6..f90dd2e 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -55,7 +55,7 @@ static unsigned int pps_cdev_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &pps->queue, wait);
 
-	return POLLIN | POLLRDNORM;
+	return (pps->last_ev != pps->read_ev) ? POLLIN | POLLRDNORM : 0;
 }
 
 static int pps_cdev_fasync(int fd, struct file *file, int on)
@@ -191,6 +191,7 @@ static long pps_cdev_ioctl(struct file *file,
 		fdata.info.assert_tu = pps->assert_tu;
 		fdata.info.clear_tu = pps->clear_tu;
 		fdata.info.current_mode = pps->current_mode;
+		pps->read_ev = pps->last_ev;
 
 		spin_unlock_irq(&pps->lock);
 
@@ -251,6 +252,34 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+ssize_t pps_cdev_read(struct file *file, char __user *buf, size_t count,
+		      loff_t *ppos)
+{
+	struct pps_device *pps = file->private_data;
+	struct pps_fdata fdata;
+
+	if (count != sizeof(struct pps_fdata))
+		return -EINVAL;
+
+	if (wait_event_interruptible(pps->queue, pps->read_ev != pps->last_ev))
+		return -ERESTARTSYS;
+
+	/* Return the fetched timestamp */
+	spin_lock_irq(&pps->lock);
+	fdata.info.assert_sequence = pps->assert_sequence;
+	fdata.info.clear_sequence = pps->clear_sequence;
+	fdata.info.assert_tu = pps->assert_tu;
+	fdata.info.clear_tu = pps->clear_tu;
+	fdata.info.current_mode = pps->current_mode;
+	pps->read_ev = pps->last_ev;
+	spin_unlock_irq(&pps->lock);
+
+	if (copy_to_user(buf, &fdata, sizeof(struct pps_fdata)))
+		return -EFAULT;
+
+	return sizeof(struct pps_fdata);
+}
+
 static int pps_cdev_release(struct inode *inode, struct file *file)
 {
 	struct pps_device *pps = container_of(inode->i_cdev,
@@ -266,6 +295,7 @@ static int pps_cdev_release(struct inode *inode, struct file *file)
 static const struct file_operations pps_cdev_fops = {
 	.owner		= THIS_MODULE,
 	.llseek		= no_llseek,
+	.read		= pps_cdev_read,
 	.poll		= pps_cdev_poll,
 	.fasync		= pps_cdev_fasync,
 	.unlocked_ioctl	= pps_cdev_ioctl,
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 1d2cd21..42f405b 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -66,6 +66,7 @@ struct pps_device {
 	int current_mode;			/* PPS mode at event time */
 
 	unsigned int last_ev;			/* last PPS event id */
+	unsigned int read_ev;			/* last PPS event read by user */
 	wait_queue_head_t queue;		/* PPS event queue */
 
 	unsigned int id;			/* PPS source unique ID */
-- 
1.7.9.5

Re: [PATCH] pps: Add support for read operations and return a useful value in poll

[Rodolfo Giometti]

On Tue, Mar 31, 2015 at 11:31:22PM +0200, Christian Riesch wrote:
> The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event
> occurs, then returns the time stamp data. While this is fine for
> lots of applications, sometimes it would be nice if the poll system
> call and a subsequent read could be used to obtain the pps data.

Nak. The read syscall can't be forced to return fix amount of
data.

Use a dedicated ioctl instead.

Ciao,

Rodolfo

-- 

HCE Engineering                      e-mail: giometti@hce-engineering.com
GNU/Linux Solutions                          giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.io
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

Re: [PATCH] pps: Add support for read operations and return a useful value in poll

[Christian Riesch]

Hi Rodolfo,

Thanks for your reply!

On Wed, Apr 1, 2015 at 10:11 AM, Rodolfo Giometti <giometti@enneenne.com> wrote:
> On Tue, Mar 31, 2015 at 11:31:22PM +0200, Christian Riesch wrote:
>> The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event
>> occurs, then returns the time stamp data. While this is fine for
>> lots of applications, sometimes it would be nice if the poll system
>> call and a subsequent read could be used to obtain the pps data.
>
> Nak. The read syscall can't be forced to return fix amount of
> data.

I just copied/modified the behavior of ptp_read() in drivers/ptp/ptp_chardev.c:

if (cnt % sizeof(struct ptp_extts_event) != 0)
        return -EINVAL;

There the amount of data is forced to be a multiple of sizeof(struct
ptp_extts_event). But if you prefer an ioctl, I can change that of
course.

>
> Use a dedicated ioctl instead.

Is it ok to pair POLLIN | POLLRDNORM in the poll function with an
ioctl? Or should returning POLLIN | POLLRDNORM mean that a read() will
not block? Should I use POLLPRI instead or something else?

Regards, Christian

Re: [PATCH] pps: Add support for read operations and return a useful value in poll

[Christian Riesch]

[sent again with Rodolfo in the list of recipients, something went
wrong, sorry!]

Hi Rodolfo,

Thanks for your reply!

On Wed, Apr 1, 2015 at 10:11 AM, Rodolfo Giometti <giometti@enneenne.com> wrote:
> On Tue, Mar 31, 2015 at 11:31:22PM +0200, Christian Riesch wrote:
>> The PPS_FETCH ioctl in drivers/pps/pps.c blocks until a new PPS event
>> occurs, then returns the time stamp data. While this is fine for
>> lots of applications, sometimes it would be nice if the poll system
>> call and a subsequent read could be used to obtain the pps data.
>
> Nak. The read syscall can't be forced to return fix amount of
> data.

I just copied/modified the behavior of ptp_read() in drivers/ptp/ptp_chardev.c:

if (cnt % sizeof(struct ptp_extts_event) != 0)
        return -EINVAL;

There the amount of data is forced to be a multiple of sizeof(struct
ptp_extts_event). But if you prefer an ioctl, I can change that of
course.

> Use a dedicated ioctl instead.

Is it ok to pair POLLIN | POLLRDNORM in the poll function with an
ioctl? Or should returning POLLIN | POLLRDNORM mean that a read() will
not block? Should I use POLLPRI instead or something else?

Regards, Christian