[Linuxptp-devel] [PATCH] PTP subsystem: implement POSIX timer interface

Discussion:

Kieran Tyrrell

2016-09-09 21:39:09 UTC

a PTP hardware clock supporting timers (alarms) can now be used via the POSIX timer (timer_create, timer_settime etc) interface

Signed-off-by: Kieran Tyrrell <***@sienda.com>
---
drivers/ptp/ptp_clock.c | 233 +++++++++++++++++++++++++++++++++++++++
drivers/ptp/ptp_private.h | 4 +
include/linux/ptp_clock_kernel.h | 2 +
kernel/signal.c | 1 +
4 files changed, 240 insertions(+)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 2e481b9..067e41c 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -36,6 +36,8 @@
#define PTP_PPS_EVENT PPS_CAPTUREASSERT
#define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)

+#define PTP_TIMER_MINIMUM_INTERVAL_NS 100000
+
/* private globals */

static dev_t ptp_devt;
@@ -43,6 +45,108 @@ static struct class *ptp_class;

static DEFINE_IDA(ptp_clocks_map);

+static int ptp_clock_gettime(struct posix_clock *pc, struct timespec *tp);
+
+static int set_device_timer_earliest(struct ptp_clock *ptp)
+{
+ struct timerqueue_node *next;
+ int err;
+ unsigned long tq_lock_flags;
+ struct timespec64 ts;
+
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ next = timerqueue_getnext(&ptp->timerqueue);
+
+ /* Skip over expired or not set timers */
+ while (next) {
+ if (next->expires.tv64 != 0)
+ break;
+ next = timerqueue_iterate_next(next);
+ }
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ if (next) {
+ ts = ktime_to_timespec64(next->expires);
+ err = ptp->info->timersettime(ptp->info, &ts);
+ if(err)
+ return err;
+ }
+
+ return 0;
+}
+
+static void ptp_alarm_work(struct work_struct *work)
+{
+ struct ptp_clock *ptp = container_of(work, struct ptp_clock, alarm_work);
+ struct task_struct *task;
+ struct siginfo info;
+ struct timerqueue_node *next;
+ struct k_itimer *kit;
+ struct timespec time_now;
+ s64 ns_now;
+ int shared;
+ bool signal_failed_to_send;
+ unsigned long tq_lock_flags;
+
+ if(0 != ptp_clock_gettime(&ptp->clock, &time_now))
+ return;
+
+ ns_now = timespec_to_ns(&time_now);
+
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ next = timerqueue_getnext(&ptp->timerqueue);
+
+ /* Skip over expired or not set timers */
+ while (next) {
+ if (next->expires.tv64 != 0)
+ break;
+ next = timerqueue_iterate_next(next);
+ }
+
+ while (next) {
+ if (next->expires.tv64 > ns_now)
+ break;
+
+ rcu_read_lock();
+
+ kit = container_of(next, struct k_itimer, it.real.timer.node);
+
+ task = pid_task(kit->it_pid, PIDTYPE_PID);
+ if (task)
+ {
+ memset(&info, 0, sizeof(info));
+ info.si_signo = SIGALRM;
+ info.si_code = SI_TIMER;
+ info._sifields._timer._tid = kit->it_id;
+ kit->sigq->info.si_sys_private = 0;
+ shared = !(kit->it_sigev_notify & SIGEV_THREAD_ID);
+ signal_failed_to_send = send_sigqueue(kit->sigq, task, shared) > 0;
+ }
+ rcu_read_unlock();
+
+ next = timerqueue_iterate_next(next);
+
+ /* update and reinsert the last one that has fired */
+ timerqueue_del(&ptp->timerqueue, &kit->it.real.timer.node);
+ if ( (0 == ktime_to_ns(kit->it.real.interval)) || signal_failed_to_send) {
+ /* this is not a periodic timer (or the signal failed to send), so stop it */
+ kit->it.real.timer.node.expires = ns_to_ktime(0);
+ }
+ else {
+ /* this IS a periodic timer, so set the next fire time */
+ kit->it.real.timer.node.expires = ktime_add(kit->it.real.timer.node.expires, kit->it.real.interval);
+ }
+ timerqueue_add(&ptp->timerqueue, &kit->it.real.timer.node);
+ }
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ set_device_timer_earliest(ptp);
+}
+
/* time stamp event queue operations */

static inline int queue_free(struct timestamp_event_queue *q)
@@ -163,12 +267,135 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct timex *tx)
return err;
}

