LinuxPPS (RESUBMIT 2): the PPS Linux implementation.

Adds support for the PPS sources connected with the CD (Carrier Detect) pin of a serial port.

...

drivers/serial/serial_core.c +4 -0

+#ifdef CONFIG_PPS_CLIENT_UART
+
+static int
+uart_register_pps_port(struct uart_state *state, struct uart_port *port)

drivers/serial/serial_core.c +20 -0

+	state->pps_info.mode = PPS_CAPTUREBOTH | \
+			PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
+			PPS_CANWAIT | PPS_TSFMT_TSPEC;
+
+	ret = pps_register_source(&state->pps_info, PPS_CAPTUREBOTH | \
+				PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
+	if (ret < 0) {
+		dev_err(port->dev, "cannot register PPS source \"%s\"\n",
+						state->pps_info.path);
+		return ret;
+	}
+	port->pps_source = ret;
+	dev_dbg(port->dev, "PPS source #%d \"%s\" added\n",
+		port->pps_source, state->pps_info.path);
+
+	return 0;
+}
+
+static void
+uart_unregister_pps_port(struct uart_state *state, struct uart_port *port)

drivers/serial/serial_core.c +23 -1

+#endif /* CONFIG_PPS_CLIENT_UART */
+
 static int uart_set_info(struct uart_state *state,
 			 struct serial_struct __user *newinfo)
 {

@@ -810,11 +859,19 @@ static int uart_set_info(struct uart_state *state,
 			(port->flags & UPF_LOW_LATENCY) ? 1 : 0;
 
  check_and_exit:
+	/* PPS support enabled/disabled? */
+	if ((old_flags & UPF_HARDPPS_CD) != (new_flags & UPF_HARDPPS_CD)) {
+		if (new_flags & UPF_HARDPPS_CD)
+			uart_register_pps_port(state, port);
+		else
+			uart_unregister_pps_port(state, port);
+	}
+
 	retval = 0;
 	if (port->type == PORT_UNKNOWN)
 		goto exit;
 	if (state->info->flags & UIF_INITIALIZED) {
-		if (((old_flags ^ port->flags) & UPF_SPD_MASK) ||
+		if (((old_flags ^ port->flags) & (UPF_SPD_MASK|UPF_HARDPPS_CD)) ||
 		    old_custom_divisor != port->custom_divisor) {
 			/*
 			 * If they're setting up a custom divisor or speed,

@@ -2148,6 +2205,12 @@ uart_configure_port(struct uart_driver *drv, struct uart_state *state,
 		port->ops->config_port(port, flags);
 	}
 
+	/*
+	 * Add the PPS support for the current port.
+	 */
+	if (port->flags & UPF_HARDPPS_CD)
+		uart_register_pps_port(state, port);
+
 	if (port->type != PORT_UNKNOWN) {
 		unsigned long flags;
 

@@ -2405,6 +2468,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *port)
 	mutex_unlock(&state->mutex);
 
 	/*
+	 * Remove PPS support from the current port.
+	 */
+	if (port->flags & UPF_HARDPPS_CD)
+		uart_unregister_pps_port(state, port);
+
+	/*
 	 * Remove the devices from the tty layer
 	 */
 	tty_unregister_device(drv->tty_driver, port->line);

Quoted 14 lines · On Thu, 6 Mar 2008 13:09:04 +0100 Rodolfo Giometti <[email protected]> wrote:

Adds support for the PPS sources connected with the CD (Carrier Detect) pin of a serial port.

...

drivers/serial/serial_core.c +9 -0

+#ifdef CONFIG_PPS_CLIENT_UART
+
+static int
+uart_register_pps_port(struct uart_state *state, struct uart_port *port)
+{
+	struct tty_driver *drv = port->info->tty->driver;
+	int ret;
+
+	state->pps_info.owner = THIS_MODULE;

drivers/serial/serial_core.c static int uart_get_info(struct uart_state *state, · line 652

Quoted 5 lines

drivers/serial/serial_core.c +5 -0

+	state->pps_info.dev = port->dev;
+	snprintf(state->pps_info.name, PPS_MAX_NAME_LEN, "%s%d",
+		drv->driver_name, port->line);
+	snprintf(state->pps_info.path, PPS_MAX_NAME_LEN, "/dev/%s%d",
+		drv->name, port->line);

umm, why are we hard-wiring "/dev" into the kernel source? Is it just for user-friendly printks?

Quoted 26 lines

drivers/serial/serial_core.c +26 -0

+	state->pps_info.mode = PPS_CAPTUREBOTH | \
+			PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
+			PPS_CANWAIT | PPS_TSFMT_TSPEC;
+
+	ret = pps_register_source(&state->pps_info, PPS_CAPTUREBOTH | \
+				PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
+	if (ret < 0) {
+		dev_err(port->dev, "cannot register PPS source \"%s\"\n",
+						state->pps_info.path);
+		return ret;
+	}
+	port->pps_source = ret;
+	dev_dbg(port->dev, "PPS source #%d \"%s\" added\n",
+		port->pps_source, state->pps_info.path);
+
+	return 0;
+}
+
+static void
+uart_unregister_pps_port(struct uart_state *state, struct uart_port *port)
+{
+	pps_unregister_source(port->pps_source);
+	dev_dbg(port->dev, "PPS source #%d \"%s\" removed\n",
+				port->pps_source, state->pps_info.path);
+}
+

drivers/serial/serial_core.c static int uart_get_info(struct uart_state *state, · line 683

Quoted 4 lines

drivers/serial/serial_core.c +4 -0

+#else
+
+#define uart_register_pps_port(state, port)	do { } while (0)
+#define uart_unregister_pps_port(state, port)	do { } while (0)

Please never implement in cpp that which can be implemented in C. These should have been static inlines. That's nicer, matches the CONFIG_PPS_CLIENT_UART=y code and provides type checking.

Quoted 8 lines

drivers/serial/serial_core.c static int uart_get_info(struct uart_state *state, · line 683

Quoted 4 lines

drivers/serial/serial_core.c +4 -0

+#else
+
+#define uart_register_pps_port(state, port)	do { } while (0)
+#define uart_unregister_pps_port(state, port)	do { } while (0)

Please never implement in cpp that which can be implemented in C. These should have been static inlines. That's nicer, matches the CONFIG_PPS_CLIENT_UART=y code and provides type checking.

Are you saying that I should do:

static inline int
uart_register_pps_port(struct uart_state *state, struct uart_port *port)

{
	/* Nop */
}

static inline void
uart_unregister_pps_port(struct uart_state *state, struct uart_port *port)

{
	/* Nop */
}

?

Quoted 23 lines · On Fri, 21 Mar 2008 12:17:36 +0100 Rodolfo Giometti <[email protected]>…

Quoted 8 lines

drivers/serial/serial_core.c static int uart_get_info(struct uart_state *state, · line 683

Quoted 4 lines

drivers/serial/serial_core.c +4 -0

+#else
+
+#define uart_register_pps_port(state, port)	do { } while (0)
+#define uart_unregister_pps_port(state, port)	do { } while (0)

Please never implement in cpp that which can be implemented in C. These should have been static inlines. That's nicer, matches the CONFIG_PPS_CLIENT_UART=y code and provides type checking.

Are you saying that I should do:

static inline int
uart_register_pps_port(struct uart_state *state, struct uart_port *port)

{
	/* Nop */
}

static inline void
uart_unregister_pps_port(struct uart_state *state, struct uart_port *port)

{
	/* Nop */
}

Quoted 1 line

?

Yes please.

+pps_core-y := pps.o kapi.o sysfs.o

drivers/pps/kapi.c new file +5 -0

+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/sched.h>
+#include <linux/time.h>

drivers/pps/kapi.c new file +17 -0

+DEFINE_IDR(pps_idr);
 
...
 
+void pps_unregister_source(int source)
+{
+	struct pps_device *pps;
+
+	spin_lock_irq(&idr_lock);
+	pps = idr_find(&pps_idr, source);
+
+	if (!pps) {
+		spin_unlock_irq(&idr_lock);
+		return;
+	}
+
+	/* This should be done first in order to deny IRQ handlers
+	 * to access PPS structs
+	 */
+

+void pps_event(int source, int event, void *data)

drivers/pps/kapi.c new file +18 -0

+{
+	struct pps_device *pps;
+	struct timespec __ts;
+	struct pps_ktime ts;
+	unsigned long flags;
+
+	/* First of all we get the time stamp... */
+	getnstimeofday(&__ts);
+
+	/* ... and translate it to PPS time data struct */
+	ts.sec = __ts.tv_sec;
+	ts.nsec = __ts.tv_nsec;
+
+	if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
+		printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
+			event, source);
+		return;
+	}

drivers/pps/kapi.c new file +55 -0

+	if (!pps)
+		return;
+
+	pr_debug("PPS event on source %d at %llu.%06u\n",
+			pps->id, ts.sec, ts.nsec);
+
+	spin_lock_irqsave(&pps->lock, flags);
+
+	/* Must call the echo function? */
+	if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
+		pps->info.echo(source, event, data);
+
+	/* Check the event */
+	pps->current_mode = pps->params.mode;
+	if (event & PPS_CAPTUREASSERT) {
+		/* We have to add an offset? */
+		if (pps->params.mode & PPS_OFFSETASSERT)
+			pps_add_offset(&ts, &pps->params.assert_off_tu);
+
+		/* Save the time stamp */
+		pps->assert_tu = ts;
+		pps->assert_sequence++;
+		pr_debug("capture assert seq #%u for source %d\n",
+			pps->assert_sequence, source);
+	}
+	if (event & PPS_CAPTURECLEAR) {
+		/* We have to add an offset? */
+		if (pps->params.mode & PPS_OFFSETCLEAR)
+			pps_add_offset(&ts, &pps->params.clear_off_tu);
+
+		/* Save the time stamp */
+		pps->clear_tu = ts;
+		pps->clear_sequence++;
+		pr_debug("capture clear seq #%u for source %d\n",
+			pps->clear_sequence, source);
+	}
+
+	pps->go = ~0;
+	wake_up_interruptible(&pps->queue);
+
+	kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
+
+	spin_unlock_irqrestore(&pps->lock, flags);
+
+	/* Now we can release the PPS source for (possible) deregistration */
+	spin_lock_irqsave(&idr_lock, flags);
+	atomic_dec(&pps->usage);
+	wake_up_all(&pps->usage_queue);
+	spin_unlock_irqrestore(&idr_lock, flags);
+}
+EXPORT_SYMBOL(pps_event);
 
...
 
+static int pps_cdev_open(struct inode *inode, struct file *file)
+{
+	struct pps_device *pps = container_of(inode->i_cdev,
+						struct pps_device, cdev);

include/linux/pps.h new file +27 -0

+	if (!found)
+		return -ENODEV;
+
+	file->private_data = pps;
+
+	return 0;
+}
 
...
 
+/* Kernel consumers */
+#define PPS_KC_HARDPPS		0	/* hardpps() (or equivalent) */
+#define PPS_KC_HARDPPS_PLL	1	/* hardpps() constrained to
+					   use a phase-locked loop */
+#define PPS_KC_HARDPPS_FLL	2	/* hardpps() constrained to
+					   use a frequency-locked loop */
+/*
+ * Here begins the implementation-specific part!
+ */
+
+struct pps_fdata {
+	struct pps_kinfo info;
+	struct pps_ktime timeout;
+};
+
+#include <linux/ioctl.h>
+
+#define PPS_CHECK		_IO('P', 0)
+#define PPS_GETPARAMS		_IOR('P', 1, struct pps_kparams *)
+#define PPS_SETPARAMS		_IOW('P', 2, struct pps_kparams *)

include/linux/pps.h new file +16 -0

+/*
+ * Global defines
+ */
+
+/* The specific PPS source info */
+struct pps_source_info {
+	char name[PPS_MAX_NAME_LEN];		/* simbolic name */
+	char path[PPS_MAX_NAME_LEN];		/* path of connected device */
+	int mode;				/* PPS's allowed mode */
+
+	void (*echo)(int source, int event, void *data); /* PPS echo function */
+
+	struct module *owner;
+	struct device *dev;
+};
+

Quoted 1 line · On Thu, 6 Mar 2008 13:09:00 +0100 Rodolfo Giometti <[email protected]> wrote:

+pps_core-y := pps.o kapi.o sysfs.o

Does it compile OK with CONFIG_SYSFS=n?

Quoted 5 lines

(quoted diff) +5 -0

+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/sched.h>
+#include <linux/time.h>

Review

Quoted 10 lines

(quoted diff) +10 -0

+#include <linux/spinlock.h>
+#include <linux/idr.h>
+#include <linux/fs.h>
+#include <linux/pps.h>
+
+/*
+ * Global variables
+ */
+
+DEFINE_SPINLOCK(idr_lock);

This name is insufficiently specific. Not only does it risk linkage errors, it makes it ahrd for poeple to work out where the symbol came from.

I renamed it to pps_idr_lock.

Quoted 20 lines

(quoted diff) +17 -0

+DEFINE_IDR(pps_idr);
 
...
 
+void pps_unregister_source(int source)
+{
+	struct pps_device *pps;
+
+	spin_lock_irq(&idr_lock);
+	pps = idr_find(&pps_idr, source);
+
+	if (!pps) {
+		spin_unlock_irq(&idr_lock);
+		return;
+	}
+
+	/* This should be done first in order to deny IRQ handlers
+	 * to access PPS structs
+	 */
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	idr_remove(&pps_idr, pps->id);
+	spin_unlock_irq(&idr_lock);
+
+	wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0);
+
+	pps_sysfs_remove_source_entry(pps);
+	pps_unregister_cdev(pps);
+	kfree(pps);
+}
+EXPORT_SYMBOL(pps_unregister_source);

The wait_event() stuff really shouldn't be here: it should be integral to the refcounting:

void pps_dev_put(struct pps_device *pps)

{
	spin_lock_irq(&pps_lock);
	if (atomic_dec_and_test(&pps->usage))
		idr_remove(&pps_idr, pps->id);
	else
		pps = NULL;
	spin_unlock_irq(&pps_lock);
	if (pps) {
		/*

		 * Might need to do the below via schedule_work() if
		 * pps_dev_put() is to be callable from atomic context
		 */
		pps_sysfs_remove_source_entry(pps);
		pps_unregister_cdev(pps);
		kfree(pps);
	}
}

As it stands, there might be deadlocks such as when a process which itself holds a ref on the pps_device (with an open fd?) calls pps_unregister_source.

Also, we need to take care that all processes which were waiting in pps_unregister_source() get to finish their cleanup before we permit rmmod to proceed. Is that handled somewhere?

Quoted 2 lines

+void pps_event(int source, int event, void *data)

Please document the API in the kernel source. I realise there's a teeny bit of documentation in pps.txt, but people don't think to look there and it tends to go out of date.

It doesn't have to be fancy formal kerneldoc - it's better to add *good* comments which tell people what they need to know. For some reason people seem to add useless obvious stuff when they do their comments in kerneldoc form.

Quoted 23 lines

(quoted diff) +23 -0

+{
+	struct pps_device *pps;
+	struct timespec __ts;
+	struct pps_ktime ts;
+	unsigned long flags;
+
+	/* First of all we get the time stamp... */
+	getnstimeofday(&__ts);
+
+	/* ... and translate it to PPS time data struct */
+	ts.sec = __ts.tv_sec;
+	ts.nsec = __ts.tv_nsec;
+
+	if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
+		printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
+			event, source);
+		return;
+	}
+
+	spin_lock_irqsave(&idr_lock, flags);
+	pps = idr_find(&pps_idr, source);
+
+	/* If we find a valid PPS source we lock it before leaving

Review

Quoted 5 lines

(quoted diff) +5 -0

+	 * the lock!
+	 */
+	if (pps)
+		atomic_inc(&pps->usage);
+	spin_unlock_irqrestore(&idr_lock, flags);

The above pattern is repeated rather a lot and could perhaps be extracted into a nice pps_dev_get() helper.

Quoted 63 lines

(quoted diff) +60 -0

+	if (!pps)
+		return;
+
+	pr_debug("PPS event on source %d at %llu.%06u\n",
+			pps->id, ts.sec, ts.nsec);
+
+	spin_lock_irqsave(&pps->lock, flags);
+
+	/* Must call the echo function? */
+	if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
+		pps->info.echo(source, event, data);
+
+	/* Check the event */
+	pps->current_mode = pps->params.mode;
+	if (event & PPS_CAPTUREASSERT) {
+		/* We have to add an offset? */
+		if (pps->params.mode & PPS_OFFSETASSERT)
+			pps_add_offset(&ts, &pps->params.assert_off_tu);
+
+		/* Save the time stamp */
+		pps->assert_tu = ts;
+		pps->assert_sequence++;
+		pr_debug("capture assert seq #%u for source %d\n",
+			pps->assert_sequence, source);
+	}
+	if (event & PPS_CAPTURECLEAR) {
+		/* We have to add an offset? */
+		if (pps->params.mode & PPS_OFFSETCLEAR)
+			pps_add_offset(&ts, &pps->params.clear_off_tu);
+
+		/* Save the time stamp */
+		pps->clear_tu = ts;
+		pps->clear_sequence++;
+		pr_debug("capture clear seq #%u for source %d\n",
+			pps->clear_sequence, source);
+	}
+
+	pps->go = ~0;
+	wake_up_interruptible(&pps->queue);
+
+	kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
+
+	spin_unlock_irqrestore(&pps->lock, flags);
+
+	/* Now we can release the PPS source for (possible) deregistration */
+	spin_lock_irqsave(&idr_lock, flags);
+	atomic_dec(&pps->usage);
+	wake_up_all(&pps->usage_queue);
+	spin_unlock_irqrestore(&idr_lock, flags);
+}
+EXPORT_SYMBOL(pps_event);
 
...
 
+static int pps_cdev_open(struct inode *inode, struct file *file)
+{
+	struct pps_device *pps = container_of(inode->i_cdev,
+						struct pps_device, cdev);
+	int found;
+
+	spin_lock_irq(&idr_lock);
+	found = idr_find(&pps_idr, pps->id) != NULL;
+

Review

Quoted 5 lines

(quoted diff) +5 -0

+	/* Lock the PPS source against (possible) deregistration */
+	if (found)
+		atomic_inc(&pps->usage);
+
+	spin_unlock_irq(&idr_lock);

That looks a bit odd. How can the pps_device not be registered in the IDR tree at this stage?

Quoted 38 lines

(quoted diff) +33 -0

+	if (!found)
+		return -ENODEV;
+
+	file->private_data = pps;
+
+	return 0;
+}
 
...
 
+/* Kernel consumers */
+#define PPS_KC_HARDPPS		0	/* hardpps() (or equivalent) */
+#define PPS_KC_HARDPPS_PLL	1	/* hardpps() constrained to
+					   use a phase-locked loop */
+#define PPS_KC_HARDPPS_FLL	2	/* hardpps() constrained to
+					   use a frequency-locked loop */
+/*
+ * Here begins the implementation-specific part!
+ */
+
+struct pps_fdata {
+	struct pps_kinfo info;
+	struct pps_ktime timeout;
+};
+
+#include <linux/ioctl.h>
+
+#define PPS_CHECK		_IO('P', 0)
+#define PPS_GETPARAMS		_IOR('P', 1, struct pps_kparams *)
+#define PPS_SETPARAMS		_IOW('P', 2, struct pps_kparams *)
+#define PPS_GETCAP		_IOR('P', 3, int *)
+#define PPS_FETCH		_IOWR('P', 4, struct pps_fdata *)
+
+#ifdef __KERNEL__
+
+#include <linux/cdev.h>

Review

Quoted 4 lines

(quoted diff) +4 -0

+#include <linux/device.h>
+
+#define PPS_VERSION		"5.0.0"
+#define PPS_MAX_SOURCES		16		/* should be enough... */

It's nice to avoid sprinkling #includes throughout the file, please. People expect to be able to see what's being included in the first screenful of the file.

Quoted 8 lines

(quoted diff) +8 -0

+/*
+ * Global defines
+ */
+
+/* The specific PPS source info */
+struct pps_source_info {
+	char name[PPS_MAX_NAME_LEN];		/* simbolic name */
+	char path[PPS_MAX_NAME_LEN];		/* path of connected device */

Review

Quoted 8 lines

(quoted diff) +8 -0

+	int mode;				/* PPS's allowed mode */
+
+	void (*echo)(int source, int event, void *data); /* PPS echo function */
+
+	struct module *owner;
+	struct device *dev;
+};
+

Quoted 40 lines

Quoted 16 lines

(quoted diff) +16 -0

+void pps_unregister_source(int source)
+{
+	struct pps_device *pps;
+
+	spin_lock_irq(&idr_lock);
+	pps = idr_find(&pps_idr, source);
+
+	if (!pps) {
+		spin_unlock_irq(&idr_lock);
+		return;
+	}
+
+	/* This should be done first in order to deny IRQ handlers
+	 * to access PPS structs
+	 */
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	idr_remove(&pps_idr, pps->id);
+	spin_unlock_irq(&idr_lock);
+
+	wait_event(pps->usage_queue, atomic_read(&pps->usage) == 0);
+
+	pps_sysfs_remove_source_entry(pps);
+	pps_unregister_cdev(pps);
+	kfree(pps);
+}
+EXPORT_SYMBOL(pps_unregister_source);

The wait_event() stuff really shouldn't be here: it should be integral to the refcounting:

void pps_dev_put(struct pps_device *pps)

{
	spin_lock_irq(&pps_lock);
	if (atomic_dec_and_test(&pps->usage))
		idr_remove(&pps_idr, pps->id);
	else
		pps = NULL;
	spin_unlock_irq(&pps_lock);
	if (pps) {
		/*

Quoted 8 lines

		 * Might need to do the below via schedule_work() if
		 * pps_dev_put() is to be callable from atomic context
		 */
		pps_sysfs_remove_source_entry(pps);
		pps_unregister_cdev(pps);
		kfree(pps);
	}
}

I don't understand where I should use this function... :'(

Quoted 4 lines

As it stands, there might be deadlocks such as when a process which itself holds a ref on the pps_device (with an open fd?) calls pps_unregister_source.

I can add a wait_event_interruptible in order to allow userland to continue by receiving a signal. It could be acceptable?

Quoted 3 lines

Also, we need to take care that all processes which were waiting in pps_unregister_source() get to finish their cleanup before we permit rmmod to proceed. Is that handled somewhere?

I don't understand the problem... this code as been added in order to avoid the case where a pps_event() is called while a process executes the pps_unregister_source(). If more processes try to execute this code the first which enters will execute idr_remove() which prevents another process to reach the wait_event()... is that wrong? =:-o

Quoted 7 lines · On Tue, 25 Mar 2008 15:44:00 +0100 Rodolfo Giometti <[email protected]>…

Quoted 4 lines

As it stands, there might be deadlocks such as when a process which itself holds a ref on the pps_device (with an open fd?) calls pps_unregister_source.

I can add a wait_event_interruptible in order to allow userland to continue by receiving a signal. It could be acceptable?

There should be no need to "wait" for anything. When the final reference to an object is released, that object is cleaned up. Just like we do for inodes, dentries, pages, files, and 100 other kernel objects.

The need to wait for something else to go away is a big red flag with "busted refcounting" written on it.

Quoted 9 lines

Quoted 3 lines

Also, we need to take care that all processes which were waiting in pps_unregister_source() get to finish their cleanup before we permit rmmod to proceed. Is that handled somewhere?

I don't understand the problem... this code as been added in order to avoid the case where a pps_event() is called while a process executes the pps_unregister_source(). If more processes try to execute this code the first which enters will execute idr_remove() which prevents another process to reach the wait_event()... is that wrong? =:-o

I was asking you!

We should get the reference counting and object lifetimes sorted out first. There should be no "wait for <object> to be released" code. Once that is in place, things like rmmod will also sort themselves out: it just won't be possible to remove the module while there are live references to objects.

Quoted 26 lines · Andrew Morton

Quoted 7 lines · On Tue, 25 Mar 2008 15:44:00 +0100 Rodolfo Giometti <[email protected]>…

Quoted 4 lines

As it stands, there might be deadlocks such as when a process which itself holds a ref on the pps_device (with an open fd?) calls pps_unregister_source.

I can add a wait_event_interruptible in order to allow userland to continue by receiving a signal. It could be acceptable?

There should be no need to "wait" for anything. When the final reference to an object is released, that object is cleaned up. Just like we do for inodes, dentries, pages, files, and 100 other kernel objects.

The need to wait for something else to go away is a big red flag with "busted refcounting" written on it.

Quoted 9 lines

Quoted 3 lines

Also, we need to take care that all processes which were waiting in pps_unregister_source() get to finish their cleanup before we permit rmmod to proceed. Is that handled somewhere?

I don't understand the problem... this code as been added in order to avoid the case where a pps_event() is called while a process executes the pps_unregister_source(). If more processes try to execute this code the first which enters will execute idr_remove() which prevents another process to reach the wait_event()... is that wrong? =:-o

Quoted 6 lines

I was asking you!

We should get the reference counting and object lifetimes sorted out first. There should be no "wait for <object> to be released" code. Once that is in place, things like rmmod will also sort themselves out: it just won't be possible to remove the module while there are live references to objects.

The problem is related to serial and parallel clients.

The PPS source related to a serial port (or a parallel one) uses the serial (or parallel) IRQ to get PPS timestamps and it could be possible that a process tries to close the PPS source while another CPU is runnig the serial IRQ, so I cannot remove the PPS object until the IRQ handler is finished its job on the PPS object.

For clients (currently none :) which define their own IRQ handler for PPS timestamps managing the problem doesn't arise at all.

Quoted 36 lines · On Tue, 1 Apr 2008 10:42:14 +0200 Rodolfo Giometti <[email protected]>…

Quoted 26 lines · Andrew Morton

Quoted 7 lines · On Tue, 25 Mar 2008 15:44:00 +0100 Rodolfo Giometti <[email protected]>…

Quoted 4 lines

As it stands, there might be deadlocks such as when a process which itself holds a ref on the pps_device (with an open fd?) calls pps_unregister_source.

I can add a wait_event_interruptible in order to allow userland to continue by receiving a signal. It could be acceptable?

There should be no need to "wait" for anything. When the final reference to an object is released, that object is cleaned up. Just like we do for inodes, dentries, pages, files, and 100 other kernel objects.

The need to wait for something else to go away is a big red flag with "busted refcounting" written on it.

Quoted 9 lines

Quoted 3 lines

Also, we need to take care that all processes which were waiting in pps_unregister_source() get to finish their cleanup before we permit rmmod to proceed. Is that handled somewhere?

I don't understand the problem... this code as been added in order to avoid the case where a pps_event() is called while a process executes the pps_unregister_source(). If more processes try to execute this code the first which enters will execute idr_remove() which prevents another process to reach the wait_event()... is that wrong? =:-o

Quoted 6 lines

I was asking you!

We should get the reference counting and object lifetimes sorted out first. There should be no "wait for <object> to be released" code. Once that is in place, things like rmmod will also sort themselves out: it just won't be possible to remove the module while there are live references to objects.

The problem is related to serial and parallel clients.

Quoted 8 lines

The PPS source related to a serial port (or a parallel one) uses the serial (or parallel) IRQ to get PPS timestamps and it could be possible that a process tries to close the PPS source while another CPU is runnig the serial IRQ, so I cannot remove the PPS object until the IRQ handler is finished its job on the PPS object.

For clients (currently none :) which define their own IRQ handler for PPS timestamps managing the problem doesn't arise at all.

This can all be handled with suitable locking and refcounting. The device which is delivering PPS interrupts has a reference on the PPS data structures. If userspace has PPS open then it also has a reference.

The thread of control which releases the last reference to the PPS data structures also frees them all up. This may require a schedule_work() if we need to support release-from-interrupt (as it appears that we do), but that's OK - we just need to be able to make the PPS data structures ineligible for new lookups while the schedule_work() is pending.

There should be no need for any thread of control to wait for any other thread of control to do anything. Get the refcounting right and everything can be done synchronously.

This can all be handled with suitable locking and refcounting. The device which is delivering PPS interrupts has a reference on the PPS data structures. If userspace has PPS open then it also has a reference.

The thread of control which releases the last reference to the PPS data structures also frees them all up. This may require a schedule_work() if we need to support release-from-interrupt (as it appears that we do), but that's OK - we just need to be able to make the PPS data structures ineligible for new lookups while the schedule_work() is pending.

There should be no need for any thread of control to wait for any other thread of control to do anything. Get the refcounting right and everything can be done synchronously.

Quoted 11 lines · Andrew Morton

This can all be handled with suitable locking and refcounting. The device which is delivering PPS interrupts has a reference on the PPS data structures. If userspace has PPS open then it also has a reference.

The thread of control which releases the last reference to the PPS data structures also frees them all up. This may require a schedule_work() if we need to support release-from-interrupt (as it appears that we do), but that's OK - we just need to be able to make the PPS data structures ineligible for new lookups while the schedule_work() is pending.

Quoted 3 lines

There should be no need for any thread of control to wait for any other thread of control to do anything. Get the refcounting right and everything can be done synchronously.

Here my solution by using get/put functions:

drivers/pps/kapi.c +31 -0

index d75c8c8..61c1569 100644
@@ -59,6 +59,62 @@ static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
  * Exported functions
  */
 
+/* pps_get_source - find a PPS source
+ *
+ * 	source: the PPS source ID.
+ *
+ * This function is used to find an already registered PPS source into the
+ * system.
+ *
+ * The function returns NULL if found nothing, otherwise it returns a pointer
+ * to the PPS source data struct (the refcounter is incremented by 1).
+ */
+
+struct pps_device *pps_get_source(int source)
+{
+	struct pps_device *pps;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pps_idr_lock, flags);
+
+	pps = idr_find(&pps_idr, source);
+	if (pps != NULL)
+		atomic_inc(&pps->usage);
+
+	spin_unlock_irqrestore(&pps_idr_lock, flags);
+
+	return pps;
+}
+
+/* pps_put_source - free the PPS source data
+ *
+ *	pps: a pointer to the PPS source.
+ *

This patch adds the kernel side of the PPS support currently named
"LinuxPPS".

diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c new file mode 100644
index 0000000..c25c91c
--- /dev/null
+++ b/drivers/pps/sysfs.c
@@ -0,0 +1,130 @@

+void pps_sysfs_remove_source_entry(struct pps_device *pps) +{
+ /* Delete info files */
+ sysfs_remove_group(&pps->dev->kobj, &pps_group);
+}
+
+int pps_sysfs_create_source_entry(struct pps_device *pps) +{
+ int ret;
+
+ /* Create info files */
+ ret = sysfs_create_group(&pps->dev->kobj, &pps_group);

Quoted 10 lines · Rodolfo Giometti

This patch adds the kernel side of the PPS support currently named
"LinuxPPS".

diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c new file mode 100644
index 0000000..c25c91c
--- /dev/null
+++ b/drivers/pps/sysfs.c
@@ -0,0 +1,130 @@

Quoted 12 lines

+void pps_sysfs_remove_source_entry(struct pps_device *pps) +{
+ /* Delete info files */
+ sysfs_remove_group(&pps->dev->kobj, &pps_group);
+}
+
+int pps_sysfs_create_source_entry(struct pps_device *pps) +{
+ int ret;
+
+ /* Create info files */
+ ret = sysfs_create_group(&pps->dev->kobj, &pps_group);

Any specific reason you are creating the group by hand, and not assign it to the default group in your class? It would will let the core handle it for you. The device add uevent to userspace will happen before you added the attributes, which is usually not nice.

Quoted 24 lines · Kay Sievers

Quoted 10 lines · Rodolfo Giometti

This patch adds the kernel side of the PPS support currently named
"LinuxPPS".

diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c new file mode 100644
index 0000000..c25c91c
--- /dev/null
+++ b/drivers/pps/sysfs.c
@@ -0,0 +1,130 @@

Quoted 12 lines

+void pps_sysfs_remove_source_entry(struct pps_device *pps) +{
+ /* Delete info files */
+ sysfs_remove_group(&pps->dev->kobj, &pps_group);
+}
+
+int pps_sysfs_create_source_entry(struct pps_device *pps) +{
+ int ret;
+
+ /* Create info files */
+ ret = sysfs_create_group(&pps->dev->kobj, &pps_group);

Quoted 4 lines

Any specific reason you are creating the group by hand, and not assign it to the default group in your class? It would will let the core handle it for you. The device add uevent to userspace will happen before you added the attributes, which is usually not nice.

No any specific reason... I just supposed it was the-right-think(TM)! :)

Can you please suggest me an URL where I can see how to rework the code?

Quoted 33 lines · Rodolfo Giometti

Quoted 24 lines · Kay Sievers

Quoted 10 lines · Rodolfo Giometti

This patch adds the kernel side of the PPS support currently named
"LinuxPPS".

diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c new file mode 100644
index 0000000..c25c91c
--- /dev/null
+++ b/drivers/pps/sysfs.c
@@ -0,0 +1,130 @@

Quoted 12 lines

+void pps_sysfs_remove_source_entry(struct pps_device *pps) +{
+ /* Delete info files */
+ sysfs_remove_group(&pps->dev->kobj, &pps_group);
+}
+
+int pps_sysfs_create_source_entry(struct pps_device *pps) +{
+ int ret;
+
+ /* Create info files */
+ ret = sysfs_create_group(&pps->dev->kobj, &pps_group);

Quoted 4 lines

Any specific reason you are creating the group by hand, and not assign it to the default group in your class? It would will let the core handle it for you. The device add uevent to userspace will happen before you added the attributes, which is usually not nice.

No any specific reason... I just supposed it was the-right-think(TM)! :)

Quoted 2 lines

Can you please suggest me an URL where I can see how to rework the code?

You can assign class->dev_attrs the array of attributes, and the driver core will create these attributes for all devices that get registered with that class. See: drivers/base/core.c :: device_add_attrs().

This patch adds the kernel side of the PPS support currently named
"LinuxPPS".

diff --git a/include/linux/pps.h b/include/linux/pps.h new file mode 100644
index 0000000..c455443
--- /dev/null
+++ b/include/linux/pps.h
@@ -0,0 +1,204 @@

+/* The main struct */
+struct pps_device {
+ struct pps_source_info info; /* PSS source info */
+
+ struct pps_kparams params; /* PPS's current params */
+

 +       __u32 assert_sequence;                  /* PPS' assert event seq # */
 +       __u32 clear_sequence;                   /* PPS' clear event seq # */
 +       struct pps_ktime assert_tu;
 +       struct pps_ktime clear_tu;
 +       int current_mode;                       /* PPS mode at event time */
 +
 +       int go;                                 /* PPS event is arrived? */
 +       wait_queue_head_t queue;                /* PPS event queue */
 +
 +       unsigned int id;                        /* PPS source unique ID */
 +       struct cdev cdev;
 +       struct device *dev;
 +       int devno;
 +       struct fasync_struct *async_queue;      /* fasync method */
 +       spinlock_t lock;

+
+ atomic_t usage; /* usage count */
+ wait_queue_head_t usage_queue;
+
+ struct class class_dev;

Quoted 31 lines · Rodolfo Giometti

This patch adds the kernel side of the PPS support currently named
"LinuxPPS".

diff --git a/include/linux/pps.h b/include/linux/pps.h new file mode 100644
index 0000000..c455443
--- /dev/null
+++ b/include/linux/pps.h
@@ -0,0 +1,204 @@

+/* The main struct */
+struct pps_device {
+ struct pps_source_info info; /* PSS source info */
+
+ struct pps_kparams params; /* PPS's current params */
+

 +       __u32 assert_sequence;                  /* PPS' assert event seq # */
 +       __u32 clear_sequence;                   /* PPS' clear event seq # */
 +       struct pps_ktime assert_tu;
 +       struct pps_ktime clear_tu;
 +       int current_mode;                       /* PPS mode at event time */
 +
 +       int go;                                 /* PPS event is arrived? */
 +       wait_queue_head_t queue;                /* PPS event queue */
 +
 +       unsigned int id;                        /* PPS source unique ID */
 +       struct cdev cdev;
 +       struct device *dev;
 +       int devno;
 +       struct fasync_struct *async_queue;      /* fasync method */
 +       spinlock_t lock;

Quoted 5 lines

+
+ atomic_t usage; /* usage count */
+ wait_queue_head_t usage_queue;
+
+ struct class class_dev;

Why is an entire class embedded into every device? :)

Quoted 38 lines · Kay Sievers

Quoted 36 lines · Rodolfo Giometti

This patch adds the kernel side of the PPS support currently named
"LinuxPPS".

diff --git a/include/linux/pps.h b/include/linux/pps.h new file mode 100644
index 0000000..c455443
--- /dev/null
+++ b/include/linux/pps.h
@@ -0,0 +1,204 @@

+/* The main struct */
+struct pps_device {
+ struct pps_source_info info; /* PSS source info */
+
+ struct pps_kparams params; /* PPS's current params */
+

 +       __u32 assert_sequence;                  /* PPS' assert event seq # */
 +       __u32 clear_sequence;                   /* PPS' clear event seq # */
 +       struct pps_ktime assert_tu;
 +       struct pps_ktime clear_tu;
 +       int current_mode;                       /* PPS mode at event time */
 +
 +       int go;                                 /* PPS event is arrived? */
 +       wait_queue_head_t queue;                /* PPS event queue */
 +
 +       unsigned int id;                        /* PPS source unique ID */
 +       struct cdev cdev;
 +       struct device *dev;
 +       int devno;
 +       struct fasync_struct *async_queue;      /* fasync method */
 +       spinlock_t lock;

+
+ atomic_t usage; /* usage count */
+ wait_queue_head_t usage_queue;
+
+ struct class class_dev;

Quoted 1 line

Why is an entire class embedded into every device? :)

Uh! =:-o

Is that not right? What do you suggest to do?

Quoted 31 lines · Rodolfo Giometti

This patch adds the kernel side of the PPS support currently named
"LinuxPPS".

diff --git a/include/linux/pps.h b/include/linux/pps.h new file mode 100644
index 0000000..c455443
--- /dev/null
+++ b/include/linux/pps.h
@@ -0,0 +1,204 @@

+/* The main struct */
+struct pps_device {
+ struct pps_source_info info; /* PSS source info */
+
+ struct pps_kparams params; /* PPS's current params */
+

 +       __u32 assert_sequence;                  /* PPS' assert event seq # */
 +       __u32 clear_sequence;                   /* PPS' clear event seq # */
 +       struct pps_ktime assert_tu;
 +       struct pps_ktime clear_tu;
 +       int current_mode;                       /* PPS mode at event time */
 +
 +       int go;                                 /* PPS event is arrived? */
 +       wait_queue_head_t queue;                /* PPS event queue */
 +
 +       unsigned int id;                        /* PPS source unique ID */
 +       struct cdev cdev;
 +       struct device *dev;
 +       int devno;
 +       struct fasync_struct *async_queue;      /* fasync method */
 +       spinlock_t lock;

Quoted 5 lines

+
+ atomic_t usage; /* usage count */
+ wait_queue_head_t usage_queue;
+
+ struct class class_dev;

Why is an entire class embedded into every device? :)

This patch adds the kernel side of the PPS support currently named "LinuxPPS".

Quoted 2 lines · On Thu, 6 Mar 2008 13:09:00 +0100 Rodolfo Giometti <[email protected]> wrote:

This patch adds the kernel side of the PPS support currently named "LinuxPPS".

powerpc:

drivers/pps/pps.c: In function 'pps_cdev_ioctl':
drivers/pps/pps.c:166: warning: format '%lld' expects type 'long long int', but argument 2 has type '__s64' drivers/pps/kapi.c: In function 'pps_event':
drivers/pps/kapi.c:225: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type '__s64' drivers/pps/sysfs.c: In function 'pps_show_assert':
drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64' drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64' drivers/pps/sysfs.c: In function 'pps_show_clear':
drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64' drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'

Quoted 7 lines · Andrew Morton

Quoted 2 lines · On Thu, 6 Mar 2008 13:09:00 +0100 Rodolfo Giometti <[email protected]> wrote:

This patch adds the kernel side of the PPS support currently named "LinuxPPS".

powerpc:

Quoted 10 lines

drivers/pps/pps.c: In function 'pps_cdev_ioctl':
drivers/pps/pps.c:166: warning: format '%lld' expects type 'long long int', but argument 2 has type '__s64' drivers/pps/kapi.c: In function 'pps_event':
drivers/pps/kapi.c:225: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type '__s64' drivers/pps/sysfs.c: In function 'pps_show_assert':
drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64' drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64' drivers/pps/sysfs.c: In function 'pps_show_clear':
drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64' drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'

Oops! I have no powerpc system... how can I solve this issue?

Quoted 19 lines · On Tue, 1 Apr 2008 10:59:50 +0200 Rodolfo Giometti <[email protected]>…

Quoted 17 lines · Andrew Morton

Quoted 2 lines · On Thu, 6 Mar 2008 13:09:00 +0100 Rodolfo Giometti <[email protected]> wrote:

This patch adds the kernel side of the PPS support currently named "LinuxPPS".

powerpc:

drivers/pps/pps.c: In function 'pps_cdev_ioctl':
drivers/pps/pps.c:166: warning: format '%lld' expects type 'long long int', but argument 2 has type '__s64' drivers/pps/kapi.c: In function 'pps_event':
drivers/pps/kapi.c:225: warning: format '%llu' expects type 'long long unsigned int', but argument 3 has type '__s64' drivers/pps/sysfs.c: In function 'pps_show_assert':
drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64' drivers/pps/sysfs.c:42: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64' drivers/pps/sysfs.c: In function 'pps_show_clear':
drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64' drivers/pps/sysfs.c:56: warning: format '%lld' expects type 'long long int', but argument 3 has type '__s64'

Quoted 1 line

Oops! I have no powerpc system... how can I solve this issue?

Use %lld (if these things can legitimately be negative - otherwise %llu) and cast the argument to (long long) or (unsigned long long).

This patch set adds the PPS support into Linux.

PPS means "pulse per second" and its API is specified by RFC 2783 (Pulse-Per-Second API for UNIX-like Operating Systems, Version 1.0).

The code has been tested with the NTPD program
(http://www.eecis.udel.edu/~mills/ntp/html/index.html) and several GPS antennae.

Quoted 5 lines · Rodolfo Giometti

This patch set adds the PPS support into Linux.

PPS means "pulse per second" and its API is specified by RFC 2783 (Pulse-Per-Second API for UNIX-like Operating Systems, Version 1.0).

Quoted 3 lines

The code has been tested with the NTPD program
(http://www.eecis.udel.edu/~mills/ntp/html/index.html) and several GPS antennae.

Hey Andrew,
Are there any remaining objections that are keeping you from picking
this patchset up?

Quoted 10 lines · On Wed, 19 Mar 2008 10:29:34 -0700 "john stultz" <[email protected]> wrote:

Quoted 8 lines · Rodolfo Giometti

This patch set adds the PPS support into Linux.

PPS means "pulse per second" and its API is specified by RFC 2783 (Pulse-Per-Second API for UNIX-like Operating Systems, Version 1.0).

The code has been tested with the NTPD program
(http://www.eecis.udel.edu/~mills/ntp/html/index.html) and several GPS antennae.

Quoted 3 lines

Hey Andrew,
Are there any remaining objections that are keeping you from picking
this patchset up?

Not as far as I know. They're in my queue-to-review. Assistance there is always useful ;)