LinuxPPS: Pulse per Second support for Linux

PATCH In review Rodolfo Giometti 8 participants 22 messages Mar 21, 2007 – May 12, 2007

Subsystems (6)

Networking Documentation TTY layer and serial drivers Char and misc drivers +2 more subsystems

lore

Share & tools

lore

Rodolfo Giometti

author maintainer

2007-03-21 07:40 UTC #0 lore raw

PATCH 1/1 LinuxPPS: Pulse per Second support for Linux

Pulse per Second (PPS) support for Linux.

Signed-off-by Rodolfo Giometti <[email protected]>

---

Please, note that this PPS implementation is not RFC 2783 fully compatible since, IMHO, the RFC simply doesn't consider PPS devices connected with special GPIOs or other ports different from serial ports and parallele ports.

Please read the following consideratios before sending to /dev/null! :)

RFC considerations
------------------

While implementing a PPS API as RFC 2783 defines and using an embedded CPU GPIO-Pin as physical link to the signal, I encountered a deeper problem:

At startup it needs a file descriptor as argument for the function time_pps_create().

This implies that the source has a /dev/... entry. This assumption is ok for the serial and parallel port, where you can do something usefull beside(!) the gathering of timestamps as it is the central task for a PPS-API. But this assumption does not work for a single purpose GPIO line. In this case even basic file-related functionality (like read() and write()) makes no sense at all and should not be a precondition for the use of a PPS-API. On the other hands may happen that a GPS antenna (which is also a PPS source) is connected to the system throught a serial line but the PPS signal is not and it uses a GPIOs, so we cannot consider the serial line as a PPS source at all!

The problem can be simply solved if you change the original RFC 2783:

pps_handle_t type is an opaque __scalar type__ used to represent a PPS source within the API

into a modified:

pps_handle_t type is an opaque __variable__ used to represent a PPS source within the API and programs should not access it directly to it due to its opacity.

This change seems to be neglibile because even the original RFC 2783 does not encourage programs to check (read: use) the pps_handle_t variable before calling the time_pps_*() functions, since each function should do this job internally.

If I intentionally separate the concept of "file descriptor" from the concept of the "PPS source" I'm obliged to provide a solution to find and register a PPS-source without using a file descriptor: it's done by the functions time_pps_findsource() and time_pps_findpath() now.

According to this current NTPD drivers' code should be modified as follows:

+#ifdef PPS_HAVE_FINDPATH
+ /* Get the PPS source's real name */
+ time_pps_readlink(link, LENPPS, path, PPS_MAX_NAME_LEN);
+

+      /* Try to find the source */
+      fd = time_pps_findpath(path, PPS_MAX_NAME_LEN, id, PPS_MAX_NAME_LEN);
+      if (fd < 0) {

+ msyslog(LOG_ERR, "refclock: cannot find PPS source \"%s\" in the system", path);
+ return 1;
+ }
+ msyslog(LOG_INFO, "refclock: found PPS source #%d \"%s\" on \"%s\"", fd, path, id);
+#endif /* PPS_HAVE_FINDPATH */
+
+

       if (time_pps_create(fd, &pps_handle) < 0) {
-              pps_handle = 0;
               msyslog(LOG_ERR, "refclock: time_pps_create failed: %m");
       }

Documentation/pps.txt new file +183 -0

new file mode 100644
index 0000000..bf13a59
@@ -0,0 +1,183 @@
+
+			PPS - Pulse Per Second
+			----------------------
+
+(C) Copyright 2007 Rodolfo Giometti <[email protected]>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+
+
+Overview
+--------
+
+LinuxPPS provides a programming interface (API) to define into the
+system several PPS sources.
+
+PPS means "pulse per second" and a PPS source is just a device which
+provides a high precision signal each second so that an application
+can use it to adjust system clock time.
+
+A PPS source can be connected to a serial port (usually to the Data
+Carrier Detect pin) or to a parallel port (ACK-pin) or to a special
+CPU's GPIOs (this is the common case in embedded systems) but in each
+case when a new pulse comes the system must apply to it a timestamp
+and record it for the userland.
+
+Common use is the combination of the NTPD as userland program with a
+GPS receiver as PPS source to obtain a wallclock-time with
+sub-millisecond synchronisation to UTC.
+
+
+RFC considerations
+------------------
+
+While implementing a PPS API as RFC 2783 defines and using an embedded
+CPU GPIO-Pin as physical link to the signal, I encountered a deeper
+problem:
+
+   At startup it needs a file descriptor as argument for the function
+   time_pps_create().
+
+This implies that the source has a /dev/... entry. This assumption is
+ok for the serial and parallel port, where you can do something
+usefull besides(!) the gathering of timestamps as it is the central
+task for a PPS-API. But this assumption does not work for a single
+purpose GPIO line. In this case even basic file-related functionality
+(like read() and write()) makes no sense at all and should not be a
+precondition for the use of a PPS-API.
+
+The problem can be simply solved if you change the original RFC 2783:
+
+    pps_handle_t type is an opaque __scalar type__ used to represent a
+    PPS source within the API
+
+into a modified:
+
+   pps_handle_t type is an opaque __variable__ used to represent a PPS
+   source within the API and programs should not access it directly to
+   it due to its opacity.
+
+This change seems to be neglibile because even the original RFC 2783
+does not encourage programs to check (read: use) the pps_handle_t
+variable before calling the time_pps_*() functions, since each
+function should do this job internally.
+
+If I intentionally separate the concept of "file descriptor" from the
+concept of the "PPS source" I'm obliged to provide a solution to find
+and register a PPS-source without using a file descriptor: it's done
+by the functions time_pps_findsource() and time_pps_findpath() now.
+
+According to this current NTPD drivers' code should be modified as
+follows:
+
++#ifdef PPS_HAVE_FINDPATH
++      /* Get the PPS source's real name */
++      fd = readlink(link, path, STRING_LEN-1);
++      if (fd <= 0)
++              strncpy(path, link, STRING_LEN);
++      else
++              path[fd] = '\0';
++
++      /* Try to find the source */
++      fd = time_pps_findpath(path, STRING_LEN, id, STRING_LEN);
++      if (fd < 0) {
++              msyslog(LOG_ERR, "refclock: cannot find PPS source \"%s\" in the system", path);
++              return 1;
++      }
++      msyslog(LOG_INFO, "refclock: found PPS source #%d \"%s\" on \"%s\"", fd, path, id);
++#endif   /* PPS_HAVE_FINDPATH */
++
++
+       if (time_pps_create(fd, &pps_handle) < 0) {
+-              pps_handle = 0;
+               msyslog(LOG_ERR, "refclock: time_pps_create failed: %m");
+       }
+
+
+Coding example
+--------------
+
+To register a PPS source into the kernel you should define a struct
+pps_source_info_s as follow:
+
+    static struct pps_source_info_s pps_ktimer_info = {
+            name         : "ktimer",
+            path         : "",
+            mode         : PPS_CAPTUREASSERT|PPS_OFFSETASSERT|PPS_ECHOASSERT| \
+                           PPS_CANWAIT|PPS_TSFMT_TSPEC,
+            echo         : pps_ktimer_echo,
+    };
+
+and then calling the function pps_register_source() in your
+intialization routine as follow:
+
+    source = pps_register_source(&pps_ktimer_info,
+			PPS_CAPTUREASSERT|PPS_OFFSETASSERT,
+			-1 /* is up to the system */);
+
+The pps_register_source() prototype is:
+
+  int pps_register_source(struct pps_source_info_s *info, int default_params, int try_id)
+
+where "info" is a pointer to a structure that describes a particular
+PPS source, "default_params" tells the system what the initial default
+parameters for the device should be (is obvious that these parameters
+must be a subset of ones defined into the struct
+pps_source_info_s which describe the capabilities of the driver)
+and "try_id" can be used to force a particular ID for your device into
+the system (just use -1 if you wish the system chooses one for you).
+
+Once you have registered a new PPS source into the system you can
+signal an assert event (for example in the interrupt handler routine)
+just using:
+
+    pps_event(source, PPS_CAPTUREASSERT, ptr);
+
+The same function may also run the defined echo function
+(pps_ktimer_echo(), passing to it the "ptr" pointer) if the user
+asked for that... etc..
+
+Please see the file drivers/pps/clients/ktimer.c for an example code.
+
+
+SYSFS support
+-------------
+
+The SYSFS support is enabled by default if the SYSFS filesystem is
+enabled in the kernel and it provides a new class:
+
+   $ ls /sys/class/pps/
+   00/  01/  02/
+
+Every directory is the ID of a PPS sources defined into the system and
+inside you find several files:
+
+    $ ls /sys/class/pps/00/
+    assert	clear  echo  mode  name  path  subsystem@  uevent
+
+Inside each "assert" and "clear" file you can find the timestamp and a
+sequence number:
+
+    $ cat cat /sys/class/pps/00/assert
+    1170026870.983207967#8
+
+Where before the "#" is the timestamp in seconds and after it is the
+sequence number. Other files are:
+
+* echo: reports if the PPS source has an echo function or not;
+
+* mode: reports available PPS functioning modes;
+
+* name: reports the PPS source's name;
+
+* path: reports the PPS source's device path, that is the device the
+  PPS source is connected to (if it exists).

MAINTAINERS +7 -0

index b0fd71b..baa3901 100644
@@ -2693,6 +2693,13 @@ P:	Michal Ostrowski
 M:	[email protected]
 S:	Maintained
 
+PPS SUPPORT
+P:	Rodolfo Giometti
+M:	[email protected]
+W:	http://wiki.enneenne.com/index.php/LinuxPPS_support
+L:	[email protected]
+S:	Maintained
+
 PREEMPTIBLE KERNEL
 P:	Robert Love
 M:	[email protected]

drivers/Kconfig +2 -0

index 050323f..bb54cab 100644
@@ -52,6 +52,8 @@ source "drivers/i2c/Kconfig"
 
 source "drivers/spi/Kconfig"
 
+source "drivers/pps/Kconfig"
+
 source "drivers/w1/Kconfig"
 
 source "drivers/hwmon/Kconfig"

drivers/Makefile +1 -0

index 3a718f5..d0007f7 100644
@@ -59,6 +59,7 @@ obj-$(CONFIG_INPUT)		+= input/
 obj-$(CONFIG_I2O)		+= message/
 obj-$(CONFIG_RTC_LIB)		+= rtc/
 obj-$(CONFIG_I2C)		+= i2c/
+obj-$(CONFIG_PPS)		+= pps/
 obj-$(CONFIG_W1)		+= w1/
 obj-$(CONFIG_HWMON)		+= hwmon/
 obj-$(CONFIG_PHONE)		+= telephony/

drivers/char/lp.c +59 -0

77 lines not on this page · raw message

drivers/pps/Kconfig new file +34 -0

new file mode 100644
index 0000000..54297e8
@@ -0,0 +1,34 @@
+#
+# Character device configuration
+#
+
+menu "PPS support"
+
+config PPS
+	tristate "PPS support"
+	depends on EXPERIMENTAL
+	---help---
+	  PPS (Pulse Per Second) is a special pulse provided by some GPS
+	  antennas. Userland can use it to get an high time reference.
+
+	  Some antennas' PPS signals are connected with the CD (Carrier
+	  Detect) pin of the serial line they use to communicate with the
+	  host. In this case use the SERIAL_LINE client support.
+
+	  Some antennas' PPS signals are connected with some special host
+	  inputs so you have to enable the corresponding client support.
+
+	  This PPS support can also be built as a module.  If so, the module
+	  will be called pps-core.
+
+config PPS_DEBUG
+	bool "PPS debugging messages"
+	depends on PPS 
+	help
+	  Say Y here if you want the PPS support to produce a bunch of debug
+	  messages to the system log.  Select this if you are having a
+	  problem with PPS support and want to see more of what is going on.
+
+source drivers/pps/clients/Kconfig
+
+endmenu

drivers/pps/Makefile new file +7 -0

new file mode 100644
index 0000000..76101cd
@@ -0,0 +1,7 @@
+#
+# Makefile for the PPS core.
+#
+
+pps_core-objs			+= pps.o kapi.o sysfs.o
+obj-$(CONFIG_PPS)		+= pps_core.o
+obj-y				+= clients/

drivers/pps/clients/Kconfig new file +38 -0