+static int ptp_timer_create(struct posix_clock *pc, struct k_itimer *kit)
+{
+ struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ int err = 0;
+ unsigned long tq_lock_flags;
+
+ if(ptp->info->timerenable == 0)
+ return -EOPNOTSUPP;
+
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ if(NULL == timerqueue_getnext(&ptp->timerqueue))
+ {
+ /* list is empty, so hardware timer is disabled, enable it */
+ err = ptp->info->timerenable(ptp->info, true);
+ }
+
+ if(0 == err)
+ {
+ timerqueue_init(&kit->it.real.timer.node);
+ /* ensure expiry time is 0 (timer disabled) */
+ kit->it.real.timer.node.expires = ns_to_ktime(0);
+ timerqueue_add(&ptp->timerqueue, &kit->it.real.timer.node);
+ }
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ return err;
+}
+
+static int ptp_timer_delete(struct posix_clock *pc, struct k_itimer *kit)
+{
+ struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ int err=0;
+ unsigned long tq_lock_flags;
+
+ if(ptp->info->timerenable == 0)
+ return -EOPNOTSUPP;
+
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ timerqueue_del(&ptp->timerqueue, &kit->it.real.timer.node);
+
+ if(NULL == timerqueue_getnext(&ptp->timerqueue))
+ {
+ /* there are no more timers set on this device, so we can disable the hardware timer */
+ err = ptp->info->timerenable(ptp->info, false);
+ }
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ return err;
+}
+
+static void ptp_timer_gettime(struct posix_clock *pc,
+ struct k_itimer *kit, struct itimerspec *tsp)
+{
+ struct timespec time_now;
+
+ if(NULL == tsp)
+ return;
+
+ if(0 != ptp_clock_gettime(pc, &time_now))
+ return;
+
+ tsp->it_interval = ktime_to_timespec(kit->it.real.interval);
+ tsp->it_value = timespec_sub(ktime_to_timespec(kit->it.real.timer.node.expires), time_now);
+}
+
+
+static int ptp_timer_settime(struct posix_clock *pc,
+ struct k_itimer *kit, int flags,
+ struct itimerspec *tsp, struct itimerspec *old)
+{
+ struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ int err;
+ unsigned long tq_lock_flags;
+ struct timespec time_now;
+ ktime_t fire_time;
+
+ if(ptp->info->timersettime == 0)
+ return -EOPNOTSUPP;
+
+ if (old) {
+ ptp_timer_gettime(pc, kit, old);
+ }
+
+ fire_time = timespec_to_ktime(tsp->it_value);
+
+ if( (fire_time.tv64 != 0) && !(flags & TIMER_ABSTIME))
+ {
+ err = ptp_clock_gettime(pc, &time_now);
+ if(err)
+ return err;
+ /* convert relative to absolute time */
+ fire_time = ktime_add(fire_time, timespec_to_ktime(time_now));
+ }
+
+ /* remove, update and reinsert the node */
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ timerqueue_del(&ptp->timerqueue, &kit->it.real.timer.node);
+
+ kit->it.real.timer.node.expires = fire_time;
+ kit->it.real.interval = timespec_to_ktime(tsp->it_interval);
+
+#ifdef PTP_TIMER_MINIMUM_INTERVAL_NS
+ if ( (ktime_to_ns(kit->it.real.interval) != 0 )
+ && (ktime_to_ns(kit->it.real.interval)<PTP_TIMER_MINIMUM_INTERVAL_NS) )
+ kit->it.real.interval = ns_to_ktime(PTP_TIMER_MINIMUM_INTERVAL_NS);
+#endif
+
+ timerqueue_add(&ptp->timerqueue, &kit->it.real.timer.node);
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ return set_device_timer_earliest(ptp);
+}
+
static struct posix_clock_operations ptp_clock_ops = {
.owner = THIS_MODULE,
.clock_adjtime = ptp_clock_adjtime,
.clock_gettime = ptp_clock_gettime,
.clock_getres = ptp_clock_getres,
.clock_settime = ptp_clock_settime,
+ .timer_create = ptp_timer_create,
+ .timer_delete = ptp_timer_delete,
+ .timer_gettime = ptp_timer_gettime,
+ .timer_settime = ptp_timer_settime,
.ioctl = ptp_ioctl,
.open = ptp_open,
.poll = ptp_poll,
@@ -217,6 +444,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
mutex_init(&ptp->tsevq_mux);
mutex_init(&ptp->pincfg_mux);
init_waitqueue_head(&ptp->tsev_wq);
+ spin_lock_init(&ptp->tq_lock);
+ timerqueue_init_head(&ptp->timerqueue);
+ INIT_WORK(&ptp->alarm_work, ptp_alarm_work);

/* Create a new device in our class. */
ptp->dev = device_create(ptp_class, parent, ptp->devid, ptp,
@@ -286,6 +516,8 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
}
EXPORT_SYMBOL(ptp_clock_unregister);

+
+
void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
{
struct pps_event_time evt;
@@ -293,6 +525,7 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
switch (event->type) {

case PTP_CLOCK_ALARM:
+ schedule_work(&ptp->alarm_work);
break;

case PTP_CLOCK_EXTTS:
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 9c5d414..d491299 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -54,6 +54,10 @@ struct ptp_clock {
struct device_attribute *pin_dev_attr;
struct attribute **pin_attr;
struct attribute_group pin_attr_group;
+
+ struct timerqueue_head timerqueue;
+ spinlock_t tq_lock;
+ struct work_struct alarm_work;
};

/*
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 6b15e16..8d953f3 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -118,6 +118,8 @@ struct ptp_clock_info {
struct ptp_clock_request *request, int on);
int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
enum ptp_pin_function func, unsigned int chan);
+ int (*timerenable)(struct ptp_clock_info *ptp, bool enable);
+ int (*timersettime)(struct ptp_clock_info *ptp, struct timespec64 *ts);
};

struct ptp_clock;
diff --git a/kernel/signal.c b/kernel/signal.c
index af21afc..e7331b3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1561,6 +1561,7 @@ out:
ret:
return ret;
}
+EXPORT_SYMBOL(send_sigqueue);

/*
* Let a parent know about the death of a child.

--
2.1.4

------------------------------------------------------------------------------

Richard Cochran

2016-09-12 09:09:14 UTC

Permalink

Post by Kieran Tyrrell
a PTP hardware clock supporting timers (alarms) can now be used via the POSIX timer (timer_create, timer_settime etc) interface
---
drivers/ptp/ptp_clock.c | 233 +++++++++++++++++++++++++++++++++++++++
drivers/ptp/ptp_private.h | 4 +
include/linux/ptp_clock_kernel.h | 2 +
kernel/signal.c | 1 +
4 files changed, 240 insertions(+)
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 2e481b9..067e41c 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -36,6 +36,8 @@
#define PTP_PPS_EVENT PPS_CAPTUREASSERT
#define PTP_PPS_MODE (PTP_PPS_DEFAULTS | PPS_CANWAIT | PPS_TSFMT_TSPEC)
+#define PTP_TIMER_MINIMUM_INTERVAL_NS 100000
+
/* private globals */
static dev_t ptp_devt;
@@ -43,6 +45,108 @@ static struct class *ptp_class;
static DEFINE_IDA(ptp_clocks_map);
+static int ptp_clock_gettime(struct posix_clock *pc, struct timespec *tp);

Please don't introduce new code with 'struct timespec'.
Use timespec64 instead.

Post by Kieran Tyrrell
+
+static int set_device_timer_earliest(struct ptp_clock *ptp)
+{
+ struct timerqueue_node *next;
+ int err;
+ unsigned long tq_lock_flags;
+ struct timespec64 ts;
+
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ next = timerqueue_getnext(&ptp->timerqueue);
+
+ /* Skip over expired or not set timers */
+ while (next) {
+ if (next->expires.tv64 != 0)
+ break;
+ next = timerqueue_iterate_next(next);
+ }
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ if (next) {
+ ts = ktime_to_timespec64(next->expires);
+ err = ptp->info->timersettime(ptp->info, &ts);
+ if(err)
+ return err;
+ }
+
+ return 0;
+}
+
+static void ptp_alarm_work(struct work_struct *work)
+{
+ struct ptp_clock *ptp = container_of(work, struct ptp_clock, alarm_work);
+ struct task_struct *task;
+ struct siginfo info;
+ struct timerqueue_node *next;
+ struct k_itimer *kit;
+ struct timespec time_now;

timespec64.

Post by Kieran Tyrrell
+ s64 ns_now;
+ int shared;
+ bool signal_failed_to_send;
+ unsigned long tq_lock_flags;
+
+ if(0 != ptp_clock_gettime(&ptp->clock, &time_now))
+ return;

The coding style requires a space after the 'if' keyword. Please run
your patches through scripts/checkpatch.pl to catch this and other
simple mistakes.

Post by Kieran Tyrrell
+ ns_now = timespec_to_ns(&time_now);
+
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ next = timerqueue_getnext(&ptp->timerqueue);
+
+ /* Skip over expired or not set timers */

Why keep expired timers in the queue at all?

Post by Kieran Tyrrell
+ while (next) {
+ if (next->expires.tv64 != 0)
+ break;
+ next = timerqueue_iterate_next(next);
+ }
+
+ while (next) {
+ if (next->expires.tv64 > ns_now)
+ break;
+
+ rcu_read_lock();
+
+ kit = container_of(next, struct k_itimer, it.real.timer.node);

This doesn't need to be under the RCU lock.

Post by Kieran Tyrrell
+ task = pid_task(kit->it_pid, PIDTYPE_PID);
+ if (task)
+ {
+ memset(&info, 0, sizeof(info));
+ info.si_signo = SIGALRM;
+ info.si_code = SI_TIMER;
+ info._sifields._timer._tid = kit->it_id;
+ kit->sigq->info.si_sys_private = 0;
+ shared = !(kit->it_sigev_notify & SIGEV_THREAD_ID);

Nor does any of this.

Wait a minute... You have 'info' on the stack, initialize it, and then?
It isn't used at all.

Post by Kieran Tyrrell
+ signal_failed_to_send = send_sigqueue(kit->sigq, task, shared) > 0;

Is there a reason not to re-use posix_timer_event() here?

Post by Kieran Tyrrell
+ }
+ rcu_read_unlock();
+
+ next = timerqueue_iterate_next(next);
+
+ /* update and reinsert the last one that has fired */
+ timerqueue_del(&ptp->timerqueue, &kit->it.real.timer.node);
+ if ( (0 == ktime_to_ns(kit->it.real.interval)) || signal_failed_to_send) {
+ /* this is not a periodic timer (or the signal failed to send), so stop it */
+ kit->it.real.timer.node.expires = ns_to_ktime(0);
+ }
+ else {
+ /* this IS a periodic timer, so set the next fire time */
+ kit->it.real.timer.node.expires = ktime_add(kit->it.real.timer.node.expires, kit->it.real.interval);
+ }
+ timerqueue_add(&ptp->timerqueue, &kit->it.real.timer.node);
+ }
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ set_device_timer_earliest(ptp);
+}
+
/* time stamp event queue operations */
static inline int queue_free(struct timestamp_event_queue *q)
@@ -163,12 +267,135 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct timex *tx)
return err;
}
+static int ptp_timer_create(struct posix_clock *pc, struct k_itimer *kit)
+{
+ struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ int err = 0;
+ unsigned long tq_lock_flags;
+
+ if(ptp->info->timerenable == 0)
+ return -EOPNOTSUPP;
+
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ if(NULL == timerqueue_getnext(&ptp->timerqueue))
+ {
+ /* list is empty, so hardware timer is disabled, enable it */
+ err = ptp->info->timerenable(ptp->info, true);
+ }
+
+ if(0 == err)
+ {
+ timerqueue_init(&kit->it.real.timer.node);
+ /* ensure expiry time is 0 (timer disabled) */
+ kit->it.real.timer.node.expires = ns_to_ktime(0);
+ timerqueue_add(&ptp->timerqueue, &kit->it.real.timer.node);
+ }
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ return err;
+}
+
+static int ptp_timer_delete(struct posix_clock *pc, struct k_itimer *kit)
+{
+ struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ int err=0;
+ unsigned long tq_lock_flags;
+
+ if(ptp->info->timerenable == 0)
+ return -EOPNOTSUPP;
+
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ timerqueue_del(&ptp->timerqueue, &kit->it.real.timer.node);
+
+ if(NULL == timerqueue_getnext(&ptp->timerqueue))
+ {
+ /* there are no more timers set on this device, so we can disable the hardware timer */
+ err = ptp->info->timerenable(ptp->info, false);
+ }
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ return err;
+}
+
+static void ptp_timer_gettime(struct posix_clock *pc,
+ struct k_itimer *kit, struct itimerspec *tsp)
+{
+ struct timespec time_now;

timespec64.

Post by Kieran Tyrrell
+
+ if(NULL == tsp)
+ return;

How can 'tsp' be NULL?

Post by Kieran Tyrrell
+ if(0 != ptp_clock_gettime(pc, &time_now))
+ return;
+
+ tsp->it_interval = ktime_to_timespec(kit->it.real.interval);
+ tsp->it_value = timespec_sub(ktime_to_timespec(kit->it.real.timer.node.expires), time_now);
+}
+
+
+static int ptp_timer_settime(struct posix_clock *pc,
+ struct k_itimer *kit, int flags,
+ struct itimerspec *tsp, struct itimerspec *old)
+{
+ struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock);
+ int err;
+ unsigned long tq_lock_flags;
+ struct timespec time_now;

timespec64.

Post by Kieran Tyrrell
+ ktime_t fire_time;
+
+ if(ptp->info->timersettime == 0)
+ return -EOPNOTSUPP;
+
+ if (old) {
+ ptp_timer_gettime(pc, kit, old);
+ }
+
+ fire_time = timespec_to_ktime(tsp->it_value);
+
+ if( (fire_time.tv64 != 0) && !(flags & TIMER_ABSTIME))
+ {
+ err = ptp_clock_gettime(pc, &time_now);
+ if(err)
+ return err;
+ /* convert relative to absolute time */
+ fire_time = ktime_add(fire_time, timespec_to_ktime(time_now));
+ }
+
+ /* remove, update and reinsert the node */
+ spin_lock_irqsave(&ptp->tq_lock, tq_lock_flags);
+
+ timerqueue_del(&ptp->timerqueue, &kit->it.real.timer.node);
+
+ kit->it.real.timer.node.expires = fire_time;
+ kit->it.real.interval = timespec_to_ktime(tsp->it_interval);
+
+#ifdef PTP_TIMER_MINIMUM_INTERVAL_NS

No need for ifdef. It is defined above.

Post by Kieran Tyrrell
+ if ( (ktime_to_ns(kit->it.real.interval) != 0 )
+ && (ktime_to_ns(kit->it.real.interval)<PTP_TIMER_MINIMUM_INTERVAL_NS) )
+ kit->it.real.interval = ns_to_ktime(PTP_TIMER_MINIMUM_INTERVAL_NS);
+#endif
+
+ timerqueue_add(&ptp->timerqueue, &kit->it.real.timer.node);
+
+ spin_unlock_irqrestore(&ptp->tq_lock, tq_lock_flags);
+
+ return set_device_timer_earliest(ptp);
+}
+
static struct posix_clock_operations ptp_clock_ops = {
.owner = THIS_MODULE,
.clock_adjtime = ptp_clock_adjtime,
.clock_gettime = ptp_clock_gettime,
.clock_getres = ptp_clock_getres,
.clock_settime = ptp_clock_settime,
+ .timer_create = ptp_timer_create,
+ .timer_delete = ptp_timer_delete,
+ .timer_gettime = ptp_timer_gettime,
+ .timer_settime = ptp_timer_settime,

Keep the tabbed alignment please.

Post by Kieran Tyrrell
.ioctl = ptp_ioctl,
.open = ptp_open,
.poll = ptp_poll,
@@ -217,6 +444,9 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
mutex_init(&ptp->tsevq_mux);
mutex_init(&ptp->pincfg_mux);
init_waitqueue_head(&ptp->tsev_wq);
+ spin_lock_init(&ptp->tq_lock);
+ timerqueue_init_head(&ptp->timerqueue);
+ INIT_WORK(&ptp->alarm_work, ptp_alarm_work);
/* Create a new device in our class. */
ptp->dev = device_create(ptp_class, parent, ptp->devid, ptp,
@@ -286,6 +516,8 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
}
EXPORT_SYMBOL(ptp_clock_unregister);
+
+

Stray newlines.

Post by Kieran Tyrrell
void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
{
struct pps_event_time evt;
@@ -293,6 +525,7 @@ void ptp_clock_event(struct ptp_clock *ptp, struct ptp_clock_event *event)
switch (event->type) {
+ schedule_work(&ptp->alarm_work);

By using a work queue, you are deferring the delivery of the signal
until some unspecified future time. That defeats the whole purpose of
using a special PHC timer in the first place. The signal delivery
should happen immediately.

Post by Kieran Tyrrell
break;
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index 9c5d414..d491299 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -54,6 +54,10 @@ struct ptp_clock {
struct device_attribute *pin_dev_attr;
struct attribute **pin_attr;
struct attribute_group pin_attr_group;
+
+ struct timerqueue_head timerqueue;
+ spinlock_t tq_lock;
+ struct work_struct alarm_work;
};
/*
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 6b15e16..8d953f3 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -118,6 +118,8 @@ struct ptp_clock_info {
struct ptp_clock_request *request, int on);
int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
enum ptp_pin_function func, unsigned int chan);
+ int (*timerenable)(struct ptp_clock_info *ptp, bool enable);
+ int (*timersettime)(struct ptp_clock_info *ptp, struct timespec64 *ts);

These need kerneldoc comments describing the usage above. I find the
name 'timerenable' confusing, because we already have 'enable', and
that method immediately activates the requested feature. The new
timerenable is actually performing a kind of resource reservation, like
the 'verify' method.

I understood what you were trying to do with 'timerenable', but we
should consider integrating the timers with the other interrupt based
functionality. For the user it makes little sense if the capabilities
advertise timers and periodic signals, for example, only to receive
EBUSY later on at run time.

In the case of the i210, there is an either/or choice regarding the
TSINTR_TT1/0 interrupts. We need to put the user in charge somehow,
like we do for mapping of auxiliary functions to HW pins.

Thanks,
Richard

Post by Kieran Tyrrell
};
struct ptp_clock;
diff --git a/kernel/signal.c b/kernel/signal.c
index af21afc..e7331b3 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
return ret;
}
+EXPORT_SYMBOL(send_sigqueue);
/*
* Let a parent know about the death of a child.
--
2.1.4
------------------------------------------------------------------------------
_______________________________________________
Linuxptp-devel mailing list
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel

------------------------------------------------------------------------------

Kieran Tyrrell

2016-09-12 13:52:03 UTC

Permalink

Post by Richard Cochran

Please don't introduce new code with 'struct timespec'.
Use timespec64 instead.

The clock_gettime and timer_settime etc prototypes are declared in struct posix_clock_operations in posix-clock.h, and used by everything that implements a POSIX clock.
The new interfaces that I have defined between the PTP module and the ethernet driver use timespec64.

Post by Richard Cochran

timespec64.

likewise here, I am reusing the existing clock_gettime function to get the current time. I assume you are not suggesting I update the whole POSIX clock implementation to use timespec64?

Post by Richard Cochran

Post by Kieran Tyrrell
+ s64 ns_now;
+ int shared;
+ bool signal_failed_to_send;
+ unsigned long tq_lock_flags;
+
+ if(0 != ptp_clock_gettime(&ptp->clock, &time_now))
+ return;

The coding style requires a space after the 'if' keyword. Please run
your patches through scripts/checkpatch.pl to catch this and other
simple mistakes.

Will do, sorry about that.

Post by Richard Cochran

Why keep expired timers in the queue at all?

it was because I used the queue to know how many timers had been created, and to simplify the code. I’ve reworked it now so that only active timers are in the queue.

Post by Richard Cochran

This doesn't need to be under the RCU lock.

Nor does any of this.
Wait a minute... You have 'info' on the stack, initialize it, and then?
It isn't used at all.

yes sorry, inactive debugging code…

Post by Richard Cochran

Post by Kieran Tyrrell
+ signal_failed_to_send = send_sigqueue(kit->sigq, task, shared) > 0;

Is there a reason not to re-use posix_timer_event() here?

Thanks! I was looking for a way ‘back through’ the posix timer, but failed to find this function! That is a much cleaner way.

Post by Richard Cochran

timespec64.

Post by Kieran Tyrrell
+
+ if(NULL == tsp)
+ return;

How can 'tsp' be NULL?

timespec64.

No need for ifdef. It is defined above.

Keep the tabbed alignment please.

Stray newlines.

I agree. Previously I was finding that sending the signals directly from ptp_clock_event unreliable (around 1 in 1,000,000 signals seemed to be getting lost) while from a work queue they all arrived safely. After further debugging I’m not sure if the problem was with the PTP module, the igb module, my test code, or even my PC. (running the same posix timer tests on the MONOTONIC clock occasionally produces kernel log messages hrtimer:interrupt took 3193ns).

I have changed this to do the work synchronously.

Post by Richard Cochran

These need kerneldoc comments describing the usage above. I find the
name 'timerenable' confusing, because we already have 'enable', and
that method immediately activates the requested feature. The new
timerenable is actually performing a kind of resource reservation, like
the 'verify' method.
I understood what you were trying to do with 'timerenable', but we
should consider integrating the timers with the other interrupt based
functionality. For the user it makes little sense if the capabilities
advertise timers and periodic signals, for example, only to receive
EBUSY later on at run time.
In the case of the i210, there is an either/or choice regarding the
TSINTR_TT1/0 interrupts. We need to put the user in charge somehow,
like we do for mapping of auxiliary functions to HW pins.

That’s a fair point, but I felt that for the POSIX timers, they should just work ‘out the box’, otherwise end user apps will have to have knowledge of the PHC hardware to function? I guess I could add an ioctl to ptp_ioctl to select which ‘hardware alarm’ is used for the POSIX timers? Then testptp.c would have to be updated to add an ioctl call before timer_create().

Post by Richard Cochran
Thanks,
Richard

Thank you for taking the time to review the code. I’ve already fixed/changed some of the things you’ve mentioned, and I will address the other comments and add the ‘alarm enable’ ioctl soon.

Best regards, Kieran.

------------------------------------------------------------------------------

Richard Cochran

2016-09-12 17:14:23 UTC

Permalink

Post by Kieran Tyrrell
The clock_gettime and timer_settime etc prototypes are declared in
struct posix_clock_operations in posix-clock.h, and used by
everything that implements a POSIX clock. The new interfaces that I
have defined between the PTP module and the ethernet driver use
timespec64.

Ok.

Post by Kieran Tyrrell
likewise here, I am reusing the existing clock_gettime function to
get the current time. I assume you are not suggesting I update the
whole POSIX clock implementation to use timespec64?

No, not unless you really want to. That effort is headed by Arnd
Bergmann, and I thought this part was finished already.

Post by Kieran Tyrrell
I agree. Previously I was finding that sending the signals directly
from ptp_clock_event unreliable (around 1 in 1,000,000 signals
seemed to be getting lost) while from a work queue they all arrived
safely. After further debugging I’m not sure if the problem was with
the PTP module, the igb module, my test code, or even my
PC. (running the same posix timer tests on the MONOTONIC clock
occasionally produces kernel log messages hrtimer:interrupt took
3193ns).

Something is fishy, yes, but we should get to the bottom of it. I can
help test this once you drop the work queue.

I have seen the interrupts on the i210 get stuck when stressing the
input time stamp function with a 1 kHz input. The interrupts simply
stop triggering. But your issue sounds different.

Thanks,
Richard

Kieran Tyrrell

2016-09-15 08:56:24 UTC

Permalink

Post by Richard Cochran

Something is fishy, yes, but we should get to the bottom of it. I can
help test this once you drop the work queue.
I have seen the interrupts on the i210 get stuck when stressing the
input time stamp function with a 1 kHz input. The interrupts simply
stop triggering. But your issue sounds different.

My signal delivery problem was a bug in the igb interrupt handler, where I was assuming I could set a new trigger time for TT0 before acknowledging the interrupt. Turns out I have to disable TT0, acknowledge the interrupt, program the new trigger time, and then re-enable TT0 for it to work. All timer signals are now delivered synchronously and reliably.

------------------------------------------------------------------------------

Richard Cochran

2016-09-15 11:33:29 UTC

Permalink

Post by Kieran Tyrrell
My signal delivery problem was a bug in the igb interrupt handler,
where I was assuming I could set a new trigger time for TT0 before
acknowledging the interrupt. Turns out I have to disable TT0,
acknowledge the interrupt, program the new trigger time, and then
re-enable TT0 for it to work. All timer signals are now delivered
synchronously and reliably.

So does this bug affect signal generation using TSINTR_TT0/1?

If so, that should be patched right away, before adding any new
features.

Thanks,
Richard

------------------------------------------------------------------------------

Kieran Tyrrell

2016-09-15 11:39:06 UTC

Permalink

Post by Richard Cochran

So does this bug affect signal generation using TSINTR_TT0/1?
If so, that should be patched right away, before adding any new
features.

I’m not sure, in the timer case it may only have been a problem when setting a new timer trigger time which has (by the time the registers were written and the interrupt acknowledged) already elapsed. I don’t have physical pins on my i210 card to attach a probe to, but I suppose this could be verified by adding debug to the interrupt handler. I’ll see if I can test this...

------------------------------------------------------------------------------

Kieran Tyrrell

2016-10-18 16:19:51 UTC

Permalink

Post by Richard Cochran
I have seen the interrupts on the i210 get stuck when stressing the
input time stamp function with a 1 kHz input. The interrupts simply
stop triggering. But your issue sounds different.
Thanks,
Richard

I suspect this may have something to do with double acknowledging the interrupts in igb_tsync_interrupt().

The TSICR register is read at the start of the interrupt handler, and then written at the end to acknowledge the interrupt(s). According to 8.16.1 (TSICR register) of the i210 datasheet however:

"Once ICR.Time_Sync is set, the internal value of this register (TSICR) should be cleared by writing 1b to all bits or cleared by a read to enable receiving an additional ICR.Time_Sync interrupt.”

So reading the TSICR register acknowledges the interrupt and clears the register. If another tsync interrupt occurs while in the interrupt handler, the second interrupt could be acknowledged without having been handled.

I tested this briefly by commenting out the TSICR write, and RX and TX timestamps are still processed correctly by ptp4l etc.

I can’t be sure this is the cause of the 1kHz input time stamp problem, but I was experiencing similar problems with the timers (alarms) that I have been working on, and this was the cause of that.

Thanks, Kieran.

Richard Cochran

2016-10-18 18:47:18 UTC

Permalink

Post by Kieran Tyrrell
So reading the TSICR register acknowledges the interrupt and clears the register. If another tsync interrupt occurs while in the interrupt handler, the second interrupt could be acknowledged without having been handled.

The igb driver is doing this on purpose. See this thread:

https://www.spinics.net/lists/netdev/msg237642.html

The extra ack is a work around for the 82580. Sounds like the i210
doesn't have this problem. If the double ack is causing missed
interrupts, then it really should be fixed in mainline.

Thanks,
Richard

Keller, Jacob E

2016-10-19 00:29:31 UTC

Permalink

Post by Kieran Tyrrell
So reading the TSICR register acknowledges the interrupt and clears
the register. If another tsync interrupt occurs while in the
interrupt handler, the second interrupt could be acknowledged
without having been handled.

    https://www.spinics.net/lists/netdev/msg237642.html
The extra ack is a work around for the 82580.  Sounds like the i210
doesn't have this problem.  If the double ack is causing missed
interrupts, then it really should be fixed in mainline.
Thanks,
Richard

Agreed. I'll take a look at this and can probably have the driver
maintainer for igb get a fix in.

Regards,
Jake

Kieran Tyrrell

2016-09-13 08:32:39 UTC

Permalink

Post by Richard Cochran

Post by Kieran Tyrrell
+ int (*timerenable)(struct ptp_clock_info *ptp, bool enable);
+ int (*timersettime)(struct ptp_clock_info *ptp, struct timespec64 *ts);

These need kerneldoc comments describing the usage above. I find the
name 'timerenable' confusing, because we already have 'enable', and
that method immediately activates the requested feature. The new
timerenable is actually performing a kind of resource reservation, like
the 'verify' method.
I understood what you were trying to do with 'timerenable', but we
should consider integrating the timers with the other interrupt based
functionality. For the user it makes little sense if the capabilities
advertise timers and periodic signals, for example, only to receive
EBUSY later on at run time.
In the case of the i210, there is an either/or choice regarding the
TSINTR_TT1/0 interrupts. We need to put the user in charge somehow,
like we do for mapping of auxiliary functions to HW pins.

I am about to rework this, to add an ioctl to allow enabling the timer on a specific channel (TT0/TT1 in the case of igb).

As it doesn’t make sense to enable both (hardware) timers at the same time (as the posix timers will only use one), should this be a simple:
PTP_ENABLE_ALARM with an int parameter to set the channel/alarm number to use? (and -1 to disable)?

Should the ptp capabilities (in the igb case) state the number of alarms as 2? (even though only one can be enabled at a time, and in fact you can create hundreds of posix timers off a single hardware alarm?

Just want to get it straight with you before I start coding, thanks!

Kieran.

------------------------------------------------------------------------------

Richard Cochran

2016-09-13 19:00:22 UTC

Permalink

Post by Kieran Tyrrell
Should the ptp capabilities (in the igb case) state the number of
alarms as 2? (even though only one can be enabled at a time, and in
fact you can create hundreds of posix timers off a single hardware
alarm?

Looking back, it is unfortunate that the ptp_clock_caps.n_alarm field
is named like that. We only need one bit to tell the user that a
driver supports timers.

In addition to that, there should some indication of the number of
interrupt sources (i210 has 2) and their connection to the timer,
periodic output, and pps features, if any. Something like
PTP_PIN_GETFUNC/_SETFUNC.

Post by Kieran Tyrrell
Just want to get it straight with you before I start coding, thanks!

I haven't given it much thought, and I am open to creative
suggestions. My only requirement is that the user will be able to
query the capabilities and dial what he wants. For the other features
we advertise the number of feature channels and the number of pins.
The user decides which channel to use on each pin. If there are fewer
pins than channels, then it is clear that he can't have everything at
once.

For now, if you would rather concentrate on the actual timer code,
just set i210's caps to n_alarm=1 and n_per_out=1 and leave the rest
as a todo.

Thanks,
Richard

------------------------------------------------------------------------------

Richard Cochran

2016-09-15 12:15:36 UTC

Permalink

I wouldn’t be comfortable removing one of the periodic outputs from
igb in case anyone is using both.

I only meant doing this for testing purposes.

I’ve added an ioctl to enable/disable the timer functionality on
either of the two channels (TT0/TT1). Of course you can only set it
on one at a time.

For testing this is ok, but in the end we will need a proper
interface, like the PTP_PIN_SETFUNC ioctl.

We now have the situation though that timer_create() will
succeed, but timer_settime() can fail, as the PTP subsystem isn’t
allocating the hardware timer any more, the user is.

I don't see why timer_create() cannot simply check whether an
interrupt channel is available, and if not, return an error.

Also the user could now disable the hardware timer while posix
timers are created and active, leaving them hanging.

Let the timer_create() call lock the interrupt channel, preventing
re-assignment until all timers have gone away.

Thanks,
Richard

------------------------------------------------------------------------------