This patchset contains several changes that improve an overall
design/performance of PPS subsystem. I'd like these patches to be
merged mainline if no one objects.
Patches 1-3 are bugfixes.
Patches 4-12 are other improvements to PPS subsystem.
Patches 13-15 add kernel consumer support.
Patch 16 adds parallel port PPS client.
Patch 17 adds parallel port PPS generator.
You can find description for my previous patchset (it describes patches
13-17 in more detailed) here: http://lkml.org/lkml/2010/2/24/189
This patchset is tested against the vanilla 2.6.36 kernel. But we are
actually using it on 2.6.33.7-rt29 rt-preempt kernel most of the time.
Those who are interested in other versions of the patchset can find
them in my git repository:
git://github.com/ago/linux-2.6.git
There is one problem however: kernel consumer works bad (if enabled)
when CONFIG_NO_HZ is enabled. The reason for this is commit
a092ff0f90cae22b2ac8028ecd2c6f6c1a9e4601. Without it hardpps() is able
to sync to 1us precision in about 10 seconds. With CONFIG_NO_HZ it is
not syncing at all. This only affects patches 13-15, others are ok.
Changelog
v3 -> v4:
* add patch 12
* fix memory leak when unregistering pps source
* don't allow to pass NULL timestamp to dcd_change() to simplify the
code because this possibility is never used
* decrease SEND_DELAY_MAX from 300ms to 100us because spending 300ms
with disabled interrupts is inacceptable while 100us can be
tolerated in some setups
* integrate Andrew Morton's patch that replaces create_rt_workqueue()
with create_workqueue()
* fix issues pointed out by Vitezslav Samel, Rodolfo Giometti,
Joe Perches, John Stultz and Andrew Morton:
* fix possible PPS device freeze due to event counter overflow
* add a workaround for a possible race in tty code to the pps-ldisc
module; it replaces a previously used workaround in pps subsystem
code which was worse because it affected all PPS clients. The
problem with the current tty code is that it uses mutexes for
locking, but ldisc reference is taken and used in atomic context
in uart_handle_dcd_change() so it can't lock mutex and therefore
is race-prone. This issue should be discussed I think, but this
patch should be ok for now.
* override pr_fmt(fmt) to automagically print module names
* add comments describing struct pps_normtime
* add arch_gettimeoffset() to raw nanoseconds in
getnstime_raw_and_real()
* use WARN_ON_ONCE instead of WARN_ON in getnstime_raw_and_real()
* convert SIGNAL_IS_SET macro in pps_parport to inline function
* add documentation for pps_gen_parport
v2 -> v3:
* add patches 1-11
* add clear_wait parameter to pps_parport
* add delay parameter to pps_gen_parport
* fix seqlock unlocking
* fix issues pointed out by Rodolfo Giometti:
* move CONFIG_NTP_PPS to drivers/pps/Kconfig
* swap parport client and generator patches
* style and typo fixes
* move patch that adds unified timestamp gathering to be the beginning
v1 -> v2:
* fix issues pointed out by John Stultz:
* style fixes
* add a about the authorship of the original code
* replace timespec with pps_normtime struct where timespec is used
in a wrong way
* fix seqlock usage in hardpps()
* unbind kernel consumer on device removal
* send raw timestamp instead of the last difference to hardpps() which
simplifies the code and is less error-prone
* update comments in the kernel consumer code to match the reality
* split the patch that adds MONOTONIC_RAW timestmaps into two
* other small fixes
Alexander Gordeev (17):
pps: trivial fixes
pps: declare variables where they are used in switch
pps: fix race in PPS_FETCH handler
pps: unify timestamp gathering
pps: access pps device by direct pointer
pps: convert printk/pr_* to dev_*
pps: move idr stuff to pps.c
pps: add async PPS event handler
pps: don't disable interrupts when using spin locks
pps: use BUG_ON for kernel API safety checks
pps: simplify conditions a bit
pps: timestamp is always passed to dcd_change()
ntp: add hardpps implementation
pps: capture MONOTONIC_RAW timestamps as well
pps: add kernel consumer support
pps: add parallel port PPS client
pps: add parallel port PPS signal generator
Documentation/ioctl/ioctl-number.txt | 2 +-
Documentation/pps/pps.txt | 46 ++++
Documentation/serial/tty.txt | 2 +-
drivers/pps/Kconfig | 10 +
drivers/pps/Makefile | 2 +-
drivers/pps/clients/Kconfig | 7 +
drivers/pps/clients/Makefile | 1 +
drivers/pps/clients/pps-ktimer.c | 44 ++--
drivers/pps/clients/pps-ldisc.c | 70 +++---
drivers/pps/clients/pps_parport.c | 247 +++++++++++++++++
drivers/pps/generators/Kconfig | 17 ++
drivers/pps/generators/Makefile | 9 +
drivers/pps/generators/pps_gen_parport.c | 275 +++++++++++++++++++
drivers/pps/kapi.c | 334 +++++++++++------------
drivers/pps/pps.c | 203 +++++++++++---
include/linux/pps.h | 7 +
include/linux/pps_kernel.h | 64 ++++-
include/linux/serial_core.h | 5 +-
include/linux/time.h | 2 +
include/linux/timex.h | 1 +
include/linux/tty_ldisc.h | 7 +-
kernel/time/ntp.c | 425 ++++++++++++++++++++++++++++-
kernel/time/timekeeping.c | 38 +++
23 files changed, 1502 insertions(+), 316 deletions(-)
create mode 100644 drivers/pps/clients/pps_parport.c
create mode 100644 drivers/pps/generators/Kconfig
create mode 100644 drivers/pps/generators/Makefile
create mode 100644 drivers/pps/generators/pps_gen_parport.c
--
1.7.2.3
Add PPS signal generator which utilizes STROBE pin of a parallel port to
send PPS signals. It uses parport abstraction layer and hrtimers to
precisely control the signal.
Signed-off-by: Alexander Gordeev <[email protected]>
---
Documentation/pps/pps.txt | 46 +++++
drivers/pps/Kconfig | 2 +
drivers/pps/Makefile | 2 +-
drivers/pps/generators/Kconfig | 17 ++
drivers/pps/generators/Makefile | 9 +
drivers/pps/generators/pps_gen_parport.c | 275 ++++++++++++++++++++++++++++++
6 files changed, 350 insertions(+), 1 deletions(-)
create mode 100644 drivers/pps/generators/Kconfig
create mode 100644 drivers/pps/generators/Makefile
create mode 100644 drivers/pps/generators/pps_gen_parport.c
diff --git a/Documentation/pps/pps.txt b/Documentation/pps/pps.txt
index 125f4ab..d35dcdd 100644
--- a/Documentation/pps/pps.txt
+++ b/Documentation/pps/pps.txt
@@ -170,3 +170,49 @@ and the run ppstest as follow:
Please, note that to compile userland programs you need the file timepps.h
(see Documentation/pps/).
+
+
+Generators
+----------
+
+Sometimes one needs to be able not only to catch PPS signals but to produce
+them also. For example, running a distributed simulation, which requires
+computers' clock to be synchronized very tightly. One way to do this is to
+invent some complicated hardware solutions but it may be neither necessary
+nor affordable. The cheap way is to load a PPS generator on one of the
+computers (master) and PPS clients on others (slaves), and use very simple
+cables to deliver signals using parallel ports, for example.
+
+Parallel port cable pinout:
+pin name master slave
+1 STROBE *------ *
+2 D0 * | *
+3 D1 * | *
+4 D2 * | *
+5 D3 * | *
+6 D4 * | *
+7 D5 * | *
+8 D6 * | *
+9 D7 * | *
+10 ACK * ------*
+11 BUSY * *
+12 PE * *
+13 SEL * *
+14 AUTOFD * *
+15 ERROR * *
+16 INIT * *
+17 SELIN * *
+18-25 GND *-----------*
+
+Please note that parallel port interrupt occurs only on high->low transition,
+so it is used for PPS assert edge. PPS clear edge can be determined only
+using polling in the interrupt handler which actually can be done way more
+precisely because interrupt handling delays can be quite big and random. So
+current parport PPS generator implementation (pps_gen_parport module) is
+geared towards using the clear edge for time synchronization.
+
+Clear edge polling is done with disabled interrupts so it's better to select
+delay between assert and clear edge as small as possible to reduce system
+latencies. But if it is too small slave won't be able to capture clear edge
+transition. The default of 30us should be good enough in most situations.
+The delay can be selected using 'delay' pps_gen_parport module parameter.
diff --git a/drivers/pps/Kconfig b/drivers/pps/Kconfig
index 4823e47..79bda60 100644
--- a/drivers/pps/Kconfig
+++ b/drivers/pps/Kconfig
@@ -40,4 +40,6 @@ config NTP_PPS
source drivers/pps/clients/Kconfig
+source drivers/pps/generators/Kconfig
+
endmenu
diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
index 98960dd..1906eb70 100644
--- a/drivers/pps/Makefile
+++ b/drivers/pps/Makefile
@@ -4,6 +4,6 @@
pps_core-y := pps.o kapi.o sysfs.o
obj-$(CONFIG_PPS) := pps_core.o
-obj-y += clients/
+obj-y += clients/ generators/
ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
diff --git a/drivers/pps/generators/Kconfig b/drivers/pps/generators/Kconfig
new file mode 100644
index 0000000..5fbd614
--- /dev/null
+++ b/drivers/pps/generators/Kconfig
@@ -0,0 +1,17 @@
+#
+# PPS generators configuration
+#
+
+if PPS
+
+comment "PPS generators support"
+
+config PPS_GENERATOR_PARPORT
+ tristate "Parallel port PPS signal generator"
+ depends on PARPORT != n && GENERIC_TIME
+ help
+ If you say yes here you get support for a PPS signal generator which
+ utilizes STROBE pin of a parallel port to send PPS signals. It uses
+ parport abstraction layer and hrtimers to precisely control the signal.
+
+endif
diff --git a/drivers/pps/generators/Makefile b/drivers/pps/generators/Makefile
new file mode 100644
index 0000000..303304a
--- /dev/null
+++ b/drivers/pps/generators/Makefile
@@ -0,0 +1,9 @@
+#
+# Makefile for PPS generators.
+#
+
+obj-$(CONFIG_PPS_GENERATOR_PARPORT) += pps_gen_parport.o
+
+ifeq ($(CONFIG_PPS_DEBUG),y)
+EXTRA_CFLAGS += -DDEBUG
+endif
diff --git a/drivers/pps/generators/pps_gen_parport.c b/drivers/pps/generators/pps_gen_parport.c
new file mode 100644
index 0000000..aa66803
--- /dev/null
+++ b/drivers/pps/generators/pps_gen_parport.c
@@ -0,0 +1,275 @@
+/*
+ * pps_gen_parport.c -- kernel parallel port PPS signal generator
+ *
+ *
+ * Copyright (C) 2009 Alexander Gordeev <[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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+
+/*
+ * TODO:
+ * fix issues when realtime clock is adjusted in a leap
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/time.h>
+#include <linux/hrtimer.h>
+#include <linux/parport.h>
+
+#define DRVDESC "parallel port PPS signal generator"
+
+#define SIGNAL 0
+#define NO_SIGNAL PARPORT_CONTROL_STROBE
+
+/* module parameters */
+
+#define SEND_DELAY_MAX 100000
+
+static unsigned int send_delay = 30000;
+MODULE_PARM_DESC(delay,
+ "Delay between setting and dropping the signal (ns)");
+module_param_named(delay, send_delay, uint, 0);
+
+
+#define SAFETY_INTERVAL 3000 /* set the hrtimer earlier for safety (ns) */
+
+/* internal per port structure */
+struct pps_generator_pp {
+ struct pardevice *pardev; /* parport device */
+ struct hrtimer timer;
+ long port_write_time; /* calibrated port write time (ns) */
+};
+
+static struct pps_generator_pp device = {
+ .pardev = NULL,
+};
+
+static int attached;
+
+/* calibrated time between a hrtimer event and the reaction */
+static long hrtimer_error = SAFETY_INTERVAL;
+
+/* the kernel hrtimer event */
+static enum hrtimer_restart hrtimer_event(struct hrtimer *timer)
+{
+ struct timespec expire_time, ts1, ts2, ts3, dts;
+ struct pps_generator_pp *dev;
+ struct parport *port;
+ long lim, delta;
+ unsigned long flags;
+
+ /* NB: approx time with blocked interrupts =
+ send_delay + 3 * SAFETY_INTERVAL */
+ local_irq_save(flags);
+
+ /* first of all we get the time stamp... */
+ getnstimeofday(&ts1);
+ expire_time = ktime_to_timespec(timer->_expires);
+ dev = container_of(timer, struct pps_generator_pp, timer);
+ lim = NSEC_PER_SEC - send_delay - dev->port_write_time;
+
+ /* check if we are late */
+ if (expire_time.tv_sec != ts1.tv_sec || ts1.tv_nsec > lim) {
+ local_irq_restore(flags);
+ pr_err("we are late this time %ld.%09ld\n",
+ ts1.tv_sec, ts1.tv_nsec);
+ goto done;
+ }
+
+ /* busy loop until the time is right for an assert edge */
+ do {
+ getnstimeofday(&ts2);
+ } while (expire_time.tv_sec == ts2.tv_sec && ts2.tv_nsec < lim);
+
+ /* set the signal */
+ port = dev->pardev->port;
+ port->ops->write_control(port, SIGNAL);
+
+ /* busy loop until the time is right for a clear edge */
+ lim = NSEC_PER_SEC - dev->port_write_time;
+ do {
+ getnstimeofday(&ts2);
+ } while (expire_time.tv_sec == ts2.tv_sec && ts2.tv_nsec < lim);
+
+ /* unset the signal */
+ port->ops->write_control(port, NO_SIGNAL);
+
+ getnstimeofday(&ts3);
+
+ local_irq_restore(flags);
+
+ /* update calibrated port write time */
+ dts = timespec_sub(ts3, ts2);
+ dev->port_write_time =
+ (dev->port_write_time + timespec_to_ns(&dts)) >> 1;
+
+done:
+ /* update calibrated hrtimer error */
+ dts = timespec_sub(ts1, expire_time);
+ delta = timespec_to_ns(&dts);
+ /* If the new error value is bigger then the old, use the new
+ * value, if not then slowly move towards the new value. This
+ * way it should be safe in bad conditions and efficient in
+ * good conditions.
+ */
+ if (delta >= hrtimer_error)
+ hrtimer_error = delta;
+ else
+ hrtimer_error = (3 * hrtimer_error + delta) >> 2;
+
+ /* update the hrtimer expire time */
+ hrtimer_set_expires(timer,
+ ktime_set(expire_time.tv_sec + 1,
+ NSEC_PER_SEC - (send_delay +
+ dev->port_write_time + SAFETY_INTERVAL +
+ 2 * hrtimer_error)));
+
+ return HRTIMER_RESTART;
+}
+
+/* calibrate port write time */
+#define PORT_NTESTS_SHIFT 5
+static void calibrate_port(struct pps_generator_pp *dev)
+{
+ struct parport *port = dev->pardev->port;
+ int i;
+ long acc = 0;
+
+ for (i = 0; i < (1 << PORT_NTESTS_SHIFT); i++) {
+ struct timespec a, b;
+ unsigned long irq_flags;
+
+ local_irq_save(irq_flags);
+ getnstimeofday(&a);
+ port->ops->write_control(port, NO_SIGNAL);
+ getnstimeofday(&b);
+ local_irq_restore(irq_flags);
+
+ b = timespec_sub(b, a);
+ acc += timespec_to_ns(&b);
+ }
+
+ dev->port_write_time = acc >> PORT_NTESTS_SHIFT;
+ pr_info("port write takes %ldns\n", dev->port_write_time);
+}
+
+static inline ktime_t next_intr_time(struct pps_generator_pp *dev)
+{
+ struct timespec ts;
+
+ getnstimeofday(&ts);
+
+ return ktime_set(ts.tv_sec +
+ ((ts.tv_nsec > 990 * NSEC_PER_MSEC) ? 1 : 0),
+ NSEC_PER_SEC - (send_delay +
+ dev->port_write_time + 3 * SAFETY_INTERVAL));
+}
+
+static void parport_attach(struct parport *port)
+{
+ if (attached) {
+ /* we already have a port */
+ return;
+ }
+
+ device.pardev = parport_register_device(port, KBUILD_MODNAME,
+ NULL, NULL, NULL, 0, &device);
+ if (!device.pardev) {
+ pr_err("couldn't register with %s\n", port->name);
+ return;
+ }
+
+ if (parport_claim_or_block(device.pardev) < 0) {
+ pr_err("couldn't claim %s\n", port->name);
+ goto err_unregister_dev;
+ }
+
+ pr_info("attached to %s\n", port->name);
+ attached = 1;
+
+ calibrate_port(&device);
+
+ hrtimer_init(&device.timer, CLOCK_REALTIME, HRTIMER_MODE_ABS);
+ device.timer.function = hrtimer_event;
+#ifdef CONFIG_PREEMPT_RT
+ /* hrtimer interrupt will run in the interrupt context with this */
+ device.timer.irqsafe = 1;
+#endif
+
+ hrtimer_start(&device.timer, next_intr_time(&device), HRTIMER_MODE_ABS);
+
+ return;
+
+err_unregister_dev:
+ parport_unregister_device(device.pardev);
+}
+
+static void parport_detach(struct parport *port)
+{
+ if (port->cad != device.pardev)
+ return; /* not our port */
+
+ hrtimer_cancel(&device.timer);
+ parport_release(device.pardev);
+ parport_unregister_device(device.pardev);
+}
+
+static struct parport_driver pps_gen_parport_driver = {
+ .name = KBUILD_MODNAME,
+ .attach = parport_attach,
+ .detach = parport_detach,
+};
+
+/* module staff */
+
+static int __init pps_gen_parport_init(void)
+{
+ int ret;
+
+ pr_info(DRVDESC "\n");
+
+ if (send_delay > SEND_DELAY_MAX) {
+ pr_err("delay value should be not greater"
+ " then %d\n", SEND_DELAY_MAX);
+ return -EINVAL;
+ }
+
+ ret = parport_register_driver(&pps_gen_parport_driver);
+ if (ret) {
+ pr_err("unable to register with parport\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit pps_gen_parport_exit(void)
+{
+ parport_unregister_driver(&pps_gen_parport_driver);
+ pr_info("hrtimer avg error is %ldns\n", hrtimer_error);
+}
+
+module_init(pps_gen_parport_init);
+module_exit(pps_gen_parport_exit);
+
+MODULE_AUTHOR("Alexander Gordeev <[email protected]>");
+MODULE_DESCRIPTION(DRVDESC);
+MODULE_LICENSE("GPL");
--
1.7.2.3
There was a race in PPS_FETCH ioctl handler when several processes want
to obtain PPS data simultaneously using sleeping PPS_FETCH. They all
sleep most of the time in the system call.
With the old approach when the first process waiting on the pps queue
is waken up it makes new system call right away and zeroes pps->go. So
other processes continue to sleep. This is a clear race condition
because of the global 'go' variable.
With the new approach pps->last_ev holds some value increasing at each
PPS event. PPS_FETCH ioctl handler saves current value to the local
variable at the very beginning so it can safely check that there is a
new event by just comparing both variables.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 4 ++--
drivers/pps/pps.c | 10 +++++++---
include/linux/pps_kernel.h | 2 +-
3 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 55f3961..3f89f5eb 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -326,8 +326,8 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
/* Wake up if captured something */
if (captured) {
- pps->go = ~0;
- wake_up_interruptible(&pps->queue);
+ pps->last_ev++;
+ wake_up_interruptible_all(&pps->queue);
kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
}
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index c76afb9..dc7e66c 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -136,6 +136,7 @@ static long pps_cdev_ioctl(struct file *file,
case PPS_FETCH: {
struct pps_fdata fdata;
+ unsigned int ev;
pr_debug("PPS_FETCH: source %d\n", pps->id);
@@ -143,11 +144,12 @@ static long pps_cdev_ioctl(struct file *file,
if (err)
return -EFAULT;
- pps->go = 0;
+ ev = pps->last_ev;
/* Manage the timeout */
if (fdata.timeout.flags & PPS_TIME_INVALID)
- err = wait_event_interruptible(pps->queue, pps->go);
+ err = wait_event_interruptible(pps->queue,
+ ev != pps->last_ev);
else {
unsigned long ticks;
@@ -159,7 +161,9 @@ static long pps_cdev_ioctl(struct file *file,
if (ticks != 0) {
err = wait_event_interruptible_timeout(
- pps->queue, pps->go, ticks);
+ pps->queue,
+ ev != pps->last_ev,
+ ticks);
if (err == 0)
return -ETIMEDOUT;
}
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index c930d11..c3aed4b 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -55,7 +55,7 @@ struct pps_device {
struct pps_ktime clear_tu;
int current_mode; /* PPS mode at event time */
- int go; /* PPS event is arrived? */
+ unsigned int last_ev; /* last PPS event id */
wait_queue_head_t queue; /* PPS event queue */
unsigned int id; /* PPS source unique ID */
--
1.7.2.3
Here are some very trivial fixes combined:
* add macro definitions to protect header file from including several times
* remove declaration for an unexistent array
* fix typos
Acked-by: Rodolfo Giometti <[email protected]>
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 2 +-
include/linux/pps_kernel.h | 7 ++++++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 1aa02db..55f3961 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -324,7 +324,7 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
captured = ~0;
}
- /* Wake up iif captured somthing */
+ /* Wake up if captured something */
if (captured) {
pps->go = ~0;
wake_up_interruptible(&pps->queue);
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index e0a193f..c930d11 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -18,6 +18,9 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
+#ifndef LINUX_PPS_KERNEL_H
+#define LINUX_PPS_KERNEL_H
+
#include <linux/pps.h>
#include <linux/cdev.h>
@@ -71,7 +74,6 @@ struct pps_device {
extern spinlock_t pps_idr_lock;
extern struct idr pps_idr;
-extern struct timespec pps_irq_ts[];
extern struct device_attribute pps_attrs[];
@@ -87,3 +89,6 @@ extern void pps_unregister_source(int source);
extern int pps_register_cdev(struct pps_device *pps);
extern void pps_unregister_cdev(struct pps_device *pps);
extern void pps_event(int source, struct pps_ktime *ts, int event, void *data);
+
+#endif /* LINUX_PPS_KERNEL_H */
+
--
1.7.2.3
Move variable declarations where they are used in pps_cdev_ioctl.
Acked-by: Rodolfo Giometti <[email protected]>
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/pps.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index ca5183b..c76afb9 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -61,8 +61,6 @@ static long pps_cdev_ioctl(struct file *file,
{
struct pps_device *pps = file->private_data;
struct pps_kparams params;
- struct pps_fdata fdata;
- unsigned long ticks;
void __user *uarg = (void __user *) arg;
int __user *iuarg = (int __user *) arg;
int err;
@@ -136,7 +134,9 @@ static long pps_cdev_ioctl(struct file *file,
break;
- case PPS_FETCH:
+ case PPS_FETCH: {
+ struct pps_fdata fdata;
+
pr_debug("PPS_FETCH: source %d\n", pps->id);
err = copy_from_user(&fdata, uarg, sizeof(struct pps_fdata));
@@ -149,6 +149,8 @@ static long pps_cdev_ioctl(struct file *file,
if (fdata.timeout.flags & PPS_TIME_INVALID)
err = wait_event_interruptible(pps->queue, pps->go);
else {
+ unsigned long ticks;
+
pr_debug("timeout %lld.%09d\n",
(long long) fdata.timeout.sec,
fdata.timeout.nsec);
@@ -185,7 +187,7 @@ static long pps_cdev_ioctl(struct file *file,
return -EFAULT;
break;
-
+ }
default:
return -ENOTTY;
break;
--
1.7.2.3
Now all PPS spin locks are never used in interrupt context so we can
safely replace spin_lock_irq*/spin_unlock_irq* with plain
spin_lock/spin_unlock.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 5 ++---
drivers/pps/pps.c | 24 ++++++++++++------------
2 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index ca3b4f8..fe832aa 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -176,7 +176,6 @@ EXPORT_SYMBOL(pps_unregister_source);
void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
void *data)
{
- unsigned long flags;
int captured = 0;
struct pps_ktime ts_real;
@@ -190,7 +189,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
timespec_to_pps_ktime(&ts_real, ts->ts_real);
- spin_lock_irqsave(&pps->lock, flags);
+ spin_lock(&pps->lock);
/* Must call the echo function? */
if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
@@ -237,7 +236,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
}
- spin_unlock_irqrestore(&pps->lock, flags);
+ spin_unlock(&pps->lock);
}
EXPORT_SYMBOL(pps_event);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index f642558..d3267f3 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -74,12 +74,12 @@ static long pps_cdev_ioctl(struct file *file,
case PPS_GETPARAMS:
dev_dbg(pps->dev, "PPS_GETPARAMS\n");
- spin_lock_irq(&pps->lock);
+ spin_lock(&pps->lock);
/* Get the current parameters */
params = pps->params;
- spin_unlock_irq(&pps->lock);
+ spin_unlock(&pps->lock);
err = copy_to_user(uarg, ¶ms, sizeof(struct pps_kparams));
if (err)
@@ -110,7 +110,7 @@ static long pps_cdev_ioctl(struct file *file,
return -EINVAL;
}
- spin_lock_irq(&pps->lock);
+ spin_lock(&pps->lock);
/* Save the new parameters */
pps->params = params;
@@ -126,7 +126,7 @@ static long pps_cdev_ioctl(struct file *file,
pps->params.mode |= PPS_CANWAIT;
pps->params.api_version = PPS_API_VERS;
- spin_unlock_irq(&pps->lock);
+ spin_unlock(&pps->lock);
break;
@@ -181,7 +181,7 @@ static long pps_cdev_ioctl(struct file *file,
}
/* Return the fetched timestamp */
- spin_lock_irq(&pps->lock);
+ spin_lock(&pps->lock);
fdata.info.assert_sequence = pps->assert_sequence;
fdata.info.clear_sequence = pps->clear_sequence;
@@ -189,7 +189,7 @@ static long pps_cdev_ioctl(struct file *file,
fdata.info.clear_tu = pps->clear_tu;
fdata.info.current_mode = pps->current_mode;
- spin_unlock_irq(&pps->lock);
+ spin_unlock(&pps->lock);
err = copy_to_user(uarg, &fdata, sizeof(struct pps_fdata));
if (err)
@@ -239,9 +239,9 @@ static void pps_device_destruct(struct device *dev)
/* release id here to protect others from using it while it's
* still in use */
- spin_lock_irq(&pps_idr_lock);
+ spin_lock(&pps_idr_lock);
idr_remove(&pps_idr, pps->id);
- spin_unlock_irq(&pps_idr_lock);
+ spin_unlock(&pps_idr_lock);
kfree(dev);
kfree(pps);
@@ -260,9 +260,9 @@ int pps_register_cdev(struct pps_device *pps)
* After idr_get_new() calling the new source will be freely available
* into the kernel.
*/
- spin_lock_irq(&pps_idr_lock);
+ spin_lock(&pps_idr_lock);
err = idr_get_new(&pps_idr, pps, &pps->id);
- spin_unlock_irq(&pps_idr_lock);
+ spin_unlock(&pps_idr_lock);
if (err < 0)
return err;
@@ -302,9 +302,9 @@ del_cdev:
cdev_del(&pps->cdev);
free_idr:
- spin_lock_irq(&pps_idr_lock);
+ spin_lock(&pps_idr_lock);
idr_remove(&pps_idr, pps->id);
- spin_unlock_irq(&pps_idr_lock);
+ spin_unlock(&pps_idr_lock);
return err;
}
--
1.7.2.3
Bitwise conjunction is distributive so we can simplify some conditions.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 6 ++----
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 54261c4..2bdfbed 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -184,8 +184,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
/* Check the event */
pps->current_mode = pps->params.mode;
- if ((event & PPS_CAPTUREASSERT) &
- (pps->params.mode & PPS_CAPTUREASSERT)) {
+ if (event & pps->params.mode & PPS_CAPTUREASSERT) {
/* We have to add an offset? */
if (pps->params.mode & PPS_OFFSETASSERT)
pps_add_offset(&ts_real,
@@ -199,8 +198,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
captured = ~0;
}
- if ((event & PPS_CAPTURECLEAR) &
- (pps->params.mode & PPS_CAPTURECLEAR)) {
+ if (event & pps->params.mode & PPS_CAPTURECLEAR) {
/* We have to add an offset? */
if (pps->params.mode & PPS_OFFSETCLEAR)
pps_add_offset(&ts_real,
--
1.7.2.3
Using device index as a pointer needs some unnecessary work to be done
every time the pointer is needed (in irq handler for example).
Using a direct pointer is much more easy (and safe as well).
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/pps-ktimer.c | 30 ++++-----
drivers/pps/clients/pps-ldisc.c | 52 ++++++++++------
drivers/pps/kapi.c | 124 +++++++++-----------------------------
drivers/pps/pps.c | 22 ++-----
include/linux/pps_kernel.h | 23 +++----
5 files changed, 90 insertions(+), 161 deletions(-)
diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
index e1bdd8b..966ead1 100644
--- a/drivers/pps/clients/pps-ktimer.c
+++ b/drivers/pps/clients/pps-ktimer.c
@@ -31,7 +31,7 @@
* Global variables
*/
-static int source;
+static struct pps_device *pps;
static struct timer_list ktimer;
/*
@@ -47,7 +47,7 @@ static void pps_ktimer_event(unsigned long ptr)
pr_info("PPS event at %lu\n", jiffies);
- pps_event(source, &ts, PPS_CAPTUREASSERT, NULL);
+ pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
mod_timer(&ktimer, jiffies + HZ);
}
@@ -56,12 +56,11 @@ static void pps_ktimer_event(unsigned long ptr)
* The echo function
*/
-static void pps_ktimer_echo(int source, int event, void *data)
+static void pps_ktimer_echo(struct pps_device *pps, int event, void *data)
{
- pr_info("echo %s %s for source %d\n",
+ dev_info(pps->dev, "echo %s %s\n",
event & PPS_CAPTUREASSERT ? "assert" : "",
- event & PPS_CAPTURECLEAR ? "clear" : "",
- source);
+ event & PPS_CAPTURECLEAR ? "clear" : "");
}
/*
@@ -84,30 +83,27 @@ static struct pps_source_info pps_ktimer_info = {
static void __exit pps_ktimer_exit(void)
{
- del_timer_sync(&ktimer);
- pps_unregister_source(source);
+ dev_info(pps->dev, "ktimer PPS source unregistered\n");
- pr_info("ktimer PPS source unregistered\n");
+ del_timer_sync(&ktimer);
+ pps_unregister_source(pps);
}
static int __init pps_ktimer_init(void)
{
- int ret;
-
- ret = pps_register_source(&pps_ktimer_info,
+ pps = pps_register_source(&pps_ktimer_info,
PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
- if (ret < 0) {
+ if (pps == NULL) {
printk(KERN_ERR "cannot register ktimer source\n");
- return ret;
+ return -ENOMEM;
}
- source = ret;
setup_timer(&ktimer, pps_ktimer_event, 0);
mod_timer(&ktimer, jiffies + HZ);
- pr_info("ktimer PPS source registered at %d\n", source);
+ dev_info(pps->dev, "ktimer PPS source registered\n");
- return 0;
+ return 0;
}
module_init(pps_ktimer_init);
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 20fc9f7..1950b15 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -23,14 +23,18 @@
#include <linux/serial_core.h>
#include <linux/tty.h>
#include <linux/pps_kernel.h>
+#include <linux/spinlock.h>
#define PPS_TTY_MAGIC 0x0001
+static DEFINE_SPINLOCK(pps_ldisc_lock);
+
static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
struct pps_event_time *ts)
{
- int id = (long)tty->disc_data;
+ struct pps_device *pps;
struct pps_event_time __ts;
+ unsigned long flags;
/* First of all we get the time stamp... */
pps_get_ts(&__ts);
@@ -39,12 +43,18 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
if (!ts) /* No. Do it ourself! */
ts = &__ts;
- /* Now do the PPS event report */
- pps_event(id, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
- NULL);
+ spin_lock_irqsave(&pps_ldisc_lock, flags);
- pr_debug("PPS %s at %lu on source #%d\n",
- status ? "assert" : "clear", jiffies, id);
+ /* Now do the PPS event report */
+ pps = (struct pps_device *)tty->disc_data;
+ if (pps != NULL) {
+ pps_event(pps, ts, status ? PPS_CAPTUREASSERT :
+ PPS_CAPTURECLEAR, NULL);
+ spin_unlock_irqrestore(&pps_ldisc_lock, flags);
+ dev_dbg(pps->dev, "PPS %s at %lu\n",
+ status ? "assert" : "clear", jiffies);
+ } else
+ spin_unlock_irqrestore(&pps_ldisc_lock, flags);
}
static int (*alias_n_tty_open)(struct tty_struct *tty);
@@ -54,7 +64,7 @@ static int pps_tty_open(struct tty_struct *tty)
struct pps_source_info info;
struct tty_driver *drv = tty->driver;
int index = tty->index + drv->name_base;
- int ret;
+ struct pps_device *pps;
info.owner = THIS_MODULE;
info.dev = NULL;
@@ -64,20 +74,22 @@ static int pps_tty_open(struct tty_struct *tty)
PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
PPS_CANWAIT | PPS_TSFMT_TSPEC;
- ret = pps_register_source(&info, PPS_CAPTUREBOTH | \
+ pps = pps_register_source(&info, PPS_CAPTUREBOTH | \
PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
- if (ret < 0) {
+ if (pps == NULL) {
pr_err("cannot register PPS source \"%s\"\n", info.path);
- return ret;
+ return -ENOMEM;
}
- tty->disc_data = (void *)(long)ret;
+
+ spin_lock_irq(&pps_ldisc_lock);
+ tty->disc_data = pps;
+ spin_unlock_irq(&pps_ldisc_lock);
/* Should open N_TTY ldisc too */
- ret = alias_n_tty_open(tty);
- if (ret < 0)
- pps_unregister_source((long)tty->disc_data);
+ if (alias_n_tty_open(tty) < 0)
+ pps_unregister_source(pps);
- pr_info("PPS source #%d \"%s\" added\n", ret, info.path);
+ dev_info(pps->dev, "source \"%s\" added\n", info.path);
return 0;
}
@@ -86,12 +98,16 @@ static void (*alias_n_tty_close)(struct tty_struct *tty);
static void pps_tty_close(struct tty_struct *tty)
{
- int id = (long)tty->disc_data;
+ struct pps_device *pps = (struct pps_device *)tty->disc_data;
- pps_unregister_source(id);
alias_n_tty_close(tty);
- pr_info("PPS source #%d removed\n", id);
+ spin_lock_irq(&pps_ldisc_lock);
+ tty->disc_data = NULL;
+ spin_unlock_irq(&pps_ldisc_lock);
+
+ dev_info(pps->dev, "removed\n");
+ pps_unregister_source(pps);
}
static struct tty_ldisc_ops pps_ldisc_ops;
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index b431d83..98d4012 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -32,11 +32,11 @@
#include <linux/slab.h>
/*
- * Global variables
+ * Local variables
*/
-DEFINE_SPINLOCK(pps_idr_lock);
-DEFINE_IDR(pps_idr);
+static DEFINE_SPINLOCK(pps_idr_lock);
+static DEFINE_IDR(pps_idr);
/*
* Local functions
@@ -60,60 +60,6 @@ static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
* Exported functions
*/
-/* pps_get_source - find a PPS source
- * @source: the PPS source ID.
- *
- * This function is used to find an already registered PPS source into the
- * system.
- *
- * The function returns NULL if found nothing, otherwise it returns a pointer
- * to the PPS source data struct (the refcounter is incremented by 1).
- */
-
-struct pps_device *pps_get_source(int source)
-{
- struct pps_device *pps;
- unsigned long flags;
-
- spin_lock_irqsave(&pps_idr_lock, flags);
-
- pps = idr_find(&pps_idr, source);
- if (pps != NULL)
- atomic_inc(&pps->usage);
-
- spin_unlock_irqrestore(&pps_idr_lock, flags);
-
- return pps;
-}
-
-/* pps_put_source - free the PPS source data
- * @pps: a pointer to the PPS source.
- *
- * This function is used to free a PPS data struct if its refcount is 0.
- */
-
-void pps_put_source(struct pps_device *pps)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&pps_idr_lock, flags);
- BUG_ON(atomic_read(&pps->usage) == 0);
-
- if (!atomic_dec_and_test(&pps->usage)) {
- pps = NULL;
- goto exit;
- }
-
- /* No more reference to the PPS source. We can safely remove the
- * PPS data struct.
- */
- idr_remove(&pps_idr, pps->id);
-
-exit:
- spin_unlock_irqrestore(&pps_idr_lock, flags);
- kfree(pps);
-}
-
/* pps_register_source - add a PPS source in the system
* @info: the PPS info struct
* @default_params: the default PPS parameters of the new source
@@ -122,10 +68,11 @@ exit:
* source is described by info's fields and it will have, as default PPS
* parameters, the ones specified into default_params.
*
- * The function returns, in case of success, the PPS source ID.
+ * The function returns, in case of success, the PPS device. Otherwise NULL.
*/
-int pps_register_source(struct pps_source_info *info, int default_params)
+struct pps_device *pps_register_source(struct pps_source_info *info,
+ int default_params)
{
struct pps_device *pps;
int id;
@@ -168,7 +115,6 @@ int pps_register_source(struct pps_source_info *info, int default_params)
init_waitqueue_head(&pps->queue);
spin_lock_init(&pps->lock);
- atomic_set(&pps->usage, 1);
/* Get new ID for the new PPS source */
if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
@@ -211,7 +157,7 @@ int pps_register_source(struct pps_source_info *info, int default_params)
pr_info("new PPS source %s at ID %d\n", info->name, id);
- return id;
+ return pps;
free_idr:
spin_lock_irq(&pps_idr_lock);
@@ -224,38 +170,33 @@ kfree_pps:
pps_register_source_exit:
printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
- return err;
+ return NULL;
}
EXPORT_SYMBOL(pps_register_source);
/* pps_unregister_source - remove a PPS source from the system
- * @source: the PPS source ID
+ * @pps: the PPS source
*
* This function is used to remove a previously registered PPS source from
* the system.
*/
-void pps_unregister_source(int source)
+void pps_unregister_source(struct pps_device *pps)
{
- struct pps_device *pps;
+ unsigned int id = pps->id;
- spin_lock_irq(&pps_idr_lock);
- pps = idr_find(&pps_idr, source);
+ pps_unregister_cdev(pps);
- if (!pps) {
- BUG();
- spin_unlock_irq(&pps_idr_lock);
- return;
- }
+ spin_lock_irq(&pps_idr_lock);
+ idr_remove(&pps_idr, pps->id);
spin_unlock_irq(&pps_idr_lock);
- pps_unregister_cdev(pps);
- pps_put_source(pps);
+ kfree(pps);
}
EXPORT_SYMBOL(pps_unregister_source);
/* pps_event - register a PPS event into the system
- * @source: the PPS source ID
+ * @pps: the PPS device
* @ts: the event timestamp
* @event: the event type
* @data: userdef pointer
@@ -263,30 +204,24 @@ EXPORT_SYMBOL(pps_unregister_source);
* This function is used by each PPS client in order to register a new
* PPS event into the system (it's usually called inside an IRQ handler).
*
- * If an echo function is associated with the PPS source it will be called
+ * If an echo function is associated with the PPS device it will be called
* as:
- * pps->info.echo(source, event, data);
+ * pps->info.echo(pps, event, data);
*/
-
-void pps_event(int source, struct pps_event_time *ts, int event, void *data)
+void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
+ void *data)
{
- struct pps_device *pps;
unsigned long flags;
int captured = 0;
struct pps_ktime ts_real;
if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
- printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
- event, source);
+ dev_err(pps->dev, "unknown event (%x)\n", event);
return;
}
- pps = pps_get_source(source);
- if (!pps)
- return;
-
- pr_debug("PPS event on source %d at %ld.%09ld\n",
- pps->id, ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
+ dev_dbg(pps->dev, "PPS event at %ld.%09ld\n",
+ ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
timespec_to_pps_ktime(&ts_real, ts->ts_real);
@@ -294,7 +229,7 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
/* Must call the echo function? */
if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
- pps->info.echo(source, event, data);
+ pps->info.echo(pps, event, data);
/* Check the event */
pps->current_mode = pps->params.mode;
@@ -308,8 +243,8 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
/* Save the time stamp */
pps->assert_tu = ts_real;
pps->assert_sequence++;
- pr_debug("capture assert seq #%u for source %d\n",
- pps->assert_sequence, source);
+ dev_dbg(pps->dev, "capture assert seq #%u\n",
+ pps->assert_sequence);
captured = ~0;
}
@@ -323,8 +258,8 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
/* Save the time stamp */
pps->clear_tu = ts_real;
pps->clear_sequence++;
- pr_debug("capture clear seq #%u for source %d\n",
- pps->clear_sequence, source);
+ dev_dbg(pps->dev, "capture clear seq #%u\n",
+ pps->clear_sequence);
captured = ~0;
}
@@ -338,8 +273,5 @@ void pps_event(int source, struct pps_event_time *ts, int event, void *data)
}
spin_unlock_irqrestore(&pps->lock, flags);
-
- /* Now we can release the PPS source for (possible) deregistration */
- pps_put_source(pps);
}
EXPORT_SYMBOL(pps_event);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index dc7e66c..1922f27 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -204,12 +204,6 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
{
struct pps_device *pps = container_of(inode->i_cdev,
struct pps_device, cdev);
- int found;
-
- found = pps_get_source(pps->id) != 0;
- if (!found)
- return -ENODEV;
-
file->private_data = pps;
return 0;
@@ -217,11 +211,6 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
static int pps_cdev_release(struct inode *inode, struct file *file)
{
- struct pps_device *pps = file->private_data;
-
- /* Free the PPS source and wake up (possible) deregistration */
- pps_put_source(pps);
-
return 0;
}
@@ -242,22 +231,23 @@ static const struct file_operations pps_cdev_fops = {
int pps_register_cdev(struct pps_device *pps)
{
int err;
+ dev_t devt;
+
+ devt = MKDEV(MAJOR(pps_devt), pps->id);
- pps->devno = MKDEV(MAJOR(pps_devt), pps->id);
cdev_init(&pps->cdev, &pps_cdev_fops);
pps->cdev.owner = pps->info.owner;
- err = cdev_add(&pps->cdev, pps->devno, 1);
+ err = cdev_add(&pps->cdev, devt, 1);
if (err) {
printk(KERN_ERR "pps: %s: failed to add char device %d:%d\n",
pps->info.name, MAJOR(pps_devt), pps->id);
return err;
}
- pps->dev = device_create(pps_class, pps->info.dev, pps->devno, NULL,
+ pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
"pps%d", pps->id);
if (IS_ERR(pps->dev))
goto del_cdev;
- dev_set_drvdata(pps->dev, pps);
pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
MAJOR(pps_devt), pps->id);
@@ -272,7 +262,7 @@ del_cdev:
void pps_unregister_cdev(struct pps_device *pps)
{
- device_destroy(pps_class, pps->devno);
+ device_destroy(pps_class, pps->dev->devt);
cdev_del(&pps->cdev);
}
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 32aa676..1aedf50 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -31,13 +31,16 @@
* Global defines
*/
+struct pps_device;
+
/* The specific PPS source info */
struct pps_source_info {
char name[PPS_MAX_NAME_LEN]; /* simbolic name */
char path[PPS_MAX_NAME_LEN]; /* path of connected device */
int mode; /* PPS's allowed mode */
- void (*echo)(int source, int event, void *data); /* PPS echo function */
+ void (*echo)(struct pps_device *pps,
+ int event, void *data); /* PPS echo function */
struct module *owner;
struct device *dev;
@@ -65,35 +68,27 @@ struct pps_device {
unsigned int id; /* PPS source unique ID */
struct cdev cdev;
struct device *dev;
- int devno;
struct fasync_struct *async_queue; /* fasync method */
spinlock_t lock;
-
- atomic_t usage; /* usage count */
};
/*
* Global variables
*/
-extern spinlock_t pps_idr_lock;
-extern struct idr pps_idr;
-
extern struct device_attribute pps_attrs[];
/*
* Exported functions
*/
-struct pps_device *pps_get_source(int source);
-extern void pps_put_source(struct pps_device *pps);
-extern int pps_register_source(struct pps_source_info *info,
- int default_params);
-extern void pps_unregister_source(int source);
+extern struct pps_device *pps_register_source(
+ struct pps_source_info *info, int default_params);
+extern void pps_unregister_source(struct pps_device *pps);
extern int pps_register_cdev(struct pps_device *pps);
extern void pps_unregister_cdev(struct pps_device *pps);
-extern void pps_event(int source, struct pps_event_time *ts, int event,
- void *data);
+extern void pps_event(struct pps_device *pps,
+ struct pps_event_time *ts, int event, void *data);
static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
struct timespec ts)
--
1.7.2.3
Remove the code that gatheres timestamp in pps_tty_dcd_change() in case
passed ts parameter is NULL because it never happens in the current
code. Fix comments as well.
Signed-off-by: Alexander Gordeev <[email protected]>
---
Documentation/serial/tty.txt | 2 +-
drivers/pps/clients/pps-ldisc.c | 8 --------
include/linux/tty_ldisc.h | 2 +-
3 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt
index 7c90050..540db41 100644
--- a/Documentation/serial/tty.txt
+++ b/Documentation/serial/tty.txt
@@ -107,7 +107,7 @@ write_wakeup() - May be called at any point between open and close.
dcd_change() - Report to the tty line the current DCD pin status
changes and the relative timestamp. The timestamp
- can be NULL.
+ cannot be NULL.
Driver Access
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 7006f85..57130a5 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -35,16 +35,8 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
struct pps_event_time *ts)
{
struct pps_device *pps;
- struct pps_event_time __ts;
unsigned long flags;
- /* First of all we get the time stamp... */
- pps_get_ts(&__ts);
-
- /* Does caller give us a timestamp? */
- if (!ts) /* No. Do it ourself! */
- ts = &__ts;
-
spin_lock_irqsave(&pps_ldisc_lock, flags);
/* Now do the PPS event report */
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 763e061..ff7dc08 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -104,7 +104,7 @@
* struct pps_event_time *ts)
*
* Tells the discipline that the DCD pin has changed its status and
- * the relative timestamp. Pointer ts can be NULL.
+ * the relative timestamp. Pointer ts cannot be NULL.
*/
#include <linux/fs.h>
--
1.7.2.3
Add parallel port PPS client. It uses a standard method for capturing
timestamps for assert edge transitions: getting a timestamp soon after
an interrupt has happened. This is not a very precise source of time
information due to interrupt handling delays. However, timestamps for
clear edge transitions are much more precise because the interrupt
handler continuously polls hardware port until the transition is done.
Hardware port operations require only about 1us so the maximum error
should not exceed this value. This was my primary goal when developing
this client.
Clear edge capture could be disabled using clear_wait parameter.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/Kconfig | 7 +
drivers/pps/clients/Makefile | 1 +
drivers/pps/clients/pps_parport.c | 247 +++++++++++++++++++++++++++++++++++++
3 files changed, 255 insertions(+), 0 deletions(-)
create mode 100644 drivers/pps/clients/pps_parport.c
diff --git a/drivers/pps/clients/Kconfig b/drivers/pps/clients/Kconfig
index 4e801bd..8520a7f 100644
--- a/drivers/pps/clients/Kconfig
+++ b/drivers/pps/clients/Kconfig
@@ -22,4 +22,11 @@ config PPS_CLIENT_LDISC
If you say yes here you get support for a PPS source connected
with the CD (Carrier Detect) pin of your serial port.
+config PPS_CLIENT_PARPORT
+ tristate "Parallel port PPS client"
+ depends on PPS && PARPORT
+ help
+ If you say yes here you get support for a PPS source connected
+ with the interrupt pin of your parallel port.
+
endif
diff --git a/drivers/pps/clients/Makefile b/drivers/pps/clients/Makefile
index 812c9b1..42517da 100644
--- a/drivers/pps/clients/Makefile
+++ b/drivers/pps/clients/Makefile
@@ -4,6 +4,7 @@
obj-$(CONFIG_PPS_CLIENT_KTIMER) += pps-ktimer.o
obj-$(CONFIG_PPS_CLIENT_LDISC) += pps-ldisc.o
+obj-$(CONFIG_PPS_CLIENT_PARPORT) += pps_parport.o
ifeq ($(CONFIG_PPS_DEBUG),y)
EXTRA_CFLAGS += -DDEBUG
diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
new file mode 100644
index 0000000..20dc52e
--- /dev/null
+++ b/drivers/pps/clients/pps_parport.c
@@ -0,0 +1,247 @@
+/*
+ * pps_parport.c -- kernel parallel port PPS client
+ *
+ *
+ * Copyright (C) 2009 Alexander Gordeev <[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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+
+/*
+ * TODO:
+ * implement echo over SEL pin
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/irqnr.h>
+#include <linux/time.h>
+#include <linux/parport.h>
+#include <linux/pps_kernel.h>
+
+#define DRVDESC "parallel port PPS client"
+
+/* module parameters */
+
+#define CLEAR_WAIT_MAX 100
+#define CLEAR_WAIT_MAX_ERRORS 5
+
+static unsigned int clear_wait = 100;
+MODULE_PARM_DESC(clear_wait,
+ "Maximum number of port reads when polling for signal clear,"
+ " zero turns clear edge capture off entirely");
+module_param(clear_wait, uint, 0);
+
+
+/* internal per port structure */
+struct pps_client_pp {
+ struct pardevice *pardev; /* parport device */
+ struct pps_device *pps; /* PPS device */
+ unsigned int cw; /* port clear timeout */
+ unsigned int cw_err; /* number of timeouts */
+};
+
+static inline int signal_is_set(struct parport *port)
+{
+ return (port->ops->read_status(port) & PARPORT_STATUS_ACK) != 0;
+}
+
+/* parport interrupt handler */
+static void parport_irq(void *handle)
+{
+ struct pps_event_time ts_assert, ts_clear;
+ struct pps_client_pp *dev = handle;
+ struct parport *port = dev->pardev->port;
+ unsigned int i;
+ unsigned long flags;
+
+ /* first of all we get the time stamp... */
+ pps_get_ts(&ts_assert);
+
+ if (dev->cw == 0)
+ /* clear edge capture disabled */
+ goto out_assert;
+
+ /* try capture the clear edge */
+ local_irq_save(flags);
+ /* check the signal (no signal means the pulse is lost this time) */
+ if (!signal_is_set(port)) {
+ local_irq_restore(flags);
+ dev_err(dev->pps->dev, "lost the signal\n");
+ goto out_assert;
+ }
+
+ /* poll the port until the signal is unset */
+ for (i = dev->cw; i; i--)
+ if (!signal_is_set(port)) {
+ pps_get_ts(&ts_clear);
+ local_irq_restore(flags);
+ dev->cw_err = 0;
+ goto out_both;
+ }
+ local_irq_restore(flags);
+
+ /* timeout */
+ dev->cw_err++;
+ if (dev->cw_err >= CLEAR_WAIT_MAX_ERRORS) {
+ dev_err(dev->pps->dev, "disabled clear edge capture after %d"
+ " timeouts\n", dev->cw_err);
+ dev->cw = 0;
+ dev->cw_err = 0;
+ }
+
+out_assert:
+ /* fire assert event */
+ pps_event_irq(dev->pps, &ts_assert,
+ PPS_CAPTUREASSERT, NULL);
+ return;
+
+out_both:
+ /* fire assert event */
+ pps_event_irq(dev->pps, &ts_assert,
+ PPS_CAPTUREASSERT, NULL);
+ /* fire clear event */
+ pps_event_irq(dev->pps, &ts_clear,
+ PPS_CAPTURECLEAR, NULL);
+ return;
+}
+
+/* the PPS echo function */
+static void pps_echo(struct pps_device *pps, int event, void *data)
+{
+ dev_info(pps->dev, "echo %s %s\n",
+ event & PPS_CAPTUREASSERT ? "assert" : "",
+ event & PPS_CAPTURECLEAR ? "clear" : "");
+}
+
+static void parport_attach(struct parport *port)
+{
+ struct pps_client_pp *device;
+ struct pps_source_info info = {
+ .name = KBUILD_MODNAME,
+ .path = "",
+ .mode = PPS_CAPTUREBOTH | \
+ PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
+ PPS_ECHOASSERT | PPS_ECHOCLEAR | \
+ PPS_CANWAIT | PPS_TSFMT_TSPEC,
+ .echo = pps_echo,
+ .owner = THIS_MODULE,
+ .dev = NULL
+ };
+
+ device = kzalloc(sizeof(struct pps_client_pp), GFP_KERNEL);
+ if (!device) {
+ pr_err("memory allocation failed, not attaching\n");
+ return;
+ }
+
+ device->pardev = parport_register_device(port, KBUILD_MODNAME,
+ NULL, NULL, parport_irq, 0, device);
+ if (!device->pardev) {
+ pr_err("couldn't register with %s\n", port->name);
+ goto err_free;
+ }
+
+ if (parport_claim_or_block(device->pardev) < 0) {
+ pr_err("couldn't claim %s\n", port->name);
+ goto err_unregister_dev;
+ }
+
+ device->pps = pps_register_source(&info,
+ PPS_CAPTUREBOTH | PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
+ if (device->pps == NULL) {
+ pr_err("couldn't register PPS source\n");
+ goto err_release_dev;
+ }
+
+ device->cw = clear_wait;
+
+ port->ops->enable_irq(port);
+
+ pr_info("attached to %s\n", port->name);
+
+ return;
+
+err_release_dev:
+ parport_release(device->pardev);
+err_unregister_dev:
+ parport_unregister_device(device->pardev);
+err_free:
+ kfree(device);
+}
+
+static void parport_detach(struct parport *port)
+{
+ struct pardevice *pardev = port->cad;
+ struct pps_client_pp *device;
+
+ /* FIXME: oooh, this is ugly! */
+ if (strcmp(pardev->name, KBUILD_MODNAME))
+ /* not our port */
+ return;
+
+ device = pardev->private;
+
+ port->ops->disable_irq(port);
+ pps_unregister_source(device->pps);
+ parport_release(pardev);
+ parport_unregister_device(pardev);
+ kfree(device);
+}
+
+static struct parport_driver pps_parport_driver = {
+ .name = KBUILD_MODNAME,
+ .attach = parport_attach,
+ .detach = parport_detach,
+};
+
+/* module staff */
+
+static int __init pps_parport_init(void)
+{
+ int ret;
+
+ pr_info(DRVDESC "\n");
+
+ if (clear_wait > CLEAR_WAIT_MAX) {
+ pr_err("clear_wait value should be not greater"
+ " then %d\n", CLEAR_WAIT_MAX);
+ return -EINVAL;
+ }
+
+ ret = parport_register_driver(&pps_parport_driver);
+ if (ret) {
+ pr_err("unable to register with parport\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static void __exit pps_parport_exit(void)
+{
+ parport_unregister_driver(&pps_parport_driver);
+}
+
+module_init(pps_parport_init);
+module_exit(pps_parport_exit);
+
+MODULE_AUTHOR("Alexander Gordeev <[email protected]>");
+MODULE_DESCRIPTION(DRVDESC);
+MODULE_LICENSE("GPL");
--
1.7.2.3
This handler should be called from an IRQ handler. It uses per-device
workqueue internally.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/pps-ktimer.c | 2 +-
drivers/pps/clients/pps-ldisc.c | 2 +-
drivers/pps/kapi.c | 95 ++++++++++++++++++++++++++++++++++++--
drivers/pps/pps.c | 14 +++++-
include/linux/pps_kernel.h | 12 +++++
5 files changed, 117 insertions(+), 8 deletions(-)
diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
index 2728469..26ed7a2 100644
--- a/drivers/pps/clients/pps-ktimer.c
+++ b/drivers/pps/clients/pps-ktimer.c
@@ -48,7 +48,7 @@ static void pps_ktimer_event(unsigned long ptr)
dev_info(pps->dev, "PPS event at %lu\n", jiffies);
- pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
+ pps_event_irq(pps, &ts, PPS_CAPTUREASSERT, NULL);
mod_timer(&ktimer, jiffies + HZ);
}
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 30789fa..7006f85 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -50,7 +50,7 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
/* Now do the PPS event report */
pps = (struct pps_device *)tty->disc_data;
if (pps != NULL) {
- pps_event(pps, ts, status ? PPS_CAPTUREASSERT :
+ pps_event_irq(pps, ts, status ? PPS_CAPTUREASSERT :
PPS_CAPTURECLEAR, NULL);
spin_unlock_irqrestore(&pps_ldisc_lock, flags);
dev_dbg(pps->dev, "PPS %s at %lu\n",
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index f5b2b78..ca3b4f8 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -32,9 +32,19 @@
#include <linux/slab.h>
/*
+ * Global variables
+ */
+
+/* PPS event workqueue */
+struct workqueue_struct *pps_event_workqueue;
+
+/*
* Local functions
*/
+static void assert_work_func(struct work_struct *work);
+static void clear_work_func(struct work_struct *work);
+
static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
{
ts->nsec += offset->nsec;
@@ -108,6 +118,9 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
init_waitqueue_head(&pps->queue);
spin_lock_init(&pps->lock);
+ INIT_WORK(&pps->assert_work, assert_work_func);
+ INIT_WORK(&pps->clear_work, clear_work_func);
+
/* Create the char device */
err = pps_register_cdev(pps);
if (err < 0) {
@@ -152,11 +165,12 @@ EXPORT_SYMBOL(pps_unregister_source);
* @event: the event type
* @data: userdef pointer
*
- * This function is used by each PPS client in order to register a new
- * PPS event into the system (it's usually called inside an IRQ handler).
+ * This function is used by PPS clients in order to register a new
+ * PPS event into the system. It should not be called from an IRQ
+ * handler. Use pps_event_irq instead.
*
- * If an echo function is associated with the PPS device it will be called
- * as:
+ * If an echo function is associated with the PPS device it will be
+ * called as:
* pps->info.echo(pps, event, data);
*/
void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
@@ -226,3 +240,76 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
spin_unlock_irqrestore(&pps->lock, flags);
}
EXPORT_SYMBOL(pps_event);
+
+/* Async event handlers */
+
+static void assert_work_func(struct work_struct *work)
+{
+ struct pps_device *pps = container_of(work,
+ struct pps_device, assert_work);
+
+ pps_event(pps, &pps->assert_work_ts, PPS_CAPTUREASSERT,
+ pps->assert_work_data);
+}
+
+static void clear_work_func(struct work_struct *work)
+{
+ struct pps_device *pps = container_of(work,
+ struct pps_device, clear_work);
+
+ pps_event(pps, &pps->clear_work_ts, PPS_CAPTURECLEAR,
+ pps->clear_work_data);
+}
+
+/* pps_event_irq - register a PPS event for deffered handling using
+ * workqueue
+ *
+ * @pps: the PPS device
+ * @ts: the event timestamp
+ * @event: the event type
+ * @data: userdef pointer
+ *
+ * This function is used by PPS clients in order to register a new
+ * PPS event into the system. It should be called from an IRQ handler
+ * only.
+ *
+ * If an echo function is associated with the PPS device it will be
+ * called as:
+ * pps->info.echo(pps, event, data);
+ */
+void pps_event_irq(struct pps_device *pps, struct pps_event_time *ts,
+ int event, void *data)
+{
+ /* check event type */
+ BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
+
+ if (event & PPS_CAPTUREASSERT) {
+ if (work_pending(&pps->assert_work)) {
+ dev_err(pps->dev, "deferred assert edge work haven't"
+ " been handled within a second\n");
+ /* FIXME: do something more intelligent
+ * then just exit */
+ } else {
+ /* now we can copy data safely */
+ pps->assert_work_ts = *ts;
+ pps->assert_work_data = data;
+
+ queue_work(pps_event_workqueue, &pps->assert_work);
+ }
+ }
+ if (event & PPS_CAPTURECLEAR) {
+ if (work_pending(&pps->clear_work)) {
+ dev_err(pps->dev, "deferred clear edge work haven't"
+ " been handled within a second\n");
+ /* FIXME: do something more intelligent
+ * then just exit */
+ } else {
+ /* now we can copy data safely */
+ pps->clear_work_ts = *ts;
+ pps->clear_work_data = data;
+
+ queue_work(pps_event_workqueue, &pps->clear_work);
+ }
+ }
+}
+EXPORT_SYMBOL(pps_event_irq);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 79b4455..f642558 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -321,18 +321,26 @@ void pps_unregister_cdev(struct pps_device *pps)
static void __exit pps_exit(void)
{
- class_destroy(pps_class);
unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
+ class_destroy(pps_class);
+ destroy_workqueue(pps_event_workqueue);
}
static int __init pps_init(void)
{
int err;
+ pps_event_workqueue = create_workqueue("pps");
+ if (!pps_event_workqueue) {
+ pr_err("failed to create workqueue\n");
+ return -ENOMEM;
+ }
+
pps_class = class_create(THIS_MODULE, "pps");
if (!pps_class) {
pr_err("failed to allocate class\n");
- return -ENOMEM;
+ err = -ENOMEM;
+ goto destroy_workqueue;
}
pps_class->dev_attrs = pps_attrs;
@@ -350,6 +358,8 @@ static int __init pps_init(void)
remove_class:
class_destroy(pps_class);
+destroy_workqueue:
+ destroy_workqueue(pps_event_workqueue);
return err;
}
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 1aedf50..5af0498 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -26,6 +26,7 @@
#include <linux/cdev.h>
#include <linux/device.h>
#include <linux/time.h>
+#include <linux/workqueue.h>
/*
* Global defines
@@ -70,6 +71,13 @@ struct pps_device {
struct device *dev;
struct fasync_struct *async_queue; /* fasync method */
spinlock_t lock;
+
+ struct work_struct assert_work;
+ struct work_struct clear_work;
+ struct pps_event_time assert_work_ts;
+ struct pps_event_time clear_work_ts;
+ void *assert_work_data;
+ void *clear_work_data;
};
/*
@@ -78,6 +86,8 @@ struct pps_device {
extern struct device_attribute pps_attrs[];
+extern struct workqueue_struct *pps_event_workqueue;
+
/*
* Exported functions
*/
@@ -89,6 +99,8 @@ extern int pps_register_cdev(struct pps_device *pps);
extern void pps_unregister_cdev(struct pps_device *pps);
extern void pps_event(struct pps_device *pps,
struct pps_event_time *ts, int event, void *data);
+extern void pps_event_irq(struct pps_device *pps,
+ struct pps_event_time *ts, int event, void *data);
static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
struct timespec ts)
--
1.7.2.3
This commit adds hardpps() implementation based upon the original one
from the NTPv4 reference kernel code from David Mills. However, it is
highly optimized towards very fast syncronization and maximum stickness
to PPS signal. The typical error is less then a microsecond.
To make it sync faster I had to throw away exponential phase filter so
that the full phase offset is corrected immediately. Then I also had to
throw away median phase filter because it gives a bigger error itself
if used without exponential filter.
Maybe we will find an appropriate filtering scheme in the future but
it's not necessary if the signal quality is ok.
Acked-by: John Stultz <[email protected]>
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/Kconfig | 7 +
include/linux/timex.h | 1 +
kernel/time/ntp.c | 425 +++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 418 insertions(+), 15 deletions(-)
diff --git a/drivers/pps/Kconfig b/drivers/pps/Kconfig
index 1afe4e0..33c9126 100644
--- a/drivers/pps/Kconfig
+++ b/drivers/pps/Kconfig
@@ -30,6 +30,13 @@ config PPS_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.
+config NTP_PPS
+ bool "PPS kernel consumer support"
+ depends on PPS
+ help
+ This option adds support for direct in-kernel time
+ syncronization using an external PPS signal.
+
source drivers/pps/clients/Kconfig
endmenu
diff --git a/include/linux/timex.h b/include/linux/timex.h
index 32d852f..d23999f 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -268,6 +268,7 @@ extern u64 tick_length;
extern void second_overflow(void);
extern void update_ntp_one_tick(void);
extern int do_adjtimex(struct timex *);
+extern void hardpps(const struct timespec *, const struct timespec *);
int read_current_timer(unsigned long *timer_val);
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index c631168..d721ba7 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -14,6 +14,7 @@
#include <linux/timex.h>
#include <linux/time.h>
#include <linux/mm.h>
+#include <linux/module.h>
/*
* NTP timekeeping variables:
@@ -74,6 +75,162 @@ static long time_adjust;
/* constant (boot-param configurable) NTP tick adjustment (upscaled) */
static s64 ntp_tick_adj;
+#ifdef CONFIG_NTP_PPS
+
+/*
+ * The following variables are used when a pulse-per-second (PPS) signal
+ * is available. They establish the engineering parameters of the clock
+ * discipline loop when controlled by the PPS signal.
+ */
+#define PPS_VALID 10 /* PPS signal watchdog max (s) */
+#define PPS_POPCORN 4 /* popcorn spike threshold (shift) */
+#define PPS_INTMIN 2 /* min freq interval (s) (shift) */
+#define PPS_INTMAX 8 /* max freq interval (s) (shift) */
+#define PPS_INTCOUNT 4 /* number of consecutive good intervals to
+ increase pps_shift or consecutive bad
+ intervals to decrease it */
+#define PPS_MAXWANDER 100000 /* max PPS freq wander (ns/s) */
+
+static int pps_valid; /* signal watchdog counter */
+static long pps_tf[3]; /* phase median filter */
+static long pps_jitter; /* current jitter (ns) */
+static struct timespec pps_fbase; /* beginning of the last freq interval */
+static int pps_shift; /* current interval duration (s) (shift) */
+static int pps_intcnt; /* interval counter */
+static s64 pps_freq; /* frequency offset (scaled ns/s) */
+static long pps_stabil; /* current stability (scaled ns/s) */
+
+/*
+ * PPS signal quality monitors
+ */
+static long pps_calcnt; /* calibration intervals */
+static long pps_jitcnt; /* jitter limit exceeded */
+static long pps_stbcnt; /* stability limit exceeded */
+static long pps_errcnt; /* calibration errors */
+
+
+/* PPS kernel consumer compensates the whole phase error immediately.
+ * Otherwise, reduce the offset by a fixed factor times the time constant.
+ */
+static inline s64 ntp_offset_chunk(s64 offset)
+{
+ if (time_status & STA_PPSTIME && time_status & STA_PPSSIGNAL)
+ return offset;
+ else
+ return shift_right(offset, SHIFT_PLL + time_constant);
+}
+
+static inline void pps_reset_freq_interval(void)
+{
+ /* the PPS calibration interval may end
+ surprisingly early */
+ pps_shift = PPS_INTMIN;
+ pps_intcnt = 0;
+}
+
+/**
+ * pps_clear - Clears the PPS state variables
+ *
+ * Must be called while holding a write on the xtime_lock
+ */
+static inline void pps_clear(void)
+{
+ pps_reset_freq_interval();
+ pps_tf[0] = 0;
+ pps_tf[1] = 0;
+ pps_tf[2] = 0;
+ pps_fbase.tv_sec = pps_fbase.tv_nsec = 0;
+ pps_freq = 0;
+}
+
+/* Decrease pps_valid to indicate that another second has passed since
+ * the last PPS signal. When it reaches 0, indicate that PPS signal is
+ * missing.
+ *
+ * Must be called while holding a write on the xtime_lock
+ */
+static inline void pps_dec_valid(void)
+{
+ if (pps_valid > 0)
+ pps_valid--;
+ else {
+ time_status &= ~(STA_PPSSIGNAL | STA_PPSJITTER |
+ STA_PPSWANDER | STA_PPSERROR);
+ pps_clear();
+ }
+}
+
+static inline void pps_set_freq(s64 freq)
+{
+ pps_freq = freq;
+}
+
+static inline int is_error_status(int status)
+{
+ return (time_status & (STA_UNSYNC|STA_CLOCKERR))
+ /* PPS signal lost when either PPS time or
+ * PPS frequency synchronization requested
+ */
+ || ((time_status & (STA_PPSFREQ|STA_PPSTIME))
+ && !(time_status & STA_PPSSIGNAL))
+ /* PPS jitter exceeded when
+ * PPS time synchronization requested */
+ || ((time_status & (STA_PPSTIME|STA_PPSJITTER))
+ == (STA_PPSTIME|STA_PPSJITTER))
+ /* PPS wander exceeded or calibration error when
+ * PPS frequency synchronization requested
+ */
+ || ((time_status & STA_PPSFREQ)
+ && (time_status & (STA_PPSWANDER|STA_PPSERROR)));
+}
+
+static inline void pps_fill_timex(struct timex *txc)
+{
+ txc->ppsfreq = shift_right((pps_freq >> PPM_SCALE_INV_SHIFT) *
+ PPM_SCALE_INV, NTP_SCALE_SHIFT);
+ txc->jitter = pps_jitter;
+ if (!(time_status & STA_NANO))
+ txc->jitter /= NSEC_PER_USEC;
+ txc->shift = pps_shift;
+ txc->stabil = pps_stabil;
+ txc->jitcnt = pps_jitcnt;
+ txc->calcnt = pps_calcnt;
+ txc->errcnt = pps_errcnt;
+ txc->stbcnt = pps_stbcnt;
+}
+
+#else /* !CONFIG_NTP_PPS */
+
+static inline s64 ntp_offset_chunk(s64 offset)
+{
+ return shift_right(offset, SHIFT_PLL + time_constant);
+}
+
+static inline void pps_reset_freq_interval(void) {}
+static inline void pps_clear(void) {}
+static inline void pps_dec_valid(void) {}
+static inline void pps_set_freq(s64 freq) {}
+
+static inline int is_error_status(int status)
+{
+ return status & (STA_UNSYNC|STA_CLOCKERR);
+}
+
+static inline void pps_fill_timex(struct timex *txc)
+{
+ /* PPS is not implemented, so these are zero */
+ txc->ppsfreq = 0;
+ txc->jitter = 0;
+ txc->shift = 0;
+ txc->stabil = 0;
+ txc->jitcnt = 0;
+ txc->calcnt = 0;
+ txc->errcnt = 0;
+ txc->stbcnt = 0;
+}
+
+#endif /* CONFIG_NTP_PPS */
+
/*
* NTP methods:
*/
@@ -177,6 +334,9 @@ void ntp_clear(void)
tick_length = tick_length_base;
time_offset = 0;
+
+ /* Clear PPS state variables */
+ pps_clear();
}
/*
@@ -242,16 +402,16 @@ void second_overflow(void)
time_status |= STA_UNSYNC;
}
- /*
- * Compute the phase adjustment for the next second. The offset is
- * reduced by a fixed factor times the time constant.
- */
+ /* Compute the phase adjustment for the next second */
tick_length = tick_length_base;
- delta = shift_right(time_offset, SHIFT_PLL + time_constant);
+ delta = ntp_offset_chunk(time_offset);
time_offset -= delta;
tick_length += delta;
+ /* Check PPS signal */
+ pps_dec_valid();
+
if (!time_adjust)
return;
@@ -361,6 +521,8 @@ static inline void process_adj_status(struct timex *txc, struct timespec *ts)
if ((time_status & STA_PLL) && !(txc->status & STA_PLL)) {
time_state = TIME_OK;
time_status = STA_UNSYNC;
+ /* restart PPS frequency calibration */
+ pps_reset_freq_interval();
}
/*
@@ -410,6 +572,8 @@ static inline void process_adjtimex_modes(struct timex *txc, struct timespec *ts
time_freq = txc->freq * PPM_SCALE;
time_freq = min(time_freq, MAXFREQ_SCALED);
time_freq = max(time_freq, -MAXFREQ_SCALED);
+ /* update pps_freq */
+ pps_set_freq(time_freq);
}
if (txc->modes & ADJ_MAXERROR)
@@ -500,7 +664,8 @@ int do_adjtimex(struct timex *txc)
}
result = time_state; /* mostly `TIME_OK' */
- if (time_status & (STA_UNSYNC|STA_CLOCKERR))
+ /* check for errors */
+ if (is_error_status(time_status))
result = TIME_ERROR;
txc->freq = shift_right((time_freq >> PPM_SCALE_INV_SHIFT) *
@@ -514,15 +679,8 @@ int do_adjtimex(struct timex *txc)
txc->tick = tick_usec;
txc->tai = time_tai;
- /* PPS is not implemented, so these are zero */
- txc->ppsfreq = 0;
- txc->jitter = 0;
- txc->shift = 0;
- txc->stabil = 0;
- txc->jitcnt = 0;
- txc->calcnt = 0;
- txc->errcnt = 0;
- txc->stbcnt = 0;
+ /* fill PPS status fields */
+ pps_fill_timex(txc);
write_sequnlock_irq(&xtime_lock);
@@ -536,6 +694,243 @@ int do_adjtimex(struct timex *txc)
return result;
}
+#ifdef CONFIG_NTP_PPS
+
+/* actually struct pps_normtime is good old struct timespec, but it is
+ * semantically different (and it is the reason why it was invented):
+ * pps_normtime.nsec has a range of ( -NSEC_PER_SEC / 2, NSEC_PER_SEC / 2 ]
+ * while timespec.tv_nsec has a range of [0, NSEC_PER_SEC) */
+struct pps_normtime {
+ __kernel_time_t sec; /* seconds */
+ long nsec; /* nanoseconds */
+};
+
+/* normalize the timestamp so that nsec is in the
+ ( -NSEC_PER_SEC / 2, NSEC_PER_SEC / 2 ] interval */
+static inline struct pps_normtime pps_normalize_ts(struct timespec ts)
+{
+ struct pps_normtime norm = {
+ .sec = ts.tv_sec,
+ .nsec = ts.tv_nsec
+ };
+
+ if (norm.nsec > (NSEC_PER_SEC >> 1)) {
+ norm.nsec -= NSEC_PER_SEC;
+ norm.sec++;
+ }
+
+ return norm;
+}
+
+/* get current phase correction and jitter */
+static inline long pps_phase_filter_get(long *jitter)
+{
+ *jitter = pps_tf[0] - pps_tf[1];
+ if (*jitter < 0)
+ *jitter = -*jitter;
+
+ /* TODO: test various filters */
+ return pps_tf[0];
+}
+
+/* add the sample to the phase filter */
+static inline void pps_phase_filter_add(long err)
+{
+ pps_tf[2] = pps_tf[1];
+ pps_tf[1] = pps_tf[0];
+ pps_tf[0] = err;
+}
+
+/* decrease frequency calibration interval length.
+ * It is halved after four consecutive unstable intervals.
+ */
+static inline void pps_dec_freq_interval(void)
+{
+ if (--pps_intcnt <= -PPS_INTCOUNT) {
+ pps_intcnt = -PPS_INTCOUNT;
+ if (pps_shift > PPS_INTMIN) {
+ pps_shift--;
+ pps_intcnt = 0;
+ }
+ }
+}
+
+/* increase frequency calibration interval length.
+ * It is doubled after four consecutive stable intervals.
+ */
+static inline void pps_inc_freq_interval(void)
+{
+ if (++pps_intcnt >= PPS_INTCOUNT) {
+ pps_intcnt = PPS_INTCOUNT;
+ if (pps_shift < PPS_INTMAX) {
+ pps_shift++;
+ pps_intcnt = 0;
+ }
+ }
+}
+
+/* update clock frequency based on MONOTONIC_RAW clock PPS signal
+ * timestamps
+ *
+ * At the end of the calibration interval the difference between the
+ * first and last MONOTONIC_RAW clock timestamps divided by the length
+ * of the interval becomes the frequency update. If the interval was
+ * too long, the data are discarded.
+ * Returns the difference between old and new frequency values.
+ */
+static long hardpps_update_freq(struct pps_normtime freq_norm)
+{
+ long delta, delta_mod;
+ s64 ftemp;
+
+ /* check if the frequency interval was too long */
+ if (freq_norm.sec > (2 << pps_shift)) {
+ time_status |= STA_PPSERROR;
+ pps_errcnt++;
+ pps_dec_freq_interval();
+ pr_err("hardpps: PPSERROR: interval too long - %ld s\n",
+ freq_norm.sec);
+ return 0;
+ }
+
+ /* here the raw frequency offset and wander (stability) is
+ * calculated. If the wander is less than the wander threshold
+ * the interval is increased; otherwise it is decreased.
+ */
+ ftemp = div_s64(((s64)(-freq_norm.nsec)) << NTP_SCALE_SHIFT,
+ freq_norm.sec);
+ delta = shift_right(ftemp - pps_freq, NTP_SCALE_SHIFT);
+ pps_freq = ftemp;
+ if (delta > PPS_MAXWANDER || delta < -PPS_MAXWANDER) {
+ pr_warning("hardpps: PPSWANDER: change=%ld\n", delta);
+ time_status |= STA_PPSWANDER;
+ pps_stbcnt++;
+ pps_dec_freq_interval();
+ } else { /* good sample */
+ pps_inc_freq_interval();
+ }
+
+ /* the stability metric is calculated as the average of recent
+ * frequency changes, but is used only for performance
+ * monitoring
+ */
+ delta_mod = delta;
+ if (delta_mod < 0)
+ delta_mod = -delta_mod;
+ pps_stabil += (div_s64(((s64)delta_mod) <<
+ (NTP_SCALE_SHIFT - SHIFT_USEC),
+ NSEC_PER_USEC) - pps_stabil) >> PPS_INTMIN;
+
+ /* if enabled, the system clock frequency is updated */
+ if ((time_status & STA_PPSFREQ) != 0 &&
+ (time_status & STA_FREQHOLD) == 0) {
+ time_freq = pps_freq;
+ ntp_update_frequency();
+ }
+
+ return delta;
+}
+
+/* correct REALTIME clock phase error against PPS signal */
+static void hardpps_update_phase(long error)
+{
+ long correction = -error;
+ long jitter;
+
+ /* add the sample to the median filter */
+ pps_phase_filter_add(correction);
+ correction = pps_phase_filter_get(&jitter);
+
+ /* Nominal jitter is due to PPS signal noise. If it exceeds the
+ * threshold, the sample is discarded; otherwise, if so enabled,
+ * the time offset is updated.
+ */
+ if (jitter > (pps_jitter << PPS_POPCORN)) {
+ pr_warning("hardpps: PPSJITTER: jitter=%ld, limit=%ld\n",
+ jitter, (pps_jitter << PPS_POPCORN));
+ time_status |= STA_PPSJITTER;
+ pps_jitcnt++;
+ } else if (time_status & STA_PPSTIME) {
+ /* correct the time using the phase offset */
+ time_offset = div_s64(((s64)correction) << NTP_SCALE_SHIFT,
+ NTP_INTERVAL_FREQ);
+ /* cancel running adjtime() */
+ time_adjust = 0;
+ }
+ /* update jitter */
+ pps_jitter += (jitter - pps_jitter) >> PPS_INTMIN;
+}
+
+/*
+ * hardpps() - discipline CPU clock oscillator to external PPS signal
+ *
+ * This routine is called at each PPS signal arrival in order to
+ * discipline the CPU clock oscillator to the PPS signal. It takes two
+ * parameters: REALTIME and MONOTONIC_RAW clock timestamps. The former
+ * is used to correct clock phase error and the latter is used to
+ * correct the frequency.
+ *
+ * This code is based on David Mills's reference nanokernel
+ * implementation. It was mostly rewritten but keeps the same idea.
+ */
+void hardpps(const struct timespec *phase_ts, const struct timespec *raw_ts)
+{
+ struct pps_normtime pts_norm, freq_norm;
+ unsigned long flags;
+
+ pts_norm = pps_normalize_ts(*phase_ts);
+
+ write_seqlock_irqsave(&xtime_lock, flags);
+
+ /* clear the error bits, they will be set again if needed */
+ time_status &= ~(STA_PPSJITTER | STA_PPSWANDER | STA_PPSERROR);
+
+ /* indicate signal presence */
+ time_status |= STA_PPSSIGNAL;
+ pps_valid = PPS_VALID;
+
+ /* when called for the first time,
+ * just start the frequency interval */
+ if (unlikely(pps_fbase.tv_sec == 0)) {
+ pps_fbase = *raw_ts;
+ write_sequnlock_irqrestore(&xtime_lock, flags);
+ return;
+ }
+
+ /* ok, now we have a base for frequency calculation */
+ freq_norm = pps_normalize_ts(timespec_sub(*raw_ts, pps_fbase));
+
+ /* check that the signal is in the range
+ * [1s - MAXFREQ us, 1s + MAXFREQ us], otherwise reject it */
+ if ((freq_norm.sec == 0) ||
+ (freq_norm.nsec > MAXFREQ * freq_norm.sec) ||
+ (freq_norm.nsec < -MAXFREQ * freq_norm.sec)) {
+ time_status |= STA_PPSJITTER;
+ /* restart the frequency calibration interval */
+ pps_fbase = *raw_ts;
+ write_sequnlock_irqrestore(&xtime_lock, flags);
+ pr_err("hardpps: PPSJITTER: bad pulse\n");
+ return;
+ }
+
+ /* signal is ok */
+
+ /* check if the current frequency interval is finished */
+ if (freq_norm.sec >= (1 << pps_shift)) {
+ pps_calcnt++;
+ /* restart the frequency calibration interval */
+ pps_fbase = *raw_ts;
+ hardpps_update_freq(freq_norm);
+ }
+
+ hardpps_update_phase(pts_norm.nsec);
+
+ write_sequnlock_irqrestore(&xtime_lock, flags);
+}
+EXPORT_SYMBOL(hardpps);
+
+#endif /* CONFIG_NTP_PPS */
+
static int __init ntp_tick_adj_setup(char *str)
{
ntp_tick_adj = simple_strtol(str, NULL, 0);
--
1.7.2.3
Add an optional feature of PPSAPI, kernel consumer support, which uses
the added hardpps() function.
Signed-off-by: Alexander Gordeev <[email protected]>
---
Documentation/ioctl/ioctl-number.txt | 2 +-
drivers/pps/Kconfig | 1 +
drivers/pps/kapi.c | 23 ++++++++++++
drivers/pps/pps.c | 62 +++++++++++++++++++++++++++++++++-
include/linux/pps.h | 7 ++++
include/linux/pps_kernel.h | 6 +++
6 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index 33223ff..de2b113 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -249,7 +249,7 @@ Code Seq#(hex) Include File Comments
'p' 40-7F linux/nvram.h
'p' 80-9F linux/ppdev.h user-space parport
<mailto:[email protected]>
-'p' A1-A4 linux/pps.h LinuxPPS
+'p' A1-A5 linux/pps.h LinuxPPS
<mailto:[email protected]>
'q' 00-1F linux/serio.h
'q' 80-FF linux/telephony.h Internet PhoneJACK, Internet LineJACK
diff --git a/drivers/pps/Kconfig b/drivers/pps/Kconfig
index 33c9126..4823e47 100644
--- a/drivers/pps/Kconfig
+++ b/drivers/pps/Kconfig
@@ -7,6 +7,7 @@ menu "PPS support"
config PPS
tristate "PPS support"
depends on EXPERIMENTAL
+ select NTP_PPS
---help---
PPS (Pulse Per Second) is a special pulse provided by some GPS
antennae. Userland can use it to get a high-precision time
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 2bdfbed..0026c99 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -26,6 +26,7 @@
#include <linux/init.h>
#include <linux/sched.h>
#include <linux/time.h>
+#include <linux/timex.h>
#include <linux/spinlock.h>
#include <linux/fs.h>
#include <linux/pps_kernel.h>
@@ -38,6 +39,12 @@
/* PPS event workqueue */
struct workqueue_struct *pps_event_workqueue;
+/* state variables to bind kernel consumer */
+/* PPS API (RFC 2783): current source and mode for ``kernel consumer'' */
+DEFINE_SPINLOCK(pps_kc_hardpps_lock);
+void *pps_kc_hardpps_dev; /* some unique pointer to device */
+int pps_kc_hardpps_mode; /* mode bits for kernel consumer */
+
/*
* Local functions
*/
@@ -141,6 +148,16 @@ EXPORT_SYMBOL(pps_register_source);
void pps_unregister_source(struct pps_device *pps)
{
+ spin_lock(&pps_kc_hardpps_lock);
+ if (pps == pps_kc_hardpps_dev) {
+ pps_kc_hardpps_mode = 0;
+ pps_kc_hardpps_dev = NULL;
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_info(pps->dev, "unbound kernel consumer"
+ " on device removal\n");
+ } else
+ spin_unlock(&pps_kc_hardpps_lock);
+
pps_unregister_cdev(pps);
/* don't have to kfree(pps) here because it will be done on
@@ -213,6 +230,12 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
captured = ~0;
}
+ /* Pass some events to kernel consumer if activated */
+ spin_lock(&pps_kc_hardpps_lock);
+ if (pps == pps_kc_hardpps_dev && event & pps_kc_hardpps_mode)
+ hardpps(&ts->ts_real, &ts->ts_raw);
+ spin_unlock(&pps_kc_hardpps_lock);
+
/* Wake up if captured something */
if (captured) {
pps->last_ev++;
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index d3267f3..6cedb8b 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -197,9 +197,69 @@ static long pps_cdev_ioctl(struct file *file,
break;
}
+ case PPS_KC_BIND: {
+ struct pps_bind_args bind_args;
+
+ dev_dbg(pps->dev, "PPS_KC_BIND\n");
+
+ /* Check the capabilities */
+ if (!capable(CAP_SYS_TIME))
+ return -EPERM;
+
+ if (copy_from_user(&bind_args, uarg,
+ sizeof(struct pps_bind_args)))
+ return -EFAULT;
+
+ /* Check for supported capabilities */
+ if ((bind_args.edge & ~pps->info.mode) != 0) {
+ dev_err(pps->dev, "unsupported capabilities (%x)\n",
+ bind_args.edge);
+ return -EINVAL;
+ }
+
+ /* Validate parameters roughly */
+ if (bind_args.tsformat != PPS_TSFMT_TSPEC ||
+ (bind_args.edge & ~PPS_CAPTUREBOTH) != 0 ||
+ bind_args.consumer != PPS_KC_HARDPPS) {
+ dev_err(pps->dev, "invalid kernel consumer bind"
+ " parameters (%x)\n", bind_args.edge);
+ return -EINVAL;
+ }
+
+ /* Check if another consumer is already bound */
+ spin_lock(&pps_kc_hardpps_lock);
+
+ if (bind_args.edge == 0)
+ if (pps_kc_hardpps_dev == pps) {
+ pps_kc_hardpps_mode = 0;
+ pps_kc_hardpps_dev = NULL;
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_info(pps->dev, "unbound kernel"
+ " consumer\n");
+ } else {
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_err(pps->dev, "selected kernel consumer"
+ " is not bound\n");
+ return -EINVAL;
+ }
+ else
+ if (pps_kc_hardpps_dev == NULL ||
+ pps_kc_hardpps_dev == pps) {
+ pps_kc_hardpps_mode = bind_args.edge;
+ pps_kc_hardpps_dev = pps;
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_info(pps->dev, "bound kernel consumer: "
+ "edge=0x%x\n", bind_args.edge);
+ } else {
+ spin_unlock(&pps_kc_hardpps_lock);
+ dev_err(pps->dev, "another kernel consumer"
+ " is already bound\n");
+ return -EINVAL;
+ }
+ break;
+ }
default:
return -ENOTTY;
- break;
}
return 0;
diff --git a/include/linux/pps.h b/include/linux/pps.h
index 0194ab0..a9bb1d9 100644
--- a/include/linux/pps.h
+++ b/include/linux/pps.h
@@ -114,11 +114,18 @@ struct pps_fdata {
struct pps_ktime timeout;
};
+struct pps_bind_args {
+ int tsformat; /* format of time stamps */
+ int edge; /* selected event type */
+ int consumer; /* selected kernel consumer */
+};
+
#include <linux/ioctl.h>
#define PPS_GETPARAMS _IOR('p', 0xa1, struct pps_kparams *)
#define PPS_SETPARAMS _IOW('p', 0xa2, struct pps_kparams *)
#define PPS_GETCAP _IOR('p', 0xa3, int *)
#define PPS_FETCH _IOWR('p', 0xa4, struct pps_fdata *)
+#define PPS_KC_BIND _IOW('p', 0xa5, struct pps_bind_args *)
#endif /* _PPS_H_ */
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 39fc7125..10e10c8 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -87,6 +87,12 @@ struct pps_device {
extern struct device_attribute pps_attrs[];
+/* state variables to bind kernel consumer */
+/* PPS API (RFC 2783): current source and mode for ``kernel consumer'' */
+extern spinlock_t pps_kc_hardpps_lock;
+extern void *pps_kc_hardpps_dev; /* some unique pointer to device */
+extern int pps_kc_hardpps_mode; /* mode bits for kernel consumer */
+
extern struct workqueue_struct *pps_event_workqueue;
/*
--
1.7.2.3
Since we now have direct pointers to struct pps_device everywhere it's
easy to use dev_* functions to print messages instead of plain printks.
Where dev_* cannot be used printks are converted to pr_*.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/pps-ktimer.c | 5 +++--
drivers/pps/clients/pps-ldisc.c | 2 ++
drivers/pps/kapi.c | 15 ++++++++-------
drivers/pps/pps.c | 25 +++++++++++++------------
4 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
index 966ead1..2728469 100644
--- a/drivers/pps/clients/pps-ktimer.c
+++ b/drivers/pps/clients/pps-ktimer.c
@@ -19,6 +19,7 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/kernel.h>
#include <linux/module.h>
@@ -45,7 +46,7 @@ static void pps_ktimer_event(unsigned long ptr)
/* First of all we get the time stamp... */
pps_get_ts(&ts);
- pr_info("PPS event at %lu\n", jiffies);
+ dev_info(pps->dev, "PPS event at %lu\n", jiffies);
pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
@@ -94,7 +95,7 @@ static int __init pps_ktimer_init(void)
pps = pps_register_source(&pps_ktimer_info,
PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
if (pps == NULL) {
- printk(KERN_ERR "cannot register ktimer source\n");
+ pr_err("cannot register PPS source\n");
return -ENOMEM;
}
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 1950b15..30789fa 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -19,6 +19,8 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/module.h>
#include <linux/serial_core.h>
#include <linux/tty.h>
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 98d4012..fbb89c6 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -19,6 +19,7 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/kernel.h>
#include <linux/module.h>
@@ -80,20 +81,20 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
/* Sanity checks */
if ((info->mode & default_params) != default_params) {
- printk(KERN_ERR "pps: %s: unsupported default parameters\n",
+ pr_err("pps: %s: unsupported default parameters\n",
info->name);
err = -EINVAL;
goto pps_register_source_exit;
}
if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
info->echo == NULL) {
- printk(KERN_ERR "pps: %s: echo function is not defined\n",
+ pr_err("pps: %s: echo function is not defined\n",
info->name);
err = -EINVAL;
goto pps_register_source_exit;
}
if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
- printk(KERN_ERR "pps: %s: unspecified time format\n",
+ pr_err("pps: %s: unspecified time format\n",
info->name);
err = -EINVAL;
goto pps_register_source_exit;
@@ -138,7 +139,7 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
if (id >= PPS_MAX_SOURCES) {
spin_unlock_irq(&pps_idr_lock);
- printk(KERN_ERR "pps: %s: too many PPS sources in the system\n",
+ pr_err("pps: %s: too many PPS sources in the system\n",
info->name);
err = -EBUSY;
goto free_idr;
@@ -150,12 +151,12 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
/* Create the char device */
err = pps_register_cdev(pps);
if (err < 0) {
- printk(KERN_ERR "pps: %s: unable to create char device\n",
+ pr_err("%s: unable to create char device\n",
info->name);
goto free_idr;
}
- pr_info("new PPS source %s at ID %d\n", info->name, id);
+ dev_info(pps->dev, "new PPS source %s\n", info->name);
return pps;
@@ -168,7 +169,7 @@ kfree_pps:
kfree(pps);
pps_register_source_exit:
- printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
+ pr_err("%s: unable to register source\n", info->name);
return NULL;
}
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 1922f27..9f7c2e8 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -19,6 +19,7 @@
* Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/kernel.h>
#include <linux/module.h>
@@ -67,7 +68,7 @@ static long pps_cdev_ioctl(struct file *file,
switch (cmd) {
case PPS_GETPARAMS:
- pr_debug("PPS_GETPARAMS: source %d\n", pps->id);
+ dev_dbg(pps->dev, "PPS_GETPARAMS\n");
spin_lock_irq(&pps->lock);
@@ -83,7 +84,7 @@ static long pps_cdev_ioctl(struct file *file,
break;
case PPS_SETPARAMS:
- pr_debug("PPS_SETPARAMS: source %d\n", pps->id);
+ dev_dbg(pps->dev, "PPS_SETPARAMS\n");
/* Check the capabilities */
if (!capable(CAP_SYS_TIME))
@@ -93,14 +94,14 @@ static long pps_cdev_ioctl(struct file *file,
if (err)
return -EFAULT;
if (!(params.mode & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR))) {
- pr_debug("capture mode unspecified (%x)\n",
+ dev_dbg(pps->dev, "capture mode unspecified (%x)\n",
params.mode);
return -EINVAL;
}
/* Check for supported capabilities */
if ((params.mode & ~pps->info.mode) != 0) {
- pr_debug("unsupported capabilities (%x)\n",
+ dev_dbg(pps->dev, "unsupported capabilities (%x)\n",
params.mode);
return -EINVAL;
}
@@ -113,7 +114,7 @@ static long pps_cdev_ioctl(struct file *file,
/* Restore the read only parameters */
if ((params.mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
/* section 3.3 of RFC 2783 interpreted */
- pr_debug("time format unspecified (%x)\n",
+ dev_dbg(pps->dev, "time format unspecified (%x)\n",
params.mode);
pps->params.mode |= PPS_TSFMT_TSPEC;
}
@@ -126,7 +127,7 @@ static long pps_cdev_ioctl(struct file *file,
break;
case PPS_GETCAP:
- pr_debug("PPS_GETCAP: source %d\n", pps->id);
+ dev_dbg(pps->dev, "PPS_GETCAP\n");
err = put_user(pps->info.mode, iuarg);
if (err)
@@ -138,7 +139,7 @@ static long pps_cdev_ioctl(struct file *file,
struct pps_fdata fdata;
unsigned int ev;
- pr_debug("PPS_FETCH: source %d\n", pps->id);
+ dev_dbg(pps->dev, "PPS_FETCH\n");
err = copy_from_user(&fdata, uarg, sizeof(struct pps_fdata));
if (err)
@@ -153,7 +154,7 @@ static long pps_cdev_ioctl(struct file *file,
else {
unsigned long ticks;
- pr_debug("timeout %lld.%09d\n",
+ dev_dbg(pps->dev, "timeout %lld.%09d\n",
(long long) fdata.timeout.sec,
fdata.timeout.nsec);
ticks = fdata.timeout.sec * HZ;
@@ -171,7 +172,7 @@ static long pps_cdev_ioctl(struct file *file,
/* Check for pending signals */
if (err == -ERESTARTSYS) {
- pr_debug("pending signal caught\n");
+ dev_dbg(pps->dev, "pending signal caught\n");
return -EINTR;
}
@@ -240,7 +241,7 @@ int pps_register_cdev(struct pps_device *pps)
err = cdev_add(&pps->cdev, devt, 1);
if (err) {
- printk(KERN_ERR "pps: %s: failed to add char device %d:%d\n",
+ pr_err("%s: failed to add char device %d:%d\n",
pps->info.name, MAJOR(pps_devt), pps->id);
return err;
}
@@ -282,14 +283,14 @@ static int __init pps_init(void)
pps_class = class_create(THIS_MODULE, "pps");
if (!pps_class) {
- printk(KERN_ERR "pps: failed to allocate class\n");
+ pr_err("failed to allocate class\n");
return -ENOMEM;
}
pps_class->dev_attrs = pps_attrs;
err = alloc_chrdev_region(&pps_devt, 0, PPS_MAX_SOURCES, "pps");
if (err < 0) {
- printk(KERN_ERR "pps: failed to allocate char device region\n");
+ pr_err("failed to allocate char device region\n");
goto remove_class;
}
--
1.7.2.3
This way less overhead is involved when running production kernel.
If you want to debug a pps client module please define DEBUG to enable
the checks.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 33 ++++++++++-----------------------
1 files changed, 10 insertions(+), 23 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index fe832aa..54261c4 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -81,25 +81,14 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
int err;
/* Sanity checks */
- if ((info->mode & default_params) != default_params) {
- pr_err("pps: %s: unsupported default parameters\n",
- info->name);
- err = -EINVAL;
- goto pps_register_source_exit;
- }
- if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
- info->echo == NULL) {
- pr_err("pps: %s: echo function is not defined\n",
- info->name);
- err = -EINVAL;
- goto pps_register_source_exit;
- }
- if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
- pr_err("pps: %s: unspecified time format\n",
- info->name);
- err = -EINVAL;
- goto pps_register_source_exit;
- }
+
+ /* default_params should be supported */
+ BUG_ON((info->mode & default_params) != default_params);
+ /* echo function should be defined if we are asked to call it */
+ BUG_ON((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
+ info->echo == NULL);
+ /* time format should be specified */
+ BUG_ON((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0);
/* Allocate memory for the new PPS source struct */
pps = kzalloc(sizeof(struct pps_device), GFP_KERNEL);
@@ -179,10 +168,8 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
int captured = 0;
struct pps_ktime ts_real;
- if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
- dev_err(pps->dev, "unknown event (%x)\n", event);
- return;
- }
+ /* check event type */
+ BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
dev_dbg(pps->dev, "PPS event at %ld.%09ld\n",
ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
--
1.7.2.3
MONOTONIC_RAW clock timestamps are ideally suited for frequency
calculation and also fit well into the original NTP hardpps design. Now
phase and frequency can be adjusted separately: the former based on
REALTIME clock and the latter based on MONOTONIC_RAW clock.
A new function getnstime_raw_and_real is added to timekeeping subsystem
to capture both timestamps at the same time and atomically.
Signed-off-by: Alexander Gordeev <[email protected]>
---
include/linux/pps_kernel.h | 3 ++-
include/linux/time.h | 2 ++
kernel/time/timekeeping.c | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 1 deletions(-)
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 5af0498..39fc7125 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -48,6 +48,7 @@ struct pps_source_info {
};
struct pps_event_time {
+ struct timespec ts_raw;
struct timespec ts_real;
};
@@ -111,7 +112,7 @@ static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
static inline void pps_get_ts(struct pps_event_time *ts)
{
- getnstimeofday(&ts->ts_real);
+ getnstime_raw_and_real(&ts->ts_raw, &ts->ts_real);
}
#endif /* LINUX_PPS_KERNEL_H */
diff --git a/include/linux/time.h b/include/linux/time.h
index 9f15ac7..1e6d3b5 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -158,6 +158,8 @@ extern unsigned int alarm_setitimer(unsigned int seconds);
extern int do_getitimer(int which, struct itimerval *value);
extern void getnstimeofday(struct timespec *tv);
extern void getrawmonotonic(struct timespec *ts);
+extern void getnstime_raw_and_real(struct timespec *ts_raw,
+ struct timespec *ts_real);
extern void getboottime(struct timespec *ts);
extern void monotonic_to_bootbased(struct timespec *ts);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 49010d8..947c7dc 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -285,6 +285,44 @@ void ktime_get_ts(struct timespec *ts)
EXPORT_SYMBOL_GPL(ktime_get_ts);
/**
+ * getnstime_raw_and_real - Returns both the time of day an raw
+ * monotonic time in a timespec format
+ * @ts_mono_raw: pointer to the timespec to be set to raw
+ * monotonic time
+ * @ts_real: pointer to the timespec to be set to the time
+ * of day
+ */
+void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
+{
+ unsigned long seq;
+ s64 nsecs_raw, nsecs_real;
+
+ WARN_ON_ONCE(timekeeping_suspended);
+
+ do {
+ u32 arch_offset;
+
+ seq = read_seqbegin(&xtime_lock);
+
+ *ts_raw = raw_time;
+ *ts_real = xtime;
+
+ nsecs_raw = timekeeping_get_ns_raw();
+ nsecs_real = timekeeping_get_ns();
+
+ /* If arch requires, add in gettimeoffset() */
+ arch_offset = arch_gettimeoffset();
+ nsecs_raw += arch_offset;
+ nsecs_real += arch_offset;
+
+ } while (read_seqretry(&xtime_lock, seq));
+
+ timespec_add_ns(ts_raw, nsecs_raw);
+ timespec_add_ns(ts_real, nsecs_real);
+}
+EXPORT_SYMBOL(getnstime_raw_and_real);
+
+/**
* do_gettimeofday - Returns the time of day in a timeval
* @tv: pointer to the timeval to be set
*
--
1.7.2.3
Since now idr is only used to manage char device id's and not used in
kernel API anymore it should be moved to pps.c. This also makes it
possible to release id only at actual device freeing so nobody can
register a pps device with the same id while our device is not freed
yet.
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/kapi.c | 56 ++-------------------------------------------------
drivers/pps/pps.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 52 insertions(+), 54 deletions(-)
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index fbb89c6..f5b2b78 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -27,19 +27,11 @@
#include <linux/sched.h>
#include <linux/time.h>
#include <linux/spinlock.h>
-#include <linux/idr.h>
#include <linux/fs.h>
#include <linux/pps_kernel.h>
#include <linux/slab.h>
/*
- * Local variables
- */
-
-static DEFINE_SPINLOCK(pps_idr_lock);
-static DEFINE_IDR(pps_idr);
-
-/*
* Local functions
*/
@@ -76,7 +68,6 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
int default_params)
{
struct pps_device *pps;
- int id;
int err;
/* Sanity checks */
@@ -117,54 +108,18 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
init_waitqueue_head(&pps->queue);
spin_lock_init(&pps->lock);
- /* Get new ID for the new PPS source */
- if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
- err = -ENOMEM;
- goto kfree_pps;
- }
-
- spin_lock_irq(&pps_idr_lock);
-
- /* Now really allocate the PPS source.
- * After idr_get_new() calling the new source will be freely available
- * into the kernel.
- */
- err = idr_get_new(&pps_idr, pps, &id);
- if (err < 0) {
- spin_unlock_irq(&pps_idr_lock);
- goto kfree_pps;
- }
-
- id = id & MAX_ID_MASK;
- if (id >= PPS_MAX_SOURCES) {
- spin_unlock_irq(&pps_idr_lock);
-
- pr_err("pps: %s: too many PPS sources in the system\n",
- info->name);
- err = -EBUSY;
- goto free_idr;
- }
- pps->id = id;
-
- spin_unlock_irq(&pps_idr_lock);
-
/* Create the char device */
err = pps_register_cdev(pps);
if (err < 0) {
pr_err("%s: unable to create char device\n",
info->name);
- goto free_idr;
+ goto kfree_pps;
}
dev_info(pps->dev, "new PPS source %s\n", info->name);
return pps;
-free_idr:
- spin_lock_irq(&pps_idr_lock);
- idr_remove(&pps_idr, id);
- spin_unlock_irq(&pps_idr_lock);
-
kfree_pps:
kfree(pps);
@@ -184,15 +139,10 @@ EXPORT_SYMBOL(pps_register_source);
void pps_unregister_source(struct pps_device *pps)
{
- unsigned int id = pps->id;
-
pps_unregister_cdev(pps);
- spin_lock_irq(&pps_idr_lock);
- idr_remove(&pps_idr, pps->id);
- spin_unlock_irq(&pps_idr_lock);
-
- kfree(pps);
+ /* don't have to kfree(pps) here because it will be done on
+ * device destruction */
}
EXPORT_SYMBOL(pps_unregister_source);
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 9f7c2e8..79b4455 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -30,6 +30,7 @@
#include <linux/cdev.h>
#include <linux/poll.h>
#include <linux/pps_kernel.h>
+#include <linux/slab.h>
/*
* Local variables
@@ -38,6 +39,9 @@
static dev_t pps_devt;
static struct class *pps_class;
+static DEFINE_SPINLOCK(pps_idr_lock);
+static DEFINE_IDR(pps_idr);
+
/*
* Char device methods
*/
@@ -229,11 +233,48 @@ static const struct file_operations pps_cdev_fops = {
.release = pps_cdev_release,
};
+static void pps_device_destruct(struct device *dev)
+{
+ struct pps_device *pps = dev_get_drvdata(dev);
+
+ /* release id here to protect others from using it while it's
+ * still in use */
+ spin_lock_irq(&pps_idr_lock);
+ idr_remove(&pps_idr, pps->id);
+ spin_unlock_irq(&pps_idr_lock);
+
+ kfree(dev);
+ kfree(pps);
+}
+
int pps_register_cdev(struct pps_device *pps)
{
int err;
dev_t devt;
+ /* Get new ID for the new PPS source */
+ if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0)
+ return -ENOMEM;
+
+ /* Now really allocate the PPS source.
+ * After idr_get_new() calling the new source will be freely available
+ * into the kernel.
+ */
+ spin_lock_irq(&pps_idr_lock);
+ err = idr_get_new(&pps_idr, pps, &pps->id);
+ spin_unlock_irq(&pps_idr_lock);
+
+ if (err < 0)
+ return err;
+
+ pps->id &= MAX_ID_MASK;
+ if (pps->id >= PPS_MAX_SOURCES) {
+ pr_err("%s: too many PPS sources in the system\n",
+ pps->info.name);
+ err = -EBUSY;
+ goto free_idr;
+ }
+
devt = MKDEV(MAJOR(pps_devt), pps->id);
cdev_init(&pps->cdev, &pps_cdev_fops);
@@ -243,13 +284,15 @@ int pps_register_cdev(struct pps_device *pps)
if (err) {
pr_err("%s: failed to add char device %d:%d\n",
pps->info.name, MAJOR(pps_devt), pps->id);
- return err;
+ goto free_idr;
}
pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
"pps%d", pps->id);
if (IS_ERR(pps->dev))
goto del_cdev;
+ pps->dev->release = pps_device_destruct;
+
pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
MAJOR(pps_devt), pps->id);
@@ -258,6 +301,11 @@ int pps_register_cdev(struct pps_device *pps)
del_cdev:
cdev_del(&pps->cdev);
+free_idr:
+ spin_lock_irq(&pps_idr_lock);
+ idr_remove(&pps_idr, pps->id);
+ spin_unlock_irq(&pps_idr_lock);
+
return err;
}
--
1.7.2.3
Add a helper function to gather timestamps. This way clients don't have
to duplicate it.
Acked-by: Rodolfo Giometti <[email protected]>
Signed-off-by: Alexander Gordeev <[email protected]>
---
drivers/pps/clients/pps-ktimer.c | 9 ++-------
drivers/pps/clients/pps-ldisc.c | 18 ++++++------------
drivers/pps/kapi.c | 19 ++++++++++++-------
include/linux/pps_kernel.h | 19 ++++++++++++++++++-
include/linux/serial_core.h | 5 +++--
include/linux/tty_ldisc.h | 5 +++--
6 files changed, 44 insertions(+), 31 deletions(-)
diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
index e7ef5b8..e1bdd8b 100644
--- a/drivers/pps/clients/pps-ktimer.c
+++ b/drivers/pps/clients/pps-ktimer.c
@@ -40,18 +40,13 @@ static struct timer_list ktimer;
static void pps_ktimer_event(unsigned long ptr)
{
- struct timespec __ts;
- struct pps_ktime ts;
+ struct pps_event_time ts;
/* First of all we get the time stamp... */
- getnstimeofday(&__ts);
+ pps_get_ts(&ts);
pr_info("PPS event at %lu\n", jiffies);
- /* ... and translate it to PPS time data struct */
- ts.sec = __ts.tv_sec;
- ts.nsec = __ts.tv_nsec;
-
pps_event(source, &ts, PPS_CAPTUREASSERT, NULL);
mod_timer(&ktimer, jiffies + HZ);
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 8e1932d..20fc9f7 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -27,26 +27,20 @@
#define PPS_TTY_MAGIC 0x0001
static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
- struct timespec *ts)
+ struct pps_event_time *ts)
{
int id = (long)tty->disc_data;
- struct timespec __ts;
- struct pps_ktime pps_ts;
+ struct pps_event_time __ts;
/* First of all we get the time stamp... */
- getnstimeofday(&__ts);
+ pps_get_ts(&__ts);
/* Does caller give us a timestamp? */
- if (ts) { /* Yes. Let's use it! */
- pps_ts.sec = ts->tv_sec;
- pps_ts.nsec = ts->tv_nsec;
- } else { /* No. Do it ourself! */
- pps_ts.sec = __ts.tv_sec;
- pps_ts.nsec = __ts.tv_nsec;
- }
+ if (!ts) /* No. Do it ourself! */
+ ts = &__ts;
/* Now do the PPS event report */
- pps_event(id, &pps_ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
+ pps_event(id, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
NULL);
pr_debug("PPS %s at %lu on source #%d\n",
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index 3f89f5eb..b431d83 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -268,11 +268,12 @@ EXPORT_SYMBOL(pps_unregister_source);
* pps->info.echo(source, event, data);
*/
-void pps_event(int source, struct pps_ktime *ts, int event, void *data)
+void pps_event(int source, struct pps_event_time *ts, int event, void *data)
{
struct pps_device *pps;
unsigned long flags;
int captured = 0;
+ struct pps_ktime ts_real;
if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
@@ -284,8 +285,10 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
if (!pps)
return;
- pr_debug("PPS event on source %d at %llu.%06u\n",
- pps->id, (unsigned long long) ts->sec, ts->nsec);
+ pr_debug("PPS event on source %d at %ld.%09ld\n",
+ pps->id, ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
+
+ timespec_to_pps_ktime(&ts_real, ts->ts_real);
spin_lock_irqsave(&pps->lock, flags);
@@ -299,10 +302,11 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
(pps->params.mode & PPS_CAPTUREASSERT)) {
/* We have to add an offset? */
if (pps->params.mode & PPS_OFFSETASSERT)
- pps_add_offset(ts, &pps->params.assert_off_tu);
+ pps_add_offset(&ts_real,
+ &pps->params.assert_off_tu);
/* Save the time stamp */
- pps->assert_tu = *ts;
+ pps->assert_tu = ts_real;
pps->assert_sequence++;
pr_debug("capture assert seq #%u for source %d\n",
pps->assert_sequence, source);
@@ -313,10 +317,11 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
(pps->params.mode & PPS_CAPTURECLEAR)) {
/* We have to add an offset? */
if (pps->params.mode & PPS_OFFSETCLEAR)
- pps_add_offset(ts, &pps->params.clear_off_tu);
+ pps_add_offset(&ts_real,
+ &pps->params.clear_off_tu);
/* Save the time stamp */
- pps->clear_tu = *ts;
+ pps->clear_tu = ts_real;
pps->clear_sequence++;
pr_debug("capture clear seq #%u for source %d\n",
pps->clear_sequence, source);
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index c3aed4b..32aa676 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -43,6 +43,10 @@ struct pps_source_info {
struct device *dev;
};
+struct pps_event_time {
+ struct timespec ts_real;
+};
+
/* The main struct */
struct pps_device {
struct pps_source_info info; /* PSS source info */
@@ -88,7 +92,20 @@ extern int pps_register_source(struct pps_source_info *info,
extern void pps_unregister_source(int source);
extern int pps_register_cdev(struct pps_device *pps);
extern void pps_unregister_cdev(struct pps_device *pps);
-extern void pps_event(int source, struct pps_ktime *ts, int event, void *data);
+extern void pps_event(int source, struct pps_event_time *ts, int event,
+ void *data);
+
+static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
+ struct timespec ts)
+{
+ kt->sec = ts.tv_sec;
+ kt->nsec = ts.tv_nsec;
+}
+
+static inline void pps_get_ts(struct pps_event_time *ts)
+{
+ getnstimeofday(&ts->ts_real);
+}
#endif /* LINUX_PPS_KERNEL_H */
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 563e234..55c8192 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -206,6 +206,7 @@
#include <linux/tty.h>
#include <linux/mutex.h>
#include <linux/sysrq.h>
+#include <linux/pps_kernel.h>
struct uart_port;
struct serial_struct;
@@ -510,10 +511,10 @@ uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
struct uart_state *state = uport->state;
struct tty_port *port = &state->port;
struct tty_ldisc *ld = tty_ldisc_ref(port->tty);
- struct timespec ts;
+ struct pps_event_time ts;
if (ld && ld->ops->dcd_change)
- getnstimeofday(&ts);
+ pps_get_ts(&ts);
uport->icount.dcd++;
#ifdef CONFIG_HARD_PPS
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 526d66f..763e061 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -101,7 +101,7 @@
* any pending driver I/O is completed.
*
* void (*dcd_change)(struct tty_struct *tty, unsigned int status,
- * struct timespec *ts)
+ * struct pps_event_time *ts)
*
* Tells the discipline that the DCD pin has changed its status and
* the relative timestamp. Pointer ts can be NULL.
@@ -109,6 +109,7 @@
#include <linux/fs.h>
#include <linux/wait.h>
+#include <linux/pps_kernel.h>
struct tty_ldisc_ops {
int magic;
@@ -143,7 +144,7 @@ struct tty_ldisc_ops {
char *fp, int count);
void (*write_wakeup)(struct tty_struct *);
void (*dcd_change)(struct tty_struct *, unsigned int,
- struct timespec *);
+ struct pps_event_time *);
struct module *owner;
--
1.7.2.3
On Thu, 2010-11-18 at 19:01 +0300, Alexander Gordeev wrote:
> MONOTONIC_RAW clock timestamps are ideally suited for frequency
> calculation and also fit well into the original NTP hardpps design. Now
> phase and frequency can be adjusted separately: the former based on
> REALTIME clock and the latter based on MONOTONIC_RAW clock.
> A new function getnstime_raw_and_real is added to timekeeping subsystem
> to capture both timestamps at the same time and atomically.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
Acked-by: John Stultz <[email protected]>
> ---
> include/linux/pps_kernel.h | 3 ++-
> include/linux/time.h | 2 ++
> kernel/time/timekeeping.c | 38 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 42 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index 5af0498..39fc7125 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -48,6 +48,7 @@ struct pps_source_info {
> };
>
> struct pps_event_time {
> + struct timespec ts_raw;
> struct timespec ts_real;
> };
>
> @@ -111,7 +112,7 @@ static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
>
> static inline void pps_get_ts(struct pps_event_time *ts)
> {
> - getnstimeofday(&ts->ts_real);
> + getnstime_raw_and_real(&ts->ts_raw, &ts->ts_real);
> }
>
> #endif /* LINUX_PPS_KERNEL_H */
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 9f15ac7..1e6d3b5 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -158,6 +158,8 @@ extern unsigned int alarm_setitimer(unsigned int seconds);
> extern int do_getitimer(int which, struct itimerval *value);
> extern void getnstimeofday(struct timespec *tv);
> extern void getrawmonotonic(struct timespec *ts);
> +extern void getnstime_raw_and_real(struct timespec *ts_raw,
> + struct timespec *ts_real);
> extern void getboottime(struct timespec *ts);
> extern void monotonic_to_bootbased(struct timespec *ts);
>
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 49010d8..947c7dc 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -285,6 +285,44 @@ void ktime_get_ts(struct timespec *ts)
> EXPORT_SYMBOL_GPL(ktime_get_ts);
>
> /**
> + * getnstime_raw_and_real - Returns both the time of day an raw
> + * monotonic time in a timespec format
> + * @ts_mono_raw: pointer to the timespec to be set to raw
> + * monotonic time
> + * @ts_real: pointer to the timespec to be set to the time
> + * of day
> + */
> +void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec *ts_real)
> +{
> + unsigned long seq;
> + s64 nsecs_raw, nsecs_real;
> +
> + WARN_ON_ONCE(timekeeping_suspended);
> +
> + do {
> + u32 arch_offset;
> +
> + seq = read_seqbegin(&xtime_lock);
> +
> + *ts_raw = raw_time;
> + *ts_real = xtime;
> +
> + nsecs_raw = timekeeping_get_ns_raw();
> + nsecs_real = timekeeping_get_ns();
> +
> + /* If arch requires, add in gettimeoffset() */
> + arch_offset = arch_gettimeoffset();
> + nsecs_raw += arch_offset;
> + nsecs_real += arch_offset;
> +
> + } while (read_seqretry(&xtime_lock, seq));
> +
> + timespec_add_ns(ts_raw, nsecs_raw);
> + timespec_add_ns(ts_real, nsecs_real);
> +}
> +EXPORT_SYMBOL(getnstime_raw_and_real);
> +
> +/**
> * do_gettimeofday - Returns the time of day in a timeval
> * @tv: pointer to the timeval to be set
> *
On Thu, Nov 18, 2010 at 07:00:58PM +0300, Alexander Gordeev wrote:
> Using device index as a pointer needs some unnecessary work to be done
> every time the pointer is needed (in irq handler for example).
> Using a direct pointer is much more easy (and safe as well).
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/clients/pps-ktimer.c | 30 ++++-----
> drivers/pps/clients/pps-ldisc.c | 52 ++++++++++------
> drivers/pps/kapi.c | 124 +++++++++-----------------------------
> drivers/pps/pps.c | 22 ++-----
> include/linux/pps_kernel.h | 23 +++----
> 5 files changed, 90 insertions(+), 161 deletions(-)
>
> diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
> index e1bdd8b..966ead1 100644
> --- a/drivers/pps/clients/pps-ktimer.c
> +++ b/drivers/pps/clients/pps-ktimer.c
> @@ -31,7 +31,7 @@
> * Global variables
> */
>
> -static int source;
> +static struct pps_device *pps;
> static struct timer_list ktimer;
>
> /*
> @@ -47,7 +47,7 @@ static void pps_ktimer_event(unsigned long ptr)
>
> pr_info("PPS event at %lu\n", jiffies);
>
> - pps_event(source, &ts, PPS_CAPTUREASSERT, NULL);
> + pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
>
> mod_timer(&ktimer, jiffies + HZ);
> }
> @@ -56,12 +56,11 @@ static void pps_ktimer_event(unsigned long ptr)
> * The echo function
> */
>
> -static void pps_ktimer_echo(int source, int event, void *data)
> +static void pps_ktimer_echo(struct pps_device *pps, int event, void *data)
> {
> - pr_info("echo %s %s for source %d\n",
> + dev_info(pps->dev, "echo %s %s\n",
> event & PPS_CAPTUREASSERT ? "assert" : "",
> - event & PPS_CAPTURECLEAR ? "clear" : "",
> - source);
> + event & PPS_CAPTURECLEAR ? "clear" : "");
> }
>
> /*
> @@ -84,30 +83,27 @@ static struct pps_source_info pps_ktimer_info = {
>
> static void __exit pps_ktimer_exit(void)
> {
> - del_timer_sync(&ktimer);
> - pps_unregister_source(source);
> + dev_info(pps->dev, "ktimer PPS source unregistered\n");
>
> - pr_info("ktimer PPS source unregistered\n");
> + del_timer_sync(&ktimer);
> + pps_unregister_source(pps);
> }
>
> static int __init pps_ktimer_init(void)
> {
> - int ret;
> -
> - ret = pps_register_source(&pps_ktimer_info,
> + pps = pps_register_source(&pps_ktimer_info,
> PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
> - if (ret < 0) {
> + if (pps == NULL) {
> printk(KERN_ERR "cannot register ktimer source\n");
> - return ret;
> + return -ENOMEM;
> }
> - source = ret;
>
> setup_timer(&ktimer, pps_ktimer_event, 0);
> mod_timer(&ktimer, jiffies + HZ);
>
> - pr_info("ktimer PPS source registered at %d\n", source);
> + dev_info(pps->dev, "ktimer PPS source registered\n");
>
> - return 0;
> + return 0;
> }
>
> module_init(pps_ktimer_init);
> diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
> index 20fc9f7..1950b15 100644
> --- a/drivers/pps/clients/pps-ldisc.c
> +++ b/drivers/pps/clients/pps-ldisc.c
> @@ -23,14 +23,18 @@
> #include <linux/serial_core.h>
> #include <linux/tty.h>
> #include <linux/pps_kernel.h>
> +#include <linux/spinlock.h>
>
> #define PPS_TTY_MAGIC 0x0001
>
> +static DEFINE_SPINLOCK(pps_ldisc_lock);
> +
> static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> struct pps_event_time *ts)
> {
> - int id = (long)tty->disc_data;
> + struct pps_device *pps;
> struct pps_event_time __ts;
> + unsigned long flags;
>
> /* First of all we get the time stamp... */
> pps_get_ts(&__ts);
> @@ -39,12 +43,18 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> if (!ts) /* No. Do it ourself! */
> ts = &__ts;
>
> - /* Now do the PPS event report */
> - pps_event(id, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
> - NULL);
> + spin_lock_irqsave(&pps_ldisc_lock, flags);
>
> - pr_debug("PPS %s at %lu on source #%d\n",
> - status ? "assert" : "clear", jiffies, id);
> + /* Now do the PPS event report */
> + pps = (struct pps_device *)tty->disc_data;
> + if (pps != NULL) {
> + pps_event(pps, ts, status ? PPS_CAPTUREASSERT :
> + PPS_CAPTURECLEAR, NULL);
> + spin_unlock_irqrestore(&pps_ldisc_lock, flags);
> + dev_dbg(pps->dev, "PPS %s at %lu\n",
> + status ? "assert" : "clear", jiffies);
I think you should swap these two lines because after you release the
lock the pps pointer may be not valid...
> + } else
> + spin_unlock_irqrestore(&pps_ldisc_lock, flags);
> }
>
> static int (*alias_n_tty_open)(struct tty_struct *tty);
> @@ -54,7 +64,7 @@ static int pps_tty_open(struct tty_struct *tty)
> struct pps_source_info info;
> struct tty_driver *drv = tty->driver;
> int index = tty->index + drv->name_base;
> - int ret;
> + struct pps_device *pps;
>
> info.owner = THIS_MODULE;
> info.dev = NULL;
> @@ -64,20 +74,22 @@ static int pps_tty_open(struct tty_struct *tty)
> PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
> PPS_CANWAIT | PPS_TSFMT_TSPEC;
>
> - ret = pps_register_source(&info, PPS_CAPTUREBOTH | \
> + pps = pps_register_source(&info, PPS_CAPTUREBOTH | \
> PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
> - if (ret < 0) {
> + if (pps == NULL) {
> pr_err("cannot register PPS source \"%s\"\n", info.path);
> - return ret;
> + return -ENOMEM;
> }
> - tty->disc_data = (void *)(long)ret;
> +
> + spin_lock_irq(&pps_ldisc_lock);
> + tty->disc_data = pps;
> + spin_unlock_irq(&pps_ldisc_lock);
Maybe this lock is useless... however, are we sure that before setting
tty->disc_data to pps its value is null? Otherwise the dcd_change may
be called with an oops! We cannot control serial port IRQ
generation... :-/
> /* Should open N_TTY ldisc too */
> - ret = alias_n_tty_open(tty);
> - if (ret < 0)
> - pps_unregister_source((long)tty->disc_data);
> + if (alias_n_tty_open(tty) < 0)
Are you sure this code style is ?canonical?? What does checkpatch say?
;)
> + pps_unregister_source(pps);
If the above function fails I suppose you should invalidate
tty->disc_data, then unregister the pps source and, in the end, return
error (I know old code returns 0 anywhere, but I think it can be fixed
right now! ;).
> - pr_info("PPS source #%d \"%s\" added\n", ret, info.path);
> + dev_info(pps->dev, "source \"%s\" added\n", info.path);
>
> return 0;
> }
> @@ -86,12 +98,16 @@ static void (*alias_n_tty_close)(struct tty_struct *tty);
>
> static void pps_tty_close(struct tty_struct *tty)
> {
> - int id = (long)tty->disc_data;
> + struct pps_device *pps = (struct pps_device *)tty->disc_data;
>
> - pps_unregister_source(id);
> alias_n_tty_close(tty);
>
> - pr_info("PPS source #%d removed\n", id);
> + spin_lock_irq(&pps_ldisc_lock);
> + tty->disc_data = NULL;
> + spin_unlock_irq(&pps_ldisc_lock);
> +
> + dev_info(pps->dev, "removed\n");
> + pps_unregister_source(pps);
> }
>
> static struct tty_ldisc_ops pps_ldisc_ops;
I suppose you can solve all your problems if you do the lock into
pps_tty_init...
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Nov 18, 2010 at 07:00:59PM +0300, Alexander Gordeev wrote:
> Since we now have direct pointers to struct pps_device everywhere it's
> easy to use dev_* functions to print messages instead of plain printks.
> Where dev_* cannot be used printks are converted to pr_*.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/clients/pps-ktimer.c | 5 +++--
> drivers/pps/clients/pps-ldisc.c | 2 ++
> drivers/pps/kapi.c | 15 ++++++++-------
> drivers/pps/pps.c | 25 +++++++++++++------------
> 4 files changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
> index 966ead1..2728469 100644
> --- a/drivers/pps/clients/pps-ktimer.c
> +++ b/drivers/pps/clients/pps-ktimer.c
> @@ -19,6 +19,7 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Instead of redefining pr_fmt() in each file you may do it once into
include file pps_kernel.h...
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -45,7 +46,7 @@ static void pps_ktimer_event(unsigned long ptr)
> /* First of all we get the time stamp... */
> pps_get_ts(&ts);
>
> - pr_info("PPS event at %lu\n", jiffies);
> + dev_info(pps->dev, "PPS event at %lu\n", jiffies);
>
> pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
>
> @@ -94,7 +95,7 @@ static int __init pps_ktimer_init(void)
> pps = pps_register_source(&pps_ktimer_info,
> PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
> if (pps == NULL) {
> - printk(KERN_ERR "cannot register ktimer source\n");
> + pr_err("cannot register PPS source\n");
> return -ENOMEM;
> }
>
> diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
> index 1950b15..30789fa 100644
> --- a/drivers/pps/clients/pps-ldisc.c
> +++ b/drivers/pps/clients/pps-ldisc.c
> @@ -19,6 +19,8 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/module.h>
> #include <linux/serial_core.h>
> #include <linux/tty.h>
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index 98d4012..fbb89c6 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -19,6 +19,7 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -80,20 +81,20 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
>
> /* Sanity checks */
> if ((info->mode & default_params) != default_params) {
> - printk(KERN_ERR "pps: %s: unsupported default parameters\n",
> + pr_err("pps: %s: unsupported default parameters\n",
> info->name);
> err = -EINVAL;
> goto pps_register_source_exit;
> }
> if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> info->echo == NULL) {
> - printk(KERN_ERR "pps: %s: echo function is not defined\n",
> + pr_err("pps: %s: echo function is not defined\n",
> info->name);
> err = -EINVAL;
> goto pps_register_source_exit;
> }
> if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
> - printk(KERN_ERR "pps: %s: unspecified time format\n",
> + pr_err("pps: %s: unspecified time format\n",
> info->name);
> err = -EINVAL;
> goto pps_register_source_exit;
> @@ -138,7 +139,7 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> if (id >= PPS_MAX_SOURCES) {
> spin_unlock_irq(&pps_idr_lock);
>
> - printk(KERN_ERR "pps: %s: too many PPS sources in the system\n",
> + pr_err("pps: %s: too many PPS sources in the system\n",
> info->name);
> err = -EBUSY;
> goto free_idr;
> @@ -150,12 +151,12 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> /* Create the char device */
> err = pps_register_cdev(pps);
> if (err < 0) {
> - printk(KERN_ERR "pps: %s: unable to create char device\n",
> + pr_err("%s: unable to create char device\n",
> info->name);
> goto free_idr;
> }
>
> - pr_info("new PPS source %s at ID %d\n", info->name, id);
> + dev_info(pps->dev, "new PPS source %s\n", info->name);
>
> return pps;
>
> @@ -168,7 +169,7 @@ kfree_pps:
> kfree(pps);
>
> pps_register_source_exit:
> - printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
> + pr_err("%s: unable to register source\n", info->name);
>
> return NULL;
> }
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 1922f27..9f7c2e8 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -19,6 +19,7 @@
> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -67,7 +68,7 @@ static long pps_cdev_ioctl(struct file *file,
>
> switch (cmd) {
> case PPS_GETPARAMS:
> - pr_debug("PPS_GETPARAMS: source %d\n", pps->id);
> + dev_dbg(pps->dev, "PPS_GETPARAMS\n");
>
> spin_lock_irq(&pps->lock);
>
> @@ -83,7 +84,7 @@ static long pps_cdev_ioctl(struct file *file,
> break;
>
> case PPS_SETPARAMS:
> - pr_debug("PPS_SETPARAMS: source %d\n", pps->id);
> + dev_dbg(pps->dev, "PPS_SETPARAMS\n");
>
> /* Check the capabilities */
> if (!capable(CAP_SYS_TIME))
> @@ -93,14 +94,14 @@ static long pps_cdev_ioctl(struct file *file,
> if (err)
> return -EFAULT;
> if (!(params.mode & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR))) {
> - pr_debug("capture mode unspecified (%x)\n",
> + dev_dbg(pps->dev, "capture mode unspecified (%x)\n",
> params.mode);
> return -EINVAL;
> }
>
> /* Check for supported capabilities */
> if ((params.mode & ~pps->info.mode) != 0) {
> - pr_debug("unsupported capabilities (%x)\n",
> + dev_dbg(pps->dev, "unsupported capabilities (%x)\n",
> params.mode);
> return -EINVAL;
> }
> @@ -113,7 +114,7 @@ static long pps_cdev_ioctl(struct file *file,
> /* Restore the read only parameters */
> if ((params.mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
> /* section 3.3 of RFC 2783 interpreted */
> - pr_debug("time format unspecified (%x)\n",
> + dev_dbg(pps->dev, "time format unspecified (%x)\n",
> params.mode);
> pps->params.mode |= PPS_TSFMT_TSPEC;
> }
> @@ -126,7 +127,7 @@ static long pps_cdev_ioctl(struct file *file,
> break;
>
> case PPS_GETCAP:
> - pr_debug("PPS_GETCAP: source %d\n", pps->id);
> + dev_dbg(pps->dev, "PPS_GETCAP\n");
>
> err = put_user(pps->info.mode, iuarg);
> if (err)
> @@ -138,7 +139,7 @@ static long pps_cdev_ioctl(struct file *file,
> struct pps_fdata fdata;
> unsigned int ev;
>
> - pr_debug("PPS_FETCH: source %d\n", pps->id);
> + dev_dbg(pps->dev, "PPS_FETCH\n");
>
> err = copy_from_user(&fdata, uarg, sizeof(struct pps_fdata));
> if (err)
> @@ -153,7 +154,7 @@ static long pps_cdev_ioctl(struct file *file,
> else {
> unsigned long ticks;
>
> - pr_debug("timeout %lld.%09d\n",
> + dev_dbg(pps->dev, "timeout %lld.%09d\n",
> (long long) fdata.timeout.sec,
> fdata.timeout.nsec);
> ticks = fdata.timeout.sec * HZ;
> @@ -171,7 +172,7 @@ static long pps_cdev_ioctl(struct file *file,
>
> /* Check for pending signals */
> if (err == -ERESTARTSYS) {
> - pr_debug("pending signal caught\n");
> + dev_dbg(pps->dev, "pending signal caught\n");
> return -EINTR;
> }
>
> @@ -240,7 +241,7 @@ int pps_register_cdev(struct pps_device *pps)
>
> err = cdev_add(&pps->cdev, devt, 1);
> if (err) {
> - printk(KERN_ERR "pps: %s: failed to add char device %d:%d\n",
> + pr_err("%s: failed to add char device %d:%d\n",
> pps->info.name, MAJOR(pps_devt), pps->id);
> return err;
> }
> @@ -282,14 +283,14 @@ static int __init pps_init(void)
>
> pps_class = class_create(THIS_MODULE, "pps");
> if (!pps_class) {
> - printk(KERN_ERR "pps: failed to allocate class\n");
> + pr_err("failed to allocate class\n");
> return -ENOMEM;
> }
> pps_class->dev_attrs = pps_attrs;
>
> err = alloc_chrdev_region(&pps_devt, 0, PPS_MAX_SOURCES, "pps");
> if (err < 0) {
> - printk(KERN_ERR "pps: failed to allocate char device region\n");
> + pr_err("failed to allocate char device region\n");
> goto remove_class;
> }
>
> --
> 1.7.2.3
>
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Nov 18, 2010 at 07:01:00PM +0300, Alexander Gordeev wrote:
> Since now idr is only used to manage char device id's and not used in
> kernel API anymore it should be moved to pps.c. This also makes it
> possible to release id only at actual device freeing so nobody can
> register a pps device with the same id while our device is not freed
> yet.
This patch must be delayed till patch 5 is not fixed.
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Nov 18, 2010 at 07:00:56PM +0300, Alexander Gordeev wrote:
> There was a race in PPS_FETCH ioctl handler when several processes want
> to obtain PPS data simultaneously using sleeping PPS_FETCH. They all
> sleep most of the time in the system call.
> With the old approach when the first process waiting on the pps queue
> is waken up it makes new system call right away and zeroes pps->go. So
> other processes continue to sleep. This is a clear race condition
> because of the global 'go' variable.
> With the new approach pps->last_ev holds some value increasing at each
> PPS event. PPS_FETCH ioctl handler saves current value to the local
> variable at the very beginning so it can safely check that there is a
> new event by just comparing both variables.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/kapi.c | 4 ++--
> drivers/pps/pps.c | 10 +++++++---
> include/linux/pps_kernel.h | 2 +-
> 3 files changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index 55f3961..3f89f5eb 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -326,8 +326,8 @@ void pps_event(int source, struct pps_ktime *ts, int event, void *data)
>
> /* Wake up if captured something */
> if (captured) {
> - pps->go = ~0;
> - wake_up_interruptible(&pps->queue);
> + pps->last_ev++;
> + wake_up_interruptible_all(&pps->queue);
>
> kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
> }
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index c76afb9..dc7e66c 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -136,6 +136,7 @@ static long pps_cdev_ioctl(struct file *file,
>
> case PPS_FETCH: {
> struct pps_fdata fdata;
> + unsigned int ev;
>
> pr_debug("PPS_FETCH: source %d\n", pps->id);
>
> @@ -143,11 +144,12 @@ static long pps_cdev_ioctl(struct file *file,
> if (err)
> return -EFAULT;
>
> - pps->go = 0;
> + ev = pps->last_ev;
>
> /* Manage the timeout */
> if (fdata.timeout.flags & PPS_TIME_INVALID)
> - err = wait_event_interruptible(pps->queue, pps->go);
> + err = wait_event_interruptible(pps->queue,
> + ev != pps->last_ev);
> else {
> unsigned long ticks;
>
> @@ -159,7 +161,9 @@ static long pps_cdev_ioctl(struct file *file,
>
> if (ticks != 0) {
> err = wait_event_interruptible_timeout(
> - pps->queue, pps->go, ticks);
> + pps->queue,
> + ev != pps->last_ev,
> + ticks);
> if (err == 0)
> return -ETIMEDOUT;
> }
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index c930d11..c3aed4b 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -55,7 +55,7 @@ struct pps_device {
> struct pps_ktime clear_tu;
> int current_mode; /* PPS mode at event time */
>
> - int go; /* PPS event is arrived? */
> + unsigned int last_ev; /* last PPS event id */
> wait_queue_head_t queue; /* PPS event queue */
>
> unsigned int id; /* PPS source unique ID */
> --
> 1.7.2.3
>
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Nov 18, 2010 at 07:01:01PM +0300, Alexander Gordeev wrote:
> This handler should be called from an IRQ handler. It uses per-device
> workqueue internally.
Can you please explain to me why do you need this patch? Maybe you can
add a verbose patch's description? :)
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/clients/pps-ktimer.c | 2 +-
> drivers/pps/clients/pps-ldisc.c | 2 +-
> drivers/pps/kapi.c | 95 ++++++++++++++++++++++++++++++++++++--
> drivers/pps/pps.c | 14 +++++-
> include/linux/pps_kernel.h | 12 +++++
> 5 files changed, 117 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
> index 2728469..26ed7a2 100644
> --- a/drivers/pps/clients/pps-ktimer.c
> +++ b/drivers/pps/clients/pps-ktimer.c
> @@ -48,7 +48,7 @@ static void pps_ktimer_event(unsigned long ptr)
>
> dev_info(pps->dev, "PPS event at %lu\n", jiffies);
>
> - pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
> + pps_event_irq(pps, &ts, PPS_CAPTUREASSERT, NULL);
>
> mod_timer(&ktimer, jiffies + HZ);
> }
> diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
> index 30789fa..7006f85 100644
> --- a/drivers/pps/clients/pps-ldisc.c
> +++ b/drivers/pps/clients/pps-ldisc.c
> @@ -50,7 +50,7 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> /* Now do the PPS event report */
> pps = (struct pps_device *)tty->disc_data;
> if (pps != NULL) {
> - pps_event(pps, ts, status ? PPS_CAPTUREASSERT :
> + pps_event_irq(pps, ts, status ? PPS_CAPTUREASSERT :
> PPS_CAPTURECLEAR, NULL);
> spin_unlock_irqrestore(&pps_ldisc_lock, flags);
> dev_dbg(pps->dev, "PPS %s at %lu\n",
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index f5b2b78..ca3b4f8 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -32,9 +32,19 @@
> #include <linux/slab.h>
>
> /*
> + * Global variables
> + */
> +
> +/* PPS event workqueue */
> +struct workqueue_struct *pps_event_workqueue;
> +
> +/*
> * Local functions
> */
>
> +static void assert_work_func(struct work_struct *work);
> +static void clear_work_func(struct work_struct *work);
> +
> static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
> {
> ts->nsec += offset->nsec;
> @@ -108,6 +118,9 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> init_waitqueue_head(&pps->queue);
> spin_lock_init(&pps->lock);
>
> + INIT_WORK(&pps->assert_work, assert_work_func);
> + INIT_WORK(&pps->clear_work, clear_work_func);
> +
> /* Create the char device */
> err = pps_register_cdev(pps);
> if (err < 0) {
> @@ -152,11 +165,12 @@ EXPORT_SYMBOL(pps_unregister_source);
> * @event: the event type
> * @data: userdef pointer
> *
> - * This function is used by each PPS client in order to register a new
> - * PPS event into the system (it's usually called inside an IRQ handler).
> + * This function is used by PPS clients in order to register a new
> + * PPS event into the system. It should not be called from an IRQ
> + * handler. Use pps_event_irq instead.
> *
> - * If an echo function is associated with the PPS device it will be called
> - * as:
> + * If an echo function is associated with the PPS device it will be
> + * called as:
> * pps->info.echo(pps, event, data);
> */
> void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> @@ -226,3 +240,76 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> spin_unlock_irqrestore(&pps->lock, flags);
> }
> EXPORT_SYMBOL(pps_event);
> +
> +/* Async event handlers */
> +
> +static void assert_work_func(struct work_struct *work)
> +{
> + struct pps_device *pps = container_of(work,
> + struct pps_device, assert_work);
> +
> + pps_event(pps, &pps->assert_work_ts, PPS_CAPTUREASSERT,
> + pps->assert_work_data);
> +}
> +
> +static void clear_work_func(struct work_struct *work)
> +{
> + struct pps_device *pps = container_of(work,
> + struct pps_device, clear_work);
> +
> + pps_event(pps, &pps->clear_work_ts, PPS_CAPTURECLEAR,
> + pps->clear_work_data);
> +}
The RFC-2783 says that (see 3.1 PPS abstraction):
The API optionally supports an "echo" feature, in which events on
the
incoming PPS signal may be reflected through software, after the
capture of the corresponding timestamp, to an output signal pin.
This feature may be used to discover an upper bound on the actual
delay between the edges of the PPS signal and the capture of the
timestamps; such information may be useful in precise calibration
of
the system.
The designation of an output pin for the echo signal, and sense and
shape of the output transition, is outside the scope of this
specification, but SHOULD be documented for each implementation.
The
output pin MAY also undergo transitions at other times besides
those
caused by PPS input events.
By applying this patch the echo function is called inside a work queue
so it depends to the scheduler. I suppose this is not acceptable,
otherwise we should drop the echo function support.
> +/* pps_event_irq - register a PPS event for deffered handling using
> + * workqueue
> + *
> + * @pps: the PPS device
> + * @ts: the event timestamp
> + * @event: the event type
> + * @data: userdef pointer
> + *
> + * This function is used by PPS clients in order to register a new
> + * PPS event into the system. It should be called from an IRQ handler
> + * only.
> + *
> + * If an echo function is associated with the PPS device it will be
> + * called as:
> + * pps->info.echo(pps, event, data);
> + */
The above comment talks about the echo function but you removed it
from the code below...
> +void pps_event_irq(struct pps_device *pps, struct pps_event_time *ts,
> + int event, void *data)
> +{
> + /* check event type */
> + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> +
> + if (event & PPS_CAPTUREASSERT) {
> + if (work_pending(&pps->assert_work)) {
> + dev_err(pps->dev, "deferred assert edge work haven't"
> + " been handled within a second\n");
> + /* FIXME: do something more intelligent
> + * then just exit */
> + } else {
> + /* now we can copy data safely */
> + pps->assert_work_ts = *ts;
> + pps->assert_work_data = data;
> +
> + queue_work(pps_event_workqueue, &pps->assert_work);
> + }
> + }
> + if (event & PPS_CAPTURECLEAR) {
> + if (work_pending(&pps->clear_work)) {
> + dev_err(pps->dev, "deferred clear edge work haven't"
> + " been handled within a second\n");
> + /* FIXME: do something more intelligent
> + * then just exit */
> + } else {
> + /* now we can copy data safely */
> + pps->clear_work_ts = *ts;
> + pps->clear_work_data = data;
> +
> + queue_work(pps_event_workqueue, &pps->clear_work);
> + }
> + }
> +}
> +EXPORT_SYMBOL(pps_event_irq);
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 79b4455..f642558 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -321,18 +321,26 @@ void pps_unregister_cdev(struct pps_device *pps)
>
> static void __exit pps_exit(void)
> {
> - class_destroy(pps_class);
> unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
> + class_destroy(pps_class);
> + destroy_workqueue(pps_event_workqueue);
> }
>
> static int __init pps_init(void)
> {
> int err;
>
> + pps_event_workqueue = create_workqueue("pps");
> + if (!pps_event_workqueue) {
> + pr_err("failed to create workqueue\n");
> + return -ENOMEM;
> + }
> +
> pps_class = class_create(THIS_MODULE, "pps");
> if (!pps_class) {
> pr_err("failed to allocate class\n");
> - return -ENOMEM;
> + err = -ENOMEM;
> + goto destroy_workqueue;
> }
> pps_class->dev_attrs = pps_attrs;
>
> @@ -350,6 +358,8 @@ static int __init pps_init(void)
>
> remove_class:
> class_destroy(pps_class);
> +destroy_workqueue:
> + destroy_workqueue(pps_event_workqueue);
>
> return err;
> }
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index 1aedf50..5af0498 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -26,6 +26,7 @@
> #include <linux/cdev.h>
> #include <linux/device.h>
> #include <linux/time.h>
> +#include <linux/workqueue.h>
>
> /*
> * Global defines
> @@ -70,6 +71,13 @@ struct pps_device {
> struct device *dev;
> struct fasync_struct *async_queue; /* fasync method */
> spinlock_t lock;
> +
> + struct work_struct assert_work;
> + struct work_struct clear_work;
> + struct pps_event_time assert_work_ts;
> + struct pps_event_time clear_work_ts;
> + void *assert_work_data;
> + void *clear_work_data;
> };
>
> /*
> @@ -78,6 +86,8 @@ struct pps_device {
>
> extern struct device_attribute pps_attrs[];
>
> +extern struct workqueue_struct *pps_event_workqueue;
> +
> /*
> * Exported functions
> */
> @@ -89,6 +99,8 @@ extern int pps_register_cdev(struct pps_device *pps);
> extern void pps_unregister_cdev(struct pps_device *pps);
> extern void pps_event(struct pps_device *pps,
> struct pps_event_time *ts, int event, void *data);
> +extern void pps_event_irq(struct pps_device *pps,
> + struct pps_event_time *ts, int event, void *data);
>
> static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
> struct timespec ts)
> --
> 1.7.2.3
>
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Nov 18, 2010 at 07:01:02PM +0300, Alexander Gordeev wrote:
> Now all PPS spin locks are never used in interrupt context so we can
> safely replace spin_lock_irq*/spin_unlock_irq* with plain
> spin_lock/spin_unlock.
This patch must be delayed till the previous one is not fixed.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Nov 18, 2010 at 07:01:03PM +0300, Alexander Gordeev wrote:
> This way less overhead is involved when running production kernel.
> If you want to debug a pps client module please define DEBUG to enable
> the checks.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/kapi.c | 33 ++++++++++-----------------------
> 1 files changed, 10 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index fe832aa..54261c4 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -81,25 +81,14 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> int err;
>
> /* Sanity checks */
> - if ((info->mode & default_params) != default_params) {
> - pr_err("pps: %s: unsupported default parameters\n",
> - info->name);
> - err = -EINVAL;
> - goto pps_register_source_exit;
> - }
> - if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> - info->echo == NULL) {
> - pr_err("pps: %s: echo function is not defined\n",
> - info->name);
> - err = -EINVAL;
> - goto pps_register_source_exit;
> - }
> - if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
> - pr_err("pps: %s: unspecified time format\n",
> - info->name);
> - err = -EINVAL;
> - goto pps_register_source_exit;
> - }
> +
> + /* default_params should be supported */
> + BUG_ON((info->mode & default_params) != default_params);
> + /* echo function should be defined if we are asked to call it */
> + BUG_ON((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> + info->echo == NULL);
> + /* time format should be specified */
> + BUG_ON((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0);
Nack.
If the userland gives us some wrong parameters this is not the same of
a kernel bug (which BUG_ON is used for). The userland must be notified
about the wrong input.
> /* Allocate memory for the new PPS source struct */
> pps = kzalloc(sizeof(struct pps_device), GFP_KERNEL);
> @@ -179,10 +168,8 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> int captured = 0;
> struct pps_ktime ts_real;
>
> - if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> - dev_err(pps->dev, "unknown event (%x)\n", event);
> - return;
> - }
> + /* check event type */
> + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
Ack.
This is a correct usage of BUG_ON. :)
> dev_dbg(pps->dev, "PPS event at %ld.%09ld\n",
> ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
> --
> 1.7.2.3
>
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Nov 18, 2010 at 07:01:04PM +0300, Alexander Gordeev wrote:
> Bitwise conjunction is distributive so we can simplify some conditions.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> drivers/pps/kapi.c | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index 54261c4..2bdfbed 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -184,8 +184,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>
> /* Check the event */
> pps->current_mode = pps->params.mode;
> - if ((event & PPS_CAPTUREASSERT) &
> - (pps->params.mode & PPS_CAPTUREASSERT)) {
> + if (event & pps->params.mode & PPS_CAPTUREASSERT) {
> /* We have to add an offset? */
> if (pps->params.mode & PPS_OFFSETASSERT)
> pps_add_offset(&ts_real,
> @@ -199,8 +198,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>
> captured = ~0;
> }
> - if ((event & PPS_CAPTURECLEAR) &
> - (pps->params.mode & PPS_CAPTURECLEAR)) {
> + if (event & pps->params.mode & PPS_CAPTURECLEAR) {
> /* We have to add an offset? */
> if (pps->params.mode & PPS_OFFSETCLEAR)
> pps_add_offset(&ts_real,
> --
> 1.7.2.3
>
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Nov 18, 2010 at 07:01:05PM +0300, Alexander Gordeev wrote:
> Remove the code that gatheres timestamp in pps_tty_dcd_change() in case
> passed ts parameter is NULL because it never happens in the current
> code. Fix comments as well.
>
> Signed-off-by: Alexander Gordeev <[email protected]>
> ---
> Documentation/serial/tty.txt | 2 +-
> drivers/pps/clients/pps-ldisc.c | 8 --------
> include/linux/tty_ldisc.h | 2 +-
> 3 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt
> index 7c90050..540db41 100644
> --- a/Documentation/serial/tty.txt
> +++ b/Documentation/serial/tty.txt
> @@ -107,7 +107,7 @@ write_wakeup() - May be called at any point between open and close.
>
> dcd_change() - Report to the tty line the current DCD pin status
> changes and the relative timestamp. The timestamp
> - can be NULL.
> + cannot be NULL.
Can you please explain why it cannot be null? O_o
Who does supply the timestamp to dcd_change?
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Thu, Nov 18, 2010 at 07:01:06PM +0300, Alexander Gordeev wrote:
> This commit adds hardpps() implementation based upon the original one
> from the NTPv4 reference kernel code from David Mills. However, it is
> highly optimized towards very fast syncronization and maximum stickness
> to PPS signal. The typical error is less then a microsecond.
> To make it sync faster I had to throw away exponential phase filter so
> that the full phase offset is corrected immediately. Then I also had to
> throw away median phase filter because it gives a bigger error itself
> if used without exponential filter.
> Maybe we will find an appropriate filtering scheme in the future but
> it's not necessary if the signal quality is ok.
This patch (and follows) adds a new functionality to PPS subsystem, so
let me suggest to you splitting in two parts this patch set: a first
patch set who fixes up the current PPS implementation, and a second
patch set who adds kernel consumer (and follows).
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Sat, 2010-11-20 at 17:13 +0100, Rodolfo Giometti wrote:
> On Thu, Nov 18, 2010 at 07:01:03PM +0300, Alexander Gordeev wrote:
> > This way less overhead is involved when running production kernel.
> > If you want to debug a pps client module please define DEBUG to enable
> > the checks.
> > Signed-off-by: Alexander Gordeev <[email protected]>
[]
> > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > @@ -179,10 +168,8 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> > int captured = 0;
> > struct pps_ktime ts_real;
> >
> > - if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> > - dev_err(pps->dev, "unknown event (%x)\n", event);
> > - return;
> > - }
> > + /* check event type */
> > + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
>
> Ack.
>
> This is a correct usage of BUG_ON. :)
I don't think that's true.
/*
* Don't use BUG() or BUG_ON() unless there's really no way out; one
* example might be detecting data structure corruption in the middle
* of an operation that can't be backed out of. If the (sub)system
* can somehow continue operating, perhaps with reduced functionality,
* it's probably not BUG-worthy.
*
* If you're tempted to BUG(), think again: is completely giving up
* really the *only* solution? There are usually better options, where
* users don't need to reboot ASAP and can mostly shut down cleanly.
*/
On Sat, Nov 20, 2010 at 09:01:41AM -0800, Joe Perches wrote:
> > > - if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> > > - dev_err(pps->dev, "unknown event (%x)\n", event);
> > > - return;
> > > - }
> > > + /* check event type */
> > > + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> >
> > Ack.
> >
> > This is a correct usage of BUG_ON. :)
>
> I don't think that's true.
>
> /*
> * Don't use BUG() or BUG_ON() unless there's really no way out; one
> * example might be detecting data structure corruption in the middle
> * of an operation that can't be backed out of. If the (sub)system
> * can somehow continue operating, perhaps with reduced functionality,
> * it's probably not BUG-worthy.
> *
> * If you're tempted to BUG(), think again: is completely giving up
> * really the *only* solution? There are usually better options, where
> * users don't need to reboot ASAP and can mostly shut down cleanly.
> */
If (event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0 then PPS's
data has been corrupted. The PPS subsystem doesn't work correctly even
if the whole system still continues running.
However I already have not acknowledged the patch.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
В Sat, 20 Nov 2010 16:49:40 +0100
Rodolfo Giometti <[email protected]> пишет:
> On Thu, Nov 18, 2010 at 07:00:59PM +0300, Alexander Gordeev wrote:
> > Since we now have direct pointers to struct pps_device everywhere it's
> > easy to use dev_* functions to print messages instead of plain printks.
> > Where dev_* cannot be used printks are converted to pr_*.
> >
> > Signed-off-by: Alexander Gordeev <[email protected]>
> > ---
> > drivers/pps/clients/pps-ktimer.c | 5 +++--
> > drivers/pps/clients/pps-ldisc.c | 2 ++
> > drivers/pps/kapi.c | 15 ++++++++-------
> > drivers/pps/pps.c | 25 +++++++++++++------------
> > 4 files changed, 26 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
> > index 966ead1..2728469 100644
> > --- a/drivers/pps/clients/pps-ktimer.c
> > +++ b/drivers/pps/clients/pps-ktimer.c
> > @@ -19,6 +19,7 @@
> > * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > */
> >
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> Instead of redefining pr_fmt() in each file you may do it once into
> include file pps_kernel.h...
Yes, but then every .c file will have to include linux/pps_kernel.h
before including linux/kernel.h (where pr_* are defined) and only you
and me will know the reason for that. Of course, we can add a comment to
every .c file to discourage others from moving includes but IMHO the
way how it is done in the patch is better. I dislike hidden
dependencies between header files very much.
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > @@ -45,7 +46,7 @@ static void pps_ktimer_event(unsigned long ptr)
> > /* First of all we get the time stamp... */
> > pps_get_ts(&ts);
> >
> > - pr_info("PPS event at %lu\n", jiffies);
> > + dev_info(pps->dev, "PPS event at %lu\n", jiffies);
> >
> > pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
> >
> > @@ -94,7 +95,7 @@ static int __init pps_ktimer_init(void)
> > pps = pps_register_source(&pps_ktimer_info,
> > PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
> > if (pps == NULL) {
> > - printk(KERN_ERR "cannot register ktimer source\n");
> > + pr_err("cannot register PPS source\n");
> > return -ENOMEM;
> > }
> >
> > diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
> > index 1950b15..30789fa 100644
> > --- a/drivers/pps/clients/pps-ldisc.c
> > +++ b/drivers/pps/clients/pps-ldisc.c
> > @@ -19,6 +19,8 @@
> > * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > */
> >
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > #include <linux/module.h>
> > #include <linux/serial_core.h>
> > #include <linux/tty.h>
> > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > index 98d4012..fbb89c6 100644
> > --- a/drivers/pps/kapi.c
> > +++ b/drivers/pps/kapi.c
> > @@ -19,6 +19,7 @@
> > * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > */
> >
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > @@ -80,20 +81,20 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> >
> > /* Sanity checks */
> > if ((info->mode & default_params) != default_params) {
> > - printk(KERN_ERR "pps: %s: unsupported default parameters\n",
> > + pr_err("pps: %s: unsupported default parameters\n",
> > info->name);
> > err = -EINVAL;
> > goto pps_register_source_exit;
> > }
> > if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> > info->echo == NULL) {
> > - printk(KERN_ERR "pps: %s: echo function is not defined\n",
> > + pr_err("pps: %s: echo function is not defined\n",
> > info->name);
> > err = -EINVAL;
> > goto pps_register_source_exit;
> > }
> > if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
> > - printk(KERN_ERR "pps: %s: unspecified time format\n",
> > + pr_err("pps: %s: unspecified time format\n",
> > info->name);
> > err = -EINVAL;
> > goto pps_register_source_exit;
> > @@ -138,7 +139,7 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> > if (id >= PPS_MAX_SOURCES) {
> > spin_unlock_irq(&pps_idr_lock);
> >
> > - printk(KERN_ERR "pps: %s: too many PPS sources in the system\n",
> > + pr_err("pps: %s: too many PPS sources in the system\n",
> > info->name);
> > err = -EBUSY;
> > goto free_idr;
> > @@ -150,12 +151,12 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> > /* Create the char device */
> > err = pps_register_cdev(pps);
> > if (err < 0) {
> > - printk(KERN_ERR "pps: %s: unable to create char device\n",
> > + pr_err("%s: unable to create char device\n",
> > info->name);
> > goto free_idr;
> > }
> >
> > - pr_info("new PPS source %s at ID %d\n", info->name, id);
> > + dev_info(pps->dev, "new PPS source %s\n", info->name);
> >
> > return pps;
> >
> > @@ -168,7 +169,7 @@ kfree_pps:
> > kfree(pps);
> >
> > pps_register_source_exit:
> > - printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
> > + pr_err("%s: unable to register source\n", info->name);
> >
> > return NULL;
> > }
> > diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> > index 1922f27..9f7c2e8 100644
> > --- a/drivers/pps/pps.c
> > +++ b/drivers/pps/pps.c
> > @@ -19,6 +19,7 @@
> > * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > */
> >
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > @@ -67,7 +68,7 @@ static long pps_cdev_ioctl(struct file *file,
> >
> > switch (cmd) {
> > case PPS_GETPARAMS:
> > - pr_debug("PPS_GETPARAMS: source %d\n", pps->id);
> > + dev_dbg(pps->dev, "PPS_GETPARAMS\n");
> >
> > spin_lock_irq(&pps->lock);
> >
> > @@ -83,7 +84,7 @@ static long pps_cdev_ioctl(struct file *file,
> > break;
> >
> > case PPS_SETPARAMS:
> > - pr_debug("PPS_SETPARAMS: source %d\n", pps->id);
> > + dev_dbg(pps->dev, "PPS_SETPARAMS\n");
> >
> > /* Check the capabilities */
> > if (!capable(CAP_SYS_TIME))
> > @@ -93,14 +94,14 @@ static long pps_cdev_ioctl(struct file *file,
> > if (err)
> > return -EFAULT;
> > if (!(params.mode & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR))) {
> > - pr_debug("capture mode unspecified (%x)\n",
> > + dev_dbg(pps->dev, "capture mode unspecified (%x)\n",
> > params.mode);
> > return -EINVAL;
> > }
> >
> > /* Check for supported capabilities */
> > if ((params.mode & ~pps->info.mode) != 0) {
> > - pr_debug("unsupported capabilities (%x)\n",
> > + dev_dbg(pps->dev, "unsupported capabilities (%x)\n",
> > params.mode);
> > return -EINVAL;
> > }
> > @@ -113,7 +114,7 @@ static long pps_cdev_ioctl(struct file *file,
> > /* Restore the read only parameters */
> > if ((params.mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
> > /* section 3.3 of RFC 2783 interpreted */
> > - pr_debug("time format unspecified (%x)\n",
> > + dev_dbg(pps->dev, "time format unspecified (%x)\n",
> > params.mode);
> > pps->params.mode |= PPS_TSFMT_TSPEC;
> > }
> > @@ -126,7 +127,7 @@ static long pps_cdev_ioctl(struct file *file,
> > break;
> >
> > case PPS_GETCAP:
> > - pr_debug("PPS_GETCAP: source %d\n", pps->id);
> > + dev_dbg(pps->dev, "PPS_GETCAP\n");
> >
> > err = put_user(pps->info.mode, iuarg);
> > if (err)
> > @@ -138,7 +139,7 @@ static long pps_cdev_ioctl(struct file *file,
> > struct pps_fdata fdata;
> > unsigned int ev;
> >
> > - pr_debug("PPS_FETCH: source %d\n", pps->id);
> > + dev_dbg(pps->dev, "PPS_FETCH\n");
> >
> > err = copy_from_user(&fdata, uarg, sizeof(struct pps_fdata));
> > if (err)
> > @@ -153,7 +154,7 @@ static long pps_cdev_ioctl(struct file *file,
> > else {
> > unsigned long ticks;
> >
> > - pr_debug("timeout %lld.%09d\n",
> > + dev_dbg(pps->dev, "timeout %lld.%09d\n",
> > (long long) fdata.timeout.sec,
> > fdata.timeout.nsec);
> > ticks = fdata.timeout.sec * HZ;
> > @@ -171,7 +172,7 @@ static long pps_cdev_ioctl(struct file *file,
> >
> > /* Check for pending signals */
> > if (err == -ERESTARTSYS) {
> > - pr_debug("pending signal caught\n");
> > + dev_dbg(pps->dev, "pending signal caught\n");
> > return -EINTR;
> > }
> >
> > @@ -240,7 +241,7 @@ int pps_register_cdev(struct pps_device *pps)
> >
> > err = cdev_add(&pps->cdev, devt, 1);
> > if (err) {
> > - printk(KERN_ERR "pps: %s: failed to add char device %d:%d\n",
> > + pr_err("%s: failed to add char device %d:%d\n",
> > pps->info.name, MAJOR(pps_devt), pps->id);
> > return err;
> > }
> > @@ -282,14 +283,14 @@ static int __init pps_init(void)
> >
> > pps_class = class_create(THIS_MODULE, "pps");
> > if (!pps_class) {
> > - printk(KERN_ERR "pps: failed to allocate class\n");
> > + pr_err("failed to allocate class\n");
> > return -ENOMEM;
> > }
> > pps_class->dev_attrs = pps_attrs;
> >
> > err = alloc_chrdev_region(&pps_devt, 0, PPS_MAX_SOURCES, "pps");
> > if (err < 0) {
> > - printk(KERN_ERR "pps: failed to allocate char device region\n");
> > + pr_err("failed to allocate char device region\n");
> > goto remove_class;
> > }
> >
> > --
> > 1.7.2.3
> >
>
> Acked-by: Rodolfo Giometti <[email protected]>
>
--
Alexander
On Sun, 2010-11-21 at 00:33 +0300, Alexander Gordeev wrote:
> В Sat, 20 Nov 2010 16:49:40 +0100
> Rodolfo Giometti <[email protected]> пишет:
> > On Thu, Nov 18, 2010 at 07:00:59PM +0300, Alexander Gordeev wrote:
> > > Since we now have direct pointers to struct pps_device everywhere it's
> > > easy to use dev_* functions to print messages instead of plain printks.
> > > Where dev_* cannot be used printks are converted to pr_*.
> > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
[]
> > > @@ -80,20 +81,20 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> > >
> > > /* Sanity checks */
> > > if ((info->mode & default_params) != default_params) {
> > > - printk(KERN_ERR "pps: %s: unsupported default parameters\n",
> > > + pr_err("pps: %s: unsupported default parameters\n",
> > > info->name);
Perhaps you want to remove the "pps:" prefixes?
> > > - printk(KERN_ERR "pps: %s: echo function is not defined\n",
> > > + pr_err("pps: %s: echo function is not defined\n",
[]
> > > - printk(KERN_ERR "pps: %s: unspecified time format\n",
> > > + pr_err("pps: %s: unspecified time format\n",
[]
> > > - printk(KERN_ERR "pps: %s: too many PPS sources in the system\n",
> > > + pr_err("pps: %s: too many PPS sources in the system\n",
[]
> > > - printk(KERN_ERR "pps: %s: unable to create char device\n",
> > > + pr_err("%s: unable to create char device\n",
It's removed here.
> > > - printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
> > > + pr_err("%s: unable to register source\n", info->name);
and here.
В Sat, 20 Nov 2010 16:44:17 +0100
Rodolfo Giometti <[email protected]> пишет:
> On Thu, Nov 18, 2010 at 07:00:58PM +0300, Alexander Gordeev wrote:
> > Using device index as a pointer needs some unnecessary work to be done
> > every time the pointer is needed (in irq handler for example).
> > Using a direct pointer is much more easy (and safe as well).
> >
> > Signed-off-by: Alexander Gordeev <[email protected]>
> > ---
> > drivers/pps/clients/pps-ktimer.c | 30 ++++-----
> > drivers/pps/clients/pps-ldisc.c | 52 ++++++++++------
> > drivers/pps/kapi.c | 124 +++++++++-----------------------------
> > drivers/pps/pps.c | 22 ++-----
> > include/linux/pps_kernel.h | 23 +++----
> > 5 files changed, 90 insertions(+), 161 deletions(-)
> >
> > diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
> > index e1bdd8b..966ead1 100644
> > --- a/drivers/pps/clients/pps-ktimer.c
> > +++ b/drivers/pps/clients/pps-ktimer.c
> > @@ -31,7 +31,7 @@
> > * Global variables
> > */
> >
> > -static int source;
> > +static struct pps_device *pps;
> > static struct timer_list ktimer;
> >
> > /*
> > @@ -47,7 +47,7 @@ static void pps_ktimer_event(unsigned long ptr)
> >
> > pr_info("PPS event at %lu\n", jiffies);
> >
> > - pps_event(source, &ts, PPS_CAPTUREASSERT, NULL);
> > + pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
> >
> > mod_timer(&ktimer, jiffies + HZ);
> > }
> > @@ -56,12 +56,11 @@ static void pps_ktimer_event(unsigned long ptr)
> > * The echo function
> > */
> >
> > -static void pps_ktimer_echo(int source, int event, void *data)
> > +static void pps_ktimer_echo(struct pps_device *pps, int event, void *data)
> > {
> > - pr_info("echo %s %s for source %d\n",
> > + dev_info(pps->dev, "echo %s %s\n",
> > event & PPS_CAPTUREASSERT ? "assert" : "",
> > - event & PPS_CAPTURECLEAR ? "clear" : "",
> > - source);
> > + event & PPS_CAPTURECLEAR ? "clear" : "");
> > }
> >
> > /*
> > @@ -84,30 +83,27 @@ static struct pps_source_info pps_ktimer_info = {
> >
> > static void __exit pps_ktimer_exit(void)
> > {
> > - del_timer_sync(&ktimer);
> > - pps_unregister_source(source);
> > + dev_info(pps->dev, "ktimer PPS source unregistered\n");
> >
> > - pr_info("ktimer PPS source unregistered\n");
> > + del_timer_sync(&ktimer);
> > + pps_unregister_source(pps);
> > }
> >
> > static int __init pps_ktimer_init(void)
> > {
> > - int ret;
> > -
> > - ret = pps_register_source(&pps_ktimer_info,
> > + pps = pps_register_source(&pps_ktimer_info,
> > PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
> > - if (ret < 0) {
> > + if (pps == NULL) {
> > printk(KERN_ERR "cannot register ktimer source\n");
> > - return ret;
> > + return -ENOMEM;
> > }
> > - source = ret;
> >
> > setup_timer(&ktimer, pps_ktimer_event, 0);
> > mod_timer(&ktimer, jiffies + HZ);
> >
> > - pr_info("ktimer PPS source registered at %d\n", source);
> > + dev_info(pps->dev, "ktimer PPS source registered\n");
> >
> > - return 0;
> > + return 0;
> > }
> >
> > module_init(pps_ktimer_init);
> > diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
> > index 20fc9f7..1950b15 100644
> > --- a/drivers/pps/clients/pps-ldisc.c
> > +++ b/drivers/pps/clients/pps-ldisc.c
> > @@ -23,14 +23,18 @@
> > #include <linux/serial_core.h>
> > #include <linux/tty.h>
> > #include <linux/pps_kernel.h>
> > +#include <linux/spinlock.h>
> >
> > #define PPS_TTY_MAGIC 0x0001
> >
> > +static DEFINE_SPINLOCK(pps_ldisc_lock);
> > +
> > static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> > struct pps_event_time *ts)
> > {
> > - int id = (long)tty->disc_data;
> > + struct pps_device *pps;
> > struct pps_event_time __ts;
> > + unsigned long flags;
> >
> > /* First of all we get the time stamp... */
> > pps_get_ts(&__ts);
> > @@ -39,12 +43,18 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> > if (!ts) /* No. Do it ourself! */
> > ts = &__ts;
> >
> > - /* Now do the PPS event report */
> > - pps_event(id, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
> > - NULL);
> > + spin_lock_irqsave(&pps_ldisc_lock, flags);
> >
> > - pr_debug("PPS %s at %lu on source #%d\n",
> > - status ? "assert" : "clear", jiffies, id);
> > + /* Now do the PPS event report */
> > + pps = (struct pps_device *)tty->disc_data;
> > + if (pps != NULL) {
> > + pps_event(pps, ts, status ? PPS_CAPTUREASSERT :
> > + PPS_CAPTURECLEAR, NULL);
> > + spin_unlock_irqrestore(&pps_ldisc_lock, flags);
> > + dev_dbg(pps->dev, "PPS %s at %lu\n",
> > + status ? "assert" : "clear", jiffies);
>
> I think you should swap these two lines because after you release the
> lock the pps pointer may be not valid...
Ah, yes, good catch, thank you!
> > + } else
> > + spin_unlock_irqrestore(&pps_ldisc_lock, flags);
> > }
> >
> > static int (*alias_n_tty_open)(struct tty_struct *tty);
> > @@ -54,7 +64,7 @@ static int pps_tty_open(struct tty_struct *tty)
> > struct pps_source_info info;
> > struct tty_driver *drv = tty->driver;
> > int index = tty->index + drv->name_base;
> > - int ret;
> > + struct pps_device *pps;
> >
> > info.owner = THIS_MODULE;
> > info.dev = NULL;
> > @@ -64,20 +74,22 @@ static int pps_tty_open(struct tty_struct *tty)
> > PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
> > PPS_CANWAIT | PPS_TSFMT_TSPEC;
> >
> > - ret = pps_register_source(&info, PPS_CAPTUREBOTH | \
> > + pps = pps_register_source(&info, PPS_CAPTUREBOTH | \
> > PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
> > - if (ret < 0) {
> > + if (pps == NULL) {
> > pr_err("cannot register PPS source \"%s\"\n", info.path);
> > - return ret;
> > + return -ENOMEM;
> > }
> > - tty->disc_data = (void *)(long)ret;
> > +
> > + spin_lock_irq(&pps_ldisc_lock);
> > + tty->disc_data = pps;
> > + spin_unlock_irq(&pps_ldisc_lock);
>
> Maybe this lock is useless... however, are we sure that before setting
> tty->disc_data to pps its value is null? Otherwise the dcd_change may
> be called with an oops! We cannot control serial port IRQ
> generation... :-/
No, locking here is necessary.
There is only one problem this spinlock protects us from: current tty
code neither disables interrupts nor doesn't ensure there are no
references to PPS ldisc from uart_handle_dcd_change() before closing it
(and removing PPS source). It relies on flushing workqueue and disabling
input. It worked good this way until dcd_change() was added which
doesn't use workqueues and is called in atomic context so can't lock
on mutex.
Imagine that (on SMP system) uart_handle_dcd_change() could obtain a
reference to ldisc and call dcd_change() until actually calling
pps_event(); then on another processor all the path from
tty_ldisc_halt() until tty_ldisc_stop() is executed. And then
pps_event() is called with illegal pps pointer.
I just thought you are right that disc_data can be set not NULL by
another ldisc and it's a problem. But actually I just realised how to
fix it completely. :)
I just have to add a spinlock to tty_struct, lock all the
uart_handle_dcd_change() with it and add a "barrier" between
tty_ldisc_halt() and tty_ldisc_close() i.e. just that:
...
spin_lock_irq();
spin_unlock_irq();
...
This "barrier" will ensure that there is no references to ldisc from
uart_handle_dcd_change(). It won't be able to obtain a new reference
after tty_ldisc_halt() so will become completely sane. Not disabling
interrupts won't be a problem because it won't be able to obtain an
ldisc reference until tty_ldisc_enable() which is called only after the
new ldisc is fully functional. If it's our ldisc than it will have both
dcd_change defined and a valid pps pointer. If it's not our ldisc it
won't have both so uart_handle_dcd_change() won't call dcd_change() at
all.
I think I'll do that as a separate patch.
> > /* Should open N_TTY ldisc too */
> > - ret = alias_n_tty_open(tty);
> > - if (ret < 0)
> > - pps_unregister_source((long)tty->disc_data);
> > + if (alias_n_tty_open(tty) < 0)
>
> Are you sure this code style is «canonical»? What does checkpatch say?
> ;)
checkpatch is completely happy about all patches. :)
But indeed it's not good. I'll fix it, thanks!
> > + pps_unregister_source(pps);
>
> If the above function fails I suppose you should invalidate
> tty->disc_data, then unregister the pps source and, in the end, return
> error (I know old code returns 0 anywhere, but I think it can be fixed
> right now! ;).
OK. :)
> > - pr_info("PPS source #%d \"%s\" added\n", ret, info.path);
> > + dev_info(pps->dev, "source \"%s\" added\n", info.path);
> >
> > return 0;
> > }
> > @@ -86,12 +98,16 @@ static void (*alias_n_tty_close)(struct tty_struct *tty);
> >
> > static void pps_tty_close(struct tty_struct *tty)
> > {
> > - int id = (long)tty->disc_data;
> > + struct pps_device *pps = (struct pps_device *)tty->disc_data;
> >
> > - pps_unregister_source(id);
> > alias_n_tty_close(tty);
> >
> > - pr_info("PPS source #%d removed\n", id);
> > + spin_lock_irq(&pps_ldisc_lock);
> > + tty->disc_data = NULL;
> > + spin_unlock_irq(&pps_ldisc_lock);
> > +
> > + dev_info(pps->dev, "removed\n");
> > + pps_unregister_source(pps);
> > }
> >
> > static struct tty_ldisc_ops pps_ldisc_ops;
>
> I suppose you can solve all your problems if you do the lock into
> pps_tty_init...
>
> Ciao,
>
> Rodolfo
>
--
Alexander
В Sat, 20 Nov 2010 13:42:34 -0800
Joe Perches <[email protected]> пишет:
> On Sun, 2010-11-21 at 00:33 +0300, Alexander Gordeev wrote:
> > В Sat, 20 Nov 2010 16:49:40 +0100
> > Rodolfo Giometti <[email protected]> пишет:
> > > On Thu, Nov 18, 2010 at 07:00:59PM +0300, Alexander Gordeev wrote:
> > > > Since we now have direct pointers to struct pps_device everywhere it's
> > > > easy to use dev_* functions to print messages instead of plain printks.
> > > > Where dev_* cannot be used printks are converted to pr_*.
> > > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> []
> > > > @@ -80,20 +81,20 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> > > >
> > > > /* Sanity checks */
> > > > if ((info->mode & default_params) != default_params) {
> > > > - printk(KERN_ERR "pps: %s: unsupported default parameters\n",
> > > > + pr_err("pps: %s: unsupported default parameters\n",
> > > > info->name);
>
> Perhaps you want to remove the "pps:" prefixes?
>
> > > > - printk(KERN_ERR "pps: %s: echo function is not defined\n",
> > > > + pr_err("pps: %s: echo function is not defined\n",
> []
> > > > - printk(KERN_ERR "pps: %s: unspecified time format\n",
> > > > + pr_err("pps: %s: unspecified time format\n",
> []
> > > > - printk(KERN_ERR "pps: %s: too many PPS sources in the system\n",
> > > > + pr_err("pps: %s: too many PPS sources in the system\n",
> []
> > > > - printk(KERN_ERR "pps: %s: unable to create char device\n",
> > > > + pr_err("%s: unable to create char device\n",
>
> It's removed here.
>
> > > > - printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
> > > > + pr_err("%s: unable to register source\n", info->name);
>
> and here.
Thanks! Strange that I overlooked it.
--
Alexander
В Sat, 20 Nov 2010 17:08:51 +0100
Rodolfo Giometti <[email protected]> пишет:
> On Thu, Nov 18, 2010 at 07:01:01PM +0300, Alexander Gordeev wrote:
> > This handler should be called from an IRQ handler. It uses per-device
> > workqueue internally.
>
> Can you please explain to me why do you need this patch? Maybe you can
> add a verbose patch's description? :)
Well, it's all about optimizing latencies on rt-preempt kernel: if
everything is done in a process context than we can use mutexes
(haven't done that yet) and don't disable interrupts, which is better
for hard real time. I measured that pps_event with kernel consumer
enabled takes 1-2us on my test machine. Not a big deal, of course...
Sorry, I've completely forgotten about an echo function. However quick
look to current clients shows that it is only used in pps-ktimer.c
for debug printing... Maybe it's not needed at all? I mean, have you
ever got any user request for this feature? If yes, it can be removed
IMHO since RFC-2783 says that it's optional. I can handle the removal in
the next version of the patchset.
If you don't want it I can have this patch on my rt branch only and
don't try to push it into mainline.
> > Signed-off-by: Alexander Gordeev <[email protected]>
> > ---
> > drivers/pps/clients/pps-ktimer.c | 2 +-
> > drivers/pps/clients/pps-ldisc.c | 2 +-
> > drivers/pps/kapi.c | 95 ++++++++++++++++++++++++++++++++++++--
> > drivers/pps/pps.c | 14 +++++-
> > include/linux/pps_kernel.h | 12 +++++
> > 5 files changed, 117 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
> > index 2728469..26ed7a2 100644
> > --- a/drivers/pps/clients/pps-ktimer.c
> > +++ b/drivers/pps/clients/pps-ktimer.c
> > @@ -48,7 +48,7 @@ static void pps_ktimer_event(unsigned long ptr)
> >
> > dev_info(pps->dev, "PPS event at %lu\n", jiffies);
> >
> > - pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL);
> > + pps_event_irq(pps, &ts, PPS_CAPTUREASSERT, NULL);
> >
> > mod_timer(&ktimer, jiffies + HZ);
> > }
> > diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
> > index 30789fa..7006f85 100644
> > --- a/drivers/pps/clients/pps-ldisc.c
> > +++ b/drivers/pps/clients/pps-ldisc.c
> > @@ -50,7 +50,7 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> > /* Now do the PPS event report */
> > pps = (struct pps_device *)tty->disc_data;
> > if (pps != NULL) {
> > - pps_event(pps, ts, status ? PPS_CAPTUREASSERT :
> > + pps_event_irq(pps, ts, status ? PPS_CAPTUREASSERT :
> > PPS_CAPTURECLEAR, NULL);
> > spin_unlock_irqrestore(&pps_ldisc_lock, flags);
> > dev_dbg(pps->dev, "PPS %s at %lu\n",
> > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > index f5b2b78..ca3b4f8 100644
> > --- a/drivers/pps/kapi.c
> > +++ b/drivers/pps/kapi.c
> > @@ -32,9 +32,19 @@
> > #include <linux/slab.h>
> >
> > /*
> > + * Global variables
> > + */
> > +
> > +/* PPS event workqueue */
> > +struct workqueue_struct *pps_event_workqueue;
> > +
> > +/*
> > * Local functions
> > */
> >
> > +static void assert_work_func(struct work_struct *work);
> > +static void clear_work_func(struct work_struct *work);
> > +
> > static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
> > {
> > ts->nsec += offset->nsec;
> > @@ -108,6 +118,9 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> > init_waitqueue_head(&pps->queue);
> > spin_lock_init(&pps->lock);
> >
> > + INIT_WORK(&pps->assert_work, assert_work_func);
> > + INIT_WORK(&pps->clear_work, clear_work_func);
> > +
> > /* Create the char device */
> > err = pps_register_cdev(pps);
> > if (err < 0) {
> > @@ -152,11 +165,12 @@ EXPORT_SYMBOL(pps_unregister_source);
> > * @event: the event type
> > * @data: userdef pointer
> > *
> > - * This function is used by each PPS client in order to register a new
> > - * PPS event into the system (it's usually called inside an IRQ handler).
> > + * This function is used by PPS clients in order to register a new
> > + * PPS event into the system. It should not be called from an IRQ
> > + * handler. Use pps_event_irq instead.
> > *
> > - * If an echo function is associated with the PPS device it will be called
> > - * as:
> > + * If an echo function is associated with the PPS device it will be
> > + * called as:
> > * pps->info.echo(pps, event, data);
> > */
> > void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> > @@ -226,3 +240,76 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> > spin_unlock_irqrestore(&pps->lock, flags);
> > }
> > EXPORT_SYMBOL(pps_event);
> > +
> > +/* Async event handlers */
> > +
> > +static void assert_work_func(struct work_struct *work)
> > +{
> > + struct pps_device *pps = container_of(work,
> > + struct pps_device, assert_work);
> > +
> > + pps_event(pps, &pps->assert_work_ts, PPS_CAPTUREASSERT,
> > + pps->assert_work_data);
> > +}
> > +
> > +static void clear_work_func(struct work_struct *work)
> > +{
> > + struct pps_device *pps = container_of(work,
> > + struct pps_device, clear_work);
> > +
> > + pps_event(pps, &pps->clear_work_ts, PPS_CAPTURECLEAR,
> > + pps->clear_work_data);
> > +}
>
> The RFC-2783 says that (see 3.1 PPS abstraction):
>
> The API optionally supports an "echo" feature, in which events on
> the
> incoming PPS signal may be reflected through software, after the
> capture of the corresponding timestamp, to an output signal pin.
> This feature may be used to discover an upper bound on the actual
> delay between the edges of the PPS signal and the capture of the
> timestamps; such information may be useful in precise calibration
> of
> the system.
>
> The designation of an output pin for the echo signal, and sense and
> shape of the output transition, is outside the scope of this
> specification, but SHOULD be documented for each implementation.
> The
> output pin MAY also undergo transitions at other times besides
> those
> caused by PPS input events.
>
> By applying this patch the echo function is called inside a work queue
> so it depends to the scheduler. I suppose this is not acceptable,
> otherwise we should drop the echo function support.
>
> > +/* pps_event_irq - register a PPS event for deffered handling using
> > + * workqueue
> > + *
> > + * @pps: the PPS device
> > + * @ts: the event timestamp
> > + * @event: the event type
> > + * @data: userdef pointer
> > + *
> > + * This function is used by PPS clients in order to register a new
> > + * PPS event into the system. It should be called from an IRQ handler
> > + * only.
> > + *
> > + * If an echo function is associated with the PPS device it will be
> > + * called as:
> > + * pps->info.echo(pps, event, data);
> > + */
>
> The above comment talks about the echo function but you removed it
> from the code below...
Well, I meant that it will be called from pps_event(). :)
Actually just copied this comment from pps_event() and thought that
it's still right somehow. Do you want me to remove this comment?
> > +void pps_event_irq(struct pps_device *pps, struct pps_event_time *ts,
> > + int event, void *data)
> > +{
> > + /* check event type */
> > + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> > +
> > + if (event & PPS_CAPTUREASSERT) {
> > + if (work_pending(&pps->assert_work)) {
> > + dev_err(pps->dev, "deferred assert edge work haven't"
> > + " been handled within a second\n");
> > + /* FIXME: do something more intelligent
> > + * then just exit */
> > + } else {
> > + /* now we can copy data safely */
> > + pps->assert_work_ts = *ts;
> > + pps->assert_work_data = data;
> > +
> > + queue_work(pps_event_workqueue, &pps->assert_work);
> > + }
> > + }
> > + if (event & PPS_CAPTURECLEAR) {
> > + if (work_pending(&pps->clear_work)) {
> > + dev_err(pps->dev, "deferred clear edge work haven't"
> > + " been handled within a second\n");
> > + /* FIXME: do something more intelligent
> > + * then just exit */
> > + } else {
> > + /* now we can copy data safely */
> > + pps->clear_work_ts = *ts;
> > + pps->clear_work_data = data;
> > +
> > + queue_work(pps_event_workqueue, &pps->clear_work);
> > + }
> > + }
> > +}
> > +EXPORT_SYMBOL(pps_event_irq);
> > diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> > index 79b4455..f642558 100644
> > --- a/drivers/pps/pps.c
> > +++ b/drivers/pps/pps.c
> > @@ -321,18 +321,26 @@ void pps_unregister_cdev(struct pps_device *pps)
> >
> > static void __exit pps_exit(void)
> > {
> > - class_destroy(pps_class);
> > unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
> > + class_destroy(pps_class);
> > + destroy_workqueue(pps_event_workqueue);
> > }
> >
> > static int __init pps_init(void)
> > {
> > int err;
> >
> > + pps_event_workqueue = create_workqueue("pps");
> > + if (!pps_event_workqueue) {
> > + pr_err("failed to create workqueue\n");
> > + return -ENOMEM;
> > + }
> > +
> > pps_class = class_create(THIS_MODULE, "pps");
> > if (!pps_class) {
> > pr_err("failed to allocate class\n");
> > - return -ENOMEM;
> > + err = -ENOMEM;
> > + goto destroy_workqueue;
> > }
> > pps_class->dev_attrs = pps_attrs;
> >
> > @@ -350,6 +358,8 @@ static int __init pps_init(void)
> >
> > remove_class:
> > class_destroy(pps_class);
> > +destroy_workqueue:
> > + destroy_workqueue(pps_event_workqueue);
> >
> > return err;
> > }
> > diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> > index 1aedf50..5af0498 100644
> > --- a/include/linux/pps_kernel.h
> > +++ b/include/linux/pps_kernel.h
> > @@ -26,6 +26,7 @@
> > #include <linux/cdev.h>
> > #include <linux/device.h>
> > #include <linux/time.h>
> > +#include <linux/workqueue.h>
> >
> > /*
> > * Global defines
> > @@ -70,6 +71,13 @@ struct pps_device {
> > struct device *dev;
> > struct fasync_struct *async_queue; /* fasync method */
> > spinlock_t lock;
> > +
> > + struct work_struct assert_work;
> > + struct work_struct clear_work;
> > + struct pps_event_time assert_work_ts;
> > + struct pps_event_time clear_work_ts;
> > + void *assert_work_data;
> > + void *clear_work_data;
> > };
> >
> > /*
> > @@ -78,6 +86,8 @@ struct pps_device {
> >
> > extern struct device_attribute pps_attrs[];
> >
> > +extern struct workqueue_struct *pps_event_workqueue;
> > +
> > /*
> > * Exported functions
> > */
> > @@ -89,6 +99,8 @@ extern int pps_register_cdev(struct pps_device *pps);
> > extern void pps_unregister_cdev(struct pps_device *pps);
> > extern void pps_event(struct pps_device *pps,
> > struct pps_event_time *ts, int event, void *data);
> > +extern void pps_event_irq(struct pps_device *pps,
> > + struct pps_event_time *ts, int event, void *data);
> >
> > static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
> > struct timespec ts)
> > --
> > 1.7.2.3
> >
>
> Ciao,
>
> Rodolfo
>
--
Alexander
В Sat, 20 Nov 2010 17:13:51 +0100
Rodolfo Giometti <[email protected]> пишет:
> On Thu, Nov 18, 2010 at 07:01:03PM +0300, Alexander Gordeev wrote:
> > This way less overhead is involved when running production kernel.
> > If you want to debug a pps client module please define DEBUG to enable
> > the checks.
> >
> > Signed-off-by: Alexander Gordeev <[email protected]>
> > ---
> > drivers/pps/kapi.c | 33 ++++++++++-----------------------
> > 1 files changed, 10 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > index fe832aa..54261c4 100644
> > --- a/drivers/pps/kapi.c
> > +++ b/drivers/pps/kapi.c
> > @@ -81,25 +81,14 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> > int err;
> >
> > /* Sanity checks */
> > - if ((info->mode & default_params) != default_params) {
> > - pr_err("pps: %s: unsupported default parameters\n",
> > - info->name);
> > - err = -EINVAL;
> > - goto pps_register_source_exit;
> > - }
> > - if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> > - info->echo == NULL) {
> > - pr_err("pps: %s: echo function is not defined\n",
> > - info->name);
> > - err = -EINVAL;
> > - goto pps_register_source_exit;
> > - }
> > - if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
> > - pr_err("pps: %s: unspecified time format\n",
> > - info->name);
> > - err = -EINVAL;
> > - goto pps_register_source_exit;
> > - }
> > +
> > + /* default_params should be supported */
> > + BUG_ON((info->mode & default_params) != default_params);
> > + /* echo function should be defined if we are asked to call it */
> > + BUG_ON((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> > + info->echo == NULL);
> > + /* time format should be specified */
> > + BUG_ON((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0);
>
> Nack.
>
> If the userland gives us some wrong parameters this is not the same of
> a kernel bug (which BUG_ON is used for). The userland must be notified
> about the wrong input.
I agree with what you said completely but this is not a user-space API.
pps_register_source() can only be called from other kernel code.
> > /* Allocate memory for the new PPS source struct */
> > pps = kzalloc(sizeof(struct pps_device), GFP_KERNEL);
> > @@ -179,10 +168,8 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> > int captured = 0;
> > struct pps_ktime ts_real;
> >
> > - if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> > - dev_err(pps->dev, "unknown event (%x)\n", event);
> > - return;
> > - }
> > + /* check event type */
> > + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
>
> Ack.
>
> This is a correct usage of BUG_ON. :)
>
> > dev_dbg(pps->dev, "PPS event at %ld.%09ld\n",
> > ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
> > --
> > 1.7.2.3
> >
>
> Ciao,
>
> Rodolfo
>
--
Alexander
В Sat, 20 Nov 2010 09:01:41 -0800
Joe Perches <[email protected]> пишет:
> On Sat, 2010-11-20 at 17:13 +0100, Rodolfo Giometti wrote:
> > On Thu, Nov 18, 2010 at 07:01:03PM +0300, Alexander Gordeev wrote:
> > > This way less overhead is involved when running production kernel.
> > > If you want to debug a pps client module please define DEBUG to enable
> > > the checks.
> > > Signed-off-by: Alexander Gordeev <[email protected]>
> []
> > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > > @@ -179,10 +168,8 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> > > int captured = 0;
> > > struct pps_ktime ts_real;
> > >
> > > - if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> > > - dev_err(pps->dev, "unknown event (%x)\n", event);
> > > - return;
> > > - }
> > > + /* check event type */
> > > + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> >
> > Ack.
> >
> > This is a correct usage of BUG_ON. :)
>
> I don't think that's true.
>
> /*
> * Don't use BUG() or BUG_ON() unless there's really no way out; one
> * example might be detecting data structure corruption in the middle
> * of an operation that can't be backed out of. If the (sub)system
> * can somehow continue operating, perhaps with reduced functionality,
> * it's probably not BUG-worthy.
> *
> * If you're tempted to BUG(), think again: is completely giving up
> * really the *only* solution? There are usually better options, where
> * users don't need to reboot ASAP and can mostly shut down cleanly.
> */
Hmm, didn't check that before. What is more appropriate in this
situation?
--
Alexander
В Sat, 20 Nov 2010 17:23:04 +0100
Rodolfo Giometti <[email protected]> пишет:
> On Thu, Nov 18, 2010 at 07:01:05PM +0300, Alexander Gordeev wrote:
> > Remove the code that gatheres timestamp in pps_tty_dcd_change() in case
> > passed ts parameter is NULL because it never happens in the current
> > code. Fix comments as well.
> >
> > Signed-off-by: Alexander Gordeev <[email protected]>
> > ---
> > Documentation/serial/tty.txt | 2 +-
> > drivers/pps/clients/pps-ldisc.c | 8 --------
> > include/linux/tty_ldisc.h | 2 +-
> > 3 files changed, 2 insertions(+), 10 deletions(-)
> >
> > diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt
> > index 7c90050..540db41 100644
> > --- a/Documentation/serial/tty.txt
> > +++ b/Documentation/serial/tty.txt
> > @@ -107,7 +107,7 @@ write_wakeup() - May be called at any point between open and close.
> >
> > dcd_change() - Report to the tty line the current DCD pin status
> > changes and the relative timestamp. The timestamp
> > - can be NULL.
> > + cannot be NULL.
>
> Can you please explain why it cannot be null? O_o
>
> Who does supply the timestamp to dcd_change?
dcd_change() is called only from one place - uart_handle_dcd_change() -
which always obtains and passes timestamps to the former. So no need to
check if ts is NULL and obtain ts again.
--
Alexander
В Sat, 20 Nov 2010 17:27:18 +0100
Rodolfo Giometti <[email protected]> пишет:
> On Thu, Nov 18, 2010 at 07:01:06PM +0300, Alexander Gordeev wrote:
> > This commit adds hardpps() implementation based upon the original one
> > from the NTPv4 reference kernel code from David Mills. However, it is
> > highly optimized towards very fast syncronization and maximum stickness
> > to PPS signal. The typical error is less then a microsecond.
> > To make it sync faster I had to throw away exponential phase filter so
> > that the full phase offset is corrected immediately. Then I also had to
> > throw away median phase filter because it gives a bigger error itself
> > if used without exponential filter.
> > Maybe we will find an appropriate filtering scheme in the future but
> > it's not necessary if the signal quality is ok.
>
> This patch (and follows) adds a new functionality to PPS subsystem, so
> let me suggest to you splitting in two parts this patch set: a first
> patch set who fixes up the current PPS implementation, and a second
> patch set who adds kernel consumer (and follows).
But the patches that add hardpps and parport client+generator depend on
the previous ones because the latter change lots of things in PPS
subsystem. I don't want to maintain them separately because we use all
of them in the production.
However I can tell here that it'll be ok for me if not all the patches
enter mainline at the same time. For example, patches 1-3 are already
ACKed and are actually bugfixes (patch 2 is trivial and patch 3 depends
on it) so I think they could be merged in the next rc. I placed them
in the beginning exactly for this reason. Whom should I mail them?
Same thing with other patches. Smaller patchset -> me (and probably
users) more happy. :)
--
Alexander
On Sun, 2010-11-21 at 03:40 +0300, Alexander Gordeev wrote:
> В Sat, 20 Nov 2010 09:01:41 -0800
> Joe Perches <[email protected]> пишет:
> > On Sat, 2010-11-20 at 17:13 +0100, Rodolfo Giometti wrote:
> > > On Thu, Nov 18, 2010 at 07:01:03PM +0300, Alexander Gordeev wrote:
> > > > This way less overhead is involved when running production kernel.
> > > > If you want to debug a pps client module please define DEBUG to enable
> > > > the checks.
> > > > Signed-off-by: Alexander Gordeev <[email protected]>
> > []
> > > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > > > @@ -179,10 +168,8 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> > > > int captured = 0;
> > > > struct pps_ktime ts_real;
> > > > - if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> > > > - dev_err(pps->dev, "unknown event (%x)\n", event);
> > > > - return;
> > > > - }
> > > > + /* check event type */
> > > > + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> > > Ack.
> > > This is a correct usage of BUG_ON. :)
> > I don't think that's true.
> > /*
> > * Don't use BUG() or BUG_ON() unless there's really no way out; one
> > * example might be detecting data structure corruption in the middle
> > * of an operation that can't be backed out of. If the (sub)system
> > * can somehow continue operating, perhaps with reduced functionality,
> > * it's probably not BUG-worthy.
> > *
> > * If you're tempted to BUG(), think again: is completely giving up
> > * really the *only* solution? There are usually better options, where
> > * users don't need to reboot ASAP and can mostly shut down cleanly.
> > */
> Hmm, didn't check that before. What is more appropriate in this
> situation?
If it's really a non-recoverable event, perhaps emit a dev_crit
and then do a module_put?
On Sun, Nov 21, 2010 at 12:33:23AM +0300, Alexander Gordeev wrote:
> ?? Sat, 20 Nov 2010 16:49:40 +0100
> Rodolfo Giometti <[email protected]> ??????????:
>
> > On Thu, Nov 18, 2010 at 07:00:59PM +0300, Alexander Gordeev wrote:
> > > Since we now have direct pointers to struct pps_device everywhere it's
> > > easy to use dev_* functions to print messages instead of plain printks.
> > > Where dev_* cannot be used printks are converted to pr_*.
> > >
> > > Signed-off-by: Alexander Gordeev <[email protected]>
> > > ---
> > > drivers/pps/clients/pps-ktimer.c | 5 +++--
> > > drivers/pps/clients/pps-ldisc.c | 2 ++
> > > drivers/pps/kapi.c | 15 ++++++++-------
> > > drivers/pps/pps.c | 25 +++++++++++++------------
> > > 4 files changed, 26 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c
> > > index 966ead1..2728469 100644
> > > --- a/drivers/pps/clients/pps-ktimer.c
> > > +++ b/drivers/pps/clients/pps-ktimer.c
> > > @@ -19,6 +19,7 @@
> > > * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > > */
> > >
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >
> > Instead of redefining pr_fmt() in each file you may do it once into
> > include file pps_kernel.h...
>
> Yes, but then every .c file will have to include linux/pps_kernel.h
> before including linux/kernel.h (where pr_* are defined) and only you
> and me will know the reason for that. Of course, we can add a comment to
> every .c file to discourage others from moving includes but IMHO the
> way how it is done in the patch is better. I dislike hidden
> dependencies between header files very much.
Ok.
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Sun, Nov 21, 2010 at 01:33:27AM +0300, Alexander Gordeev wrote:
> > > - if (ret < 0) {
> > > + if (pps == NULL) {
> > > pr_err("cannot register PPS source \"%s\"\n", info.path);
> > > - return ret;
> > > + return -ENOMEM;
> > > }
> > > - tty->disc_data = (void *)(long)ret;
> > > +
> > > + spin_lock_irq(&pps_ldisc_lock);
> > > + tty->disc_data = pps;
> > > + spin_unlock_irq(&pps_ldisc_lock);
> >
> > Maybe this lock is useless... however, are we sure that before setting
> > tty->disc_data to pps its value is null? Otherwise the dcd_change may
> > be called with an oops! We cannot control serial port IRQ
> > generation... :-/
>
> No, locking here is necessary.
> There is only one problem this spinlock protects us from: current tty
> code neither disables interrupts nor doesn't ensure there are no
> references to PPS ldisc from uart_handle_dcd_change() before closing it
> (and removing PPS source). It relies on flushing workqueue and disabling
> input. It worked good this way until dcd_change() was added which
> doesn't use workqueues and is called in atomic context so can't lock
> on mutex.
>
> Imagine that (on SMP system) uart_handle_dcd_change() could obtain a
> reference to ldisc and call dcd_change() until actually calling
> pps_event(); then on another processor all the path from
> tty_ldisc_halt() until tty_ldisc_stop() is executed. And then
> pps_event() is called with illegal pps pointer.
>
> I just thought you are right that disc_data can be set not NULL by
> another ldisc and it's a problem. But actually I just realised how to
> fix it completely. :)
>
> I just have to add a spinlock to tty_struct, lock all the
> uart_handle_dcd_change() with it and add a "barrier" between
> tty_ldisc_halt() and tty_ldisc_close() i.e. just that:
>
> ...
> spin_lock_irq();
> spin_unlock_irq();
> ...
>
> This "barrier" will ensure that there is no references to ldisc from
> uart_handle_dcd_change(). It won't be able to obtain a new reference
> after tty_ldisc_halt() so will become completely sane. Not disabling
> interrupts won't be a problem because it won't be able to obtain an
> ldisc reference until tty_ldisc_enable() which is called only after the
> new ldisc is fully functional. If it's our ldisc than it will have both
> dcd_change defined and a valid pps pointer. If it's not our ldisc it
> won't have both so uart_handle_dcd_change() won't call dcd_change() at
> all.
>
> I think I'll do that as a separate patch.
Excuse me but IMHO you should solve all your problems if you do the lock into
pps_tty_init/cleanup instead of into pps_tty_open/close.
spin_lock_irq();
err = tty_register_ldisc(N_PPS, &pps_ldisc_ops);
if (err)
pr_err("can't register PPS line discipline\n");
else
pr_info("PPS line discipline registered\n");
spin_unlock_irq();
And the same into cleanup.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Sun, Nov 21, 2010 at 02:23:02AM +0300, Alexander Gordeev wrote:
> ?? Sat, 20 Nov 2010 17:08:51 +0100
> Rodolfo Giometti <[email protected]> ??????????:
>
> > On Thu, Nov 18, 2010 at 07:01:01PM +0300, Alexander Gordeev wrote:
> > > This handler should be called from an IRQ handler. It uses per-device
> > > workqueue internally.
> >
> > Can you please explain to me why do you need this patch? Maybe you can
> > add a verbose patch's description? :)
>
> Well, it's all about optimizing latencies on rt-preempt kernel: if
> everything is done in a process context than we can use mutexes
> (haven't done that yet) and don't disable interrupts, which is better
> for hard real time. I measured that pps_event with kernel consumer
> enabled takes 1-2us on my test machine. Not a big deal, of course...
>
> Sorry, I've completely forgotten about an echo function. However quick
> look to current clients shows that it is only used in pps-ktimer.c
> for debug printing... Maybe it's not needed at all? I mean, have you
> ever got any user request for this feature? If yes, it can be removed
> IMHO since RFC-2783 says that it's optional. I can handle the removal in
> the next version of the patchset.
>
> If you don't want it I can have this patch on my rt branch only and
> don't try to push it into mainline.
The echo function is not used as far as I know, so it's ok for me to
remove echo support if you get better performance.
In case we can study how to reimplement it! ;)
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Sun, Nov 21, 2010 at 03:13:05AM +0300, Alexander Gordeev wrote:
> ?? Sat, 20 Nov 2010 17:13:51 +0100
> Rodolfo Giometti <[email protected]> ??????????:
>
> > On Thu, Nov 18, 2010 at 07:01:03PM +0300, Alexander Gordeev wrote:
> > > This way less overhead is involved when running production kernel.
> > > If you want to debug a pps client module please define DEBUG to enable
> > > the checks.
> > >
> > > Signed-off-by: Alexander Gordeev <[email protected]>
> > > ---
> > > drivers/pps/kapi.c | 33 ++++++++++-----------------------
> > > 1 files changed, 10 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > > index fe832aa..54261c4 100644
> > > --- a/drivers/pps/kapi.c
> > > +++ b/drivers/pps/kapi.c
> > > @@ -81,25 +81,14 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
> > > int err;
> > >
> > > /* Sanity checks */
> > > - if ((info->mode & default_params) != default_params) {
> > > - pr_err("pps: %s: unsupported default parameters\n",
> > > - info->name);
> > > - err = -EINVAL;
> > > - goto pps_register_source_exit;
> > > - }
> > > - if ((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> > > - info->echo == NULL) {
> > > - pr_err("pps: %s: echo function is not defined\n",
> > > - info->name);
> > > - err = -EINVAL;
> > > - goto pps_register_source_exit;
> > > - }
> > > - if ((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
> > > - pr_err("pps: %s: unspecified time format\n",
> > > - info->name);
> > > - err = -EINVAL;
> > > - goto pps_register_source_exit;
> > > - }
> > > +
> > > + /* default_params should be supported */
> > > + BUG_ON((info->mode & default_params) != default_params);
> > > + /* echo function should be defined if we are asked to call it */
> > > + BUG_ON((info->mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) != 0 &&
> > > + info->echo == NULL);
> > > + /* time format should be specified */
> > > + BUG_ON((info->mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0);
> >
> > Nack.
> >
> > If the userland gives us some wrong parameters this is not the same of
> > a kernel bug (which BUG_ON is used for). The userland must be notified
> > about the wrong input.
>
> I agree with what you said completely but this is not a user-space API.
> pps_register_source() can only be called from other kernel code.
Yes, but in turn pps_register_source() is called in a function called
from userspace. The user should know if he/she cannot safely register
his/her new PPS source.
The BUG_ON() should be used for a kernel bug like "Ehi! This can't
happen! Data are corrupted"... but here we are just checking some
input parameters.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Sun, Nov 21, 2010 at 03:40:08AM +0300, Alexander Gordeev wrote:
> ?? Sat, 20 Nov 2010 09:01:41 -0800
> Joe Perches <[email protected]> ??????????:
>
> > On Sat, 2010-11-20 at 17:13 +0100, Rodolfo Giometti wrote:
> > > On Thu, Nov 18, 2010 at 07:01:03PM +0300, Alexander Gordeev wrote:
> > > > This way less overhead is involved when running production kernel.
> > > > If you want to debug a pps client module please define DEBUG to enable
> > > > the checks.
> > > > Signed-off-by: Alexander Gordeev <[email protected]>
> > []
> > > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> > > > @@ -179,10 +168,8 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
> > > > int captured = 0;
> > > > struct pps_ktime ts_real;
> > > >
> > > > - if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> > > > - dev_err(pps->dev, "unknown event (%x)\n", event);
> > > > - return;
> > > > - }
> > > > + /* check event type */
> > > > + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> > >
> > > Ack.
> > >
> > > This is a correct usage of BUG_ON. :)
> >
> > I don't think that's true.
> >
> > /*
> > * Don't use BUG() or BUG_ON() unless there's really no way out; one
> > * example might be detecting data structure corruption in the middle
> > * of an operation that can't be backed out of. If the (sub)system
> > * can somehow continue operating, perhaps with reduced functionality,
> > * it's probably not BUG-worthy.
> > *
> > * If you're tempted to BUG(), think again: is completely giving up
> > * really the *only* solution? There are usually better options, where
> > * users don't need to reboot ASAP and can mostly shut down cleanly.
> > */
>
> Hmm, didn't check that before. What is more appropriate in this
> situation?
IMHO here you can use BUG_ON() but not into pps_register_source().
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On Sun, Nov 21, 2010 at 03:44:02AM +0300, Alexander Gordeev wrote:
> ?? Sat, 20 Nov 2010 17:23:04 +0100
> Rodolfo Giometti <[email protected]> ??????????:
>
> > On Thu, Nov 18, 2010 at 07:01:05PM +0300, Alexander Gordeev wrote:
> > > Remove the code that gatheres timestamp in pps_tty_dcd_change() in case
> > > passed ts parameter is NULL because it never happens in the current
> > > code. Fix comments as well.
> > >
> > > Signed-off-by: Alexander Gordeev <[email protected]>
> > > ---
> > > Documentation/serial/tty.txt | 2 +-
> > > drivers/pps/clients/pps-ldisc.c | 8 --------
> > > include/linux/tty_ldisc.h | 2 +-
> > > 3 files changed, 2 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/Documentation/serial/tty.txt b/Documentation/serial/tty.txt
> > > index 7c90050..540db41 100644
> > > --- a/Documentation/serial/tty.txt
> > > +++ b/Documentation/serial/tty.txt
> > > @@ -107,7 +107,7 @@ write_wakeup() - May be called at any point between open and close.
> > >
> > > dcd_change() - Report to the tty line the current DCD pin status
> > > changes and the relative timestamp. The timestamp
> > > - can be NULL.
> > > + cannot be NULL.
> >
> > Can you please explain why it cannot be null? O_o
> >
> > Who does supply the timestamp to dcd_change?
>
> dcd_change() is called only from one place - uart_handle_dcd_change() -
> which always obtains and passes timestamps to the former. So no need to
> check if ts is NULL and obtain ts again.
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
> Maybe this lock is useless... however, are we sure that before setting
> tty->disc_data to pps its value is null? Otherwise the dcd_change may
> be called with an oops! We cannot control serial port IRQ
> generation... :-/
tty->disc_data is LDISC private. It may be any arbitary value on entry to
the ldisc open method.
> > + spin_lock_irq(&pps_ldisc_lock);
> > + tty->disc_data = NULL;
> > + spin_unlock_irq(&pps_ldisc_lock);
And you don't need to set it to NULL after - some ldiscs do this mostly
as a debug aid.
Alan
В Sun, 21 Nov 2010 14:12:26 +0000
Alan Cox <[email protected]> пишет:
>
> > Maybe this lock is useless... however, are we sure that before setting
> > tty->disc_data to pps its value is null? Otherwise the dcd_change may
> > be called with an oops! We cannot control serial port IRQ
> > generation... :-/
>
> tty->disc_data is LDISC private. It may be any arbitary value on entry to
> the ldisc open method.
>
>
> > > + spin_lock_irq(&pps_ldisc_lock);
> > > + tty->disc_data = NULL;
> > > + spin_unlock_irq(&pps_ldisc_lock);
>
> And you don't need to set it to NULL after - some ldiscs do this mostly
> as a debug aid.
It was a part of attempt to do a workaround. But it still is not safe
enough because it doesn't protect from a sudden ldisc change while we
are at the beginning of pps_tty_dcd_change() i.e. haven't acquired spin
lock yet. The real fix is ready, I'll send it soon.
--
Alexander
В Sun, 21 Nov 2010 09:26:58 +0100
Rodolfo Giometti <[email protected]> пишет:
> On Sun, Nov 21, 2010 at 01:33:27AM +0300, Alexander Gordeev wrote:
> > > > - if (ret < 0) {
> > > > + if (pps == NULL) {
> > > > pr_err("cannot register PPS source \"%s\"\n", info.path);
> > > > - return ret;
> > > > + return -ENOMEM;
> > > > }
> > > > - tty->disc_data = (void *)(long)ret;
> > > > +
> > > > + spin_lock_irq(&pps_ldisc_lock);
> > > > + tty->disc_data = pps;
> > > > + spin_unlock_irq(&pps_ldisc_lock);
> > >
> > > Maybe this lock is useless... however, are we sure that before setting
> > > tty->disc_data to pps its value is null? Otherwise the dcd_change may
> > > be called with an oops! We cannot control serial port IRQ
> > > generation... :-/
> >
> > No, locking here is necessary.
> > There is only one problem this spinlock protects us from: current tty
> > code neither disables interrupts nor doesn't ensure there are no
> > references to PPS ldisc from uart_handle_dcd_change() before closing it
> > (and removing PPS source). It relies on flushing workqueue and disabling
> > input. It worked good this way until dcd_change() was added which
> > doesn't use workqueues and is called in atomic context so can't lock
> > on mutex.
> >
> > Imagine that (on SMP system) uart_handle_dcd_change() could obtain a
> > reference to ldisc and call dcd_change() until actually calling
> > pps_event(); then on another processor all the path from
> > tty_ldisc_halt() until tty_ldisc_stop() is executed. And then
> > pps_event() is called with illegal pps pointer.
> >
> > I just thought you are right that disc_data can be set not NULL by
> > another ldisc and it's a problem. But actually I just realised how to
> > fix it completely. :)
> >
> > I just have to add a spinlock to tty_struct, lock all the
> > uart_handle_dcd_change() with it and add a "barrier" between
> > tty_ldisc_halt() and tty_ldisc_close() i.e. just that:
> >
> > ...
> > spin_lock_irq();
> > spin_unlock_irq();
> > ...
> >
> > This "barrier" will ensure that there is no references to ldisc from
> > uart_handle_dcd_change(). It won't be able to obtain a new reference
> > after tty_ldisc_halt() so will become completely sane. Not disabling
> > interrupts won't be a problem because it won't be able to obtain an
> > ldisc reference until tty_ldisc_enable() which is called only after the
> > new ldisc is fully functional. If it's our ldisc than it will have both
> > dcd_change defined and a valid pps pointer. If it's not our ldisc it
> > won't have both so uart_handle_dcd_change() won't call dcd_change() at
> > all.
> >
> > I think I'll do that as a separate patch.
>
> Excuse me but IMHO you should solve all your problems if you do the lock into
> pps_tty_init/cleanup instead of into pps_tty_open/close.
>
> spin_lock_irq();
> err = tty_register_ldisc(N_PPS, &pps_ldisc_ops);
> if (err)
> pr_err("can't register PPS line discipline\n");
> else
> pr_info("PPS line discipline registered\n");
> spin_unlock_irq();
>
> And the same into cleanup.
Sorry, I don't understand how this is supposed to help. Anyway, a new
patch is underway.
--
Alexander