new file mode 100644
index 0000000..1da1503
@@ -0,0 +1,38 @@
+#
+# LinuxPPS clients configuration
+#
+
+if PPS
+
+comment "PPS clients support"
+
+config PPS_CLIENT_KTIMER
+	tristate "Kernel timer client (Testing client, use for debug)"
+	help
+	  If you say yes here you get support for a PPS debugging client
+	  which uses a kernel timer to generate the PPS signal.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ktimer.o.
+
+comment "UART serial support (forced off)"
+	depends on ! ( SERIAL_CORE != n && ! ( PPS = m && SERIAL_CORE = y ) )
+
+config PPS_CLIENT_UART
+	bool "UART serial support"
+	depends on SERIAL_CORE != n && ! ( PPS = m && SERIAL_CORE = y )
+	help
+	  If you say yes here you get support for a PPS source connected
+	  with the CD (Carrier Detect) pin of your serial port.
+
+comment "Parallel printer support (forced off)"
+	depends on ! ( PRINTER != n && ! ( PPS = m && PRINTER = y ) )
+
+config PPS_CLIENT_LP
+	bool "Parallel printer support"
+	depends on PRINTER != n && ! ( PPS = m && PRINTER = y )
+	help
+	  If you say yes here you get support for a PPS source connected
+	  with the interrupt pin of your parallel port.
+
+endif

drivers/pps/clients/Makefile new file +6 -0

new file mode 100644
index 0000000..3ecca41
@@ -0,0 +1,6 @@
+#
+# Makefile for miscellaneous I2C chip drivers.
+#
+
+obj-$(CONFIG_PPS_CLIENT_KTIMER)	+= ktimer.o
+

drivers/pps/clients/ktimer.c new file +107 -0

107 lines not on this page · raw message

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

198 lines not on this page · raw message

drivers/pps/pps.c new file +374 -0

374 lines not on this page · raw message

drivers/pps/sysfs.c new file +210 -0

210 lines not on this page · raw message

drivers/serial/8250.c +3 -0

index 98ec861..543c7cb 100644
@@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
 		up->ier |= UART_IER_MSI;
 	if (up->capabilities & UART_CAP_UUE)
 		up->ier |= UART_IER_UUE | UART_IER_RTOIE;
+	if (up->port.flags & UPF_HARDPPS_CD)
+		up->ier |= UART_IER_MSI;	/* enable interrupts */
 
 	serial_out(up, UART_IER, up->ier);

@@ -2718,6 +2720,7 @@ void serial8250_unregister_port(int line)
 	struct uart_8250_port *uart = &serial8250_ports[line];
 
 	mutex_lock(&serial_mutex);
+
 	uart_remove_one_port(&serial8250_reg, &uart->port);
 	if (serial8250_isa_devs) {
 		uart->port.flags &= ~UPF_BOOT_AUTOCONF;

drivers/serial/serial_core.c +72 -1

107 lines not on this page · raw message

include/linux/netlink.h +1 -0

index 2a20f48..f8d77e6 100644
@@ -24,6 +24,7 @@
 /* leave room for NETLINK_DM (DM Events) */
 #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
 #define NETLINK_ECRYPTFS	19
+#define NETLINK_PPSAPI          20	/* linuxPPS support */
 
 #define MAX_LINKS 32

include/linux/parport.h +16 -0

index 80682aa..8861aa0 100644
@@ -104,6 +104,11 @@ typedef enum {
 #include <asm/ptrace.h>
 #include <asm/semaphore.h>
 
+#ifdef CONFIG_PPS_CLIENT_LP
+#include <linux/timepps.h>
+#include <linux/pps.h>
+#endif
+
 /* Define this later. */
 struct parport;
 struct pardevice;

@@ -323,6 +328,11 @@ struct parport {
 
 	struct list_head full_list;
 	struct parport *slaves[3];
+
+#ifdef CONFIG_PPS_CLIENT_LP
+	struct pps_source_info_s pps_info;
+	int pps_source;		/* PPS source ID */
+#endif
 };
 
 #define DEFAULT_SPIN_TIME 500 /* us */

@@ -513,6 +523,12 @@ extern int parport_daisy_select (struct parport *port, int daisy, int mode);
 /* Lowlevel drivers _can_ call this support function to handle irqs.  */
 static __inline__ void parport_generic_irq(int irq, struct parport *port)
 {
+#ifdef CONFIG_PPS_CLIENT_LP
+	pps_event(port->pps_source, PPS_CAPTUREASSERT, port);
+	dbg("parport_pc: PPS assert event at %lu on source #%d",
+		jiffies, port->pps_source);
+#endif
+
 	parport_ieee1284_interrupt (irq, port);
 	read_lock(&port->cad_lock);
 	if (port->cad && port->cad->irq_func)

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

99 lines not on this page · raw message

include/linux/serial_core.h +29 -6

index 586aaba..690daa5 100644
@@ -145,6 +145,11 @@
 #include <linux/tty.h>
 #include <linux/mutex.h>
 
+#ifdef CONFIG_PPS_CLIENT_UART
+#include <linux/timepps.h>
+#include <linux/pps.h>
+#endif
+
 struct uart_port;
 struct uart_info;
 struct serial_struct;

@@ -223,6 +228,9 @@ struct uart_port {
 	unsigned char		regshift;		/* reg offset shift */
 	unsigned char		iotype;			/* io access style */
 	unsigned char		unused1;
+#ifdef CONFIG_PPS_CLIENT_UART
+	int			pps_source;		/* PPS source ID */
+#endif
 
 #define UPIO_PORT		(0)
 #define UPIO_HUB6		(1)

@@ -264,7 +272,8 @@ struct uart_port {
 #define UPF_IOREMAP		((__force upf_t) (1 << 31))
 
 #define UPF_CHANGE_MASK		((__force upf_t) (0x17fff))
-#define UPF_USR_MASK		((__force upf_t) (UPF_SPD_MASK|UPF_LOW_LATENCY))
+#define UPF_USR_MASK		((__force upf_t) (UPF_SPD_MASK|UPF_LOW_LATENCY\
+							|UPF_HARDPPS_CD))
 
 	unsigned int		mctrl;			/* current modem ctrl settings */
 	unsigned int		timeout;		/* character-based timeout */

@@ -295,6 +304,10 @@ struct uart_state {
 	struct uart_info	*info;
 	struct uart_port	*port;
 
+#ifdef CONFIG_PPS_CLIENT_UART
+        struct pps_source_info_s pps_info;
+#endif
+
 	struct mutex		mutex;
 };

@@ -459,13 +472,23 @@ uart_handle_dcd_change(struct uart_port *port, unsigned int status)
 {
 	struct uart_info *info = port->info;
 
-	port->icount.dcd++;
-
-#ifdef CONFIG_HARD_PPS
-	if ((port->flags & UPF_HARDPPS_CD) && status)
-		hardpps();
+#ifdef CONFIG_PPS_CLIENT_UART
+	if (port->flags & UPF_HARDPPS_CD) {
+		if (status) {
+			pps_event(port->pps_source, PPS_CAPTUREASSERT, port);
+			dbg("serial8250: PPS assert event at %lu on source #%d",
+				jiffies, port->pps_source);
+		}
+		else {
+			pps_event(port->pps_source, PPS_CAPTURECLEAR, port);
+			dbg("serial8250: PPS clear event at %lu on source #%d",
+				jiffies, port->pps_source);
+		}
+	}
 #endif
 
+	port->icount.dcd++;
+
 	if (info->flags & UIF_CHECK_CD) {
 		if (status)
 			wake_up_interruptible(&info->open_wait);

include/linux/timepps.h new file +519 -0

519 lines not on this page · raw message

Jon K Hellan

2007-03-21 08:05 UTC #1 lore raw

Quoted 11 lines · Rodolfo Giometti

Pulse per Second (PPS) support for Linux.
Signed-off-by Rodolfo Giometti <[email protected]>
---
Please, note that this PPS implementation is not RFC 2783 fully compatible since, IMHO, the RFC simply doesn't consider PPS devices connected with special GPIOs or other ports different from serial ports and parallele ports.

Have you received any comments on this from the NTP community? From Ulrich Windl?

Jon Kåre Hellan, UNINETT, Trondheim, Norway

Rodolfo Giometti

author maintainer

2007-03-21 08:08 UTC #2 lore raw

Quoted 2 lines · Jon K Hellan

Have you received any comments on this from the NTP community? From Ulrich Windl?

Not yet... that's why I reposted my patch.

Thanks,

Rodolfo

Signature · 4 lines

GNU/Linux Solutions                  e-mail:    [email protected]
Linux Device Driver                             [email protected]
Embedded Systems                     		[email protected]
UNIX programming                     phone:     +39 349 2432127

Lennart Sorensen

2007-03-21 15:34 UTC #3 lore raw

In reply to Jon K Hellan

Quoted 2 lines · Jon K Hellan

Have you received any comments on this from the NTP community? From Ulrich Windl?

Well as just a user, this patch seems to work, and is actively being worked on and maintained for modern kernels (which unforunately Ulrich Windl's patch hardly appears to be).

This one also seems much less intrusive, doesn't break serial console systems (I sent a patch to Ulrich Windl to fix that, due to one of many places where pointers are dereferenced without being checked for !NULL first).

Is LinuxPPS better? I have no idea, and I don't think I am qualified to say one way or another. I do know that I could apply it to 2.6.18 with minimal work, while I couldn't get Ulrich Windl's patch to do so (too much has changed since 2.6.15, while I did manage to get it to apply to 2.6.16 with quite a bit of work).

Having PPS support actually part of the kernel would make it much simpler to deal with in the future, and perhaps ntp would start being compiled with such support regularly.

--
Len Sorensen

Reg Clemens

2007-03-21 17:19 UTC #4 lore raw

In reply to Rodolfo Giometti

Just one point with your patch.
A couple of years ago there was a long discussion on the Linux Kernel mailing list
as to how USER codes were not supposed to use KERNEL include files. Now I still
see __KERNEL__ all over the place in the linux=source includes so mabe this never went anywhere, but you MIGHT have to provide a separate patch (addition) to the Linux (as opposed to Linux-kernel) include files with timepps.h

Signature · 2 lines

                                        Reg.Clemens
                                        [email protected]

Rodolfo Giometti

author maintainer

2007-03-21 20:36 UTC #5 lore raw

Quoted 9 lines · Reg Clemens

Just one point with your patch.
A couple of years ago there was a long discussion on the Linux Kernel mailing list
as to how USER codes were not supposed to use KERNEL include files. Now I still
see __KERNEL__ all over the place in the linux=source includes so mabe this never went anywhere, but you MIGHT have to provide a separate patch (addition) to the Linux (as opposed to Linux-kernel) include files with timepps.h

Do you mean I should separate file timepps.h into two files? One for the kernel and one for the userland?

I'm sorry but I don't understand what you suggest. Can you explain it?

Thanks,

Rodolfo

Signature · 4 lines

GNU/Linux Solutions                  e-mail:    [email protected]
Linux Device Driver                             [email protected]
Embedded Systems                     		[email protected]
UNIX programming                     phone:     +39 349 2432127

Reg Clemens

2007-03-21 22:37 UTC #6 lore raw

Quoted 11 lines · Reg Clemens

Quoted 9 lines
Just one point with your patch.
A couple of years ago there was a long discussion on the Linux Kernel mailing list
as to how USER codes were not supposed to use KERNEL include files. Now I still
see __KERNEL__ all over the place in the linux=source includes so mabe this never went anywhere, but you MIGHT have to provide a separate patch (addition) to the Linux (as opposed to Linux-kernel) include files with timepps.h

Quoted 3 lines

Do you mean I should separate file timepps.h into two files? One for the kernel and one for the userland?

Well, Im sure you are dealing with some of the Linux Kernel people, so its whatever they want. But dont be surprised if they ask for a *.h file thats ONLY
for Kernel use. And I have NO idea how you would get the USER version into the Linux distribution itself.

Signature · 2 lines

                                        Reg.Clemens
                                        [email protected]

Rodolfo Giometti

author maintainer

2007-04-26 21:00 UTC #7 lore raw

Quoted 11 lines · Reg Clemens

Quoted 11 lines · Reg Clemens
Quoted 9 lines
Just one point with your patch.
A couple of years ago there was a long discussion on the Linux Kernel mailing list
as to how USER codes were not supposed to use KERNEL include files. Now I still
see __KERNEL__ all over the place in the linux=source includes so mabe this never went anywhere, but you MIGHT have to provide a separate patch (addition) to the Linux (as opposed to Linux-kernel) include files with timepps.h

Quoted 8 lines

Quoted 3 lines
Do you mean I should separate file timepps.h into two files? One for the kernel and one for the userland?
Well, Im sure you are dealing with some of the Linux Kernel people, so its whatever they want. But dont be surprised if they ask for a *.h file thats ONLY
for Kernel use. And I have NO idea how you would get the USER version into the Linux distribution itself.

According to this suggestion I've modified the LinuxPPS code. Please refere to the GIT repository and the LinuxPPS wiki for further info.

Now I hope LinuxPPS can be ready for the Linux source tree! :)

Ciao,

Rodolfo

Signature · 4 lines

GNU/Linux Solutions                  e-mail:    [email protected]
Linux Device Driver                             [email protected]
Embedded Systems                     		[email protected]
UNIX programming                     phone:     +39 349 2432127

Rodolfo Giometti

author maintainer

2007-05-02 19:33 UTC #8 lore raw

PATCH 1/1 LinuxPPS: Pulse per Second support for Linux

In reply to Rodolfo Giometti

Pulse per Second (PPS) support for Linux.

Signed-off-by Rodolfo Giometti <[email protected]>

---

Here my last release of PPS support for Linux.

The difference against my last patch is about all userland specific code (timepps.h) which has been removed, I hope now you can consider adding it into kernel source tree!

Please, let me know if I still should fix something else.

---

Documentation/pps.txt new file +183 -0

new file mode 100644
index 0000000..bf13a59
@@ -0,0 +1,183 @@
+
+			PPS - Pulse Per Second
+			----------------------
+
+(C) Copyright 2007 Rodolfo Giometti <[email protected]>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+
+
+Overview
+--------
+
+LinuxPPS provides a programming interface (API) to define into the
+system several PPS sources.
+
+PPS means "pulse per second" and a PPS source is just a device which
+provides a high precision signal each second so that an application
+can use it to adjust system clock time.
+
+A PPS source can be connected to a serial port (usually to the Data
+Carrier Detect pin) or to a parallel port (ACK-pin) or to a special
+CPU's GPIOs (this is the common case in embedded systems) but in each
+case when a new pulse comes the system must apply to it a timestamp
+and record it for the userland.
+
+Common use is the combination of the NTPD as userland program with a
+GPS receiver as PPS source to obtain a wallclock-time with
+sub-millisecond synchronisation to UTC.
+
+
+RFC considerations
+------------------
+
+While implementing a PPS API as RFC 2783 defines and using an embedded
+CPU GPIO-Pin as physical link to the signal, I encountered a deeper
+problem:
+
+   At startup it needs a file descriptor as argument for the function
+   time_pps_create().
+
+This implies that the source has a /dev/... entry. This assumption is
+ok for the serial and parallel port, where you can do something
+usefull besides(!) the gathering of timestamps as it is the central
+task for a PPS-API. But this assumption does not work for a single
+purpose GPIO line. In this case even basic file-related functionality
+(like read() and write()) makes no sense at all and should not be a
+precondition for the use of a PPS-API.
+
+The problem can be simply solved if you change the original RFC 2783:
+
+    pps_handle_t type is an opaque __scalar type__ used to represent a
+    PPS source within the API
+
+into a modified:
+
+   pps_handle_t type is an opaque __variable__ used to represent a PPS
+   source within the API and programs should not access it directly to
+   it due to its opacity.
+
+This change seems to be neglibile because even the original RFC 2783
+does not encourage programs to check (read: use) the pps_handle_t
+variable before calling the time_pps_*() functions, since each
+function should do this job internally.
+
+If I intentionally separate the concept of "file descriptor" from the
+concept of the "PPS source" I'm obliged to provide a solution to find
+and register a PPS-source without using a file descriptor: it's done
+by the functions time_pps_findsource() and time_pps_findpath() now.
+
+According to this current NTPD drivers' code should be modified as
+follows:
+
++#ifdef PPS_HAVE_FINDPATH
++      /* Get the PPS source's real name */
++      fd = readlink(link, path, STRING_LEN-1);
++      if (fd <= 0)
++              strncpy(path, link, STRING_LEN);
++      else
++              path[fd] = '\0';
++
++      /* Try to find the source */
++      fd = time_pps_findpath(path, STRING_LEN, id, STRING_LEN);
++      if (fd < 0) {
++              msyslog(LOG_ERR, "refclock: cannot find PPS source \"%s\" in the system", path);
++              return 1;
++      }
++      msyslog(LOG_INFO, "refclock: found PPS source #%d \"%s\" on \"%s\"", fd, path, id);
++#endif   /* PPS_HAVE_FINDPATH */
++
++
+       if (time_pps_create(fd, &pps_handle) < 0) {
+-              pps_handle = 0;
+               msyslog(LOG_ERR, "refclock: time_pps_create failed: %m");
+       }
+
+
+Coding example
+--------------
+
+To register a PPS source into the kernel you should define a struct
+pps_source_info_s as follow:
+
+    static struct pps_source_info_s pps_ktimer_info = {
+            name         : "ktimer",
+            path         : "",
+            mode         : PPS_CAPTUREASSERT|PPS_OFFSETASSERT|PPS_ECHOASSERT| \
+                           PPS_CANWAIT|PPS_TSFMT_TSPEC,
+            echo         : pps_ktimer_echo,
+    };
+
+and then calling the function pps_register_source() in your
+intialization routine as follow:
+
+    source = pps_register_source(&pps_ktimer_info,
+			PPS_CAPTUREASSERT|PPS_OFFSETASSERT,
+			-1 /* is up to the system */);
+
+The pps_register_source() prototype is:
+
+  int pps_register_source(struct pps_source_info_s *info, int default_params, int try_id)
+
+where "info" is a pointer to a structure that describes a particular
+PPS source, "default_params" tells the system what the initial default
+parameters for the device should be (is obvious that these parameters
+must be a subset of ones defined into the struct
+pps_source_info_s which describe the capabilities of the driver)
+and "try_id" can be used to force a particular ID for your device into
+the system (just use -1 if you wish the system chooses one for you).
+
+Once you have registered a new PPS source into the system you can
+signal an assert event (for example in the interrupt handler routine)
+just using:
+
+    pps_event(source, PPS_CAPTUREASSERT, ptr);
+
+The same function may also run the defined echo function
+(pps_ktimer_echo(), passing to it the "ptr" pointer) if the user
+asked for that... etc..
+
+Please see the file drivers/pps/clients/ktimer.c for an example code.
+
+
+SYSFS support
+-------------
+
+The SYSFS support is enabled by default if the SYSFS filesystem is
+enabled in the kernel and it provides a new class:
+
+   $ ls /sys/class/pps/
+   00/  01/  02/
+
+Every directory is the ID of a PPS sources defined into the system and
+inside you find several files:
+
+    $ ls /sys/class/pps/00/
+    assert	clear  echo  mode  name  path  subsystem@  uevent
+
+Inside each "assert" and "clear" file you can find the timestamp and a
+sequence number:
+
+    $ cat cat /sys/class/pps/00/assert
+    1170026870.983207967#8
+
+Where before the "#" is the timestamp in seconds and after it is the
+sequence number. Other files are:
+
+* echo: reports if the PPS source has an echo function or not;
+
+* mode: reports available PPS functioning modes;
+
+* name: reports the PPS source's name;
+
+* path: reports the PPS source's device path, that is the device the
+  PPS source is connected to (if it exists).

MAINTAINERS +7 -0

index b0fd71b..baa3901 100644
@@ -2693,6 +2693,13 @@ P:	Michal Ostrowski
 M:	[email protected]
 S:	Maintained
 
+PPS SUPPORT
+P:	Rodolfo Giometti
+M:	[email protected]
+W:	http://wiki.enneenne.com/index.php/LinuxPPS_support
+L:	[email protected]
+S:	Maintained
+
 PREEMPTIBLE KERNEL
 P:	Robert Love
 M:	[email protected]

drivers/Kconfig +2 -0

index 050323f..bb54cab 100644
@@ -52,6 +52,8 @@ source "drivers/i2c/Kconfig"
 
 source "drivers/spi/Kconfig"
 
+source "drivers/pps/Kconfig"
+
 source "drivers/w1/Kconfig"
 
 source "drivers/hwmon/Kconfig"

drivers/Makefile +1 -0

index 3a718f5..d0007f7 100644
@@ -59,6 +59,7 @@ obj-$(CONFIG_INPUT)		+= input/
 obj-$(CONFIG_I2O)		+= message/
 obj-$(CONFIG_RTC_LIB)		+= rtc/
 obj-$(CONFIG_I2C)		+= i2c/
+obj-$(CONFIG_PPS)		+= pps/
 obj-$(CONFIG_W1)		+= w1/
 obj-$(CONFIG_HWMON)		+= hwmon/
 obj-$(CONFIG_PHONE)		+= telephony/

drivers/char/lp.c +59 -0

77 lines not on this page · raw message

drivers/pps/Kconfig new file +34 -0

new file mode 100644
index 0000000..54297e8
@@ -0,0 +1,34 @@
+#
+# Character device configuration
+#
+
+menu "PPS support"
+
+config PPS
+	tristate "PPS support"
+	depends on EXPERIMENTAL
+	---help---
+	  PPS (Pulse Per Second) is a special pulse provided by some GPS
+	  antennas. Userland can use it to get an high time reference.
+
+	  Some antennas' PPS signals are connected with the CD (Carrier
+	  Detect) pin of the serial line they use to communicate with the
+	  host. In this case use the SERIAL_LINE client support.
+
+	  Some antennas' PPS signals are connected with some special host
+	  inputs so you have to enable the corresponding client support.
+
+	  This PPS support can also be built as a module.  If so, the module
+	  will be called pps-core.
+
+config PPS_DEBUG
+	bool "PPS debugging messages"
+	depends on PPS 
+	help
+	  Say Y here if you want the PPS support to produce a bunch of debug
+	  messages to the system log.  Select this if you are having a
+	  problem with PPS support and want to see more of what is going on.
+
+source drivers/pps/clients/Kconfig
+
+endmenu

drivers/pps/Makefile new file +7 -0

new file mode 100644
index 0000000..76101cd
@@ -0,0 +1,7 @@
+#
+# Makefile for the PPS core.
+#
+
+pps_core-objs			+= pps.o kapi.o sysfs.o
+obj-$(CONFIG_PPS)		+= pps_core.o
+obj-y				+= clients/

drivers/pps/clients/Kconfig new file +38 -0

new file mode 100644
index 0000000..1da1503
@@ -0,0 +1,38 @@
+#
+# LinuxPPS clients configuration
+#
+
+if PPS
+
+comment "PPS clients support"
+
+config PPS_CLIENT_KTIMER
+	tristate "Kernel timer client (Testing client, use for debug)"
+	help
+	  If you say yes here you get support for a PPS debugging client
+	  which uses a kernel timer to generate the PPS signal.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ktimer.o.
+
+comment "UART serial support (forced off)"
+	depends on ! ( SERIAL_CORE != n && ! ( PPS = m && SERIAL_CORE = y ) )
+
+config PPS_CLIENT_UART
+	bool "UART serial support"
+	depends on SERIAL_CORE != n && ! ( PPS = m && SERIAL_CORE = y )
+	help
+	  If you say yes here you get support for a PPS source connected
+	  with the CD (Carrier Detect) pin of your serial port.
+
+comment "Parallel printer support (forced off)"
+	depends on ! ( PRINTER != n && ! ( PPS = m && PRINTER = y ) )
+
+config PPS_CLIENT_LP
+	bool "Parallel printer support"
+	depends on PRINTER != n && ! ( PPS = m && PRINTER = y )
+	help
+	  If you say yes here you get support for a PPS source connected
+	  with the interrupt pin of your parallel port.
+
+endif

drivers/pps/clients/Makefile new file +6 -0

new file mode 100644
index 0000000..3ecca41
@@ -0,0 +1,6 @@
+#
+# Makefile for miscellaneous I2C chip drivers.
+#
+
+obj-$(CONFIG_PPS_CLIENT_KTIMER)	+= ktimer.o
+

drivers/pps/clients/ktimer.c new file +106 -0

106 lines not on this page · raw message

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

197 lines not on this page · raw message

drivers/pps/pps.c new file +373 -0

373 lines not on this page · raw message

drivers/pps/sysfs.c new file +209 -0

209 lines not on this page · raw message

drivers/serial/8250.c +3 -0

index 98ec861..543c7cb 100644
@@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
 		up->ier |= UART_IER_MSI;
 	if (up->capabilities & UART_CAP_UUE)
 		up->ier |= UART_IER_UUE | UART_IER_RTOIE;
+	if (up->port.flags & UPF_HARDPPS_CD)
+		up->ier |= UART_IER_MSI;	/* enable interrupts */
 
 	serial_out(up, UART_IER, up->ier);

@@ -2718,6 +2720,7 @@ void serial8250_unregister_port(int line)
 	struct uart_8250_port *uart = &serial8250_ports[line];
 
 	mutex_lock(&serial_mutex);
+
 	uart_remove_one_port(&serial8250_reg, &uart->port);
 	if (serial8250_isa_devs) {
 		uart->port.flags &= ~UPF_BOOT_AUTOCONF;

drivers/serial/serial_core.c +71 -1

106 lines not on this page · raw message

include/linux/netlink.h +1 -0

index 2a20f48..f8d77e6 100644
@@ -24,6 +24,7 @@
 /* leave room for NETLINK_DM (DM Events) */
 #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
 #define NETLINK_ECRYPTFS	19
+#define NETLINK_PPSAPI          20	/* linuxPPS support */
 
 #define MAX_LINKS 32

include/linux/parport.h +15 -0

index 80682aa..21cc9c4 100644
@@ -104,6 +104,10 @@ typedef enum {
 #include <asm/ptrace.h>
 #include <asm/semaphore.h>
 
+#ifdef CONFIG_PPS_CLIENT_LP
+#include <linux/pps.h>
+#endif
+
 /* Define this later. */
 struct parport;
 struct pardevice;

@@ -323,6 +327,11 @@ struct parport {
 
 	struct list_head full_list;
 	struct parport *slaves[3];
+
+#ifdef CONFIG_PPS_CLIENT_LP
+	struct pps_source_info_s pps_info;
+	int pps_source;		/* PPS source ID */
+#endif
 };
 
 #define DEFAULT_SPIN_TIME 500 /* us */

@@ -513,6 +522,12 @@ extern int parport_daisy_select (struct parport *port, int daisy, int mode);
 /* Lowlevel drivers _can_ call this support function to handle irqs.  */
 static __inline__ void parport_generic_irq(int irq, struct parport *port)
 {
+#ifdef CONFIG_PPS_CLIENT_LP
+	pps_event(port->pps_source, PPS_CAPTUREASSERT, port);
+	dbg("parport_pc: PPS assert event at %lu on source #%d",
+		jiffies, port->pps_source);
+#endif
+
 	parport_ieee1284_interrupt (irq, port);
 	read_lock(&port->cad_lock);
 	if (port->cad && port->cad->irq_func)

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

207 lines not on this page · raw message

include/linux/serial_core.h +28 -6

index 586aaba..aec0443 100644
@@ -145,6 +145,10 @@
 #include <linux/tty.h>
 #include <linux/mutex.h>
 
+#ifdef CONFIG_PPS_CLIENT_UART
+#include <linux/pps.h>
+#endif
+
 struct uart_port;
 struct uart_info;
 struct serial_struct;

@@ -223,6 +227,9 @@ struct uart_port {
 	unsigned char		regshift;		/* reg offset shift */
 	unsigned char		iotype;			/* io access style */
 	unsigned char		unused1;
+#ifdef CONFIG_PPS_CLIENT_UART
+	int			pps_source;		/* PPS source ID */
+#endif
 
 #define UPIO_PORT		(0)
 #define UPIO_HUB6		(1)

@@ -264,7 +271,8 @@ struct uart_port {
 #define UPF_IOREMAP		((__force upf_t) (1 << 31))
 
 #define UPF_CHANGE_MASK		((__force upf_t) (0x17fff))
-#define UPF_USR_MASK		((__force upf_t) (UPF_SPD_MASK|UPF_LOW_LATENCY))
+#define UPF_USR_MASK		((__force upf_t) (UPF_SPD_MASK|UPF_LOW_LATENCY\
+							|UPF_HARDPPS_CD))
 
 	unsigned int		mctrl;			/* current modem ctrl settings */
 	unsigned int		timeout;		/* character-based timeout */

@@ -295,6 +303,10 @@ struct uart_state {
 	struct uart_info	*info;
 	struct uart_port	*port;
 
+#ifdef CONFIG_PPS_CLIENT_UART
+        struct pps_source_info_s pps_info;
+#endif
+
 	struct mutex		mutex;
 };

@@ -459,13 +471,23 @@ uart_handle_dcd_change(struct uart_port *port, unsigned int status)
 {
 	struct uart_info *info = port->info;
 
-	port->icount.dcd++;
-
-#ifdef CONFIG_HARD_PPS
-	if ((port->flags & UPF_HARDPPS_CD) && status)
-		hardpps();
+#ifdef CONFIG_PPS_CLIENT_UART
+	if (port->flags & UPF_HARDPPS_CD) {
+		if (status) {
+			pps_event(port->pps_source, PPS_CAPTUREASSERT, port);
+			dbg("serial8250: PPS assert event at %lu on source #%d",
+				jiffies, port->pps_source);
+		}
+		else {
+			pps_event(port->pps_source, PPS_CAPTURECLEAR, port);
+			dbg("serial8250: PPS clear event at %lu on source #%d",
+				jiffies, port->pps_source);
+		}
+	}
 #endif
 
+	port->icount.dcd++;
+
 	if (info->flags & UIF_CHECK_CD) {
 		if (status)
 			wake_up_interruptible(&info->open_wait);

john stultz

2007-05-02 21:07 UTC #9 lore raw

Quoted 6 lines · Rodolfo Giometti

Pulse per Second (PPS) support for Linux.
Signed-off-by Rodolfo Giometti <[email protected]>
---

Quoted 7 lines

Here my last release of PPS support for Linux.
The difference against my last patch is about all userland specific code (timepps.h) which has been removed, I hope now you can consider adding it into kernel source tree!
Please, let me know if I still should fix something else.

Please inline your patch, rather then attaching them. It makes it very difficult to discuss when it is attached.

drivers/pps/clients/ktimer.c line 3

Quoted 9 lines

drivers/pps/clients/ktimer.c new file +3 -0

new file mode 100644
index 0000000..7514389
@@ -0,0 +1,106 @@
+/*
+ * ktimer.c -- kernel timer test client
+ *

Could you use a better name, like pps_ktimer_test.c, so it is less generic?

Same goes for your kabi.c and sysfs.c files.

drivers/serial/8250.c serial8250_set_termios(struct uart_port *port, struct ktermios *termios, · line 2009

Quoted 11 lines

drivers/serial/8250.c +2 -0

index 98ec861..543c7cb 100644
@@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
 		up->ier |= UART_IER_MSI;
 	if (up->capabilities & UART_CAP_UUE)
 		up->ier |= UART_IER_UUE | UART_IER_RTOIE;
+	if (up->port.flags & UPF_HARDPPS_CD)
+		up->ier |= UART_IER_MSI;	/* enable interrupts */

This isn't covered by a #ifdef, so is this always safe? Should it be in a separate patch?

Unfortunately I don't have any hardware to play with this, but I'd suggest you send this to Andrew Morton for inclusion into his tree for testing.

thanks
-john

Rodolfo Giometti

author maintainer

2007-05-03 10:03 UTC #10 lore raw

Quoted 3 lines · john stultz

Please inline your patch, rather then attaching them. It makes it very difficult to discuss when it is attached.

Ok.

Quoted 7 lines

drivers/pps/clients/ktimer.c line 3
Quoted 5 lines
+++ b/drivers/pps/clients/ktimer.c
drivers/pps/clients/ktimer.c new file +3 -0
@@ -0,0 +1,106 @@
+/*
+ * ktimer.c -- kernel timer test client
+ *
Could you use a better name, like pps_ktimer_test.c, so it is less generic?

Ok. But consider that this is just a testing program.

Quoted 1 line

Same goes for your kabi.c and sysfs.c files.

Why? These files don't generate .ko files.

Quoted 10 lines

drivers/serial/8250.c serial8250_set_termios(struct uart_port *port, struct ktermios *termios, · line 2009
Quoted 7 lines
drivers/serial/8250.c +2 -0
@@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
 		up->ier |= UART_IER_MSI;
 	if (up->capabilities & UART_CAP_UUE)
 		up->ier |= UART_IER_UUE | UART_IER_RTOIE;
+	if (up->port.flags & UPF_HARDPPS_CD)
+		up->ier |= UART_IER_MSI;	/* enable interrupts */
 
This isn't covered by a #ifdef, so is this always safe? Should it be in a separate patch?

No, this part is regarding the serial driver itself. Maybe it can be placed in a separate patch but it is still about the PPS support for Linux...

Quoted 3 lines

Unfortunately I don't have any hardware to play with this, but I'd suggest you send this to Andrew Morton for inclusion into his tree for testing.

I'll do it.

Thanks,

Rodolfo

Signature · 4 lines

GNU/Linux Solutions                  e-mail:    [email protected]
Linux Device Driver                             [email protected]
Embedded Systems                     		[email protected]
UNIX programming                     phone:     +39 349 2432127

Andrew Morton

2007-05-10 07:29 UTC #11 lore raw

In reply to Rodolfo Giometti

Quoted 1 line · On Wed, 2 May 2007 21:33:15 +0200 Rodolfo Giometti <[email protected]>…

Pulse per Second (PPS) support for Linux.

Have a patch:

From: Andrew Morton <[email protected]>

Review comments:

- Running a timer once per second will make the super-low-power people upset.

- This uses netlink? Is that interface documented anywhere?

Please check with Dave Miller that this:

	#define NETLINK_PPSAPI          20

reservation is OK.

- This:

	if ((nlpps->tsformat != PPS_TSFMT_TSPEC) != 0 ) {

is weird. I changed it to

	if (nlpps->tsformat != PPS_TSFMT_TSPEC) {

- This:

	timeout += nlpps->timeout.tv_nsec/(1000000000/HZ);

probably won't work on i386. We use do_div() for 64/32 divides. I'll find out when I compile it.

It's nice to use NSEC_PER_SEC rather than having to count all those zeroes.

- The code uses interruptible_sleep_on_timeout(). That API is deprecated and is racy. Please convert to wait_event_interruptible_timeout().

Ditto interruptible_sleep_on()

- This:

        memset(pps_source, 0, sizeof(struct pps_s) * PPS_MAX_SOURCES);

was unneeded. The C startup code already did that.

- All these separators:

+/* --- Input function ------------------------------------------------------ */

aren't typical for kernel code. I left them in, but please consider removing them all.

- This:

	static void pps_class_release(struct class_device *cdev)
	{
		/* Nop??? */
	}

is a bug and it earns you a nastygram from Greg. These objects must be dynamically allocated - this is not optional.

- What's this doing in 8250.c?

+ if (up->port.flags & UPF_HARDPPS_CD)
+ up->ier |= UART_IER_MSI; /* enable interrupts */

Please fully describe the reasons for this change in the changelog, and in a code comment and then get the change reviewed by Russell King <[email protected]>.

- Please document within the changelog the other changes to the serial code and we'll ask Russell to take a look at those as well.

- The Kconfig purports to support CONFIG_PPS=m. Does that actually work?

We have a bunch of code in random other drivers which is dependent upon CONFIG_PPS_CLIENT_foo. The problem is that if a kernel was compiled with CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that kernel, it won't actually work because lp, serial etc weren't correctly configured when _they_ were built.

This sort of cross-module coupling is considered to be a bad thing, but I'm not really sure it's all that important.

- Please split the patch up into a series of patches: one for pps core and one for each of the clients (servers?): one for lp, one for serial, etc.

Try to arrange for that series of patches to build and run at each stage of application.

Please don't lose my changes when you do so ;)

Please review the changes I made and a) stick to the same style and b) fix up any sites which I missed.

- Please remove all the typedefs:

+typedef struct ntp_fp {
+typedef union pps_timeu {
+typedef struct pps_info {
+typedef struct pps_params {

and just use `struct ntp_fp' everywhere.

- The above four structures are communicated with userspace, yes?

I believe that they will not work correctly when 32-bit userspace is communicating with a 64-bit kernel. Alignments change and sizeof(long) changes.

You don't want to have to write compat code. I suggest that you redo those structures in terms of __u32, __u64, etc. You probably need to use attribute((packed)) too, not sure.

Then let's get that part carefully reviewed (Arnd Bergmann <[email protected]> is my go-to guru on this) and please test it carefully.

Yeah, you just haven't got a chance that something as huge and as complex as struct pps_netlink_msg will survive the 32->64 transition.

- Please ensure that `make headers_check' passes OK (you'll hear from me if it doesn't ;))

- Can we get rid of the private dbg, err and info macros? Surely there are generic ones somewhere.

- struct pps_s has volatiles in it. Please remove them. There's lots of discussion on this topic on linux-kernel just today.

- Why did the

	port->icount.dcd++;

get moved in uart_handle_dcd_change()?

- In lots of places you do:

+#ifdef CONFIG_PPS_CLIENT_UART
+#include <linux/pps.h>
+#endif

Please remove the ifdefs at all these sites and make the header file handle it.

- It no longer compiles, because netlink_kernel_create now requires a `struct mutex *'. I stuck a NULL in there, which is permitted. But I don't know if that was a good thing - please check this.

Please also chase the net guys with a pointy stick for failing to document their damned APIs.

- Generally: code looks OK and is probably useful. Please keep going ;)

Code changes:

- fix docs a bit

- uninline lp_pps_echo(): we take its address.

- remove unneeded and undesirable casts of void*'s

- coding-style fixes

- various changes to the use of the timer API.

- Remove lots of inlinings. The compiler gets this right.

- DEFINE_SPINLOCK is required: SPIN_LOCK_UNLOCKED defeats lockdep.

Cc Rodolfo Giometti <[email protected]>

Cc john stultz <[email protected]>

Cc Roman Zippel <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 Documentation/pps.txt        |    4 -
 drivers/char/lp.c            |    7 +--
 drivers/pps/clients/Kconfig  |    8 +--
 drivers/pps/clients/ktimer.c |   19 +++-----
 drivers/pps/kapi.c           |   40 ++++++++---------
 drivers/pps/pps.c            |   75 +++++++++++----------------------
 drivers/pps/sysfs.c          |   14 +++---
 drivers/serial/8250.c        |    1 
 include/linux/pps.h          |    9 ++-
 9 files changed, 77 insertions(+), 100 deletions(-)

Documentation/pps.txt renamed from Documentation/pps.txt~linuxpps-pulse-per-second-support-for-linux-fix +2 -2

diff -puN Documentation/pps.txt~linuxpps-pulse-per-second-support-for-linux-fix Documentation/pps.txt
@@ -49,7 +49,7 @@ problem:
 
 This implies that the source has a /dev/... entry. This assumption is
 ok for the serial and parallel port, where you can do something
-usefull besides(!) the gathering of timestamps as it is the central
+useful besides(!) the gathering of timestamps as it is the central
 task for a PPS-API. But this assumption does not work for a single
 purpose GPIO line. In this case even basic file-related functionality
 (like read() and write()) makes no sense at all and should not be a

@@ -167,7 +167,7 @@ inside you find several files:
 Inside each "assert" and "clear" file you can find the timestamp and a
 sequence number:
 
-    $ cat cat /sys/class/pps/00/assert
+    $ cat /sys/class/pps/00/assert
     1170026870.983207967#8
 
 Where before the "#" is the timestamp in seconds and after it is the

drivers/char/lp.c renamed from drivers/char/lp.c~linuxpps-pulse-per-second-support-for-linux-fix +3 -4

diff -puN drivers/char/lp.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/char/lp.c
@@ -750,9 +750,9 @@ static struct console lpcons = {
 
 #ifdef CONFIG_PPS_CLIENT_LP
 
-static inline void lp_pps_echo(int source, int event, void *data)
+static void lp_pps_echo(int source, int event, void *data)
 {
-	struct parport *port = (struct parport *) data;
+	struct parport *port = data;
 	unsigned char status = parport_read_status(port);
 
 	/* echo event via SEL bit */

@@ -860,8 +860,7 @@ static int lp_register(int nr, struct pa
 		else
 			info("PPS source #%d \"%s\" added to the system",
 					port->pps_source, port->pps_info.path);
-	}
-	else {
+	} else {
 		port->pps_source = -1;
 		err("PPS support disabled due port \"%s\" is in polling mode",
 			port->pps_info.path);

drivers/pps/clients/Kconfig renamed from drivers/pps/clients/Kconfig~linuxpps-pulse-per-second-support-for-linux-fix +4 -4

diff -puN drivers/pps/clients/Kconfig~linuxpps-pulse-per-second-support-for-linux-fix drivers/pps/clients/Kconfig
@@ -16,21 +16,21 @@ config PPS_CLIENT_KTIMER
 	  will be called ktimer.o.
 
 comment "UART serial support (forced off)"
-	depends on ! ( SERIAL_CORE != n && ! ( PPS = m && SERIAL_CORE = y ) )
+	depends on ! (SERIAL_CORE != n && !(PPS = m && SERIAL_CORE = y))
 
 config PPS_CLIENT_UART
 	bool "UART serial support"
-	depends on SERIAL_CORE != n && ! ( PPS = m && SERIAL_CORE = y )
+	depends on SERIAL_CORE != n && !(PPS = m && SERIAL_CORE = y)
 	help
 	  If you say yes here you get support for a PPS source connected
 	  with the CD (Carrier Detect) pin of your serial port.
 
 comment "Parallel printer support (forced off)"
-	depends on ! ( PRINTER != n && ! ( PPS = m && PRINTER = y ) )
+	depends on !( PRINTER != n && !(PPS = m && PRINTER = y))
 
 config PPS_CLIENT_LP
 	bool "Parallel printer support"
-	depends on PRINTER != n && ! ( PPS = m && PRINTER = y )
+	depends on PRINTER != n && !(PPS = m && PRINTER = y)
 	help
 	  If you say yes here you get support for a PPS source connected
 	  with the interrupt pin of your parallel port.

drivers/pps/clients/ktimer.c renamed from drivers/pps/clients/ktimer.c~linuxpps-pulse-per-second-support-for-linux-fix +8 ~~-11~~

diff -puN drivers/pps/clients/ktimer.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/pps/clients/ktimer.c
@@ -35,14 +35,13 @@ static struct timer_list ktimer;
 
 /* --- The kernel timer ----------------------------------------------------- */
 
-static void pps_ktimer_event(unsigned long ptr) {
+static void pps_ktimer_event(unsigned long ptr)
+{
 	info("PPS event at %lu", jiffies);
 
 	pps_event(source, PPS_CAPTUREASSERT, NULL);
 
-	/* Rescheduling */
-	ktimer.expires = jiffies+HZ;   /* 1 second */
-	add_timer(&ktimer);
+	mod_timer(&ktimer, jiffies + HZ);
 }
 
 /* --- The echo function ---------------------------------------------------- */

@@ -50,9 +49,9 @@ static void pps_ktimer_event(unsigned lo
 static void pps_ktimer_echo(int source, int event, void *data)
 {
 	info("echo %s %s for source %d",
-	 event&PPS_CAPTUREASSERT ? "assert" : "",
-	 event&PPS_CAPTURECLEAR ? "clear" : "",
-	 source);
+		event & PPS_CAPTUREASSERT ? "assert" : "",
+		event & PPS_CAPTURECLEAR ? "clear" : "",
+		source);
 }
 
 /* --- The PPS info struct -------------------------------------------------- */

@@ -88,10 +87,8 @@ static int __init pps_ktimer_init(void)
 	}
 	source = ret;
 
-	init_timer(&ktimer);
-	ktimer.function = pps_ktimer_event;
-	ktimer.expires = jiffies+HZ;   /* 1 second */
-	add_timer(&ktimer);
+	setup_timer(&ktimer, pps_ktimer_event, 0);
+	mod_timer(&ktimer, jiffies + HZ);
 
 	info("ktimer PPS source registered at %d", source);

drivers/pps/kapi.c renamed from drivers/pps/kapi.c~linuxpps-pulse-per-second-support-for-linux-fix +20 ~~-20~~

diff -puN drivers/pps/kapi.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/pps/kapi.c
@@ -24,7 +24,6 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/time.h>
-
 #include <linux/pps.h>
 
 /* --- Local functions ----------------------------------------------------- */

@@ -44,7 +43,8 @@ static void pps_add_offset(struct timesp
 
 /* --- Exported functions -------------------------------------------------- */
 
-static inline int __pps_register_source(struct pps_source_info_s *info, int default_params, int try_id)
+static int __pps_register_source(struct pps_source_info_s *info,
+				int default_params, int try_id)
 {
 	int i;

@@ -54,8 +54,7 @@ static inline int __pps_register_source(
 			return -EBUSY;
 		}
 		i = try_id;
-	}
-	else {
+	} else {
 		for (i = 0; i < PPS_MAX_SOURCES; i++)
 			if (!pps_is_allocated(i))
 				break;

@@ -66,15 +65,16 @@ static inline int __pps_register_source(
 	}
 
 	/* Sanity checks */
-	if ((info->mode&default_params) != default_params) {
+	if ((info->mode & default_params) != default_params) {
 		err("unsupported default parameters");
 		return -EINVAL;
 	}
-	if ((info->mode&(PPS_ECHOASSERT|PPS_ECHOCLEAR)) != 0 && info->echo == NULL) {
+	if ((info->mode & (PPS_ECHOASSERT|PPS_ECHOCLEAR)) != 0 &&
+			info->echo == NULL) {
 		err("echo function is not defined");
 		return -EINVAL;
 	}
-	if ((info->mode&(PPS_TSFMT_TSPEC|PPS_TSFMT_NTPFP)) == 0) {
+	if ((info->mode & (PPS_TSFMT_TSPEC|PPS_TSFMT_NTPFP)) == 0) {
 		err("unspecified time format");
 		return -EINVAL;
 	}

@@ -89,7 +89,8 @@ static inline int __pps_register_source(
 	return i;
 }
 
-int pps_register_source(struct pps_source_info_s *info, int default_params, int try_id)
+int pps_register_source(struct pps_source_info_s *info, int default_params,
+			int try_id)
 {
 	unsigned long flags;
 	int i, ret;

@@ -108,8 +109,9 @@ int pps_register_source(struct pps_sourc
 
 	return i;
 }
+EXPORT_SYMBOL(pps_register_source);
 
-static inline void __pps_unregister_source(struct pps_source_info_s *info)
+static void __pps_unregister_source(struct pps_source_info_s *info)
 {
 	int i;

@@ -136,6 +138,7 @@ void pps_unregister_source(struct pps_so
 	__pps_unregister_source(info);
 	spin_unlock_irqrestore(&pps_lock, flags);
 }
+EXPORT_SYMBOL(pps_unregister_source);
 
 void pps_event(int source, int event, void *data)
 {

@@ -153,21 +156,22 @@ void pps_event(int source, int event, vo
 		err("unknow source for event!");
 		return;
 	}
-	if ((event&(PPS_CAPTUREASSERT|PPS_CAPTURECLEAR)) == 0 ) {
+	if ((event & (PPS_CAPTUREASSERT|PPS_CAPTURECLEAR)) == 0 ) {
 		err("unknow event (%x) for source %d", event, source);
 		return;
 	}
 
 	/* Must call the echo function? */
-	if ((pps_source[source].params.mode&(PPS_ECHOASSERT|PPS_ECHOCLEAR)) != 0)
+	if ((pps_source[source].params.mode & (PPS_ECHOASSERT|PPS_ECHOCLEAR)) != 0)
 		pps_source[source].info->echo(source, event, data);
 
 	/* Check the event */
 	pps_source[source].current_mode = pps_source[source].params.mode;
-	if (event&PPS_CAPTUREASSERT) {
+	if (event & PPS_CAPTUREASSERT) {
 		/* We have to add an offset? */
 		if (pps_source[source].params.mode&PPS_OFFSETASSERT)
-			pps_add_offset(&ts, &pps_source[source].params.assert_off_tu.tspec);
+			pps_add_offset(&ts,
+				&pps_source[source].params.assert_off_tu.tspec);
 
 		/* Save the time stamp */
 		pps_source[source].assert_tu.tspec = ts;

@@ -175,10 +179,11 @@ void pps_event(int source, int event, vo
 		dbg("capture assert seq #%lu for source %d",
 			pps_source[source].assert_sequence, source);
 	}
-	if (event&PPS_CAPTURECLEAR) {
+	if (event & PPS_CAPTURECLEAR) {
 		/* We have to add an offset? */
 		if (pps_source[source].params.mode&PPS_OFFSETCLEAR)
-			pps_add_offset(&ts, &pps_source[source].params.clear_off_tu.tspec);
+			pps_add_offset(&ts,
+				&pps_source[source].params.clear_off_tu.tspec);
 
 		/* Save the time stamp */
 		pps_source[source].clear_tu.tspec = ts;

@@ -189,9 +194,4 @@ void pps_event(int source, int event, vo
 
 	wake_up_interruptible(&pps_source[source].queue);
 }
-
-/* --- Exported functions -------------------------------------------------- */
-
-EXPORT_SYMBOL(pps_register_source);
-EXPORT_SYMBOL(pps_unregister_source);
 EXPORT_SYMBOL(pps_event);

drivers/pps/pps.c renamed from drivers/pps/pps.c~linuxpps-pulse-per-second-support-for-linux-fix +26 ~~-49~~

232 lines not on this page · raw message

drivers/pps/sysfs.c renamed from drivers/pps/sysfs.c~linuxpps-pulse-per-second-support-for-linux-fix +8 -6

diff -puN drivers/pps/sysfs.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/pps/sysfs.c
@@ -112,7 +112,8 @@ static struct class pps_class = {
 
 /* ----- Public functions --------------------------------------------- */
 
-void pps_sysfs_remove_source_entry(struct pps_source_info_s *info) {
+void pps_sysfs_remove_source_entry(struct pps_source_info_s *info)
+{
 	int i;
 
 	/* Sanity checks */

@@ -136,7 +137,8 @@ void pps_sysfs_remove_source_entry(struc
 	class_device_unregister(&info->class_dev);
 }
 
-int pps_sysfs_create_source_entry(struct pps_source_info_s *info, int id) {
+int pps_sysfs_create_source_entry(struct pps_source_info_s *info, int id)
+{
 	char buf[32];
 	int i, ret;

@@ -161,14 +163,14 @@ int pps_sysfs_create_source_entry(struct
 	/* Create info files */
 
 	/* Create file "assert" and "clear" according to source capability */
-	if (info->mode&PPS_CAPTUREASSERT) {
+	if (info->mode & PPS_CAPTUREASSERT) {
 		ret = class_device_create_file(&info->class_dev,
 					&pps_class_device_attributes[0]);
 		i = 0;
 		if (unlikely(ret))
 			goto error_class_device_create_file;
 	}
-	if (info->mode&PPS_CAPTURECLEAR) {
+	if (info->mode & PPS_CAPTURECLEAR) {
 		ret = class_device_create_file(&info->class_dev,
 					&pps_class_device_attributes[1]);
 		i = 1;

@@ -185,7 +187,7 @@ int pps_sysfs_create_source_entry(struct
 
 	return 0;
 
-error_class_device_create_file :
+error_class_device_create_file:
 	while (--i >= 0)
 		class_device_remove_file(&info->class_dev,
 					&pps_class_device_attributes[i]);

@@ -193,7 +195,7 @@ error_class_device_create_file :
 	class_device_unregister(&info->class_dev);
 	/* Here the  release() method was already called */
 
-error_class_device_register :
+error_class_device_register:
 
 	return ret;
 }

drivers/serial/8250.c renamed from drivers/serial/8250.c~linuxpps-pulse-per-second-support-for-linux-fix +0 -1

diff -puN drivers/serial/8250.c~linuxpps-pulse-per-second-support-for-linux-fix drivers/serial/8250.c
@@ -2820,7 +2820,6 @@ void serial8250_unregister_port(int line
 	struct uart_8250_port *uart = &serial8250_ports[line];
 
 	mutex_lock(&serial_mutex);
-
 	uart_remove_one_port(&serial8250_reg, &uart->port);
 	if (serial8250_isa_devs) {
 		uart->port.flags &= ~UPF_BOOT_AUTOCONF;

include/linux/pps.h renamed from include/linux/pps.h~linuxpps-pulse-per-second-support-for-linux-fix +6 -3

diff -puN include/linux/pps.h~linuxpps-pulse-per-second-support-for-linux-fix include/linux/pps.h
@@ -185,7 +185,8 @@ extern spinlock_t pps_lock;
 
 /* --- Global functions ---------------------------------------------------- */
 
-static inline int pps_is_allocated(int source) {
+static inline int pps_is_allocated(int source)
+{
 	return pps_source[source].info != NULL;
 }

@@ -193,11 +194,13 @@ static inline int pps_is_allocated(int s
 
 /* --- Exported functions -------------------------------------------------- */
 
-extern int pps_register_source(struct pps_source_info_s *info, int default_params, int try_id);
+extern int pps_register_source(struct pps_source_info_s *info,
+				int default_params, int try_id);
 extern void pps_unregister_source(struct pps_source_info_s *info);
 extern void pps_event(int source, int event, void *data);
 
-extern int pps_sysfs_create_source_entry(struct pps_source_info_s *info, int id);
+extern int pps_sysfs_create_source_entry(struct pps_source_info_s *info,
+				int id);
 extern void pps_sysfs_remove_source_entry(struct pps_source_info_s *info);
 extern int pps_sysfs_register(void);
 extern void pps_sysfs_unregister(void);

Andrew Morton

2007-05-10 09:50 UTC #12 lore raw

Quoted 2 lines · On Thu, 10 May 2007 00:27:40 -0700 Andrew Morton <[email protected]>…

- Can we get rid of the private dbg, err and info macros? Surely there are generic ones somewhere.

i386 allmodconfig:

In file included from drivers/usb/misc/uss720.c:48:
include/linux/usb.h:1505:1: warning: "dbg" redefined In file included from include/linux/parport.h:108,

                 from drivers/usb/misc/uss720.c:46:

include/linux/pps.h:139:1: warning: this is the location of the previous definition In file included from drivers/usb/misc/uss720.c:48:
include/linux/usb.h:1511:1: warning: "err" redefined
In file included from include/linux/parport.h:108,

                 from drivers/usb/misc/uss720.c:46:

include/linux/pps.h:145:1: warning: this is the location of the previous definition In file included from drivers/usb/misc/uss720.c:48:
include/linux/usb.h:1513:1: warning: "info" redefined
In file included from include/linux/parport.h:108,

                 from drivers/usb/misc/uss720.c:46:

include/linux/pps.h:147:1: warning: this is the location of the previous definition

Rodolfo Giometti

author maintainer

2007-05-10 10:59 UTC #13 lore raw

In reply to Andrew Morton

Quoted 6 lines · Andrew Morton

Please check with Dave Miller that this:
	#define NETLINK_PPSAPI          20
reservation is OK.

Hello, as you can see here Andrew Morton asked to me to check with you about NETLINK_PPSAPI reservation.

Please, let me know if it's ok.

Thanks,

Rodolfo

Signature · 4 lines

GNU/Linux Solutions                  e-mail:    [email protected]
Linux Device Driver                             [email protected]
Embedded Systems                     		[email protected]
UNIX programming                     phone:     +39 349 2432127

David Miller

maintainer

2007-05-10 11:01 UTC #14 lore raw

From: Rodolfo Giometti <[email protected]>
Date: Thu, 10 May 2007 12:58:37 +0200

Quoted 12 lines · Andrew Morton

Quoted 6 lines
Please check with Dave Miller that this:
	#define NETLINK_PPSAPI          20
reservation is OK.
Hello, as you can see here Andrew Morton asked to me to check with you about NETLINK_PPSAPI reservation.
Please, let me know if it's ok.

It's not OK, please use the generic netlink interface and as such you will not need to allocate any numbers at all.

Documentation/networking/generic_netlink.txt gives a link to some infomration on this topic.

Rodolfo Giometti

author maintainer

2007-05-10 11:46 UTC #15 lore raw

Quoted 6 lines · David Miller

It's not OK, please use the generic netlink interface and as such you will not need to allocate any numbers at all.
Documentation/networking/generic_netlink.txt gives a link to some infomration on this topic.

If I well understand doing like this means that I have to modify also userland API, is that true?

I know that you are forcing in using this new interface for new kernel projects, but if I have to change my code I need also change NTPD related code and this is frustrating. I have to interact with both kernel developers and NTPD ones... :)

It could be acceptable let me use id "20" (or just another number) to allow Andrew Morton and other LinuxPPS users to test this new support? Please, consider that this is not a new project, it was developed since 2005 when this new interface was not available.

Thanks for your attention,

Rodolfo

Signature · 4 lines

GNU/Linux Solutions                  e-mail:    [email protected]
Linux Device Driver                             [email protected]
Embedded Systems                     		[email protected]
UNIX programming                     phone:     +39 349 2432127

David Miller

maintainer

2007-05-10 11:51 UTC #16 lore raw

From: Rodolfo Giometti <[email protected]>
Date: Thu, 10 May 2007 13:45:03 +0200

Quoted 10 lines · David Miller

Quoted 6 lines
It's not OK, please use the generic netlink interface and as such you will not need to allocate any numbers at all.
Documentation/networking/generic_netlink.txt gives a link to some infomration on this topic.
If I well understand doing like this means that I have to modify also userland API, is that true?

Yes.

Quoted 9 lines

I know that you are forcing in using this new interface for new kernel projects, but if I have to change my code I need also change NTPD related code and this is frustrating. I have to interact with both kernel developers and NTPD ones... :)
It could be acceptable let me use id "20" (or just another number) to allow Andrew Morton and other LinuxPPS users to test this new support? Please, consider that this is not a new project, it was developed since 2005 when this new interface was not available.

Being a 2005 project means only that you've been out of tree for nearly 2 years.

Sorry, we are not allocating a netlink IDs for folks, and we're doing it exactly because generic netlink avoids all the fixed numbering API issues.

David Miller

maintainer

2007-05-10 11:54 UTC #17 lore raw

BTW, please remove the linuxpps list from the CC: for future postings in this thread, it bounces every one of my emails back because it only allows postings from subscribers.

Thanks a lot.

Rodolfo Giometti

author maintainer

2007-05-11 00:30 UTC #18 lore raw

Info about the new netlink layer userland API

In reply to David Miller

Quoted 6 lines · David Miller

It's not OK, please use the generic netlink interface and as such you will not need to allocate any numbers at all.
Documentation/networking/generic_netlink.txt gives a link to some infomration on this topic.

Where can I find some infos about userland programming _without_ using the libnl library?

There are something similar to the magic command:

   ret = socket(PF_NETLINK, SOCK_RAW, NETLINK_PPSAPI);

in this new netlink API?

Thanks for your help,

Rodolfo

Signature · 4 lines

GNU/Linux Solutions                  e-mail:    [email protected]
Linux Device Driver                             [email protected]
Embedded Systems                     		[email protected]
UNIX programming                     phone:     +39 349 2432127

Rodolfo Giometti

author maintainer

2007-05-12 06:00 UTC #19 lore raw

In reply to Andrew Morton

Hello,

here my new patch with a lot of fixes.

The only issue not still fixed is the one related with:

        #define NETLINK_PPSAPI          20

I need time to resolve it.

Follows my comments and then the patch, hope now I can came back into -mm tree again! :)

Quoted 4 lines · On Thu, May 10, 2007 at 12:27:52AM -0700, [email protected] wrote:

Review comments:
- Running a timer once per second will make the super-low-power people upset.

The ktimer modules is just for debugging pourpose and it's not needed into real working system.

Quoted 7 lines

- This uses netlink? Is that interface documented anywhere?
Please check with Dave Miller that this:
      #define NETLINK_PPSAPI          20
reservation is OK.

Is not ok. To be fixed.

Quoted 7 lines

- This:

      if ((nlpps->tsformat != PPS_TSFMT_TSPEC) != 0 ) {

is weird. I changed it to

      if (nlpps->tsformat != PPS_TSFMT_TSPEC) {

Fixed.

Quoted 9 lines

- This:
      timeout += nlpps->timeout.tv_nsec/(1000000000/HZ);
probably won't work on i386. We use do_div() for 64/32 divides. I'll find out when I compile it.
It's nice to use NSEC_PER_SEC rather than having to count all those zeroes.

Fixed.

Quoted 4 lines

- The code uses interruptible_sleep_on_timeout(). That API is deprecated and is racy. Please convert to wait_event_interruptible_timeout().
Ditto interruptible_sleep_on()

Fixed.

Quoted 5 lines

- This:
        memset(pps_source, 0, sizeof(struct pps_s) * PPS_MAX_SOURCES);
was unneeded. The C startup code already did that.

Fixed.

Quoted 3 lines

- All these separators:
+/* --- Input function ------------------------------------------------------

+*/

Quoted 3 lines

aren't typical for kernel code. I left them in, but please consider removing them all.

Fixed.

Quoted 9 lines

- This:
      static void pps_class_release(struct class_device *cdev)
      {
              /* Nop??? */
      }
is a bug and it earns you a nastygram from Greg. These objects must be dynamically allocated - this is not optional.

It could be acceptable defining this function as void?

Review

Quoted 8 lines

- What's this doing in 8250.c?

(quoted diff) +2 -0

+     if (up->port.flags & UPF_HARDPPS_CD)
+             up->ier |= UART_IER_MSI;        /* enable interrupts */
 
  Please fully describe the reasons for this change in the changelog, and in
  a code comment and then get the change reviewed by Russell King
  <[email protected]>.

If user specify a serial port as PPS source we enable IRQ on that port.

Quoted 2 lines

- Please document within the changelog the other changes to the serial code and we'll ask Russell to take a look at those as well.

OK. I'll do it.

Quoted 1 line

- The Kconfig purports to support CONFIG_PPS=m. Does that actually work?

Yes. It works...

Quoted 22 lines

We have a bunch of code in random other drivers which is dependent upon CONFIG_PPS_CLIENT_foo. The problem is that if a kernel was compiled with CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that kernel, it won't actually work because lp, serial etc weren't correctly configured when _they_ were built.
This sort of cross-module coupling is considered to be a bad thing, but I'm not really sure it's all that important.
- Please split the patch up into a series of patches: one for pps core and one for each of the clients (servers?): one for lp, one for serial, etc.
Try to arrange for that series of patches to build and run at each stage of application.
Please don't lose my changes when you do so ;)
Please review the changes I made and a) stick to the same style and b) fix up any sites which I missed.
- Please remove all the typedefs:

Review

Quoted 6 lines

(quoted diff) +4 -0

+typedef struct ntp_fp {
+typedef union pps_timeu {
+typedef struct pps_info {
+typedef struct pps_params {
 
  and just use `struct ntp_fp' everywhere.

Those typedefs are defined in PPS specifications (please, see RFC 2783).

Quoted 10 lines

- The above four structures are communicated with userspace, yes?
I believe that they will not work correctly when 32-bit userspace is communicating with a 64-bit kernel. Alignments change and sizeof(long) changes.
You don't want to have to write compat code. I suggest that you redo those structures in terms of __u32, __u64, etc. You probably need to use attribute((packed)) too, not sure.

Quoted 5 lines

Then let's get that part carefully reviewed (Arnd Bergmann <[email protected]> is my go-to guru on this) and please test it carefully.
Yeah, you just haven't got a chance that something as huge and as complex as struct pps_netlink_msg will survive the 32->64 transition.

The same as above. These structure are fixed by RFC 2783.

Quoted 2 lines

- Please ensure that `make headers_check' passes OK (you'll hear from me if it doesn't ;))

Done.

Quoted 2 lines

- Can we get rid of the private dbg, err and info macros? Surely there are generic ones somewhere.

They are very useful to LinuxPPS users who can enable/disable them by configuration menu.

Also I'm planning to add a dinamic enable/disable mechanism...

Quoted 2 lines

- struct pps_s has volatiles in it. Please remove them. There's lots of discussion on this topic on linux-kernel just today.

Fixed.

Quoted 5 lines

- Why did the
      port->icount.dcd++;
get moved in uart_handle_dcd_change()?

That's why we have to register the PPS interrupt as soon as possible! Even few CPU instructions dalay may result in a bad time settings.

Review

Quoted 8 lines

- In lots of places you do:

(quoted diff) +3 -0

+#ifdef CONFIG_PPS_CLIENT_UART
+#include <linux/pps.h>
+#endif
 
  Please remove the ifdefs at all these sites and make the header file
  handle it.

Fixed.

Quoted 5 lines

- It no longer compiles, because netlink_kernel_create now requires a
`struct mutex *'. I stuck a NULL in there, which is permitted. But I don't> know if that was a good thing - please check this.
Please also chase the net guys with a pointy stick for failing to document their damned APIs.

This should vanish when new netlink layer will be used.

Quoted 1 line

- Generally: code looks OK and is probably useful. Please keep going ;)

Hope I forgot nothing!

Ciao,

Rodolfo

----

Documentation/pps.txt new file +183 -0

new file mode 100644
index 0000000..f07b098
@@ -0,0 +1,183 @@
+
+			PPS - Pulse Per Second
+			----------------------
+
+(C) Copyright 2007 Rodolfo Giometti <[email protected]>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+
+
+Overview
+--------
+
+LinuxPPS provides a programming interface (API) to define into the
+system several PPS sources.
+
+PPS means "pulse per second" and a PPS source is just a device which
+provides a high precision signal each second so that an application
+can use it to adjust system clock time.
+
+A PPS source can be connected to a serial port (usually to the Data
+Carrier Detect pin) or to a parallel port (ACK-pin) or to a special
+CPU's GPIOs (this is the common case in embedded systems) but in each
+case when a new pulse comes the system must apply to it a timestamp
+and record it for the userland.
+
+Common use is the combination of the NTPD as userland program with a
+GPS receiver as PPS source to obtain a wallclock-time with
+sub-millisecond synchronisation to UTC.
+
+
+RFC considerations
+------------------
+
+While implementing a PPS API as RFC 2783 defines and using an embedded
+CPU GPIO-Pin as physical link to the signal, I encountered a deeper
+problem:
+
+   At startup it needs a file descriptor as argument for the function
+   time_pps_create().
+
+This implies that the source has a /dev/... entry. This assumption is
+ok for the serial and parallel port, where you can do something
+useful besides(!) the gathering of timestamps as it is the central
+task for a PPS-API. But this assumption does not work for a single
+purpose GPIO line. In this case even basic file-related functionality
+(like read() and write()) makes no sense at all and should not be a
+precondition for the use of a PPS-API.
+
+The problem can be simply solved if you change the original RFC 2783:
+
+    pps_handle_t type is an opaque __scalar type__ used to represent a
+    PPS source within the API
+
+into a modified:
+
+   pps_handle_t type is an opaque __variable__ used to represent a PPS
+   source within the API and programs should not access it directly to
+   it due to its opacity.
+
+This change seems to be neglibile because even the original RFC 2783
+does not encourage programs to check (read: use) the pps_handle_t
+variable before calling the time_pps_*() functions, since each
+function should do this job internally.
+
+If I intentionally separate the concept of "file descriptor" from the
+concept of the "PPS source" I'm obliged to provide a solution to find
+and register a PPS-source without using a file descriptor: it's done
+by the functions time_pps_findsource() and time_pps_findpath() now.
+
+According to this current NTPD drivers' code should be modified as
+follows:
+
++#ifdef PPS_HAVE_FINDPATH
++      /* Get the PPS source's real name */
++      fd = readlink(link, path, STRING_LEN-1);
++      if (fd <= 0)
++              strncpy(path, link, STRING_LEN);
++      else
++              path[fd] = '\0';
++
++      /* Try to find the source */
++      fd = time_pps_findpath(path, STRING_LEN, id, STRING_LEN);
++      if (fd < 0) {
++              msyslog(LOG_ERR, "refclock: cannot find PPS source \"%s\" in the system", path);
++              return 1;
++      }
++      msyslog(LOG_INFO, "refclock: found PPS source #%d \"%s\" on \"%s\"", fd, path, id);
++#endif   /* PPS_HAVE_FINDPATH */
++
++
+       if (time_pps_create(fd, &pps_handle) < 0) {
+-              pps_handle = 0;
+               msyslog(LOG_ERR, "refclock: time_pps_create failed: %m");
+       }
+
+
+Coding example
+--------------
+
+To register a PPS source into the kernel you should define a struct
+pps_source_info_s as follow:
+
+    static struct pps_source_info_s pps_ktimer_info = {
+            name         : "ktimer",
+            path         : "",
+            mode         : PPS_CAPTUREASSERT|PPS_OFFSETASSERT|PPS_ECHOASSERT| \
+                           PPS_CANWAIT|PPS_TSFMT_TSPEC,
+            echo         : pps_ktimer_echo,
+    };
+
+and then calling the function pps_register_source() in your
+intialization routine as follow:
+
+    source = pps_register_source(&pps_ktimer_info,
+			PPS_CAPTUREASSERT|PPS_OFFSETASSERT,
+			-1 /* is up to the system */);
+
+The pps_register_source() prototype is:
+
+  int pps_register_source(struct pps_source_info_s *info, int default_params, int try_id)
+
+where "info" is a pointer to a structure that describes a particular
+PPS source, "default_params" tells the system what the initial default
+parameters for the device should be (is obvious that these parameters
+must be a subset of ones defined into the struct
+pps_source_info_s which describe the capabilities of the driver)
+and "try_id" can be used to force a particular ID for your device into
+the system (just use -1 if you wish the system chooses one for you).
+
+Once you have registered a new PPS source into the system you can
+signal an assert event (for example in the interrupt handler routine)
+just using:
+
+    pps_event(source, PPS_CAPTUREASSERT, ptr);
+
+The same function may also run the defined echo function
+(pps_ktimer_echo(), passing to it the "ptr" pointer) if the user
+asked for that... etc..
+
+Please see the file drivers/pps/clients/ktimer.c for an example code.
+
+
+SYSFS support
+-------------
+
+The SYSFS support is enabled by default if the SYSFS filesystem is
+enabled in the kernel and it provides a new class:
+
+   $ ls /sys/class/pps/
+   00/  01/  02/
+
+Every directory is the ID of a PPS sources defined into the system and
+inside you find several files:
+
+    $ ls /sys/class/pps/00/
+    assert	clear  echo  mode  name  path  subsystem@  uevent
+
+Inside each "assert" and "clear" file you can find the timestamp and a
+sequence number:
+
+    $ cat /sys/class/pps/00/assert
+    1170026870.983207967#8
+
+Where before the "#" is the timestamp in seconds and after it is the
+sequence number. Other files are:
+
+* echo: reports if the PPS source has an echo function or not;
+
+* mode: reports available PPS functioning modes;
+
+* name: reports the PPS source's name;
+
+* path: reports the PPS source's device path, that is the device the
+  PPS source is connected to (if it exists).

MAINTAINERS +7 -0

index cfd26dd..dd6784d 100644
@@ -2860,6 +2860,13 @@ P:	Michal Ostrowski
 M:	[email protected]
 S:	Maintained
 
+PPS SUPPORT
+P:	Rodolfo Giometti
+M:	[email protected]
+W:	http://wiki.enneenne.com/index.php/LinuxPPS_support
+L:	[email protected]
+S:	Maintained
+
 PREEMPTIBLE KERNEL
 P:	Robert Love
 M:	[email protected]

drivers/Kconfig +2 -0

index 050323f..bb54cab 100644
@@ -52,6 +52,8 @@ source "drivers/i2c/Kconfig"
 
 source "drivers/spi/Kconfig"
 
+source "drivers/pps/Kconfig"
+
 source "drivers/w1/Kconfig"
 
 source "drivers/hwmon/Kconfig"

drivers/Makefile +1 -0

index 26ca903..97dc041 100644
@@ -59,6 +59,7 @@ obj-$(CONFIG_INPUT)		+= input/
 obj-$(CONFIG_I2O)		+= message/
 obj-$(CONFIG_RTC_LIB)		+= rtc/
 obj-y				+= i2c/
+obj-$(CONFIG_PPS)		+= pps/
 obj-$(CONFIG_W1)		+= w1/
 obj-$(CONFIG_HWMON)		+= hwmon/
 obj-$(CONFIG_PHONE)		+= telephony/

drivers/char/lp.c +58 -0

76 lines not on this page · raw message

drivers/pps/Kconfig new file +34 -0

new file mode 100644
index 0000000..54297e8
@@ -0,0 +1,34 @@
+#
+# Character device configuration
+#
+
+menu "PPS support"
+
+config PPS
+	tristate "PPS support"
+	depends on EXPERIMENTAL
+	---help---
+	  PPS (Pulse Per Second) is a special pulse provided by some GPS
+	  antennas. Userland can use it to get an high time reference.
+
+	  Some antennas' PPS signals are connected with the CD (Carrier
+	  Detect) pin of the serial line they use to communicate with the
+	  host. In this case use the SERIAL_LINE client support.
+
+	  Some antennas' PPS signals are connected with some special host
+	  inputs so you have to enable the corresponding client support.
+
+	  This PPS support can also be built as a module.  If so, the module
+	  will be called pps-core.
+
+config PPS_DEBUG
+	bool "PPS debugging messages"
+	depends on PPS 
+	help
+	  Say Y here if you want the PPS support to produce a bunch of debug
+	  messages to the system log.  Select this if you are having a
+	  problem with PPS support and want to see more of what is going on.
+
+source drivers/pps/clients/Kconfig
+
+endmenu

drivers/pps/Makefile new file +7 -0

new file mode 100644
index 0000000..76101cd
@@ -0,0 +1,7 @@
+#
+# Makefile for the PPS core.
+#
+
+pps_core-objs			+= pps.o kapi.o sysfs.o
+obj-$(CONFIG_PPS)		+= pps_core.o
+obj-y				+= clients/

drivers/pps/clients/Kconfig new file +38 -0

new file mode 100644
index 0000000..a87cd37
@@ -0,0 +1,38 @@
+#
+# LinuxPPS clients configuration
+#
+
+if PPS
+
+comment "PPS clients support"
+
+config PPS_CLIENT_KTIMER
+	tristate "Kernel timer client (Testing client, use for debug)"
+	help
+	  If you say yes here you get support for a PPS debugging client
+	  which uses a kernel timer to generate the PPS signal.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ktimer.o.
+
+comment "UART serial support (forced off)"
+	depends on ! (SERIAL_CORE != n && !(PPS = m && SERIAL_CORE = y))
+
+config PPS_CLIENT_UART
+	bool "UART serial support"
+	depends on SERIAL_CORE != n && !(PPS = m && SERIAL_CORE = y)
+	help
+	  If you say yes here you get support for a PPS source connected
+	  with the CD (Carrier Detect) pin of your serial port.
+
+comment "Parallel printer support (forced off)"
+	depends on !( PRINTER != n && !(PPS = m && PRINTER = y))
+
+config PPS_CLIENT_LP
+	bool "Parallel printer support"
+	depends on PRINTER != n && !(PPS = m && PRINTER = y)
+	help
+	  If you say yes here you get support for a PPS source connected
+	  with the interrupt pin of your parallel port.
+
+endif

drivers/pps/clients/Makefile new file +6 -0

new file mode 100644
index 0000000..3ecca41
@@ -0,0 +1,6 @@
+#
+# Makefile for miscellaneous I2C chip drivers.
+#
+
+obj-$(CONFIG_PPS_CLIENT_KTIMER)	+= ktimer.o
+

drivers/pps/clients/ktimer.c new file +113 -0

113 lines not on this page · raw message

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

202 lines not on this page · raw message

drivers/pps/pps.c new file +371 -0

371 lines not on this page · raw message

drivers/pps/sysfs.c new file +219 -0

219 lines not on this page · raw message

drivers/serial/8250.c +2 -0

index 48e259a..3e6ed91 100644
@@ -2101,6 +2101,8 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios,
 		up->ier |= UART_IER_MSI;
 	if (up->capabilities & UART_CAP_UUE)
 		up->ier |= UART_IER_UUE | UART_IER_RTOIE;
+	if (up->port.flags & UPF_HARDPPS_CD)
+		up->ier |= UART_IER_MSI;	/* enable interrupts */
 
 	serial_out(up, UART_IER, up->ier);

drivers/serial/serial_core.c +68 -1

103 lines not on this page · raw message

include/linux/netlink.h +1 -0

index 2e23353..9d55343 100644
@@ -24,6 +24,7 @@
 /* leave room for NETLINK_DM (DM Events) */
 #define NETLINK_SCSITRANSPORT	18	/* SCSI Transports */
 #define NETLINK_ECRYPTFS	19
+#define NETLINK_PPSAPI          20	/* linuxPPS support */
 
 #define MAX_LINKS 32

include/linux/parport.h +12 -0

index 9cdd694..f53d9f4 100644
@@ -100,6 +100,7 @@ typedef enum {
 #include <linux/proc_fs.h>
 #include <linux/spinlock.h>
 #include <linux/wait.h>
+#include <linux/pps.h>
 #include <asm/system.h>
 #include <asm/ptrace.h>
 #include <asm/semaphore.h>

@@ -327,6 +328,11 @@ struct parport {
 
 	struct list_head full_list;
 	struct parport *slaves[3];
+
+#ifdef CONFIG_PPS_CLIENT_LP
+	struct pps_source_info_s pps_info;
+	int pps_source;		/* PPS source ID */
+#endif
 };
 
 #define DEFAULT_SPIN_TIME 500 /* us */

@@ -517,6 +523,12 @@ extern int parport_daisy_select (struct parport *port, int daisy, int mode);
 /* Lowlevel drivers _can_ call this support function to handle irqs.  */
 static __inline__ void parport_generic_irq(int irq, struct parport *port)
 {
+#ifdef CONFIG_PPS_CLIENT_LP
+	pps_event(port->pps_source, PPS_CAPTUREASSERT, port);
+	pps_dbg("parport_pc: PPS assert event at %lu on source #%d",
+		jiffies, port->pps_source);
+#endif
+
 	parport_ieee1284_interrupt (irq, port);
 	read_lock(&port->cad_lock);
 	if (port->cad && port->cad->irq_func)

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

229 lines not on this page · raw message

include/linux/serial_core.h +25 -6

index aa2653a..01a6547 100644
@@ -148,6 +148,7 @@
 #include <linux/sched.h>
 #include <linux/tty.h>
 #include <linux/mutex.h>
+#include <linux/pps.h>
 
 struct uart_port;
 struct uart_info;

@@ -227,6 +228,9 @@ struct uart_port {
 	unsigned char		regshift;		/* reg offset shift */
 	unsigned char		iotype;			/* io access style */
 	unsigned char		unused1;
+#ifdef CONFIG_PPS_CLIENT_UART
+	int			pps_source;		/* PPS source ID */
+#endif
 
 #define UPIO_PORT		(0)
 #define UPIO_HUB6		(1)

@@ -271,7 +275,8 @@ struct uart_port {
 #define UPF_IOREMAP		((__force upf_t) (1 << 31))
 
 #define UPF_CHANGE_MASK		((__force upf_t) (0x17fff))
-#define UPF_USR_MASK		((__force upf_t) (UPF_SPD_MASK|UPF_LOW_LATENCY))
+#define UPF_USR_MASK		((__force upf_t) (UPF_SPD_MASK|UPF_LOW_LATENCY\
+							|UPF_HARDPPS_CD))
 
 	unsigned int		mctrl;			/* current modem ctrl settings */
 	unsigned int		timeout;		/* character-based timeout */

@@ -303,6 +308,10 @@ struct uart_state {
 	struct uart_info	*info;
 	struct uart_port	*port;
 
+#ifdef CONFIG_PPS_CLIENT_UART
+        struct pps_source_info_s pps_info;
+#endif
+
 	struct mutex		mutex;
 };

@@ -467,13 +476,23 @@ uart_handle_dcd_change(struct uart_port *port, unsigned int status)
 {
 	struct uart_info *info = port->info;
 
-	port->icount.dcd++;
-
-#ifdef CONFIG_HARD_PPS
-	if ((port->flags & UPF_HARDPPS_CD) && status)
-		hardpps();
+#ifdef CONFIG_PPS_CLIENT_UART
+	if (port->flags & UPF_HARDPPS_CD) {
+		if (status) {
+			pps_event(port->pps_source, PPS_CAPTUREASSERT, port);
+			pps_dbg("serial8250: PPS assert event at %lu on source #%d",
+				jiffies, port->pps_source);
+		}
+		else {
+			pps_event(port->pps_source, PPS_CAPTURECLEAR, port);
+			pps_dbg("serial8250: PPS clear event at %lu on source #%d",
+				jiffies, port->pps_source);
+		}
+	}
 #endif
 
+	port->icount.dcd++;
+
 	if (info->flags & UIF_CHECK_CD) {
 		if (status)
 			wake_up_interruptible(&info->open_wait);

Andrew Morton

2007-05-12 06:18 UTC #20 lore raw

Quoted 4 lines · On Fri, 11 May 2007 23:55:37 +0200 Rodolfo Giometti <[email protected]>…

Hello,
here my new patch with a lot of fixes.

Quoted 8 lines

The only issue not still fixed is the one related with:
	#define NETLINK_PPSAPI          20
I need time to resolve it.
Follows my comments and then the patch, hope now I can came back into -mm tree again! :)

Well I suppose I could toss it in there for a bit of review-and-test. But I'll need to drop it again because we do need to split this patch into the series of patches, please.

You should do this earlier rather than later because it improves reviewability.

Quoted 11 lines

Quoted 9 lines
- This:
	static void pps_class_release(struct class_device *cdev)
	{
		/* Nop??? */
	}
is a bug and it earns you a nastygram from Greg. These objects must be dynamically allocated - this is not optional.
It could be acceptable defining this function as void?

No, it needs to be a proper release function, like all the other ones around the place.

This comes up again and again and again and I recently asked Greg to direct me to (or to write) suitable documentation, and I think he did, but I lost it. Greg, can you remind us please?

Quoted 22 lines

Quoted 22 lines
We have a bunch of code in random other drivers which is dependent upon CONFIG_PPS_CLIENT_foo. The problem is that if a kernel was compiled with CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that kernel, it won't actually work because lp, serial etc weren't correctly configured when _they_ were built.
This sort of cross-module coupling is considered to be a bad thing, but I'm not really sure it's all that important.
- Please split the patch up into a series of patches: one for pps core and one for each of the clients (servers?): one for lp, one for serial, etc.
Try to arrange for that series of patches to build and run at each stage of application.
Please don't lose my changes when you do so ;)
Please review the changes I made and a) stick to the same style and b) fix up any sites which I missed.
- Please remove all the typedefs:

Quoted 8 lines

include/linux/pps.h
Quoted 6 lines
include/linux/pps.h new file +4 -0
+typedef struct ntp_fp {
+typedef union pps_timeu {
+typedef struct pps_info {
+typedef struct pps_params {
 
  and just use `struct ntp_fp' everywhere.
Those typedefs are defined in PPS specifications (please, see RFC 2783).

We don't use typedefs in-kernel. Please convert the code to use `struct ntp_fp' everywhere.

For RFC compatibility to userspace you can do

#ifndef __KERNEL__
typedef struct ntp_fp ntp_fp_t;
...
#endif

Quoted 10 lines

Quoted 10 lines
- The above four structures are communicated with userspace, yes?
I believe that they will not work correctly when 32-bit userspace is communicating with a 64-bit kernel. Alignments change and sizeof(long) changes.
You don't want to have to write compat code. I suggest that you redo those structures in terms of __u32, __u64, etc. You probably need to use attribute((packed)) too, not sure.

Quoted 7 lines

Quoted 5 lines
Then let's get that part carefully reviewed (Arnd Bergmann <[email protected]> is my go-to guru on this) and please test it carefully.
Yeah, you just haven't got a chance that something as huge and as complex as struct pps_netlink_msg will survive the 32->64 transition.
The same as above. These structure are fixed by RFC 2783.

Your answer has no relationship to my question.

The problem here is that under a 64-bit kernel we require that applications which use this structure definition work correctly when they are compiled to generate 32-bit code and when they are compiled to generate 64-bit code.

Furthermore we should aim to to have to code work correctly across different version of the compiler, and when different compiler options are used, and when altogether different compilers are used.

It is not clear to me that your definition is sufficiently defensive against _any_ of these things.

Quoted 10 lines

Quoted 2 lines
- Please ensure that `make headers_check' passes OK (you'll hear from me if it doesn't ;))
Done.
Quoted 2 lines
- Can we get rid of the private dbg, err and info macros? Surely there are generic ones somewhere.
They are very useful to LinuxPPS users who can enable/disable them by configuration menu.

You misunderstand. I'm not saying "remove the callsites". I'm saying "remove the definitions".

Because we already have things like pr_debug() and pr_info(), so new code should use those rather than reinventing them.

Plus, we already have at least 52 different implementations of "dbg" in the tree and your 53rd one didn't compile because it clashed with someone else's. This is the compiler sending us a message: "use the exiting
infrastructure". If that infrastructure is insufficient then let's
improve it.

Greg KH

2007-05-12 07:08 UTC #21 lore raw

Quoted 32 lines · Andrew Morton

Quoted 4 lines · On Fri, 11 May 2007 23:55:37 +0200 Rodolfo Giometti <[email protected]>…
Hello,
here my new patch with a lot of fixes.
Quoted 8 lines
The only issue not still fixed is the one related with:
	#define NETLINK_PPSAPI          20
I need time to resolve it.
Follows my comments and then the patch, hope now I can came back into -mm tree again! :)
Well I suppose I could toss it in there for a bit of review-and-test. But I'll need to drop it again because we do need to split this patch into the series of patches, please.
You should do this earlier rather than later because it improves reviewability.
Quoted 10 lines
Quoted 9 lines
- This:
	static void pps_class_release(struct class_device *cdev)
	{
		/* Nop??? */
	}
is a bug and it earns you a nastygram from Greg. These objects must be dynamically allocated - this is not optional.

Quoted 8 lines

Quoted 1 line
It could be acceptable defining this function as void?
No, it needs to be a proper release function, like all the other ones around the place.
This comes up again and again and again and I recently asked Greg to direct me to (or to write) suitable documentation, and I think he did, but I lost it. Greg, can you remind us please?

I need to put it in some permanent place, but basically the problem is that if you need to shut the kernel up by using an empty function for a release, then you are not understanding why the kernel was trying to warn you in the first place :(

You need to free your memory for the class_device in the release function, you can not have a static structure, or rely on some other reference count to handle the cleanup properly.

Also note that 'struct class_device' is going away and you should use 'struct device' instead. That too needs to be documented better, and is on my list of things to do...

thanks,

greg k-h